Merge pull request #1554 from paulp/scope-lookup

Improvements to scope lookup.

7 files changed, 122 insertions, 95 deletions

diff --git a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
index 5922d67a94..ba68c6b866 100644
--- a/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
+++ b/src/compiler/scala/tools/nsc/symtab/classfile/ClassfileParser.scala
@@ -562,7 +562,7 @@ abstract class ClassfileParser {
         0 until in.nextChar foreach (_ => parseMethod())
         val needsConstructor = (
              !sawPrivateConstructor
-          && instanceScope.lookup(nme.CONSTRUCTOR) == NoSymbol
+          && !(instanceScope containsName nme.CONSTRUCTOR)
           && (sflags & INTERFACE) == 0
         )
         if (needsConstructor)
diff --git a/src/compiler/scala/tools/nsc/transform/Flatten.scala b/src/compiler/scala/tools/nsc/transform/Flatten.scala
index e2913bea0d..672a11ec30 100644
--- a/src/compiler/scala/tools/nsc/transform/Flatten.scala
+++ b/src/compiler/scala/tools/nsc/transform/Flatten.scala
@@ -18,18 +18,14 @@ abstract class Flatten extends InfoTransform {
   /** the following two members override abstract members in Transform */
   val phaseName: String = "flatten"
 
-  /** Updates the owning scope with the given symbol; returns the old symbol.
+  /** Updates the owning scope with the given symbol, unlinking any others.
    */
-  private def replaceSymbolInCurrentScope(sym: Symbol): Symbol = exitingFlatten {
+  private def replaceSymbolInCurrentScope(sym: Symbol): Unit = exitingFlatten {
     val scope = sym.owner.info.decls
-    val old   = scope lookup sym.name andAlso scope.unlink
+    val old   = (scope lookupUnshadowedEntries sym.name).toList
+    old foreach (scope unlink _)
     scope enter sym
-
-    if (old eq NoSymbol)
-      log(s"lifted ${sym.fullLocationString}")
-    else
-      log(s"lifted ${sym.fullLocationString} after unlinking existing $old from scope.")
-
+    log(s"lifted ${sym.fullLocationString}" + ( if (old.isEmpty) "" else " after unlinking $old from scope." ))
     old
   }
 
diff --git a/src/compiler/scala/tools/nsc/typechecker/Contexts.scala b/src/compiler/scala/tools/nsc/typechecker/Contexts.scala
index 92e2bc186e..03d30a6029 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Contexts.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Contexts.scala
@@ -758,14 +758,10 @@ trait Contexts { self: Analyzer =>
     def lookupSymbol(name: Name, qualifies: Symbol => Boolean): NameLookup = {
       var lookupError: NameLookup  = null       // set to non-null if a definite error is encountered
       var inaccessible: NameLookup = null       // records inaccessible symbol for error reporting in case none is found
-      var defEntry: ScopeEntry     = null       // the scope entry of defSym, if defined in a local scope
       var defSym: Symbol           = NoSymbol   // the directly found symbol
       var pre: Type                = NoPrefix   // the prefix type of defSym, if a class member
-      var cx: Context              = this
-      var needsQualifier           = false      // working around package object overloading bug
-
-      def defEntrySymbol  = if (defEntry eq null) NoSymbol else defEntry.sym
-      def localScopeDepth = if (defEntry eq null) 0 else cx.scope.nestingLevel - defEntry.owner.nestingLevel
+      var cx: Context              = this       // the context under consideration
+      var symbolDepth: Int         = -1         // the depth of the directly found symbol
 
       def finish(qual: Tree, sym: Symbol): NameLookup = (
         if (lookupError ne null) lookupError
@@ -781,7 +777,7 @@ trait Contexts { self: Analyzer =>
           || unit.exists && s.sourceFile != unit.source.file
         )
       )
-      def requiresQualifier(s: Symbol) = needsQualifier || (
+      def requiresQualifier(s: Symbol) = (
            s.owner.isClass
         && !s.owner.isPackageClass
         && !s.isTypeParameterOrSkolem
@@ -789,23 +785,6 @@ trait Contexts { self: Analyzer =>
       def lookupInPrefix(name: Name)    = pre member name filter qualifies
       def accessibleInPrefix(s: Symbol) = isAccessible(s, pre, superAccess = false)
 
-      def correctForPackageObject(sym: Symbol): Symbol = {
-        if (sym.isTerm && isInPackageObject(sym, pre.typeSymbol)) {
-          val sym1 = lookupInPrefix(sym.name)
-          if ((sym1 eq NoSymbol) || (sym eq sym1)) sym else {
-            needsQualifier = true
-            log(s"""
-              |  !!! Overloaded package object member resolved incorrectly.
-              |        prefix: $pre
-              |     Discarded: ${sym.defString}
-              |         Using: ${sym1.defString}
-              """.stripMargin)
-            sym1
-          }
-        }
-        else sym
-      }
-
       def searchPrefix = {
         cx = cx.enclClass
         val found0 = lookupInPrefix(name)
@@ -817,31 +796,35 @@ trait Contexts { self: Analyzer =>
       }
       // cx.scope eq null arises during FixInvalidSyms in Duplicators
       while (defSym == NoSymbol && (cx ne NoContext) && (cx.scope ne null)) {
-        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
-        defSym   = defEntrySymbol filter qualifies map correctForPackageObject orElse searchPrefix
+        pre         = cx.enclClass.prefix
+        val entries = (cx.scope lookupUnshadowedEntries name filter (e => qualifies(e.sym))).toList
+        defSym      = entries match {
+          case Nil       => searchPrefix
+          case hd :: tl  =>
+            // we have a winner: record the symbol depth
+            symbolDepth = (cx.depth - cx.scope.nestingLevel) + hd.depth
+            if (tl.isEmpty) hd.sym
+            else logResult(s"!!! lookup overloaded")(cx.owner.newOverloaded(pre, entries map (_.sym)))
+        }
         if (!defSym.exists)
-          cx = cx.outer
+          cx = cx.outer // push further outward
       }
+      if (symbolDepth < 0)
+        symbolDepth = cx.depth
 
-      val symbolDepth    = cx.depth - localScopeDepth
       var impSym: Symbol = NoSymbol
-      var imports        = Context.this.imports   // impSym != NoSymbol => it is imported from imports.head
+      var imports        = Context.this.imports
       def imp1           = imports.head
+      def imp2           = imports.tail.head
+      def imp1Explicit   = imp1 isExplicitImport name
+      def imp2Explicit   = imp2 isExplicitImport name
 
       while (!qualifies(impSym) && imports.nonEmpty && imp1.depth > symbolDepth) {
         impSym = importedAccessibleSymbol(imp1, name)
         if (!impSym.exists)
           imports = imports.tail
       }
+
       if (defSym.exists && impSym.exists) {
         // imported symbols take precedence over package-owned symbols in different compilation units.
         if (isPackageOwnedInDifferentUnit(defSym))
@@ -862,40 +845,33 @@ trait Contexts { self: Analyzer =>
           finish(EmptyTree, defSym)
       }
       else if (impSym.exists) {
-        // Imports against which we will test impSym for any ambiguities
-        var importsTail  = imports.tail
-        val imp1Explicit = imp1 isExplicitImport name
-        def imp2         = importsTail.head
-        def sameDepth    = imp1.depth == imp2.depth
-        def isDone       = importsTail.isEmpty || imp1Explicit && !sameDepth
-
+        def sameDepth  = imp1.depth == imp2.depth
+        def needsCheck = if (sameDepth) imp1Explicit == imp2Explicit else imp1Explicit || imp2Explicit
+        def isDone     = imports.tail.isEmpty || (!sameDepth && imp1Explicit)
+        def ambiguous  = needsCheck && isAmbiguousImport(imp1, imp2, name) && {
+          lookupError = ambiguousImports(imp1, imp2)
+          true
+        }
+        // Ambiguity check between imports.
+        // The same name imported again is potentially ambiguous if the name is:
+        //  - after explicit import, explicitly imported again at the same or lower depth
+        //  - after explicit import, wildcard imported at lower depth
+        //  - after wildcard import, wildcard imported at the same depth
+        // Under all such conditions isAmbiguousImport is called, which will
+        // examine the imports in case they are importing the same thing; if that
+        // can't be established conclusively, an error is issued.
         while (lookupError == null && !isDone) {
           val other = importedAccessibleSymbol(imp2, name)
-          // Ambiguity check between imports.
-          // The same name imported again is potentially ambiguous if the name is:
-          //  - after explicit import, explicitly imported again at the same or lower depth
-          //  - after explicit import, wildcard imported at lower depth
-          //  - after wildcard import, wildcard imported at the same depth
-          // Under all such conditions isAmbiguousImport is called, which will
-          // examine the imports in case they are importing the same thing; if that
-          // can't be established conclusively, an error is issued.
-          if (qualifies(other)) {
-            val imp2Explicit = imp2 isExplicitImport name
-            val needsCheck = (
-              if (sameDepth) imp1Explicit == imp2Explicit
-              else imp1Explicit || imp2Explicit
-            )
-            log(s"Import ambiguity: imp1=$imp1, imp2=$imp2, sameDepth=$sameDepth, needsCheck=$needsCheck")
-            if (needsCheck && isAmbiguousImport(imp1, imp2, name))
-              lookupError = ambiguousImports(imp1, imp2)
-            else if (imp2Explicit) {
-              // if we weren't ambiguous and imp2 is explicit, imp2 replaces imp1
-              // as the current winner.
-              impSym  = other
-              imports = importsTail
-            }
+          // if the competing import is unambiguous and explicit, it is the new winner.
+          val isNewWinner = qualifies(other) && !ambiguous && imp2Explicit
+          // imports is imp1 :: imp2 :: rest.
+          // If there is a new winner, it is imp2, and imports drops imp1.
+          // If there is not, imp1 is still the winner, and it drops imp2.
+          if (isNewWinner) {
+            impSym = other
+            imports = imports.tail
           }
-          importsTail = importsTail.tail
+          else imports = imp1 :: imports.tail.tail
         }
         // optimization: don't write out package prefixes
         finish(resetPos(imp1.qual.duplicate), impSym)
diff --git a/src/compiler/scala/tools/nsc/typechecker/Namers.scala b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
index 5e537e3bb3..3bf7976241 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Namers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Namers.scala
@@ -380,8 +380,8 @@ trait Namers extends MethodSynthesis {
       if (sym eq NoSymbol) return
 
       val ctx    = if (context.owner.isPackageObjectClass) context.outer else context
-      val module = if (sym.isModule) sym else ctx.scope lookup tree.name.toTermName
-      val clazz  = if (sym.isClass) sym else ctx.scope lookup tree.name.toTypeName
+      val module = if (sym.isModule) sym else ctx.scope lookupModule tree.name
+      val clazz  = if (sym.isClass) sym else ctx.scope lookupClass tree.name
       val fails  = (
            module.isModule
         && clazz.isClass
diff --git a/src/reflect/scala/reflect/internal/Scopes.scala b/src/reflect/scala/reflect/internal/Scopes.scala
index a4b541e34d..352bc8fdc9 100644
--- a/src/reflect/scala/reflect/internal/Scopes.scala
+++ b/src/reflect/scala/reflect/internal/Scopes.scala
@@ -25,8 +25,9 @@ trait Scopes extends api.Scopes { self: SymbolTable =>
      */
     var next: ScopeEntry = null
 
+    def depth = owner.nestingLevel
     override def hashCode(): Int = sym.name.start
-    override def toString(): String = sym.toString()
+    override def toString() = s"$sym (depth=$depth)"
   }
 
   /**
@@ -216,14 +217,46 @@ trait Scopes extends api.Scopes { self: SymbolTable =>
       }
     }
 
-    /** lookup a symbol
-     *
-     *  @param name ...
-     *  @return     ...
+    /** Lookup a module or a class, filtering out matching names in scope
+     *  which do not match that requirement.
+     */
+    def lookupModule(name: Name): Symbol = lookupAll(name.toTermName) find (_.isModule) getOrElse NoSymbol
+    def lookupClass(name: Name): Symbol  = lookupAll(name.toTypeName) find (_.isClass) getOrElse NoSymbol
+
+    /** True if the name exists in this scope, false otherwise. */
+    def containsName(name: Name) = lookupEntry(name) != null
+
+    /** Lookup a symbol.
      */
     def lookup(name: Name): Symbol = {
       val e = lookupEntry(name)
-      if (e eq null) NoSymbol else e.sym
+      if (e eq null) NoSymbol
+      else if (lookupNextEntry(e) eq null) e.sym
+      else {
+        // We shouldn't get here: until now this method was picking a random
+        // symbol when there was more than one with the name, so this should
+        // only be called knowing that there are 0-1 symbols of interest. So, we
+        // can safely return an overloaded symbol rather than throwing away the
+        // rest of them. Most likely we still break, but at least we will break
+        // in an understandable fashion (unexpectedly overloaded symbol) rather
+        // than a non-deterministic bizarre one (see any bug involving overloads
+        // in package objects.)
+        val alts = lookupAll(name).toList
+        log("!!! scope lookup of $name found multiple symbols: $alts")
+        // FIXME - how is one supposed to create an overloaded symbol without
+        // knowing the correct owner? Using the symbol owner is not correct;
+        // say for instance this is List's scope and the symbols are its three
+        // mkString members. Those symbols are owned by TraversableLike, which
+        // is no more meaningful an owner than NoSymbol given that we're in
+        // List. Maybe it makes no difference who owns the overloaded symbol, in
+        // which case let's establish that and have a canonical creation method.
+        //
+        // FIXME - a similar question for prefix, although there are more
+        // clues from the symbols on that one, as implemented here. In general
+        // the distinct list is one type and lub becomes the identity.
+        val prefix = lub(alts map (_.info.prefix) distinct)
+        NoSymbol.newOverloaded(prefix, alts)
+      }
     }
 
     /** Returns an iterator yielding every symbol with given name in this scope.
@@ -231,7 +264,20 @@ trait Scopes extends api.Scopes { self: SymbolTable =>
     def lookupAll(name: Name): Iterator[Symbol] = new Iterator[Symbol] {
       var e = lookupEntry(name)
       def hasNext: Boolean = e ne null
-      def next(): Symbol = { val r = e.sym; e = lookupNextEntry(e); r }
+      def next(): Symbol = try e.sym finally e = lookupNextEntry(e)
+    }
+
+    def lookupAllEntries(name: Name): Iterator[ScopeEntry] = new Iterator[ScopeEntry] {
+      var e = lookupEntry(name)
+      def hasNext: Boolean = e ne null
+      def next(): ScopeEntry = try e finally e = lookupNextEntry(e)
+    }
+
+    def lookupUnshadowedEntries(name: Name): Iterator[ScopeEntry] = {
+      lookupEntry(name) match {
+        case null => Iterator.empty
+        case e    => lookupAllEntries(name) filter (e1 => (e eq e1) || (e.depth == e1.depth && e.sym != e1.sym))
+      }
     }
 
     /** lookup a symbol entry matching given name.
diff --git a/src/reflect/scala/reflect/internal/Symbols.scala b/src/reflect/scala/reflect/internal/Symbols.scala
index 8cebfabe6f..53a236fa3c 100644
--- a/src/reflect/scala/reflect/internal/Symbols.scala
+++ b/src/reflect/scala/reflect/internal/Symbols.scala
@@ -1672,12 +1672,23 @@ trait Symbols extends api.Symbols { self: SymbolTable =>
 
     def filter(cond: Symbol => Boolean): Symbol =
       if (isOverloaded) {
-        val alts = alternatives
-        val alts1 = alts filter cond
-        if (alts1 eq alts) this
+        var changed = false
+        var alts0: List[Symbol] = alternatives
+        var alts1: List[Symbol] = Nil
+
+        while (alts0.nonEmpty) {
+          if (cond(alts0.head))
+            alts1 ::= alts0.head
+          else
+            changed = true
+
+          alts0 = alts0.tail
+        }
+
+        if (!changed) this
         else if (alts1.isEmpty) NoSymbol
         else if (alts1.tail.isEmpty) alts1.head
-        else owner.newOverloaded(info.prefix, alts1)
+        else owner.newOverloaded(info.prefix, alts1.reverse)
       }
       else if (cond(this)) this
       else NoSymbol
diff --git a/test/files/neg/dbldef.check b/test/files/neg/dbldef.check
index 3ee63475e4..b896c4cdcf 100644
--- a/test/files/neg/dbldef.check
+++ b/test/files/neg/dbldef.check
@@ -6,9 +6,7 @@ dbldef.scala:1: error: type mismatch;
  required: Int
 case class test0(x: Int, x: Float)
                  ^
-dbldef.scala:1: error: type mismatch;
- found   : Float
- required: Int
+dbldef.scala:1: error: in class test0, multiple overloaded alternatives of x define default arguments
 case class test0(x: Int, x: Float)
            ^
 three errors found