Merge pull request #606 from scala/backport-lts-3.3-23757

Backport "Warn unused masking imports, remove obsolete unused options" to 3.3 LTS

Author: tgodzik

compiler/src/dotty/tools/dotc/ast/untpd.scala

@@ -138,6 +138,7 @@ object untpd extends Trees.Instance[Untyped] with UntypedTreeInfo {
       case Ident(rename: TermName) => rename
       case _ => name
 
+    /** It's a masking import if `!isWildcard`. */
     def isUnimport = rename == nme.WILDCARD
   }
 

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

@@ -193,12 +193,6 @@ private sealed trait WarningSettings:
       ChoiceWithHelp("patvars","Warn if a variable bound in a pattern is unused"),
       //ChoiceWithHelp("inlined", "Apply -Wunused to inlined expansions"), // TODO
       ChoiceWithHelp("linted", "Enable -Wunused:imports,privates,locals,implicits"),
-      ChoiceWithHelp(
-        name = "strict-no-implicit-warn",
-        description = """Same as -Wunused:imports, only for imports of explicit named members.
-                        |NOTE : This overrides -Wunused:imports and NOT set by -Wunused:all""".stripMargin
-      ),
-      ChoiceWithHelp("unsafe-warn-patvars", "Deprecated alias for `patvars`"),
     ),
     default = Nil
   )

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

@@ -7,9 +7,10 @@ import dotty.tools.dotc.config.ScalaSettings
 import dotty.tools.dotc.core.Contexts.*
 import dotty.tools.dotc.core.Flags.*
 import dotty.tools.dotc.core.Names.{Name, SimpleName, DerivedName, TermName, termName}
-import dotty.tools.dotc.core.NameOps.{isAnonymousFunctionName, isReplWrapperName, isContextFunction, setterName}
 import dotty.tools.dotc.core.NameKinds.{
   BodyRetainerName, ContextBoundParamName, ContextFunctionParamName, DefaultGetterName, WildcardParamName}
+import dotty.tools.dotc.core.NameOps.{isAnonymousFunctionName, isReplWrapperName, isContextFunction, setterName}
+import dotty.tools.dotc.core.Scopes.newScope
 import dotty.tools.dotc.core.StdNames.nme
 import dotty.tools.dotc.core.Symbols.{ClassSymbol, NoSymbol, Symbol, defn, isDeprecated, requiredClass, requiredModule}
 import dotty.tools.dotc.core.Types.*
@@ -19,6 +20,7 @@ import dotty.tools.dotc.rewrites.Rewrites
 import dotty.tools.dotc.transform.MegaPhase.MiniPhase
 import dotty.tools.dotc.typer.{ImportInfo, Typer}
 import dotty.tools.dotc.typer.Deriving.OriginalTypeClass
+import dotty.tools.dotc.typer.Implicits.{ContextualImplicits, RenamedImplicitRef}
 import dotty.tools.dotc.util.{Property, Spans, SrcPos}, Spans.Span
 import dotty.tools.dotc.util.Chars.{isLineBreakChar, isWhitespace}
 import dotty.tools.dotc.util.chaining.*
@@ -115,6 +117,14 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
           args.foreach(_.withAttachment(ForArtifact, ()))
       case _ =>
     ctx
+  override def transformApply(tree: Apply)(using Context): tree.type =
+    // check for multiversal equals
+    tree match
+    case Apply(Select(left, nme.Equals | nme.NotEquals), right :: Nil) =>
+      val caneq = defn.CanEqualClass.typeRef.appliedTo(left.tpe.widen :: right.tpe.widen :: Nil)
+      resolveScoped(caneq)
+    case _ =>
+    tree
 
   override def prepareForAssign(tree: Assign)(using Context): Context =
     tree.lhs.putAttachment(AssignmentTarget, ()) // don't take LHS reference as a read
@@ -195,6 +205,16 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
       refInfos.register(tree)
     tree
 
+  override def prepareForStats(trees: List[Tree])(using Context): Context =
+    // gather local implicits while ye may
+    if !ctx.owner.isClass then
+      if trees.exists(t => t.isDef && t.symbol.is(Given) && t.symbol.isLocalToBlock) then
+        val scope = newScope.openForMutations
+        for tree <- trees if tree.isDef && tree.symbol.is(Given) do
+          scope.enter(tree.symbol.name, tree.symbol)
+        return ctx.fresh.setScope(scope)
+    ctx
+
   override def transformOther(tree: Tree)(using Context): tree.type =
     tree match
     case imp: Import =>
@@ -277,6 +297,8 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
              alt.symbol == sym
           || nm.isTypeName && alt.symbol.isAliasType && alt.info.dealias.typeSymbol == sym
         sameSym && alt.symbol.isAccessibleFrom(qtpe)
+      def hasAltMemberNamed(nm: Name) = qtpe.member(nm).hasAltWith(_.symbol.isAccessibleFrom(qtpe))
+
       def loop(sels: List[ImportSelector]): ImportSelector | Null = sels match
         case sel :: sels =>
           val matches =
@@ -293,9 +315,17 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
                 else
                   !sym.is(Given) // Normal wildcard, check that the symbol is not a given (but can be implicit)
               }
+            else if sel.isUnimport then
+              val masksMatchingMember =
+                   name != nme.NO_NAME
+                && sels.exists(x => x.isWildcard && !x.isGiven)
+                && !name.exists(_.toTermName != sel.name) // import a.b as _, b must match name
+                && (hasAltMemberNamed(sel.name) || hasAltMemberNamed(sel.name.toTypeName))
+              if masksMatchingMember then
+                refInfos.sels.put(sel, ()) // imprecise due to precedence but errs on the side of false negative
+              false
             else
-              // if there is an explicit name, it must match
-                 !name.exists(_.toTermName != sel.rename)
+                 !name.exists(_.toTermName != sel.rename) // if there is an explicit name, it must match
               && (prefix.eq(NoPrefix) || qtpe =:= prefix)
               && (hasAltMember(sel.name) || hasAltMember(sel.name.toTypeName))
           if matches then sel else loop(sels)
@@ -363,6 +393,38 @@ class CheckUnused private (phaseMode: PhaseMode, suffix: String) extends MiniPha
     if candidate != NoContext && candidate.isImportContext && importer != null then
       refInfos.sels.put(importer, ())
   end resolveUsage
+
+  /** Simulate implicit search for contextual implicits in lexical scope and mark any definitions or imports as used.
+   *  Avoid cached ctx.implicits because it needs the precise import context that introduces the given.
+   */
+  def resolveScoped(tp: Type)(using Context): Unit =
+    var done = false
+    val ctxs = ctx.outersIterator
+    while !done && ctxs.hasNext do
+      val cur = ctxs.next()
+      val implicitRefs: List[ImplicitRef] =
+        if (cur.isClassDefContext) cur.owner.thisType.implicitMembers
+        else if (cur.isImportContext) cur.importInfo.nn.importedImplicits
+        else if (cur.isNonEmptyScopeContext) cur.scope.implicitDecls
+        else Nil
+      implicitRefs.find(ref => ref.underlyingRef.widen <:< tp) match
+      case Some(found: TermRef) =>
+        refInfos.addRef(found.denot.symbol)
+        if cur.isImportContext then
+          cur.importInfo.nn.selectors.find(sel => sel.isGiven || sel.rename == found.name) match
+          case Some(sel) =>
+            refInfos.sels.put(sel, ())
+          case _ =>
+        return
+      case Some(found: RenamedImplicitRef) if cur.isImportContext =>
+        refInfos.addRef(found.underlyingRef.denot.symbol)
+        cur.importInfo.nn.selectors.find(sel => sel.rename == found.implicitName) match
+        case Some(sel) =>
+          refInfos.sels.put(sel, ())
+        case _ =>
+        return
+      case _ =>
+  end resolveScoped
 end CheckUnused
 
 object CheckUnused:
@@ -568,7 +630,6 @@ object CheckUnused:
         || m.is(Synthetic)
         || m.hasAnnotation(dd.UnusedAnnot)          // param of unused method
         || sym.owner.name.isContextFunction         // a ubiquitous parameter
-        || sym.isCanEqual
         || sym.info.dealias.typeSymbol.match        // more ubiquity
            case dd.DummyImplicitClass | dd.SubTypeClass | dd.SameTypeClass => true
            case tps =>
@@ -600,7 +661,6 @@ object CheckUnused:
     def checkLocal(sym: Symbol, pos: SrcPos) =
       if ctx.settings.WunusedHas.locals
         && !sym.is(InlineProxy)
-        && !sym.isCanEqual
       then
         if sym.is(Mutable) && infos.asss(sym) then
           warnAt(pos)(UnusedSymbol.localVars)
@@ -628,12 +688,10 @@ object CheckUnused:
               warnAt(pos)(UnusedSymbol.unsetPrivates)
 
     def checkImports() =
-      // TODO check for unused masking import
       import scala.jdk.CollectionConverters.given
       import Rewrites.ActionPatch
       type ImpSel = (Import, ImportSelector)
-      def isUsable(imp: Import, sel: ImportSelector): Boolean =
-        sel.isImportExclusion || infos.sels.containsKey(sel) || imp.isLoose(sel)
+      def isUsed(sel: ImportSelector): Boolean = infos.sels.containsKey(sel)
       def warnImport(warnable: ImpSel, actions: List[CodeAction] = Nil): Unit =
         val (imp, sel) = warnable
         val msg = UnusedSymbol.imports(actions)
@@ -642,7 +700,7 @@ object CheckUnused:
         warnAt(sel.srcPos)(msg, origin)
 
       if !actionable then
-        for imp <- infos.imps.keySet.nn.asScala; sel <- imp.selectors if !isUsable(imp, sel) do
+        for imp <- infos.imps.keySet.nn.asScala; sel <- imp.selectors if !isUsed(sel) do
           warnImport(imp -> sel)
       else
         // If the rest of the line is blank, include it in the final edit position. (Delete trailing whitespace.)
@@ -697,7 +755,7 @@ object CheckUnused:
         while index < sortedImps.length do
           val nextImport = sortedImps.indexSatisfying(from = index + 1)(_.isPrimaryClause) // next import statement
           if sortedImps.indexSatisfying(from = index, until = nextImport): imp =>
-              imp.selectors.exists(!isUsable(imp, _)) // check if any selector in statement was unused
+              imp.selectors.exists(!isUsed(_)) // check if any selector in statement was unused
           < nextImport then
             // if no usable selectors in the import statement, delete it entirely.
             // if there is exactly one usable selector, then replace with just that selector (i.e., format it).
@@ -706,7 +764,7 @@ object CheckUnused:
             // Reminder that first clause span includes the keyword, so delete point-to-start instead.
             val existing = sortedImps.slice(index, nextImport)
             val (keeping, deleting) = existing.iterator.flatMap(imp => imp.selectors.map(imp -> _)).toList
-                                      .partition(isUsable(_, _))
+                                      .partition((imp, sel) => isUsed(sel))
             if keeping.isEmpty then
               val editPos = existing.head.srcPos.sourcePos.withSpan:
                 Span(start = existing.head.srcPos.span.start, end = existing.last.srcPos.span.end)
@@ -908,8 +966,6 @@ object CheckUnused:
     def isSerializationSupport: Boolean =
       sym.is(Method) && serializationNames(sym.name.toTermName) && sym.owner.isClass
         && sym.owner.derivesFrom(defn.JavaSerializableClass)
-    def isCanEqual: Boolean =
-      sym.isOneOf(GivenOrImplicit) && sym.info.finalResultType.baseClasses.exists(_.derivesFrom(defn.CanEqualClass))
     def isMarkerTrait: Boolean =
       sym.info.hiBound.resultType.allMembers.forall: d =>
         val m = d.symbol
@@ -933,12 +989,6 @@ object CheckUnused:
     def boundTpe: Type = sel.bound match
       case untpd.TypedSplice(tree) => tree.tpe
       case _ => NoType
-    /** This is used to ignore exclusion imports of the form import `qual.member as _`
-     *  because `sel.isUnimport` is too broad for old style `import concurrent._`.
-     */
-    def isImportExclusion: Boolean = sel.renamed match
-      case untpd.Ident(nme.WILDCARD) => true
-      case _ => false
 
   extension (imp: Import)(using Context)
     /** Is it the first import clause in a statement? `a.x` in `import a.x, b.{y, z}` */
@@ -949,21 +999,6 @@ object CheckUnused:
     def isGeneratedByEnum: Boolean =
       imp.symbol.exists && imp.symbol.owner.is(Enum, butNot = Case)
 
-    /** Under -Wunused:strict-no-implicit-warn, avoid false positives
-     *  if this selector is a wildcard that might import implicits or
-     *  specifically does import an implicit.
-     *  Similarly, import of CanEqual must not warn, as it is always witness.
-     */
-    def isLoose(sel: ImportSelector): Boolean =
-      if ctx.settings.WunusedHas.strictNoImplicitWarn then
-        if sel.isWildcard
-          || imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit))
-          || imp.expr.tpe.member(sel.name.toTypeName).hasAltWith(_.symbol.isOneOf(GivenOrImplicit))
-        then return true
-      if sel.isWildcard && sel.isGiven
-      then imp.expr.tpe.allMembers.exists(_.symbol.isCanEqual)
-      else imp.expr.tpe.member(sel.name.toTermName).hasAltWith(_.symbol.isCanEqual)
-
   extension (pos: SrcPos)
     def isZeroExtentSynthetic: Boolean = pos.span.isSynthetic && pos.span.isZeroExtent
     def isSynthetic: Boolean = pos.span.isSynthetic && pos.span.exists

tests/pos/i17762.scala

@@ -85,12 +85,12 @@ object InnerMostCheck:
     val a = Set(1)
 
 object IgnoreExclusion:
-  import collection.mutable.{Set => _} // OK
-  import collection.mutable.{Map => _} // OK
+  import collection.mutable.{Map => _, Set => _, *} // OK??
   import collection.mutable.{ListBuffer} // warn
   def check =
     val a = Set(1)
     val b = Map(1 -> 2)
+    def c = Seq(42)
 /**
   * Some given values for the test
   */

tests/warn/i15503a.scala

@@ -1,4 +1,4 @@
-//> using options  -Wunused:strict-no-implicit-warn
+//> using options -Wunused:imports
 
 package foo.unused.strict.test:
   package a:
@@ -7,15 +7,15 @@ package foo.unused.strict.test:
     val z: Int = 2
     def f: Int = 3
   package b:
-    import a.given // OK
-    import a._ // OK
-    import a.* // OK
-    import a.x // OK
-    import a.y // OK
+    import a.given // warn
+    import a._ // warn
+    import a.* // warn
+    import a.x // warn
+    import a.y // warn
     import a.z // warn
     import a.f // warn
   package c:
-    import a.given // OK
+    import a.given // warn
     import a.x // OK
     import a.y // OK
     import a.z // OK
@@ -28,8 +28,8 @@ package foo.implicits.resolution:
   object A { implicit val x: X = new X }
   object B { implicit val y: Y = new Y }
   class C {
-    import A._ // OK
-    import B._ // OK
+    import A.given // warn
+    import B.given // OK
     def t = implicitly[X]
   }
 
@@ -44,7 +44,7 @@ package foo.unused.summon.inlines:
     given willBeUsed: (A & B) = new A with B {}
 
   package use:
-    import lib.{A, B, C, willBeUnused, willBeUsed} //OK
+    import lib.{A, B, C, willBeUnused, willBeUsed} // warn
     import compiletime.summonInline //OK
 
     transparent inline given conflictInside: C =
@@ -56,4 +56,4 @@ package foo.unused.summon.inlines:
       ???
 
     val b: B = summon[B]
-    val c: C = summon[C]
+    val c: C = summon[C]

tests/warn/i15503j.scala

@@ -0,0 +1,33 @@
+//> using options -Wunused:all
+
+class SomeType
+
+def testIt(st1: SomeType, st2: SomeType): Boolean =
+  given CanEqual[SomeType, SomeType] = CanEqual.derived
+  st1 == st2
+
+object HasCanEqual:
+  given f: CanEqual[SomeType, SomeType] = CanEqual.derived
+
+object UsesCanEqual:
+  import HasCanEqual.given
+  def testIt(st1: SomeType, st2: SomeType): Boolean =
+    st1 == st2
+
+object UsesCanEqual2:
+  import HasCanEqual.f
+  def testIt(st1: SomeType, st2: SomeType): Boolean =
+    st1 != st2
+
+object UsesCanEqual3:
+  import HasCanEqual.f as g
+  def testIt(st1: SomeType, st2: SomeType): Boolean =
+    st1 != st2
+
+def warnable(st1: SomeType, st2: SomeType): Boolean =
+  given CanEqual[SomeType, SomeType] = CanEqual.derived // warn
+  st1.toString == st2.toString
+
+def importable(st1: SomeType, st2: SomeType): Boolean =
+  import HasCanEqual.given // warn
+  st1.toString == st2.toString

tests/warn/i17762.scala

@@ -0,0 +1,11 @@
+//> using options -Wunused:imports
+
+import scala.util.Try as _ // warn
+
+class Promise(greeting: String):
+  override def toString = greeting
+
+@main def test = println:
+  import scala.concurrent.{Promise as _, *}, ExecutionContext.Implicits.given
+  val promise = new Promise("world")
+  Future(s"hello, $promise")

tests/warn/i23758.scala

@@ -1,15 +1,15 @@
-
-//> using options -Werror -Wunused:all
+//> using options -Wunused:all
 
 import scala.language.strictEquality
 
 class Box[T](x: T) derives CanEqual:
   def y = x
 
 def f[A, B](a: A, b: B)(using CanEqual[A, B]) = a == b // no warn
+def z[A, B](a: A, b: B)(using ce: CanEqual[A, B]) = a.toString == b.toString // no warn
 
 def g =
-  import Box.given // no warn
+  import Box.given // warn
   "42".length
 
 @main def test() = println: