SI-8134 SI-5954 Fix companions in package object under separate comp.

The tests cases enclosed exhibited two failures modes under
separate compilation.

  1. When a synthetic companion object for a case- or implicit-class
     defined in a package object is called for,
     `Namer#ensureCompanionObject` is used to check for an explicitly
     defined companion before decided to create a synthetic one.
     This lookup of an existing companion symbol by `companionObjectOf`
     would locate a symbol backed by a class file which was in the
     scope of the enclosing package class. Furthermore, because the
     owner of that symbol is the package object class that has now
     been noted as corresponding to a source file in the current
     run, the class-file backed module symbol is *also* deemed to
     be from the current run. (This logic is in `Run#compiles`.)

     Thinking the companion module already existed, no synthetic
     module was created, which would lead to a crash in extension
     methods, which needs to add methods to it.

  2. In cases when the code explicitly contains the companion pair,
     we still ran into problems in the backend whereby the class-file
     based and source-file based symbols for the module ended up in
     the same scope (of the package class). This tripped an assertion
     in `Symbol#companionModule`.

We get into these problems because of the eager manner in which
class-file based package object are opened in `openPackageModule`.
The members of the module are copied into the scope of the enclosing
package:

    scala> ScalaPackage.info.member(nme.List)
    res0: $r#59116.intp#45094.global#28436.Symbol#29451 = value List#2462

    scala> ScalaPackage.info.member(nme.PACKAGE).info.member(nme.List)
    res1: $r#59116.intp#45094.global#28436.Symbol#29451 = value List#2462

This seems to require a two-pronged defense:

  1. When we attach a pre-existing symbol for a package object symbol
     to the tree of its new source, unlink the "forwarder" symbols
     (its decls from the enclosing
     package class.

  2. In `Flatten`, in the spirit of `replaceSymbolInCurrentScope`,
     remove static member modules from the scope of the enclosing
     package object (aka `exitingFlatten(nestedModule.owner)`).

This commit also removes the warnings about defining companions
in package objects and converts those neg tests to pos (with
-Xfatal-warnings to prove they are warning free.)

Defining nested classes/objects in package objects still has
a drawback: you can't shift a class from the package to the
package object, or vice versa, in a binary compatible manner,
because of the `package$` prefix on the flattened name of
nested classes. For this reason, the `-Xlint` warning about
this remains. This issue is tracked as SI-4344.

However, if one heeds this warning and incrementatlly recompiles,
we no longer need to run into a DoubleDefinition error (which
was dressed up with a more specific diagnostic in SI-5760.)
The neg test case for that bug has been converted to a pos.

3 files changed, 18 insertions, 32 deletions

diff --git a/src/compiler/scala/tools/nsc/transform/Flatten.scala b/src/compiler/scala/tools/nsc/transform/Flatten.scala
index b4329965fc..976314227b 100644
--- a/src/compiler/scala/tools/nsc/transform/Flatten.scala
+++ b/src/compiler/scala/tools/nsc/transform/Flatten.scala
@@ -20,12 +20,18 @@ abstract class Flatten extends InfoTransform {
   /** Updates the owning scope with the given symbol, unlinking any others.
    */
   private def replaceSymbolInCurrentScope(sym: Symbol): Unit = exitingFlatten {
+    removeSymbolInCurrentScope(sym)
+    sym.owner.info.decls enter sym
+  }
+
+  private def removeSymbolInCurrentScope(sym: Symbol): Unit = exitingFlatten {
     val scope = sym.owner.info.decls
     val old   = (scope lookupUnshadowedEntries sym.name).toList
-    old foreach (scope unlink _)
-    scope enter sym
+    old foreach {entry =>
+      scope unlink entry
+    }
     def old_s = old map (_.sym) mkString ", "
-    debuglog(s"In scope of ${sym.owner}, unlinked $old_s and entered $sym")
+    if (old.nonEmpty) debuglog(s"In scope of ${sym.owner}, unlinked $old_s")
   }
 
   private def liftClass(sym: Symbol) {
@@ -71,9 +77,9 @@ abstract class Flatten extends InfoTransform {
 
             for (sym <- decls) {
               if (sym.isTerm && !sym.isStaticModule) {
-                decls1 enter sym
-                if (sym.isModule)
-                  sym.moduleClass setFlag LIFTED
+                  decls1 enter sym
+                  if (sym.isModule)
+                    sym.moduleClass setFlag LIFTED
               } else if (sym.isClass)
                 liftSymbol(sym)
             }
@@ -121,6 +127,8 @@ abstract class Flatten extends InfoTransform {
           val liftedBuffer = liftedDefs(tree.symbol.enclosingTopLevelClass.owner)
           val index = liftedBuffer.length
           liftedBuffer.insert(index, super.transform(tree))
+          if (tree.symbol.sourceModule.isStaticModule)
+            removeSymbolInCurrentScope(tree.symbol.sourceModule)
           EmptyTree
         case _ =>
           super.transform(tree)
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 14695f5939..48c2c55d7c 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -436,6 +436,10 @@ trait Namers extends MethodSynthesis {
       var m: Symbol = context.scope lookupModule tree.name
       val moduleFlags = tree.mods.flags | MODULE
       if (m.isModule && !m.isPackage && inCurrentScope(m) && (currentRun.canRedefine(m) || m.isSynthetic)) {
+        if (m.isPackageObject) {
+          val packageScope = m.enclosingPackageClass.rawInfo.decls
+          packageScope.filter(_.owner != m.enclosingPackageClass).toList.foreach(packageScope unlink _)
+        }
         updatePosFlags(m, tree.pos, moduleFlags)
         setPrivateWithin(tree, m)
         m.moduleClass andAlso (setPrivateWithin(tree, _))
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 6b5afce993..58b7c330d9 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -1796,32 +1796,6 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
       if (mdef.symbol == PredefModule)
         ensurePredefParentsAreInSameSourceFile(impl2)
 
-      // SI-5954. On second compile of a companion class contained in a package object we end up
-      // with some confusion of names which leads to having two symbols with the same name in the
-      // same owner. Until that can be straightened out we will warn on companion objects in package
-      // objects. But this code also tries to be friendly by distinguishing between case classes and
-      // user written companion pairs
-      def warnPackageObjectMembers(mdef : ModuleDef) = for (m <- mdef.symbol.info.members) {
-        // ignore synthetic objects, because the "companion" object to a case class is synthetic and
-        // we only want one error per case class
-        if (!m.isSynthetic) {
-          // can't handle case classes in package objects
-          if (m.isCaseClass) pkgObjectWarning(m, mdef, "case")
-          // can't handle companion class/object pairs in package objects
-          else if ((m.isClass && m.companionModule != NoSymbol && !m.companionModule.isSynthetic) ||
-                   (m.isModule && m.companionClass != NoSymbol && !m.companionClass.isSynthetic))
-                     pkgObjectWarning(m, mdef, "companion")
-        }
-
-        def pkgObjectWarning(m : Symbol, mdef : ModuleDef, restricted : String) = {
-          val pkgName = mdef.symbol.ownerChain find (_.isPackage) map (_.decodedName) getOrElse mdef.symbol.toString
-          context.warning(if (m.pos.isDefined) m.pos else mdef.pos, s"${m} should be placed directly in package ${pkgName} instead of package object ${pkgName}. Under some circumstances companion objects and case classes in package objects can fail to recompile. See https://issues.scala-lang.org/browse/SI-5954.")
-        }
-      }
-
-      if (mdef.symbol.isPackageObject)
-        warnPackageObjectMembers(mdef)
-
       treeCopy.ModuleDef(mdef, typedMods, mdef.name, impl2) setType NoType
     }