Merge pull request #7652 from dotty-staging/fix-#7650b

Fix #7650: Drop stale toplevel definitions

Author: odersky

Diff for: compiler/src/dotty/tools/dotc/core/Denotations.scala

@@ -116,6 +116,9 @@ object Denotations {
      */
     def filterWithFlags(required: FlagSet, excluded: FlagSet)(implicit ctx: Context): PreDenotation
 
+    /** Map `f` over all single denotations and aggregate the results with `g`. */
+    def aggregate[T](f: SingleDenotation => T, g: (T, T) => T): T
+
     private var cachedPrefix: Type = _
     private var cachedAsSeenFrom: AsSeenFromResult = _
     private var validAsSeenFrom: Period = Nowhere
@@ -1132,6 +1135,7 @@ object Denotations {
       if (denots.exists && denots.matches(this)) NoDenotation else this
     def filterWithFlags(required: FlagSet, excluded: FlagSet)(implicit ctx: Context): SingleDenotation =
       if (required.isEmpty && excluded.isEmpty || compatibleWith(required, excluded)) this else NoDenotation
+    def aggregate[T](f: SingleDenotation => T, g: (T, T) => T): T = f(this)
 
     type AsSeenFromResult = SingleDenotation
     protected def computeAsSeenFrom(pre: Type)(implicit ctx: Context): SingleDenotation = {
@@ -1286,6 +1290,8 @@ object Denotations {
       derivedUnion(denot1 filterDisjoint denot, denot2 filterDisjoint denot)
     def filterWithFlags(required: FlagSet, excluded: FlagSet)(implicit ctx: Context): PreDenotation =
       derivedUnion(denot1.filterWithFlags(required, excluded), denot2.filterWithFlags(required, excluded))
+    def aggregate[T](f: SingleDenotation => T, g: (T, T) => T): T =
+      g(denot1.aggregate(f, g), denot2.aggregate(f, g))
     protected def derivedUnion(denot1: PreDenotation, denot2: PreDenotation) =
       if ((denot1 eq this.denot1) && (denot2 eq this.denot2)) this
       else denot1 union denot2

Diff for: compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -16,6 +16,7 @@ import annotation.tailrec
 import util.SimpleIdentityMap
 import util.Stats
 import java.util.WeakHashMap
+import scala.util.control.NonFatal
 import config.Config
 import reporting.diagnostic.Message
 import reporting.diagnostic.messages.BadSymbolicReference
@@ -625,6 +626,10 @@ object SymDenotations {
     def isPackageObject(implicit ctx: Context): Boolean =
       name.isPackageObjectName && owner.is(Package) && this.is(Module)
 
+    /** Is this symbol a toplevel definition in a package object? */
+    def isWrappedToplevelDef(given Context): Boolean =
+      !isConstructor && owner.isPackageObject
+
     /** Is this symbol an abstract type? */
     final def isAbstractType(implicit ctx: Context): Boolean = this.is(DeferredType)
 
@@ -1527,6 +1532,14 @@ object SymDenotations {
       myBaseTypeCachePeriod = Nowhere
     }
 
+    def invalidateMemberCaches(sym: Symbol)(given Context): Unit =
+      if myMemberCache != null then myMemberCache.invalidate(sym.name)
+      if !sym.flagsUNSAFE.is(Private) then
+        invalidateMemberNamesCache()
+        if sym.isWrappedToplevelDef then
+          val outerCache = sym.owner.owner.asClass.classDenot.myMemberCache
+          if outerCache != null then outerCache.invalidate(sym.name)
+
     override def copyCaches(from: SymDenotation, phase: Phase)(implicit ctx: Context): this.type = {
       from match {
         case from: ClassDenotation =>
@@ -1726,11 +1739,9 @@ object SymDenotations {
     }
 
     /** Enter a symbol in given `scope` without potentially replacing the old copy. */
-    def enterNoReplace(sym: Symbol, scope: MutableScope)(implicit ctx: Context): Unit = {
+    def enterNoReplace(sym: Symbol, scope: MutableScope)(given Context): Unit =
       scope.enter(sym)
-      if (myMemberCache != null) myMemberCache.invalidate(sym.name)
-      if (!sym.flagsUNSAFE.is(Private)) invalidateMemberNamesCache()
-    }
+      invalidateMemberCaches(sym)
 
     /** Replace symbol `prev` (if defined in current class) by symbol `replacement`.
      *  If `prev` is not defined in current class, do nothing.
@@ -2071,6 +2082,7 @@ object SymDenotations {
 
     private var packageObjsCache: List[ClassDenotation] = _
     private var packageObjsRunId: RunId = NoRunId
+    private var ambiguityWarningIssued: Boolean = false
 
     /** The package objects in this class */
     def packageObjs(implicit ctx: Context): List[ClassDenotation] = {
@@ -2122,19 +2134,61 @@ object SymDenotations {
         case pcls :: pobjs1 =>
           if (pcls.isCompleting) recur(pobjs1, acc)
           else {
-            // A package object inherits members from `Any` and `Object` which
-            // should not be accessible from the package prefix.
             val pmembers = pcls.computeNPMembersNamed(name).filterWithPredicate { d =>
+              // Drop members of `Any` and `Object`
               val owner = d.symbol.maybeOwner
               (owner ne defn.AnyClass) && (owner ne defn.ObjectClass)
             }
             recur(pobjs1, acc.union(pmembers))
           }
         case nil =>
           val directMembers = super.computeNPMembersNamed(name)
-          if (acc.exists) acc.union(directMembers.filterWithPredicate(!_.symbol.isAbsent()))
-          else directMembers
+          if !acc.exists then directMembers
+          else acc.union(directMembers.filterWithPredicate(!_.symbol.isAbsent())) match
+            case d: DenotUnion => dropStale(d)
+            case d => d
       }
+
+      def dropStale(multi: DenotUnion): PreDenotation =
+        val compiledNow = multi.filterWithPredicate(d =>
+          d.symbol.isDefinedInCurrentRun || d.symbol.associatedFile == null
+            // if a symbol does not have an associated file, assume it is defined
+            // in the current run anyway. This is true for packages, and also can happen for pickling and
+            // from-tasty tests that generate a fresh symbol and then re-use it in the next run.
+          )
+        if compiledNow.exists then compiledNow
+        else
+          val assocFiles = multi.aggregate(d => Set(d.symbol.associatedFile), _ union _)
+          if assocFiles.size == 1 then
+            multi // they are all overloaded variants from the same file
+          else
+            // pick the variant(s) from the youngest class file
+            val lastModDate = assocFiles.map(_.lastModified).max
+            val youngest = assocFiles.filter(_.lastModified == lastModDate)
+            val chosen = youngest.head
+            def ambiguousFilesMsg(f: AbstractFile) =
+              em"""Toplevel definition $name is defined in
+                  |  $chosen
+                  |and also in
+                  |  $f"""
+            if youngest.size > 1 then
+              throw TypeError(i"""${ambiguousFilesMsg(youngest.tail.head)}
+                                 |One of these files should be removed from the classpath.""")
+
+            // Warn if one of the older files comes from a different container.
+            // In that case picking the youngest file is not necessarily what we want,
+            // since the older file might have been loaded from a jar earlier in the
+            // classpath.
+            def sameContainer(f: AbstractFile): Boolean =
+              try f.container == chosen.container catch case NonFatal(ex) => true
+            if !ambiguityWarningIssued then
+              for conflicting <- assocFiles.find(!sameContainer(_)) do
+                ctx.warning(i"""${ambiguousFilesMsg(conflicting)}
+                               |Keeping only the definition in $chosen""")
+                ambiguityWarningIssued = true
+            multi.filterWithPredicate(_.symbol.associatedFile == chosen)
+      end dropStale
+
       if (symbol `eq` defn.ScalaPackageClass) {
         val denots = super.computeNPMembersNamed(name)
         if (denots.exists) denots
@@ -2154,17 +2208,17 @@ object SymDenotations {
       recur(packageObjs, super.memberNames(keepOnly))
     }
 
-    /** If another symbol with the same name is entered, unlink it,
-     *  and, if symbol is a package object, invalidate the packageObj cache.
+    /** If another symbol with the same name is entered, unlink it.
+     *  If symbol is a package object, invalidate the packageObj cache.
      *  @return  `sym` is not already entered
      */
     override def proceedWithEnter(sym: Symbol, mscope: MutableScope)(implicit ctx: Context): Boolean = {
       val entry = mscope.lookupEntry(sym.name)
       if (entry != null) {
         if (entry.sym == sym) return false
         mscope.unlink(entry)
-        if (sym.name.isPackageObjectName) packageObjsRunId = NoRunId
       }
+      if (sym.name.isPackageObjectName) packageObjsRunId = NoRunId
       true
     }
 

Diff for: compiler/src/dotty/tools/dotc/reporting/diagnostic/messages.scala

@@ -1908,10 +1908,15 @@ object messages {
   }
 
   case class PackageNameAlreadyDefined(pkg: Symbol)(implicit ctx: Context) extends Message(PackageNameAlreadyDefinedID) {
-    val msg: String = em"${pkg} is already defined, cannot be a ${hl("package")}"
+    val (where, or) =
+      if pkg.associatedFile == null then ("", "")
+      else (s" in ${pkg.associatedFile}", " or delete the containing class file")
+    val msg: String = em"""${pkg.name} is the name of $pkg$where.
+                          |It cannot be used at the same time as the name of a package."""
     val kind: String = "Naming"
     val explanation: String =
-      em"An ${hl("object")} cannot have the same name as an existing ${hl("package")}. Rename either one of them."
+      em"""An ${hl("object")} or other toplevel definition cannot have the same name as an existing ${hl("package")}.
+          |Rename either one of them$or."""
   }
 
   case class UnapplyInvalidNumberOfArguments(qual: untpd.Tree, argTypes: List[Type])(implicit ctx: Context)

Diff for: compiler/src/dotty/tools/dotc/typer/Namer.scala

@@ -302,21 +302,48 @@ class Namer { typer: Typer =>
 
     typr.println(i"creating symbol for $tree in ${ctx.mode}")
 
-    def checkNoConflict(name: Name): Name = {
-      def errorName(msg: => String) = {
-        ctx.error(msg, tree.sourcePos)
-        name.freshened
-      }
-      def preExisting = ctx.effectiveScope.lookup(name)
-      if (ctx.owner.is(PackageClass))
-        if (preExisting.isDefinedInCurrentRun)
-          errorName(s"${preExisting.showLocated} has already been compiled once during this run")
-        else name
+    /** Check that a new definition with given name and privacy status
+     *  in current context would not conflict with existing currently
+     *  compiled definitions.
+     *  The logic here is very subtle and fragile due to the fact that
+     *  we are not allowed to force anything.
+     */
+    def checkNoConflict(name: Name, isPrivate: Boolean): Name =
+      val owner = ctx.owner
+      var conflictsDetected = false
+
+      def conflict(conflicting: Symbol) =
+        val where: String =
+          if conflicting.owner == owner then ""
+          else if conflicting.owner.isPackageObject then i" in ${conflicting.associatedFile}"
+          else i" in ${conflicting.owner}"
+        ctx.error(i"$name is already defined as $conflicting$where", tree.sourcePos)
+        conflictsDetected = true
+
+      def checkNoConflictIn(owner: Symbol) =
+        val preExisting = owner.unforcedDecls.lookup(name)
+        if (preExisting.isDefinedInCurrentRun || preExisting.lastKnownDenotation.is(Package))
+           && (!preExisting.lastKnownDenotation.is(Private) || preExisting.owner.is(Package))
+        then conflict(preExisting)
+
+      def pkgObjs(pkg: Symbol) =
+        pkg.denot.asInstanceOf[PackageClassDenotation].packageObjs.map(_.symbol)
+
+      if owner.is(PackageClass) then
+        checkNoConflictIn(owner)
+        for pkgObj <- pkgObjs(owner) do
+          checkNoConflictIn(pkgObj)
       else
-        if ((!ctx.owner.isClass || name.isTypeName) && preExisting.exists)
-          errorName(i"$name is already defined as $preExisting")
-        else name
-    }
+        def preExisting = ctx.effectiveScope.lookup(name)
+        if (!owner.isClass || name.isTypeName) && preExisting.exists then
+          conflict(preExisting)
+        else if owner.isPackageObject && !isPrivate && name != nme.CONSTRUCTOR then
+          checkNoConflictIn(owner.owner)
+          for pkgObj <- pkgObjs(owner.owner) if pkgObj != owner do
+            checkNoConflictIn(pkgObj)
+
+      if conflictsDetected then name.freshened else name
+    end checkNoConflict
 
     /** Create new symbol or redefine existing symbol under lateCompile. */
     def createOrRefine[S <: Symbol](
@@ -348,17 +375,17 @@ class Namer { typer: Typer =>
 
     tree match {
       case tree: TypeDef if tree.isClassDef =>
-        val name = checkNoConflict(tree.name).asTypeName
         val flags = checkFlags(tree.mods.flags &~ GivenOrImplicit)
+        val name = checkNoConflict(tree.name, flags.is(Private)).asTypeName
         val cls =
           createOrRefine[ClassSymbol](tree, name, flags, ctx.owner,
             cls => adjustIfModule(new ClassCompleter(cls, tree)(ctx), tree),
             ctx.newClassSymbol(ctx.owner, name, _, _, _, tree.nameSpan, ctx.source.file))
         cls.completer.asInstanceOf[ClassCompleter].init()
         cls
       case tree: MemberDef =>
-        val name = checkNoConflict(tree.name)
         var flags = checkFlags(tree.mods.flags)
+        val name = checkNoConflict(tree.name, flags.is(Private))
         tree match
           case tree: ValOrDefDef =>
             if tree.unforcedRhs == EmptyTree

Diff for: compiler/src/dotty/tools/dotc/typer/Typer.scala

@@ -1877,22 +1877,22 @@ class Typer extends Namer
     assignType(cpy.Import(imp)(expr1, selectors1), sym)
   }
 
-  def typedPackageDef(tree: untpd.PackageDef)(implicit ctx: Context): Tree = {
+  def typedPackageDef(tree: untpd.PackageDef)(implicit ctx: Context): Tree =
     val pid1 = typedExpr(tree.pid, AnySelectionProto)(ctx.addMode(Mode.InPackageClauseName))
     val pkg = pid1.symbol
-    pid1 match {
-      case pid1: RefTree if pkg.exists =>
-        if (!pkg.is(Package)) ctx.error(PackageNameAlreadyDefined(pkg), tree.sourcePos)
+    pid1 match
+      case pid1: RefTree if pkg.is(Package) =>
         val packageCtx = ctx.packageContext(tree, pkg)
         var stats1 = typedStats(tree.stats, pkg.moduleClass)(packageCtx)._1
         if (!ctx.isAfterTyper)
           stats1 = stats1 ++ typedBlockStats(MainProxies.mainProxies(stats1))(packageCtx)._1
         cpy.PackageDef(tree)(pid1, stats1).withType(pkg.termRef)
       case _ =>
         // Package will not exist if a duplicate type has already been entered, see `tests/neg/1708.scala`
-        errorTree(tree, i"package ${tree.pid.name} does not exist")
-    }
-  }
+        errorTree(tree,
+          if pkg.exists then PackageNameAlreadyDefined(pkg)
+          else i"package ${tree.pid.name} does not exist")
+  end typedPackageDef
 
   def typedAnnotated(tree: untpd.Annotated, pt: Type)(implicit ctx: Context): Tree = {
     val annot1 = typedExpr(tree.annot, defn.AnnotationClass.typeRef)

Diff for: docs/docs/reference/dropped-features/package-objects.md

@@ -42,3 +42,6 @@ in a source file `src.scala`, it could be invoked from the command line using a
 "program name" is mangled it is recommended to always put `main` methods in explicitly named objects.
 
 **Note 3:** The notion of `private` is independent of whether a definition is wrapped or not. A `private` toplevel definition is always visible from everywhere in the enclosing package.
+
+**Note 4:** If several toplevel definitions are overloaded variants with the same name,
+they must all come from the same source file.

Diff for: tests/neg/toplevel-doubledef/Test_2.scala

@@ -1,2 +1,2 @@
-def hello(x: String): Unit =
+def hello(x: String): Unit = // error: has already been compiled
   println(s"hi, $x")

Diff for: tests/neg/toplevel-doubledef/moredefs_1.scala

@@ -1,3 +1,5 @@
 trait A
 
 def f(x: A) = s"A"
+
+def g(): Unit = ()

Diff for: tests/neg/toplevel-overload/Test_3.scala

@@ -0,0 +1,5 @@
+trait B
+
+def f(x: B) = s"B" // error: has already been compiled
+
+private def g(): Unit = () // OK, since it is private

Diff for: tests/neg/toplevel-overload/defs_1.scala

@@ -0,0 +1,6 @@
+package foo
+package bar
+  class C
+  def hello(x: String): Unit =
+    println(s"hello: $x")
+

Diff for: tests/neg/toplevel-overload/defs_2.scala

@@ -0,0 +1,2 @@
+package foo
+def bar = 2  // error: bar is a package

Diff for: tests/neg/toplevel-overload/moredefs_1.scala

@@ -0,0 +1 @@
+@main def Test() = println("hi")

Diff for: tests/neg/toplevel-package/defs_1.scala

@@ -0,0 +1 @@
+object Test

Diff for: tests/neg/toplevel-package/moredefs_2.scala

@@ -1,13 +1,13 @@
 
 class x(val s: String)
 
-object y {
+object yy {
   def foo = 3
 }
 
 object X2 {
   def bar = "hi"
 }
 
-class X3
+class X4
 

Diff for: tests/pos/i7650/Test_1.scala

@@ -4,7 +4,7 @@ object Test extends App {
   assert((new x("abc")).s == "abc")
 
   assert(y("abc") == 3)
-  assert(y.foo == 3)
+  assert(yy.foo == 3)
 
   val x2: X2 = "abc"
   assert(X2.bar == "hi")