From dd9ce77c281e009784fce7a064e5dcb95d6291c7 Mon Sep 17 00:00:00 2001 From: Jamie5 Date: Sun, 2 Feb 2020 11:04:33 -0800 Subject: [PATCH] Handle inlined literals in AST walker (#985) * Handle inlined literals in AST walker * comments * comments --- scala/private/phases/phase_compile.bzl | 1 + scala/private/rules/scala_library.bzl | 4 +- .../dependency_analyzer/src/main/BUILD | 16 ++ .../dependencyanalyzer/AstUsedJarFinder.scala | 14 ++ .../dependencyanalyzer/ScalaVersion.scala | 158 ++++++++++++++++++ .../dependency_analyzer/src/test/BUILD | 16 +- .../AstUsedJarFinderTest.scala | 121 ++++++++++++++ .../dependencyanalyzer/ScalaVersionTest.scala | 146 ++++++++++++++++ 8 files changed, 473 insertions(+), 3 deletions(-) create mode 100644 third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala create mode 100644 third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala diff --git a/scala/private/phases/phase_compile.bzl b/scala/private/phases/phase_compile.bzl index 3148da668..2aa683bba 100644 --- a/scala/private/phases/phase_compile.bzl +++ b/scala/private/phases/phase_compile.bzl @@ -47,6 +47,7 @@ def phase_compile_library_for_plugin_bootstrapping(ctx, p): for target in p.scalac_provider.default_classpath + ctx.attr.exports ], unused_dependency_checker_mode = "off", + buildijar = ctx.attr.build_ijar, ) return _phase_compile_default(ctx, p, args) diff --git a/scala/private/rules/scala_library.bzl b/scala/private/rules/scala_library.bzl index a4dd770f2..c1f99aad5 100644 --- a/scala/private/rules/scala_library.bzl +++ b/scala/private/rules/scala_library.bzl @@ -152,7 +152,9 @@ def _scala_library_for_plugin_bootstrapping_impl(ctx): # the scala compiler plugin used for dependency analysis is compiled using `scala_library`. # in order to avoid cyclic dependencies `scala_library_for_plugin_bootstrapping` was created for this purpose, # which does not contain plugin related attributes, and thus avoids the cyclic dependency issue -_scala_library_for_plugin_bootstrapping_attrs = {} +_scala_library_for_plugin_bootstrapping_attrs = { + "build_ijar": attr.bool(default = True), +} _scala_library_for_plugin_bootstrapping_attrs.update(implicit_deps) diff --git a/third_party/dependency_analyzer/src/main/BUILD b/third_party/dependency_analyzer/src/main/BUILD index b5f9798a9..e9e00a221 100644 --- a/third_party/dependency_analyzer/src/main/BUILD +++ b/third_party/dependency_analyzer/src/main/BUILD @@ -2,6 +2,21 @@ licenses(["notice"]) # 3-clause BSD load("//scala:scala.bzl", "scala_library_for_plugin_bootstrapping") +scala_library_for_plugin_bootstrapping( + name = "scala_version", + srcs = [ + "io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala", + ], + # As this contains macros we shouldn't make an ijar + build_ijar = False, + resources = ["resources/scalac-plugin.xml"], + visibility = ["//visibility:public"], + deps = [ + "//external:io_bazel_rules_scala/dependency/scala/scala_compiler", + "//external:io_bazel_rules_scala/dependency/scala/scala_reflect", + ], +) + scala_library_for_plugin_bootstrapping( name = "dependency_analyzer", srcs = [ @@ -14,6 +29,7 @@ scala_library_for_plugin_bootstrapping( resources = ["resources/scalac-plugin.xml"], visibility = ["//visibility:public"], deps = [ + ":scala_version", "//external:io_bazel_rules_scala/dependency/scala/scala_compiler", "//external:io_bazel_rules_scala/dependency/scala/scala_reflect", ], 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 5d67cd05f..d1d0a1710 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 @@ -37,6 +37,20 @@ class AstUsedJarFinder( node.original.foreach(fullyExploreTree) } case node: Literal => + // We should examine OriginalTreeAttachment but that was only + // added in 2.12.4, so include a version check + ScalaVersion.conditional( + Some("2.12.4"), + None, + """ + node.attachments + .get[global.treeChecker.OriginalTreeAttachment] + .foreach { attach => + fullyExploreTree(attach.original) + } + """ + ) + node.value.value match { case tpe: Type => exploreType(tpe) diff --git a/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala new file mode 100644 index 000000000..e7be56c5a --- /dev/null +++ b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala @@ -0,0 +1,158 @@ +package third_party.dependency_analyzer.src.main.io.bazel.rulesscala.dependencyanalyzer + +import scala.language.experimental.macros +import scala.reflect.macros.blackbox + +object ScalaVersion { + val Current: ScalaVersion = ScalaVersion(util.Properties.versionNumberString) + + def apply(versionString: String): ScalaVersion = { + versionString.split("\\.") match { + case Array(superMajor, major, minor) => + new ScalaVersion(superMajor.toInt, major.toInt, minor.toInt) + case _ => + throw new Exception(s"Failed to parse version $versionString") + } + } + + /** + * Runs [code] only if minVersion and maxVersion constraints are met. + * + * NOTE: This method should be used only rarely. Most of the time + * just comparing versions in code should be enough. This is needed + * only when the code we want to run can't compile under certain + * versions. The reason to use this rarely is the API's inflexibility + * and the difficulty in debugging this code. + * + * Each of minVersionOpt and maxVersionOpt can either be None + * to signify that there is no restriction on this bound, + * or it can be a string of a full version number such as "2.12.10". + * + * When set to a version number, the bounds are inclusive. + * For example, a maxVersion of "2.12.10" will accept version "2.12.10". + * + * Note only literal strings are accepted and inlined variables are accepted. + * If any non-inlined variables are passed the code will fail to compile. + * Inlined variables are generally those declared final on an object which + * do not have a type attached. + * + * valid: + * conditional(Some("2.12.4"), None, "foo()") + * invalid: + * conditional(MinVersionForFoo, None, "foo()") + */ + def conditional( + minVersionOpt: Option[String], + maxVersionOpt: Option[String], + code: String + ): Unit = + macro conditionalImpl + + def conditionalImpl( + c: blackbox.Context + )( + minVersionOpt: c.Expr[Option[String]], + maxVersionOpt: c.Expr[Option[String]], + code: c.Expr[String] + ): c.Tree = { + import c.{universe => u} + + // Due to non-deterministic code generation of quasiquotes, we do + // not use them + // See https://github.com/scala/bug/issues/11008 + // Eventually once we stop supporting all versions which don't have + // the bugfix, we can use quasiquotes as desired + + def extractStringFromTree(tree: c.Tree): Option[String] = { + tree match { + case u.Literal(u.Constant(s: String)) => + Some(s) + case _ => + None + } + } + + def extractStringOption(expr: c.Expr[Option[String]]): Option[String] = { + expr.tree match { + case u.Apply( + u.TypeApply( + u.Select(u.Select(u.Ident(u.TermName("scala")), u.TermName("Some")), u.TermName("apply")), + List(u.TypeTree())), + str :: Nil + ) if extractStringFromTree(str).nonEmpty => + extractStringFromTree(str) + case u.Select(u.Ident(u.TermName("scala")), u.TermName("None")) => + None + case _ => + c.error( + expr.tree.pos, + "Parameter must be passed as an Option[String] literal such as " + + "Some(\"2.12.10\") or None") + None + } + } + + def extractString(expr: c.Expr[String]): String = { + extractStringFromTree(expr.tree).getOrElse { + c.error( + expr.tree.pos, + "Parameter must be passed as a string literal such as \"2.12.10\"") + "" + } + } + + val meetsMinVersionRequirement = { + val minVersionOptValue = extractStringOption(minVersionOpt) + + // Note: Unit tests do not test that this bound is inclusive rather + // than exclusive so be careful when changing this code not to + // accidentally make this an exclusive bound (see ScalaVersionTest for + // details) + minVersionOptValue.forall(version => Current >= ScalaVersion(version)) + } + + val meetsMaxVersionRequirement = { + val maxVersionOptValue = extractStringOption(maxVersionOpt) + // Note: Unit tests do not test that this bound is inclusive rather + // than exclusive so be careful when changing this code not to + // accidentally make this an exclusive bound (see ScalaVersionTest for + // details) + maxVersionOptValue.forall(version => Current <= ScalaVersion(version)) + } + + if (meetsMinVersionRequirement && meetsMaxVersionRequirement) { + c.parse(extractString(code)) + } else { + u.EmptyTree + } + } +} + +class ScalaVersion private( + private val superMajor: Int, + private val major: Int, + private val minor: Int +) extends Ordered[ScalaVersion] { + override def compare(that: ScalaVersion): Int = { + if (this.superMajor != that.superMajor) { + this.superMajor.compareTo(that.superMajor) + } else if (this.major != that.major) { + this.major.compareTo(that.major) + } else { + this.minor.compareTo(that.minor) + } + } + + override def equals(obj: Any): Boolean = { + obj match { + case that: ScalaVersion => + compare(that) == 0 + case _ => + false + } + } + + override def toString: String = { + s"$superMajor.$major.$minor" + } +} diff --git a/third_party/dependency_analyzer/src/test/BUILD b/third_party/dependency_analyzer/src/test/BUILD index 659c7ed06..e23a848ce 100644 --- a/third_party/dependency_analyzer/src/test/BUILD +++ b/third_party/dependency_analyzer/src/test/BUILD @@ -14,17 +14,29 @@ scala_test( "io/bazel/rulesscala/dependencyanalyzer/AstUsedJarFinderTest.scala", ], jvm_flags = common_jvm_flags, - unused_dependency_checker_mode = "off", 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/main:scala_version", "//third_party/utils/src/test:test_util", "@scalac_rules_commons_io//jar", ], ) +scala_test( + name = "scala_version_test", + size = "small", + srcs = [ + "io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala", + ], + deps = [ + "//external:io_bazel_rules_scala/dependency/scala/scala_library", + "//external:io_bazel_rules_scala/dependency/scala/scala_reflect", + "//third_party/dependency_analyzer/src/main:scala_version", + ], +) + scala_test( name = "strict_deps_test", size = "small", 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 bd54fddf1..b499bceeb 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 @@ -5,10 +5,15 @@ import java.nio.file.Path import org.apache.commons.io.FileUtils import org.scalatest._ 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 import third_party.utils.src.test.io.bazel.rulesscala.utils.TestUtil import third_party.utils.src.test.io.bazel.rulesscala.utils.TestUtil.DependencyAnalyzerTestParams +// NOTE: Some tests are version-dependent as some false positives +// cannot be fixed in older versions of Scala for various reasons. +// Hence make sure to look at any version checks to understand +// which versions do and don't support which cases. class AstUsedJarFinderTest extends FunSuite { private def withSandbox(action: Sandbox => Unit): Unit = { val tmpDir = Files.createTempDirectory("dependency_analyzer_test_temp") @@ -152,6 +157,23 @@ class AstUsedJarFinderTest extends FunSuite { } } + /** + * In a situation where B depends indirectly on A, ensure + * that the dependency analyzer recognizes this fact. + */ + private def checkIndirectDependencyDetected( + aCode: String, + bCode: String + ): Unit = { + withSandbox { sandbox => + sandbox.compileWithoutAnalyzer(aCode) + sandbox.checkUnusedDepsErrorReported( + code = bCode, + expectedUnusedDeps = List("A") + ) + } + } + test("simple composition in indirect") { checkIndirectDependencyDetected( aCode = @@ -302,6 +324,43 @@ class AstUsedJarFinderTest extends FunSuite { ) } + test("inlined literal is direct") { + // Note: For a constant to be inlined + // - it must not have a type declaration such as `: Int`. + // (this appears to be the case in practice at least) + // (is this documented anywhere???) + // - some claim it must start with a capital letter, though + // this does not seem to be the case. Nevertheless we do that + // anyways. + // + // Hence it is possible that as newer versions of scala + // are released then this test may need to be updated to + // conform to changing requirements of what is inlined. + + // Note that in versions of scala < 2.12.4 we cannot detect + // such a situation. Hence we will have a false positive here + // for those older versions, which we verify in test. + + val aCode = + s""" + |object A { + | final val Inlined = 123 + |} + |""".stripMargin + val bCode = + s""" + |object B { + | val d: Int = A.Inlined + |} + |""".stripMargin + + if (ScalaVersion.Current >= ScalaVersion("2.12.4")) { + checkDirectDependencyRecognized(aCode = aCode, bCode = bCode) + } else { + checkIndirectDependencyDetected(aCode = aCode, bCode = bCode) + } + } + test("unspecified default argument type is indirect") { checkIndirectDependencyDetected( aCode = "class A", @@ -331,4 +390,66 @@ class AstUsedJarFinderTest extends FunSuite { ) } } + + test("java interface field and method is direct") { + withSandbox { sandbox => + sandbox.compileJava( + className = "A", + code = "public interface A { int a = 42; }" + ) + val bCode = + """ + |class B { + | def foo(x: A): Unit = {} + | val b = A.a + |} + |""".stripMargin + + // Unlike other tests, this one includes both access to an inlined + // variable and taking the class A as an argument. In theory, + // this test should work for all supported versions just like + // test `java interface method argument is direct` since they + // both have a method taking A as an argument. + // + // However, it does not work for all versions. It is unclear why but + // presumably there were various compiler improvements. + if (ScalaVersion.Current >= ScalaVersion("2.12.0")) { + sandbox.checkStrictDepsErrorsReported( + bCode, + expectedStrictDeps = List("A") + ) + } else { + sandbox.checkUnusedDepsErrorReported( + bCode, + expectedUnusedDeps = List("A") + ) + } + } + } + + test("java interface field is direct") { + withSandbox { sandbox => + sandbox.compileJava( + className = "A", + code = "public interface A { int a = 42; }" + ) + val bCode = + """ + |class B { + | val b = A.a + |} + |""".stripMargin + if (ScalaVersion.Current >= ScalaVersion("2.12.4")) { + sandbox.checkStrictDepsErrorsReported( + bCode, + expectedStrictDeps = List("A") + ) + } else { + sandbox.checkUnusedDepsErrorReported( + bCode, + expectedUnusedDeps = List("A") + ) + } + } + } } diff --git a/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala new file mode 100644 index 000000000..c3d46075c --- /dev/null +++ b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala @@ -0,0 +1,146 @@ +package third_party.dependency_analyzer.src.test.io.bazel.rulesscala.dependencyanalyzer + +import org.scalatest._ +import third_party.dependency_analyzer.src.main.io.bazel.rulesscala.dependencyanalyzer.ScalaVersion + +class ScalaVersionTest extends FunSuite { + test("version comparison works") { + // Test that when a > b, all the comparisons are as expected + def testOrder(a: String, b: String): Unit = { + val va = ScalaVersion(a) + val vb = ScalaVersion(b) + + assert(!(va == vb)) + assert(va != vb) + assert(!(va < vb)) + assert(!(va <= vb)) + assert(va > vb) + assert(va >= vb) + + assert(!(vb == va)) + assert(vb != va) + assert(vb < va) + assert(vb <= va) + assert(!(vb > va)) + assert(!(vb >= va)) + } + + def testEqual(a: String, b: String): Unit = { + val va = ScalaVersion(a) + val vb = ScalaVersion(b) + + assert(va == vb) + assert(!(va != vb)) + assert(!(va < vb)) + assert(va <= vb) + assert(!(va > vb)) + assert(va >= vb) + } + + testEqual("1.1.1", "1.1.1") + testEqual("1.2.3", "1.2.3") + testEqual("30.20.10", "30.20.10") + + testOrder("1.2.3", "1.0.0") + testOrder("1.2.1", "1.2.0") + testOrder("1.2.0", "1.1.9") + testOrder("2.12.12", "2.12.11") + testOrder("2.12.0", "2.1.50") + } + + test("macro works") { + // These are rather duplicative unfortunately as the person + // who wrote the macro is not very smart + + // We use versions like 1.0.0 and 500.0.0 so that even + // as versions of scala change the test won't need to be updated + + // Note: this test unfortunately does not test that the min and max + // bounds are inclusive rather than exclusive, because this code has to + // compile across all supported scala versions and we can't get an + // inlineable constant with the version string. In theory there may + // be complex solutions such as making this a template file and + // inserting the version, but that seems rather overdifficult. + // + // As version-differing behavior should be tested in unit tests anyways, + // with their own version bounds checks, this seems an acceptable risk + // given the costs of fixing. + + // No bounds + { + var hit = false + ScalaVersion.conditional( + None, + None, + "hit = true" + ) + assert(hit) + } + + // Min bounds hit + { + var hit = false + ScalaVersion.conditional( + Some("1.0.0"), + None, + "hit = true" + ) + assert(hit) + } + + // Min bounds not hit + { + var hit = false + ScalaVersion.conditional( + Some("500.0.0"), + None, + "hit = true" + ) + assert(!hit) + } + + // Max bounds hit + { + var hit = false + ScalaVersion.conditional( + None, + Some("500.0.0"), + "hit = true" + ) + assert(hit) + } + + // Max bounds not hit + { + var hit = false + ScalaVersion.conditional( + None, + Some("1.0.0"), + "hit = true" + ) + assert(!hit) + } + + // Min-max bound hit + { + var hit = false + ScalaVersion.conditional( + Some("1.0.0"), + Some("500.0.0"), + "hit = true" + ) + assert(hit) + } + + // Min-max bound not hit + { + var hit = false + ScalaVersion.conditional( + Some("500.0.0"), + Some("1000.0.0"), + "hit = true" + ) + assert(!hit) + } + } +}