Include location in error report (bazelbuild#988)

* Include location in error report

* add test

third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinder.scala

@@ -8,28 +8,66 @@ class AstUsedJarFinder(
 ) {
   import global._
 
-  def findUsedJars: Set[AbstractFile] = {
-    val jars = collection.mutable.Set[AbstractFile]()
+  def findUsedJars: Map[AbstractFile, Global#Position] = {
+    val jars = collection.mutable.Map[AbstractFile, global.Position]()
 
-    def handleType(tpe: Type): Unit = {
+    def recordUse(source: AbstractFile, pos: Position): Unit = {
+      // We prefer to report locations which have information (e.g.
+      // we don't want NoPosition).
+      if (!jars.contains(source) || !jars(source).isDefined) {
+        jars.put(source, pos)
+      }
+    }
+
+    def handleType(tpe: Type, pos: Position): Unit = {
       val sym = tpe.typeSymbol
       val assocFile = sym.associatedFile
       if (assocFile.path.endsWith(".class"))
         assocFile.underlyingSource.foreach { source =>
-          jars.add(source)
+          recordUse(source, pos)
         }
     }
 
-    def exploreType(tpe: Type): Unit = {
-      handleType(tpe)
-      tpe.typeArgs.foreach(exploreType)
+    def exploreType(tpe: Type, pos: Position): Unit = {
+      handleType(tpe, pos)
+      tpe.typeArgs.foreach(exploreType(_, pos))
     }
 
     def fullyExploreTree(tree: Tree): Unit = {
       exploreTree(tree)
       tree.foreach(exploreTree)
     }
 
+    def exploreClassfileAnnotArg(arg: ClassfileAnnotArg, pos: Position): Unit = {
+      arg match {
+        case LiteralAnnotArg(value) =>
+          exploreConstant(value, pos)
+        case ArrayAnnotArg(args) =>
+          args.foreach(exploreClassfileAnnotArg(_, pos))
+        case NestedAnnotArg(info) =>
+          exploreAnnotationInfo(info)
+        case _ =>
+      }
+    }
+    def exploreAnnotationInfo(annot: AnnotationInfo): Unit = {
+      // It would be nice if we could just do
+      //    fullyExploreTree(annot.tree)
+      // Unfortunately that tree is synthetic and hence doesn't have
+      // positions attached. Hence we examine the components that
+      // go into that tree separately, as those do have positions.
+      exploreType(annot.tpe, annot.pos)
+      annot.scalaArgs.foreach(fullyExploreTree)
+      annot.javaArgs.values.foreach(exploreClassfileAnnotArg(_, annot.pos))
+    }
+
+    def exploreConstant(value: Constant, pos: Position): Unit = {
+      value.value match {
+        case tpe: Type =>
+          exploreType(tpe, pos)
+        case _ =>
+      }
+    }
+
     def exploreTree(tree: Tree): Unit = {
       tree match {
         case node: TypeTree =>
@@ -51,11 +89,7 @@ class AstUsedJarFinder(
             """
           )
 
-          node.value.value match {
-            case tpe: Type =>
-              exploreType(tpe)
-            case _ =>
-          }
+          exploreConstant(node.value, tree.pos)
         case _ =>
       }
 
@@ -69,19 +103,17 @@ class AstUsedJarFinder(
 
       if (shouldExamine) {
         if (tree.hasSymbolField) {
-          tree.symbol.annotations.foreach { annot =>
-            annot.tree.foreach(fullyExploreTree)
-          }
+          tree.symbol.annotations.foreach(exploreAnnotationInfo)
         }
         if (tree.tpe != null) {
-          exploreType(tree.tpe)
+          exploreType(tree.tpe, tree.pos)
         }
       }
     }
 
     currentRun.units.foreach { unit =>
       unit.body.foreach(fullyExploreTree)
     }
-    jars.toSet
+    jars.toMap
   }
 }

third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala

@@ -65,37 +65,63 @@ class DependencyAnalyzer(val global: Global) extends Plugin {
   }
 
   private def runAnalysis(): Unit = {
-    val usedJars = findUsedJars
-    val usedJarPaths = if (!isWindows) usedJars.map(_.path) else usedJars.map(_.path.replaceAll("\\\\", "/"))
+    val usedJarsToPositions = findUsedJarsAndPositions
+    val usedJarPathToPositions =
+      if (!isWindows) {
+        usedJarsToPositions.map { case (jar, pos) =>
+          jar.path -> pos
+        }
+      } else {
+        usedJarsToPositions.map { case (jar, pos) =>
+          jar.path.replaceAll("\\\\", "/") -> pos
+        }
+      }
 
     if (settings.unusedDepsMode != AnalyzerMode.Off) {
-      reportUnusedDepsFoundIn(usedJarPaths)
+      reportUnusedDepsFoundIn(usedJarPathToPositions)
     }
 
     if (settings.strictDepsMode != AnalyzerMode.Off) {
-      reportIndirectTargetsFoundIn(usedJarPaths)
+      reportIndirectTargetsFoundIn(usedJarPathToPositions)
     }
   }
 
-  private def reportIndirectTargetsFoundIn(usedJarPaths: Set[String]): Unit = {
+  private def reportIndirectTargetsFoundIn(
+    usedJarPathAndPositions: Map[String, global.Position]
+  ): Unit = {
     val errors =
-      usedJarPaths
-        .filterNot(settings.directTargetSet.jarSet.contains)
-        .flatMap(settings.indirectTargetSet.targetFromJarOpt)
-        .map { target =>
-          s"""Target '$target' is used but isn't explicitly declared, please add it to the deps.
-             |You can use the following buildozer command:
-             |buildozer 'add deps $target' ${settings.currentTarget}""".stripMargin
+      usedJarPathAndPositions
+        .filterNot { case (jarPath, _) =>
+          settings.directTargetSet.jarSet.contains(jarPath)
+        }
+        .flatMap { case (jarPath, pos) =>
+          settings.indirectTargetSet.targetFromJarOpt(jarPath).map { target =>
+            target -> pos
+          }
+        }
+        .map { case (target, pos) =>
+          val message =
+            s"""Target '$target' is used but isn't explicitly declared, please add it to the deps.
+               |You can use the following buildozer command:
+               |buildozer 'add deps $target' ${settings.currentTarget}""".stripMargin
+          message -> pos
         }
 
     warnOrError(settings.strictDepsMode, errors)
   }
 
-  private def reportUnusedDepsFoundIn(usedJarPaths: Set[String]): Unit = {
+  private def reportUnusedDepsFoundIn(
+    usedJarPathAndPositions: Map[String, global.Position]
+  ): Unit = {
     val directJarPaths = settings.directTargetSet.jarSet
 
+
     val usedTargets =
-      usedJarPaths.flatMap(settings.directTargetSet.targetFromJarOpt)
+      usedJarPathAndPositions
+        .flatMap { case (jar, _) =>
+          settings.directTargetSet.targetFromJarOpt(jar)
+        }
+        .toSet
 
     val unusedTargets = directJarPaths
       // This .get is safe because [jar] was gotten from [directJarPaths]
@@ -106,29 +132,44 @@ class DependencyAnalyzer(val global: Global) extends Plugin {
 
     val toWarnOrError =
       unusedTargets.map { target =>
-        s"""Target '$target' is specified as a dependency to ${settings.currentTarget} but isn't used, please remove it from the deps.
-           |You can use the following buildozer command:
-           |buildozer 'remove deps $target' ${settings.currentTarget}
-           |""".stripMargin
+        val message =
+          s"""Target '$target' is specified as a dependency to ${settings.currentTarget} but isn't used, please remove it from the deps.
+             |You can use the following buildozer command:
+             |buildozer 'remove deps $target' ${settings.currentTarget}
+             |""".stripMargin
+        (message, global.NoPosition: global.Position)
       }
 
-    warnOrError(settings.unusedDepsMode, toWarnOrError)
+    warnOrError(settings.unusedDepsMode, toWarnOrError.toMap)
   }
 
   private def warnOrError(
     analyzerMode: AnalyzerMode,
-    errors: Set[String]
+    errors: Map[String, global.Position]
   ): Unit = {
-    val reportFunction: String => Unit = analyzerMode match {
-      case AnalyzerMode.Error => global.reporter.error(global.NoPosition, _)
-      case AnalyzerMode.Warn => global.reporter.warning(global.NoPosition, _)
-      case AnalyzerMode.Off => _ => ()
+    val reportFunction: (String, global.Position) => Unit = analyzerMode match {
+      case AnalyzerMode.Error =>
+        { case (message, pos) =>
+          global.reporter.error(pos, message)
+        }
+      case AnalyzerMode.Warn =>
+      { case (message, pos) =>
+        global.reporter.warning(pos, message)
+      }
+      case AnalyzerMode.Off => (_, _) => ()
     }
 
-    errors.foreach(reportFunction)
+    errors.foreach { case (message, pos) =>
+      reportFunction(message, pos)
+    }
   }
 
-  private def findUsedJars: Set[AbstractFile] = {
+  /**
+   *
+   * @return map of used jar file -> representative position in file where
+   *         it was used
+   */
+  private def findUsedJarsAndPositions: Map[AbstractFile, global.Position] = {
     settings.dependencyTrackingMethod match {
       case DependencyTrackingMethod.HighLevel =>
         new HighLevelCrawlUsedJarFinder(global).findUsedJars

third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/HighLevelCrawlUsedJarFinder.scala

@@ -8,13 +8,13 @@ class HighLevelCrawlUsedJarFinder(
 ) {
   import global.Symbol
 
-  def findUsedJars: Set[AbstractFile] = {
+  def findUsedJars: Map[AbstractFile, Global#Position] = {
     val jars = collection.mutable.Set[AbstractFile]()
 
     global.exitingTyper {
       walkTopLevels(global.RootClass, jars)
     }
-    jars.toSet
+    jars.map(jar => jar -> global.NoPosition).toMap
   }
 
   private def walkTopLevels(root: Symbol, jars: collection.mutable.Set[AbstractFile]): Unit = {

third_party/dependency_analyzer/src/test/BUILD

@@ -15,6 +15,7 @@ scala_test(
     ],
     jvm_flags = common_jvm_flags,
     deps = [
+        "//external:io_bazel_rules_scala/dependency/scala/scala_compiler",
         "//external:io_bazel_rules_scala/dependency/scala/scala_library",
         "//external:io_bazel_rules_scala/dependency/scala/scala_reflect",
         "//third_party/dependency_analyzer/src/main:dependency_analyzer",

third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala

@@ -4,6 +4,7 @@ import java.nio.file.Files
 import java.nio.file.Path
 import org.apache.commons.io.FileUtils
 import org.scalatest._
+import scala.tools.nsc.reporters.StoreReporter
 import third_party.dependency_analyzer.src.main.io.bazel.rulesscala.dependencyanalyzer.DependencyTrackingMethod
 import third_party.dependency_analyzer.src.main.io.bazel.rulesscala.dependencyanalyzer.ScalaVersion
 import third_party.utils.src.test.io.bazel.rulesscala.utils.JavaCompileUtil
@@ -29,11 +30,13 @@ class AstUsedJarFinderTest extends FunSuite {
     def compileWithoutAnalyzer(
       code: String
     ): Unit = {
-      TestUtil.runCompiler(
-        code = code,
-        extraClasspath = List(tmpDir.toString),
-        outputPathOpt = Some(tmpDir)
-      )
+      val errors =
+        TestUtil.runCompiler(
+          code = code,
+          extraClasspath = List(tmpDir.toString),
+          outputPathOpt = Some(tmpDir)
+        )
+      assert(errors.isEmpty)
     }
 
     def compileJava(
@@ -50,7 +53,7 @@ class AstUsedJarFinderTest extends FunSuite {
     def checkStrictDepsErrorsReported(
       code: String,
       expectedStrictDeps: List[String]
-    ): Unit = {
+    ): List[StoreReporter#Info] = {
       val errors =
         TestUtil.runCompiler(
           code = code,
@@ -67,11 +70,17 @@ class AstUsedJarFinderTest extends FunSuite {
         )
 
       assert(errors.size == expectedStrictDeps.size)
+      errors.foreach { err =>
+        // We should be emitting errors with positions
+        assert(err.pos.isDefined)
+      }
 
       expectedStrictDeps.foreach { dep =>
         val expectedError = s"Target '$dep' is used but isn't explicitly declared, please add it to the deps"
-        assert(errors.exists(_.contains(expectedError)))
+        assert(errors.exists(_.msg.contains(expectedError)))
       }
+
+      errors
     }
 
     def checkUnusedDepsErrorReported(
@@ -94,10 +103,14 @@ class AstUsedJarFinderTest extends FunSuite {
         )
 
       assert(errors.size == expectedUnusedDeps.size)
+      errors.foreach { err =>
+        // As an unused dep we shouldn't include a position or anything like that
+        assert(!err.pos.isDefined)
+      }
 
       expectedUnusedDeps.foreach { dep =>
         val expectedError = s"Target '$dep' is specified as a dependency to ${TestUtil.defaultTarget} but isn't used, please remove it from the deps."
-        assert(errors.exists(_.contains(expectedError)))
+        assert(errors.exists(_.msg.contains(expectedError)))
       }
     }
   }
@@ -452,4 +465,44 @@ class AstUsedJarFinderTest extends FunSuite {
       }
     }
   }
+
+  test("classOf in class Java annotation is direct") {
+    withSandbox { sandbox =>
+      sandbox.compileJava(
+        className = "Category",
+        code =
+          s"""
+             |public @interface Category {
+             |    Class<?> value();
+             |}
+             |""".stripMargin
+      )
+      sandbox.compileWithoutAnalyzer("class UnitTests")
+      sandbox.checkStrictDepsErrorsReported(
+        """
+          |@Category(classOf[UnitTests])
+          |class C
+          |""".stripMargin,
+        expectedStrictDeps = List("UnitTests", "Category")
+      )
+    }
+  }
+
+  test("position of strict deps error is correct") {
+    // While we do ensure that generally strict deps errors have
+    // a position in the other tests, here we make sure that that
+    // position is correctly computed.
+    withSandbox { sandbox =>
+      sandbox.compileWithoutAnalyzer("class A")
+      val errors =
+        sandbox.checkStrictDepsErrorsReported(
+          "class B(a: A)",
+          expectedStrictDeps = List("A")
+        )
+      assert(errors.size == 1)
+      val pos = errors(0).pos
+      assert(pos.line == 1)
+      assert(pos.column == 12)
+    }
+  }
 }

third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/StrictDepsTest.scala

@@ -25,6 +25,7 @@ class StrictDepsTest extends FunSuite {
           )
         )
     )
+      .map(_.msg)
   }
 
   test("error on indirect dependency target") {