From 916178197fd627ab2a918ed3166507c52a68c3b1 Mon Sep 17 00:00:00 2001 From: Jamie5 Date: Thu, 6 Feb 2020 20:39:20 -0800 Subject: [PATCH] Include location in error report (#988) * Include location in error report * add test --- .../dependencyanalyzer/AstUsedJarFinder.scala | 66 +++++++++---- .../DependencyAnalyzer.scala | 93 +++++++++++++------ .../HighLevelCrawlUsedJarFinder.scala | 4 +- .../dependency_analyzer/src/test/BUILD | 1 + .../AstUsedJarFinderTest.scala | 69 ++++++++++++-- .../dependencyanalyzer/StrictDepsTest.scala | 1 + .../UnusedDependencyCheckerTest.scala | 1 + .../io/bazel/rulesscala/utils/TestUtil.scala | 13 ++- 8 files changed, 191 insertions(+), 57 deletions(-) diff --git a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinder.scala b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinder.scala index d1d0a1710c..7867c93822 100644 --- a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinder.scala +++ b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinder.scala @@ -8,21 +8,29 @@ 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 = { @@ -30,6 +38,36 @@ class AstUsedJarFinder( 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,12 +103,10 @@ 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) } } } @@ -82,6 +114,6 @@ class AstUsedJarFinder( currentRun.units.foreach { unit => unit.body.foreach(fullyExploreTree) } - jars.toSet + jars.toMap } } diff --git a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala index 9da4ecee19..847188f209 100644 --- a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala +++ b/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 diff --git a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/HighLevelCrawlUsedJarFinder.scala b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/HighLevelCrawlUsedJarFinder.scala index cbb20c5504..18e8ae2cde 100644 --- a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/HighLevelCrawlUsedJarFinder.scala +++ b/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 = { diff --git a/third_party/dependency_analyzer/src/test/BUILD b/third_party/dependency_analyzer/src/test/BUILD index e23a848ceb..3e09162a0c 100644 --- a/third_party/dependency_analyzer/src/test/BUILD +++ b/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", diff --git a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala index b499bceeb1..9f0537d4b8 100644 --- a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala +++ b/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) + } + } } diff --git a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/StrictDepsTest.scala b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/StrictDepsTest.scala index 6ec2144c9a..1f837a1f4f 100644 --- a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/StrictDepsTest.scala +++ b/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") { diff --git a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/UnusedDependencyCheckerTest.scala b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/UnusedDependencyCheckerTest.scala index ef7d8f0013..97a5a650d7 100644 --- a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/UnusedDependencyCheckerTest.scala +++ b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/UnusedDependencyCheckerTest.scala @@ -22,6 +22,7 @@ class UnusedDependencyCheckerTest extends FunSuite { ) ) ) + .map(_.msg) } test("error on unused direct dependencies") { diff --git a/third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala b/third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala index bd6a663bff..e379aa7041 100644 --- a/third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala +++ b/third_party/utils/src/test/io/bazel/rulesscala/utils/TestUtil.scala @@ -88,7 +88,7 @@ object TestUtil { extraClasspath: List[String] = List.empty, dependencyAnalyzerParamsOpt: Option[DependencyAnalyzerTestParams] = None, outputPathOpt: Option[Path] = None - ): List[String] = { + ): List[StoreReporter#Info] = { val dependencyAnalyzerOptions = dependencyAnalyzerParamsOpt .map(getDependencyAnalyzerOptions) @@ -106,7 +106,7 @@ object TestUtil { code: String, compileOptions: String, output: AbstractFile - ): List[String] = { + ): List[StoreReporter#Info] = { // TODO: Optimize and cache global. val options = CommandLineParser.tokenize(compileOptions) val reporter = new StoreReporter() @@ -119,9 +119,14 @@ object TestUtil { val global = new Global(settings, reporter) val run = new global.Run - val toCompile = new BatchSourceFile("", code) + + // It is important that the source name when compiling code + // looks like a valid scala file - + // this causes the compiler to report positions correctly. And + // tests verify that positions are reported successfully. + val toCompile = new BatchSourceFile("CompiledCode.scala", code) run.compileSources(List(toCompile)) - reporter.infos.collect({ case msg if msg.severity == reporter.ERROR => msg.msg }).toList + reporter.infos.filter(_.severity == reporter.ERROR).toList } private lazy val baseDir = System.getProperty("user.dir")