Merge pull request #1383 from paulp/issue/5604

Issue/5604

19 files changed, 274 insertions, 49 deletions

diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
index c3fca13374..18e9ae620e 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenASM.scala
@@ -604,11 +604,18 @@ abstract class GenASM extends SubComponent with BytecodeWriters {
         val internalName = cachedJN.toString()
         val trackedSym = jsymbol(sym)
         reverseJavaName.get(internalName) match {
-          case None         =>
+          case Some(oldsym) if oldsym.exists && trackedSym.exists =>
+            assert(
+              // In contrast, neither NothingClass nor NullClass show up bytecode-level.
+              (oldsym == trackedSym) || (oldsym == RuntimeNothingClass) || (oldsym == RuntimeNullClass),
+              s"""|Different class symbols have the same bytecode-level internal name:
+                  |     name: $internalName
+                  |   oldsym: ${oldsym.fullNameString}
+                  |  tracked: ${trackedSym.fullNameString}
+              """.stripMargin
+            )
+          case _ =>
             reverseJavaName.put(internalName, trackedSym)
-          case Some(oldsym) =>
-            assert((oldsym == trackedSym) || (oldsym == RuntimeNothingClass) || (oldsym == RuntimeNullClass), // In contrast, neither NothingClass nor NullClass show up bytecode-level.
-                   "how can getCommonSuperclass() do its job if different class symbols get the same bytecode-level internal name: " + internalName)
         }
       }
 
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index df8eb9c6b9..55ec8bead4 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -161,6 +161,9 @@ trait Namers extends MethodSynthesis {
       else innerNamer
     }
 
+    // FIXME - this logic needs to be thoroughly explained
+    // and justified.  I know it's wrong with repect to package
+    // objects, but I think it's also wrong in other ways.
     protected def conflict(newS: Symbol, oldS: Symbol) = (
        (   !oldS.isSourceMethod
         || nme.isSetterName(newS.name)
@@ -188,6 +191,19 @@ trait Namers extends MethodSynthesis {
 
     /** Enter symbol into given scope and return symbol itself */
     def enterInScope(sym: Symbol, scope: Scope): Symbol = {
+      // FIXME - this is broken in a number of ways.
+      //
+      // 1) If "sym" allows overloading, that is not itself sufficient to skip
+      // the check, because "prev.sym" also must allow overloading.
+      //
+      // 2) There is nothing which reconciles a package's scope with
+      // the package object's scope.  This is the source of many bugs
+      // with e.g. defining a case class in a package object.  When
+      // compiling against classes, the class symbol is created in the
+      // package and in the package object, and the conflict is undetected.
+      // There is also a non-deterministic outcome for situations like
+      // an object with the same name as a method in the package object.
+
       // allow for overloaded methods
       if (!allowsOverload(sym)) {
         val prev = scope.lookupEntry(sym.name)
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 2344e71883..6e222459c9 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -605,23 +605,31 @@ trait Typers extends Modes with Adaptations with Tags {
       }
 
     /** Is `sym` defined in package object of package `pkg`?
+     *  Since sym may be defined in some parent of the package object,
+     *  we cannot inspect its owner only; we have to go through the
+     *  info of the package object.  However to avoid cycles we'll check
+     *  what other ways we can before pushing that way.
      */
     private def isInPackageObject(sym: Symbol, pkg: Symbol) = {
-      def isInPkgObj(sym: Symbol) =
-        !sym.owner.isPackage && {
-          sym.owner.isPackageObjectClass &&
-            sym.owner.owner == pkg ||
-            pkg.isInitialized && {
-              // need to be careful here to not get a cyclic reference during bootstrap
-              val pkgobj = pkg.info.member(nme.PACKAGEkw)
-              pkgobj.isInitialized &&
-                (pkgobj.info.member(sym.name).alternatives contains sym)
-            }
+      val pkgClass = if (pkg.isTerm) pkg.moduleClass else pkg
+      def matchesInfo = (
+        pkg.isInitialized && {
+          // need to be careful here to not get a cyclic reference during bootstrap
+          val module = pkg.info member nme.PACKAGEkw
+          module.isInitialized && (module.info.member(sym.name).alternatives contains sym)
         }
-      pkg.isPackageClass && {
+      )
+      def isInPkgObj(sym: Symbol) = (
+           !sym.isPackage
+        && !sym.owner.isPackageClass
+        && (sym.owner ne NoSymbol)
+        && (sym.owner.owner == pkgClass || matchesInfo)
+      )
+
+      pkgClass.isPackageClass && (
         if (sym.isOverloaded) sym.alternatives forall isInPkgObj
         else isInPkgObj(sym)
-      }
+      )
     }
 
     /** Post-process an identifier or selection node, performing the following:
@@ -4768,6 +4776,22 @@ trait Typers extends Modes with Adaptations with Tags {
           defSym = rootMirror.EmptyPackageClass.tpe.nonPrivateMember(name)
           defSym != NoSymbol
         }
+        def correctForPackageObject(sym: Symbol): Symbol = {
+          if (sym.isTerm && isInPackageObject(sym, pre.typeSymbol)) {
+            val sym1 = pre member sym.name
+            if ((sym1 eq NoSymbol) || (sym eq sym1)) sym else {
+              qual = gen.mkAttributedQualifier(pre)
+              log(s"""
+                |  !!! Overloaded package object member resolved incorrectly.
+                |        prefix: $pre
+                |     Discarded: ${sym.defString}
+                |         Using: ${sym1.defString}
+                """.stripMargin)
+              sym1
+            }
+          }
+          else sym
+        }
         def startingIdentContext = (
           // ignore current variable scope in patterns to enforce linearity
           if ((mode & (PATTERNmode | TYPEPATmode)) == 0) context
@@ -4779,11 +4803,11 @@ trait Typers extends Modes with Adaptations with Tags {
         // which are methods (note: if we don't do that
         // case x :: xs in class List would return the :: method)
         // unless they are stable or are accessors (the latter exception is for better error messages).
-        def qualifies(sym: Symbol): Boolean = {
-          sym.hasRawInfo &&       // this condition avoids crashing on self-referential pattern variables
-          reallyExists(sym) &&
-          ((mode & PATTERNmode | FUNmode) != (PATTERNmode | FUNmode) || !sym.isSourceMethod || sym.hasFlag(ACCESSOR))
-        }
+        def qualifies(sym: Symbol): Boolean = (
+             sym.hasRawInfo  // this condition avoids crashing on self-referential pattern variables
+          && reallyExists(sym)
+          && ((mode & PATTERNmode | FUNmode) != (PATTERNmode | FUNmode) || !sym.isSourceMethod || sym.hasFlag(ACCESSOR))
+        )
 
         if (defSym == NoSymbol) {
           var defEntry: ScopeEntry = null // the scope entry of defSym, if defined in a local scope
@@ -4791,32 +4815,17 @@ trait Typers extends Modes with Adaptations with Tags {
           var cx = startingIdentContext
           while (defSym == NoSymbol && cx != NoContext && (cx.scope ne null)) { // cx.scope eq null arises during FixInvalidSyms in Duplicators
             pre = cx.enclClass.prefix
+            // !!! FIXME.  This call to lookupEntry is at the root of all the
+            // bad behavior with overloading in package objects.  lookupEntry
+            // just takes the first symbol it finds in scope, ignoring the rest.
+            // When a selection on a package object arrives here, the first
+            // overload is always chosen.  "correctForPackageObject" exists to
+            // undo that decision.  Obviously it would be better not to do it in
+            // the first place; however other things seem to be tied to obtaining
+            // that ScopeEntry, specifically calculating the nesting depth.
             defEntry = cx.scope.lookupEntry(name)
-            if ((defEntry ne null) && qualifies(defEntry.sym)) {
-              // Right here is where SI-1987, overloading in package objects, can be
-              // seen to go wrong. There is an overloaded symbol, but when referring
-              // to the unqualified identifier from elsewhere in the package, only
-              // the last definition is visible. So overloading mis-resolves and is
-              // definition-order dependent, bad things. See run/t1987.scala.
-              //
-              // I assume the actual problem involves how/where these symbols are entered
-              // into the scope. But since I didn't figure out how to fix it that way, I
-              // catch it here by looking up package-object-defined symbols in the prefix.
-              if (isInPackageObject(defEntry.sym, pre.typeSymbol)) {
-                defSym = pre.member(defEntry.sym.name)
-                if (defSym ne defEntry.sym) {
-                  qual = gen.mkAttributedQualifier(pre)
-                  log(s"""
-                    |  !!! Overloaded package object member resolved incorrectly.
-                    |        prefix: $pre
-                    |     Discarded: ${defEntry.sym.defString}
-                    |         Using: ${defSym.defString}
-                    """.stripMargin)
-                }
-              }
-              else
-                defSym = defEntry.sym
-            }
+            if ((defEntry ne null) && qualifies(defEntry.sym))
+              defSym = correctForPackageObject(defEntry.sym)
             else {
               cx = cx.enclClass
               val foundSym = pre.member(name) filter qualifies
diff --git a/src/reflect/scala/reflect/internal/TreeGen.scala b/src/reflect/scala/reflect/internal/TreeGen.scala
index ebf0998573..c1753fc5a1 100644
--- a/src/reflect/scala/reflect/internal/TreeGen.scala
+++ b/src/reflect/scala/reflect/internal/TreeGen.scala
@@ -172,10 +172,29 @@ abstract class TreeGen extends macros.TreeBuilder {
     if (qual.symbol != null && (qual.symbol.isEffectiveRoot || qual.symbol.isEmptyPackage))
       mkAttributedIdent(sym)
     else {
+      // Have to recognize anytime a selection is made on a package
+      // so it can be rewritten to foo.bar.`package`.name rather than
+      // foo.bar.name if name is in the package object.
+      // TODO - factor out the common logic between this and
+      // the Typers method "isInPackageObject", used in typedIdent.
+      val qualsym = (
+        if (qual.tpe ne null) qual.tpe.typeSymbol
+        else if (qual.symbol ne null) qual.symbol
+        else NoSymbol
+      )
+      val needsPackageQualifier = (
+           (sym ne null)
+        && qualsym.isPackage
+        && !sym.isDefinedInPackage
+      )
       val pkgQualifier =
-        if (sym != null && sym.owner.isPackageObjectClass && sym.effectiveOwner == qual.tpe.typeSymbol) {
-          val obj = sym.owner.sourceModule
-          Select(qual, nme.PACKAGE) setSymbol obj setType singleType(qual.tpe, obj)
+        if (needsPackageQualifier) {
+          // The owner of a symbol which requires package qualification may be the
+          // package object iself, but it also could be any superclass of the package
+          // object.  In the latter case, we must go through the qualifier's info
+          // to obtain the right symbol.
+          val packageObject = if (sym.owner.isModuleClass) sym.owner.sourceModule else qual.tpe member nme.PACKAGE
+          Select(qual, nme.PACKAGE) setSymbol packageObject setType singleType(qual.tpe, packageObject)
         }
         else qual
 
diff --git a/test/files/pos/t5604b/T_1.scala b/test/files/pos/t5604b/T_1.scala
new file mode 100644
index 0000000000..179dcb10c6
--- /dev/null
+++ b/test/files/pos/t5604b/T_1.scala
@@ -0,0 +1,6 @@
+// sandbox/t5604/T.scala
+package t6504
+
+trait T {
+  def foo: Boolean = false
+}
diff --git a/test/files/pos/t5604b/T_2.scala b/test/files/pos/t5604b/T_2.scala
new file mode 100644
index 0000000000..179dcb10c6
--- /dev/null
+++ b/test/files/pos/t5604b/T_2.scala
@@ -0,0 +1,6 @@
+// sandbox/t5604/T.scala
+package t6504
+
+trait T {
+  def foo: Boolean = false
+}
diff --git a/test/files/pos/t5604b/Test_1.scala b/test/files/pos/t5604b/Test_1.scala
new file mode 100644
index 0000000000..f7c58ebe83
--- /dev/null
+++ b/test/files/pos/t5604b/Test_1.scala
@@ -0,0 +1,7 @@
+// sandbox/t5604/Test.scala
+package t6504
+
+object Test {
+  def blerg1(a: Any): Any = if (foo) blerg1(0)
+  def blerg2(a: Any): Any = if (t6504.foo) blerg2(0)
+}
diff --git a/test/files/pos/t5604b/Test_2.scala b/test/files/pos/t5604b/Test_2.scala
new file mode 100644
index 0000000000..f7c58ebe83
--- /dev/null
+++ b/test/files/pos/t5604b/Test_2.scala
@@ -0,0 +1,7 @@
+// sandbox/t5604/Test.scala
+package t6504
+
+object Test {
+  def blerg1(a: Any): Any = if (foo) blerg1(0)
+  def blerg2(a: Any): Any = if (t6504.foo) blerg2(0)
+}
diff --git a/test/files/pos/t5604b/pack_1.scala b/test/files/pos/t5604b/pack_1.scala
new file mode 100644
index 0000000000..f50d568bfa
--- /dev/null
+++ b/test/files/pos/t5604b/pack_1.scala
@@ -0,0 +1,5 @@
+// sandbox/t5604/pack.scala
+package t6504
+
+object `package` extends T {
+}
diff --git a/test/files/run/t5604.check b/test/files/run/t5604.check
new file mode 100644
index 0000000000..53a2fc8894
--- /dev/null
+++ b/test/files/run/t5604.check
@@ -0,0 +1,8 @@
+long
+double
+long
+double
+long
+double
+long
+double
diff --git a/test/files/run/t5604.scala b/test/files/run/t5604.scala
new file mode 100644
index 0000000000..a06c8aab3e
--- /dev/null
+++ b/test/files/run/t5604.scala
@@ -0,0 +1,50 @@
+// a.scala
+// Fri Jan 13 11:31:47 PST 2012
+
+package foo {
+  object regular extends Duh {
+    def buh(n: Long)   = println("long")
+    def buh(n: Double) = println("double")
+  }
+  class regular {
+    import regular._
+
+    duh(33L)
+    duh(3.0d)
+    foo.regular.duh(33L)
+    foo.regular.duh(3.0d)
+    buh(66L)
+    buh(6.0d)
+    foo.regular.buh(66L)
+    foo.regular.buh(6.0d)
+  }
+
+  trait Duh {
+    def duh(n: Long)   = println("long")
+    def duh(n: Double) = println("double")
+  }
+  package object bar extends Duh {
+    def buh(n: Long)   = println("long")
+    def buh(n: Double) = println("double")
+  }
+  package bar {
+    object Main {
+      def main(args:Array[String]) {
+        duh(33L)
+        duh(3.0d)
+        foo.bar.duh(33L)
+        foo.bar.duh(3.0d)
+        buh(66L)
+        buh(6.0d)
+        foo.bar.buh(66L)
+        foo.bar.buh(6.0d)
+      }
+    }
+  }
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    foo.bar.Main.main(null)
+  }
+}
diff --git a/test/pending/pos/overloading-boundaries.scala b/test/pending/pos/overloading-boundaries.scala
new file mode 100644
index 0000000000..d2e9fdbb12
--- /dev/null
+++ b/test/pending/pos/overloading-boundaries.scala
@@ -0,0 +1,37 @@
+package bar {
+  object bippy extends (Double => String) {
+    def apply(x: Double): String = "Double"
+  }
+}
+
+package object bar {
+  def bippy(x: Int, y: Int, z: Int) = "(Int, Int, Int)"
+}
+
+object Test {
+  def main(args: Array[String]): Unit = {
+    println(bar.bippy(5.5d))
+    println(bar.bippy(1, 2, 3))
+  }
+}
+
+/****
+
+% scalac3 a.scala
+a.scala:13: error: not enough arguments for method bippy: (x: Int, y: Int, z: Int)String.
+Unspecified value parameters y, z.
+    println(bar.bippy(5.5d))
+                     ^
+one error found
+
+# Comment out the call to bar.bippy(5.5d) - compiles
+% scalac3 a.scala
+
+# Compiles only from pure source though - if classes are present, fails.
+% scalac3 a.scala
+a.scala:2: error: bippy is already defined as method bippy in package object bar
+  object bippy extends (Double => String) {
+         ^
+one error found
+
+****/
diff --git a/test/pending/pos/t4695/T_1.scala b/test/pending/pos/t4695/T_1.scala
new file mode 100644
index 0000000000..70fb1a7f21
--- /dev/null
+++ b/test/pending/pos/t4695/T_1.scala
@@ -0,0 +1,4 @@
+package foo
+
+class Bar { }
+package object Bar { }
diff --git a/test/pending/pos/t4695/T_2.scala b/test/pending/pos/t4695/T_2.scala
new file mode 100644
index 0000000000..70fb1a7f21
--- /dev/null
+++ b/test/pending/pos/t4695/T_2.scala
@@ -0,0 +1,4 @@
+package foo
+
+class Bar { }
+package object Bar { }
diff --git a/test/pending/pos/t5877.scala b/test/pending/pos/t5877.scala
new file mode 100644
index 0000000000..b77605f7f2
--- /dev/null
+++ b/test/pending/pos/t5877.scala
@@ -0,0 +1,5 @@
+package foo { }
+
+package object foo {
+  implicit class Foo(val s: String) { }
+}
diff --git a/test/pending/pos/t5954/T_1.scala b/test/pending/pos/t5954/T_1.scala
new file mode 100644
index 0000000000..0064c596b6
--- /dev/null
+++ b/test/pending/pos/t5954/T_1.scala
@@ -0,0 +1,8 @@
+package p {
+  package base {
+    class X
+  }
+  package object base {
+    case class B()
+  }
+}
diff --git a/test/pending/pos/t5954/T_2.scala b/test/pending/pos/t5954/T_2.scala
new file mode 100644
index 0000000000..0064c596b6
--- /dev/null
+++ b/test/pending/pos/t5954/T_2.scala
@@ -0,0 +1,8 @@
+package p {
+  package base {
+    class X
+  }
+  package object base {
+    case class B()
+  }
+}
diff --git a/test/pending/pos/t5954/T_3.scala b/test/pending/pos/t5954/T_3.scala
new file mode 100644
index 0000000000..0064c596b6
--- /dev/null
+++ b/test/pending/pos/t5954/T_3.scala
@@ -0,0 +1,8 @@
+package p {
+  package base {
+    class X
+  }
+  package object base {
+    case class B()
+  }
+}
diff --git a/test/pending/pos/t6225.scala b/test/pending/pos/t6225.scala
new file mode 100644
index 0000000000..d7dff3c419
--- /dev/null
+++ b/test/pending/pos/t6225.scala
@@ -0,0 +1,11 @@
+package library.x {
+  class X {
+    class Foo
+    implicit val foo = new Foo
+  }
+}
+package library { package object x extends X }
+package app {
+  import library.x._
+  object App { implicitly[Foo] }
+}