Merge pull request #9939 from som-snytt/issue/12326

Import warnings have an origin
scala · Mar 3, 2022 · 0bcc7dd · 0bcc7dd
2 parents 3c25bcb + c663e3f
commit 0bcc7dd
Show file tree

Hide file tree

Showing 10 changed files with 84 additions and 22 deletions.
diff --git a/src/compiler/scala/tools/nsc/Reporting.scala b/src/compiler/scala/tools/nsc/Reporting.scala
@@ -111,6 +111,7 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio
     private def issueWarning(warning: Message): Unit = {
       def verbose = warning match {
         case Message.Deprecation(_, msg, site, origin, version) => s"[${warning.category.name} @ $site | origin=$origin | version=${version.filterString}] $msg"
+        case Message.Origin(_, msg, category, site, origin @ _) => s"[${category.name} @ $site] $msg"  // origin text is obvious at caret
         case Message.Plain(_, msg, category, site) => s"[${category.name} @ $site] $msg"
       }
       wconf.action(warning) match {
@@ -192,7 +193,7 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio
       impl(sym)
     } else ""
 
-    def deprecationWarning(pos: Position, msg: String, since: String, site: String, origin: String): Unit =
+    override def deprecationWarning(pos: Position, msg: String, since: String, site: String, origin: String): Unit =
       issueIfNotSuppressed(Message.Deprecation(pos, msg, site, origin, Version.fromString(since)))
 
     def deprecationWarning(pos: Position, origin: Symbol, site: Symbol, msg: String, since: String): Unit =
@@ -262,6 +263,10 @@ trait Reporting extends internal.Reporting { self: ast.Positions with Compilatio
     def warning(pos: Position, msg: String, category: WarningCategory, site: Symbol): Unit =
       warning(pos, msg, category, siteName(site))
 
+    // Provide an origin for the warning.
+    def warning(pos: Position, msg: String, category: WarningCategory, site: Symbol, origin: String): Unit =
+      issueIfNotSuppressed(Message.Origin(pos, msg, category, siteName(site), origin))
+
     // used by Global.deprecationWarnings, which is used by sbt
     def deprecationWarnings: List[(Position, String)] = summaryMap(Action.WarningSummary, WarningCategory.Deprecation).toList.map(p => (p._1, p._2.msg))
     def uncheckedWarnings: List[(Position, String)]   = summaryMap(Action.WarningSummary, WarningCategory.Unchecked).toList.map(p => (p._1, p._2.msg))
@@ -305,8 +310,12 @@ object Reporting {
   }
 
   object Message {
+    // an ordinary Message has a `category` for filtering and the `site` where it was issued
     final case class Plain(pos: Position, msg: String, category: WarningCategory, site: String) extends Message
 
+    // a Plain message with an `origin` which should not be empty. For example, the origin of an unused import is the fully-qualified selection
+    final case class Origin(pos: Position, msg: String, category: WarningCategory, site: String, origin: String) extends Message
+
     // `site` and `origin` may be empty
     final case class Deprecation(pos: Position, msg: String, site: String, origin: String, since: Version) extends Message {
       def category: WarningCategory = WarningCategory.Deprecation
@@ -502,6 +511,7 @@ object Reporting {
     final case class DeprecatedOrigin(pattern: Regex) extends MessageFilter {
       def matches(message: Message): Boolean = message match {
         case m: Message.Deprecation => pattern.matches(m.origin)
+        case m: Message.Origin      => pattern.matches(m.origin)
         case _ => false
       }
     }
@@ -558,7 +568,7 @@ object Reporting {
         regex(s.substring(5)).map(SitePattern)
       } else if (s.startsWith("origin=")) {
         regex(s.substring(7)).map(DeprecatedOrigin)
-      } else if(s.startsWith("since")) {
+      } else if (s.startsWith("since")) {
         def fail = Left(s"invalid since filter: `$s`; required shape: `since<1.2.3`, `since=3.2`, `since>2`")
         if (s.length < 6) fail
         else {

diff --git a/src/compiler/scala/tools/nsc/typechecker/Contexts.scala b/src/compiler/scala/tools/nsc/typechecker/Contexts.scala
@@ -65,45 +65,34 @@ trait Contexts { self: Analyzer =>
   )
 
   private lazy val allUsedSelectors =
-    mutable.Map[ImportInfo, Set[ImportSelector]]() withDefaultValue Set()
+    mutable.Map.empty[ImportInfo, Set[ImportSelector]].withDefaultValue(Set.empty)
   private lazy val allImportInfos =
-    mutable.Map[CompilationUnit, List[(ImportInfo, Symbol)]]() withDefaultValue Nil
+    mutable.Map.empty[CompilationUnit, List[(ImportInfo, Symbol)]].withDefaultValue(Nil)
 
-  def warnUnusedImports(unit: CompilationUnit) = {
-    def cullUnusedSelections(infos0: List[(ImportInfo, Symbol)]): List[(Position, Symbol)] = {
-      var unused = List.empty[(Position, Symbol)]
+  def warnUnusedImports(unit: CompilationUnit) = if (!unit.isJava) {
+    def warnUnusedSelections(infos0: List[(ImportInfo, Symbol)]): Unit = {
+      type Culled = (Position, Symbol, String)
+      var unused = List.empty[Culled]
       @tailrec def loop(infos: List[(ImportInfo, Symbol)]): Unit =
         infos match {
           case (info, owner) :: rest =>
             val used = allUsedSelectors.remove(info).getOrElse(Set.empty)
-            // since we are going in reverse order, add unused selectors from right to left, last to first
             def checkSelectors(selectors: List[ImportSelector]): Unit =
               selectors match {
                 case selector :: rest =>
                   checkSelectors(rest)
                   if (!selector.isMask && !used(selector))
-                    unused ::= ((info.posOf(selector), owner))
+                    unused ::= ((info.posOf(selector), owner, info.fullSelectorString(selector)))
                 case _ =>
               }
             checkSelectors(info.tree.selectors)
             loop(rest)
           case _ =>
         }
       loop(infos0)
-      unused
+      unused.foreach { case (pos, owner, origin) => runReporting.warning(pos, "Unused import", WarningCategory.UnusedImports, owner, origin) }
     }
-    @tailrec def warnUnusedSelections(unused: List[(Position, Symbol)]): Unit =
-      unused match {
-        case (pos, site) :: rest =>
-          runReporting.warning(pos, "Unused import", WarningCategory.UnusedImports, site = site)
-          warnUnusedSelections(rest)
-        case _ =>
-      }
-    if (!unit.isJava)
-      allImportInfos.remove(unit) match {
-        case Some(importInfos) => warnUnusedSelections(cullUnusedSelections(importInfos))
-        case _                 => ()
-      }
+    allImportInfos.remove(unit).foreach(warnUnusedSelections)
   }
 
   var lastAccessCheckDetails: String = ""
@@ -1875,6 +1864,9 @@ trait Contexts { self: Analyzer =>
       }
     }
 
+    def fullSelectorString(s: ImportSelector): String =
+      s"${if (qual.tpe.isError) tree.toString else qual.tpe.typeSymbol.fullName}.${selectorString(s)}"
+
     private def selectorString(s: ImportSelector): String =
       if (s.isWildcard) "_"
       else if (s.isRename) s"${s.name} => ${s.rename}"

diff --git a/test/files/neg/t12326.check b/test/files/neg/t12326.check
@@ -0,0 +1,6 @@
+t12326.scala:5: warning: Unused import
+import scala.collection.mutable._
+                                ^
+error: No warnings can be incurred under -Werror.
+1 warning
+1 error
diff --git a/test/files/neg/t12326.scala b/test/files/neg/t12326.scala
@@ -0,0 +1,9 @@
+// scalac: -Werror -Wunused:imports -Wconf:site=p.T:s
+
+package p
+
+import scala.collection.mutable._
+
+trait T {
+  import scala.concurrent.ExecutionContext.Implicits._
+}
diff --git a/test/files/neg/t12326b.check b/test/files/neg/t12326b.check
@@ -0,0 +1,6 @@
+t12326b.scala:5: warning: Unused import
+import scala.collection.mutable.{ListBuffer, Map, Set}
+                                 ^
+error: No warnings can be incurred under -Werror.
+1 warning
+1 error
diff --git a/test/files/neg/t12326b.scala b/test/files/neg/t12326b.scala
@@ -0,0 +1,9 @@
+// scalac: -Werror -Wunused:imports -Wconf:site=p.T:s,origin=scala.collection.mutable.Map:s,origin=scala.collection.mutable.Set:s
+
+package p
+
+import scala.collection.mutable.{ListBuffer, Map, Set}
+
+trait T {
+  import scala.concurrent.ExecutionContext.Implicits._
+}
diff --git a/test/files/neg/t12326c.check b/test/files/neg/t12326c.check
@@ -0,0 +1,6 @@
+t12326c.scala:8: warning: Unused import
+  import scala.concurrent.ExecutionContext.Implicits._
+                                                     ^
+error: No warnings can be incurred under -Werror.
+1 warning
+1 error
diff --git a/test/files/neg/t12326c.scala b/test/files/neg/t12326c.scala
@@ -0,0 +1,9 @@
+// scalac: -Werror -Wunused:imports -Wconf:origin=scala.collection.mutable.*:s
+
+package p
+
+import scala.collection.mutable.{ListBuffer, Map, Set}
+
+trait T {
+  import scala.concurrent.ExecutionContext.Implicits._
+}
diff --git a/test/files/pos/t12326.scala b/test/files/pos/t12326.scala
@@ -0,0 +1,7 @@
+// scalac: -Werror -Wunused:imports -Wconf:origin=scala.collection.mutable._:s,origin=scala.concurrent.ExecutionContext.Implicits._:s
+
+import scala.collection.mutable._
+
+trait T {
+  import scala.concurrent.ExecutionContext.Implicits._
+}
diff --git a/test/files/pos/t12326b.scala b/test/files/pos/t12326b.scala
@@ -0,0 +1,8 @@
+// scalac: -Werror -Wunused:imports
+
+import annotation._
+
+@nowarn("origin=scala.concurrent.ExecutionContext.Implicits._")
+trait T {
+  import scala.concurrent.ExecutionContext.Implicits._
+}