Refactor resolveUsage, cache Precedence for correctness

compiler/src/dotty/tools/dotc/transform/CheckUnused.scala

@@ -63,13 +63,13 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
       resolveUsage(tree.tpe.classSymbol, tree.name, tree.tpe.importPrefix.skipPackageObject)
     tree
 
-  // import x.y; x may be rewritten x.y, also import x.z as y
+  // import x.y; y may be rewritten x.y, also import x.z as y
   override def transformSelect(tree: Select)(using Context): tree.type =
     val name = tree.removeAttachment(OriginalName).getOrElse(nme.NO_NAME)
     if tree.span.isSynthetic && tree.symbol == defn.TypeTest_unapply then
       tree.qualifier.tpe.underlying.finalResultType match
-      case AppliedType(_, args) => // if tycon.typeSymbol == defn.TypeTestClass
-        val res = args(1)
+      case AppliedType(_, args) => // tycon.typeSymbol == defn.TypeTestClass
+        val res = args(1) // T in TypeTest[-S, T]
         val target = res.dealias.typeSymbol
         resolveUsage(target, target.name, res.importPrefix.skipPackageObject) // case _: T =>
       case _ =>
@@ -156,7 +156,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
         case Select(t, _) => loop(t)
         case _            =>
       tree.getAttachment(OriginalTypeClass).foreach(loop)
-    if tree.symbol.isAllOf(Deferred | Given | HasDefault) then
+    if tree.symbol.isAllOf(DeferredGivenFlags) then
       resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix)
     tree
 
@@ -173,7 +173,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
     traverseAnnotations(tree.symbol)
     if tree.symbol.is(Inline) then
       refInfos.inliners -= 1
-    if tree.symbol.isAllOf(Deferred | Given | HasDefault) then
+    if tree.symbol.isAllOf(DeferredGivenFlags) then
       resolveUsage(defn.Compiletime_deferred, nme.NO_NAME, NoPrefix)
     tree
 
@@ -260,30 +260,37 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
       refInfos.refs.addOne(sym)
 
   /** Look up a reference in enclosing contexts to determine whether it was introduced by a definition or import.
-   *  The binding of highest precedence must be correct.
+   *  The binding of highest precedence must then be correct.
+   *
+   *  Unqualified locals and fully qualified globals are neither imported nor in scope;
+   *  e.g., in `scala.Int`, `scala` is in scope for typer, but here we reverse-engineer the attribution.
+   *  For Select, lint does not look up `<empty>.scala` (so top-level syms look like magic) but records `scala.Int`.
+   *  For Ident, look-up finds the root import as usual. A competing import is OK because higher precedence.
    */
   def resolveUsage(sym: Symbol, name: Name, prefix: Type)(using Context): Unit =
+    import PrecedenceLevels.*
+
     def matchingSelector(info: ImportInfo): ImportSelector | Null =
       val qtpe = info.site
-      def loop(sels: List[ImportSelector]): ImportSelector | Null =
-        sels match
-        case sel :: sels if sel.isWildcard =>
-          // the qualifier must have the target symbol as a member
-          val matches = qtpe.member(sym.name).hasAltWith(_.symbol == sym)
-            && {
-              if sel.isGiven then // Further check that the symbol is a given or implicit and conforms to the bound
-                sym.isOneOf(Given | Implicit)
+      def hasAltMember(nm: Name) = qtpe.member(nm).hasAltWith(_.symbol == sym)
+      def loop(sels: List[ImportSelector]): ImportSelector | Null = sels match
+        case sel :: sels =>
+          val matches =
+            if sel.isWildcard then
+              // the qualifier must have the target symbol as a member
+              hasAltMember(sym.name) && {
+                if sel.isGiven then // Further check that the symbol is a given or implicit and conforms to the bound
+                     sym.isOneOf(GivenOrImplicit)
                   && (sel.bound.isEmpty || sym.info.finalResultType <:< sel.boundTpe)
                   && (prefix.eq(NoPrefix) || qtpe =:= prefix)
-              else
-                !sym.is(Given) // Normal wildcard, check that the symbol is not a given (but can be implicit)
-            }
-          if matches then sel else loop(sels)
-        case sel :: sels =>
-          def hasAltMember(nm: Name) = qtpe.member(nm).hasAltWith(_.symbol == sym)
-          // if there is an explicit name, it must match
-          val matches = !name.exists(_.toTermName != sel.rename) &&
-            (prefix.eq(NoPrefix) || qtpe =:= prefix) && (hasAltMember(sel.name) || hasAltMember(sel.name.toTypeName))
+                else
+                  !sym.is(Given) // Normal wildcard, check that the symbol is not a given (but can be implicit)
+              }
+            else
+              // if there is an explicit name, it must match
+                 !name.exists(_.toTermName != sel.rename)
+              && (prefix.eq(NoPrefix) || qtpe =:= prefix)
+              && (hasAltMember(sel.name) || hasAltMember(sel.name.toTypeName))
           if matches then sel else loop(sels)
         case nil => null
       loop(info.selectors)
@@ -293,28 +300,17 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
       && ctxsym.thisType.baseClasses.contains(sym.owner)
       && ctxsym.thisType.member(sym.name).hasAltWith(d => d.containsSym(sym) && !name.exists(_ != d.name))
 
-    def addCached(where: Context): Unit =
+    // Attempt to cache a result at the given context. Not all contexts bear a cache, including NoContext.
+    // If there is already any result for the name and prefix, do nothing.
+    def addCached(where: Context, result: Precedence): Unit =
       if where.moreProperties ne null then
         where.property(resolvedKey) match
-        case Some(res) =>
-          val np = (name, prefix)
-          res.seen.updateWith(sym):
-            case svs @ Some(vs) => if vs.exists((n, p) => n == name && p =:= prefix) then svs else Some(np :: vs)
-            case _ => Some(np :: Nil)
-        case _ =>
-
-    if !sym.exists then return
-
-    // Names are resolved by definitions and imports, which have four precedence levels:
-    object PrecedenceLevels:
-      opaque type Precedence = Int
-      inline def NoPrecedence: Precedence = 5
-      inline def OtherUnit: Precedence = 4   // root import or def from another compilation unit via enclosing package
-      inline def Wildcard: Precedence = 3    // wildcard import
-      inline def NamedImport: Precedence = 2 // specific import
-      inline def Definition: Precedence = 1  // def from this compilation unit
-      extension (p: Precedence) inline def weakerThan(q: Precedence) = p > q
-    import PrecedenceLevels.*
+        case Some(resolved) =>
+          resolved.record(sym, name, prefix, result)
+        case none =>
+
+    // Avoid spurious NoSymbol and also primary ctors which are never warned about.
+    if !sym.exists || sym.isPrimaryConstructor then return
 
     // Find the innermost, highest precedence. Contexts have no nesting levels but assume correctness.
     // If the sym is an enclosing definition (the owner of a context), it does not count toward usages.
@@ -329,21 +325,28 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
     while !done && ctxs.hasNext do
       val cur = ctxs.next()
       if cur.owner eq sym then
+        addCached(cachePoint, Definition)
         return // found enclosing definition
       else if isLocal then
         if cur.owner eq sym.owner then
           done = true // for local def, just checking that it is not enclosing
       else
-        cached =
+        val cachedPrecedence =
           cur.property(resolvedKey) match
-          case Some(res) =>
+          case Some(resolved) =>
             // conservative, cache must be nested below the result context
-            if precedence == NoPrecedence then
-              cachePoint = cur
-            res.saw(sym, name, prefix)
-          case _ => false
+            if precedence.isNone then
+              cachePoint = cur // no result yet, and future result could be cached here
+            resolved.hasRecord(sym, name, prefix)
+          case none => NoPrecedence
+        cached = !cachedPrecedence.isNone
         if cached then
-          candidate = cur
+          // if prefer cached precedence, then discard previous result
+          if precedence.weakerThan(cachedPrecedence) then
+            candidate = NoContext
+            importer = null
+            cachePoint = cur // actual cache context
+            precedence = cachedPrecedence // actual cached precedence
           done = true
         else if cur.isImportContext then
           val sel = matchingSelector(cur.importInfo.nn)
@@ -368,7 +371,7 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
           if sym.srcPos.sourcePos.source == ctx.source then
             precedence = Definition
             candidate = cur
-            importer = null
+            importer = null // ignore import in same scope; we can't check nesting level
             done = true
           else if precedence.weakerThan(OtherUnit) then
             precedence = OtherUnit
@@ -378,10 +381,13 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
     refInfos.refs.addOne(sym)
     if candidate != NoContext && candidate.isImportContext && importer != null then
       refInfos.sels.put(importer, ())
-    if !cached then
-      addCached(cachePoint)
+    // possibly record that we have performed this look-up
+    // if no result was found, take it as Definition (local or rooted head of fully qualified path)
+    val adjusted = if precedence.isNone then Definition else precedence
+    if !cached && (cachePoint ne NoContext) then
+      addCached(cachePoint, adjusted)
     if cachePoint ne ctx then
-      addCached(ctx)
+      addCached(ctx, adjusted) // at this ctx, since cachePoint may be far up the outer chain
   end resolveUsage
 end CheckUnused
 
@@ -450,11 +456,35 @@ object CheckUnused:
     var inliners = 0 // depth of inline def (not inlined yet)
   end RefInfos
 
-  // Symbols already resolved in the given Context (with name and prefix of lookup)
+  // Symbols already resolved in the given Context (with name and prefix of lookup).
   class Resolved:
-    val seen = mutable.Map.empty[Symbol, List[(Name, Type)]].withDefaultValue(Nil)
-    def saw(sym: Symbol, name: Name, pre: Type)(using Context): Boolean =
-      seen(sym).exists((n, p) => n == name && p =:= pre)
+    import PrecedenceLevels.*
+    private val seen = mutable.Map.empty[Symbol, List[(Name, Type, Precedence)]].withDefaultValue(Nil)
+    // if a result has been recorded, return it; otherwise, NoPrecedence.
+    def hasRecord(symbol: Symbol, name: Name, prefix: Type)(using Context): Precedence =
+      seen(symbol).find((n, p, _) => n == name && p =:= prefix) match
+      case Some((_, _, r)) => r
+      case none => NoPrecedence
+    // "record" the look-up result, if there is not already a result for the name and prefix.
+    def record(symbol: Symbol, name: Name, prefix: Type, result: Precedence)(using Context): Unit =
+      require(NoPrecedence.weakerThan(result))
+      seen.updateWith(symbol):
+        case svs @ Some(vs) =>
+          if vs.exists((n, p, _) => n == name && p =:= prefix) then svs
+          else Some((name, prefix, result) :: vs)
+        case none => Some((name, prefix, result) :: Nil)
+
+  // Names are resolved by definitions and imports, which have four precedence levels:
+  object PrecedenceLevels:
+    opaque type Precedence = Int
+    inline def NoPrecedence: Precedence = 5
+    inline def OtherUnit: Precedence = 4   // root import or def from another compilation unit via enclosing package
+    inline def Wildcard: Precedence = 3    // wildcard import
+    inline def NamedImport: Precedence = 2 // specific import
+    inline def Definition: Precedence = 1  // def from this compilation unit
+    extension (p: Precedence)
+      inline def weakerThan(q: Precedence): Boolean = p > q
+      inline def isNone: Boolean = p == NoPrecedence
 
   def reportUnused()(using Context): Unit =
     for (msg, pos, origin) <- warnings do
@@ -479,7 +509,7 @@ object CheckUnused:
 
     def checkPrivate(sym: Symbol, pos: SrcPos) =
       if ctx.settings.WunusedHas.privates
-        && !sym.isConstructor
+        && !sym.isPrimaryConstructor
         && sym.is(Private, butNot = SelfName | Synthetic | CaseAccessor)
         && !sym.isSerializationSupport
         && !(sym.is(Mutable) && sym.isSetter && sym.owner.is(Trait)) // tracks sym.underlyingSymbol sibling getter

tests/warn/i15503i.scala

@@ -319,3 +319,52 @@ package foo.test.i17117:
       val test = t1.test
     }
   }
+
+// manual testing of cached look-ups
+package deeply:
+  object Deep:
+    def f(): Unit =
+      def g(): Unit =
+        def h(): Unit =
+          println(Deep)
+          println(Deep)
+          println(Deep)
+        h()
+      g()
+    override def toString = "man, that is deep!"
+/* result cache saves before context traversal and import/member look-up
+CHK object Deep
+recur * 10 = context depth is 10 between reference and definition
+CHK method println
+recur = was 19 at predef where max root is 21
+CHK object Deep
+recur = cached result
+CHK method println
+recur
+CHK object Deep
+recur
+*/
+
+package constructors:
+  class C private (i: Int): // warn param
+    def this() = this(27)
+    private def this(s: String) = this(s.toInt) // warn ctor
+    def c = new C(42)
+    private def f(i: Int) = i // warn overloaded member
+    private def f(s: String) = s
+    def g = f("hello") // use one of overloaded member
+
+  class D private (i: Int):
+    private def this() = this(42) // no warn used by companion
+    def d = i
+  object D:
+    def apply(): D = new D()
+
+package reversed: // reverse-engineered
+  class C:
+    def c: scala.Int = 42 // Int marked used; lint does not look up <empty>.scala
+  class D:
+    def d: Int = 27 // Int is found in root import scala.*
+  class E:
+    import scala.* // no warn because root import (which is cached! by previous lookup) is lower precedence
+    def e: Int = 27

tests/warn/i16639a.scala

@@ -1,7 +1,7 @@
 //> using options  -Wunused:all -source:3.3
 
 class Bippy(a: Int, b: Int) {
-  private def this(c: Int) = this(c, c)
+  private def this(c: Int) = this(c, c) // warn
   private def boop(x: Int)            = x+a+b     // warn
     private def bippy(x: Int): Int      = bippy(x)  // warn TODO: could warn
   final private val MILLIS1           = 2000      // warn no warn, /Dotty:Warn

tests/warn/unused-privates.scala

@@ -2,7 +2,7 @@
 //> using options -deprecation -Wunused:privates,locals
 //
 class Bippy(a: Int, b: Int) {
-  private def this(c: Int) = this(c, c)           // NO warn
+  private def this(c: Int) = this(c, c)           // warn (DO warn, was NO)
   private def bippy(x: Int): Int      = bippy(x)  // warn
   private def boop(x: Int)            = x+a+b     // warn
   final private val MILLIS1           = 2000      // warn, scala2: no warn, might have been inlined