From 64f183673140ac467f6c5231e2ce7f49f545e366 Mon Sep 17 00:00:00 2001 From: Jamie Date: Thu, 30 Jan 2020 09:42:31 -0800 Subject: [PATCH 1/3] Handle inlined literals in AST walker --- scala/private/phases/phase_compile.bzl | 1 + scala/private/rules/scala_library.bzl | 2 + .../dependency_analyzer/src/main/BUILD | 16 +++ .../dependencyanalyzer/AstUsedJarFinder.scala | 14 ++ .../dependencyanalyzer/ScalaVersion.scala | 112 +++++++++++++++ .../dependency_analyzer/src/test/BUILD | 16 ++- .../AstUsedJarFinderTest.scala | 111 ++++++++++++++ .../dependencyanalyzer/ScalaVersionTest.scala | 136 ++++++++++++++++++ 8 files changed, 406 insertions(+), 2 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 1e730a574..ee07de381 100644 --- a/scala/private/phases/phase_compile.bzl +++ b/scala/private/phases/phase_compile.bzl @@ -59,6 +59,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 17356b843..cfd4d395a 100644 --- a/scala/private/rules/scala_library.bzl +++ b/scala/private/rules/scala_library.bzl @@ -161,6 +161,8 @@ _scala_library_for_plugin_bootstrapping_attrs.update( common_attrs_for_plugin_bootstrapping, ) +_scala_library_for_plugin_bootstrapping_attrs["build_ijar"] = attr.bool(default = True) + def make_scala_library_for_plugin_bootstrapping(*extras): return rule( attrs = _dicts.add( 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 99cdc7d9a..f8a15dde9 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( + "2.12.4", + "", + """ + 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..9d6346047 --- /dev/null +++ b/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/ScalaVersion.scala @@ -0,0 +1,112 @@ +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 minVersion and maxVersion can either be the empty string ("") + * 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". + * + * Note only literal strings are accepted, no variables etc. i.e. + * + * valid: + * conditional(minVersion = "2.12.4", maxVersion = "", code = "foo()") + * invalid: + * conditional(minVersion = MinVersionForFoo, maxVersion = "", code = "foo()") + */ + def conditional( + minVersion: String, + maxVersion: String, + code: String + ): Unit = + macro conditionalImpl + + def conditionalImpl( + c: blackbox.Context + )( + minVersion: c.Expr[String], + maxVersion: c.Expr[String], + code: c.Expr[String] + ): c.Tree = { + import c.{universe => u} + import u.Quasiquote + def extractString(expr: c.Expr[String]): String = { + expr.tree match { + case u.Literal(u.Constant(s: String)) => + s + case _ => + c.error( + expr.tree.pos, + "Parameter must be passed as a string literal such as \"2.12.10\"") + "" + } + } + + val meetsMinVersionRequirement = { + val minVersionStr = extractString(minVersion) + minVersionStr == "" || Current >= ScalaVersion(minVersionStr) + } + + val meetsMaxVersionRequirement = { + val maxVersionStr = extractString(maxVersion) + maxVersionStr == "" || Current <= ScalaVersion(maxVersionStr) + } + + if (meetsMinVersionRequirement && meetsMaxVersionRequirement) { + c.parse(extractString(code)) + } else { + q"" + } + } +} + +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 8066720f1..12515decf 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,6 +5,7 @@ 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 @@ -152,6 +153,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 +320,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("java interface method argument is direct") { withSandbox { sandbox => sandbox.compileJava( @@ -318,4 +373,60 @@ 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 + + // It is unclear why this only works with these versions 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..192f92678 --- /dev/null +++ b/third_party/dependency_analyzer/src/test/io/bazel/rulesscala/dependencyanalyzer/ScalaVersionTest.scala @@ -0,0 +1,136 @@ +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") { + // i.e. a>b + def testGreater(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) + + // Lesser versions + 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") + + testGreater("1.2.3", "1.0.0") + testGreater("1.2.1", "1.2.0") + testGreater("1.2.0", "1.1.9") + testGreater("2.12.12", "2.12.11") + testGreater("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 + + // No bounds + { + var hit = false + ScalaVersion.conditional( + "", + "", + "hit = true" + ) + assert(hit) + } + + // Min bounds hit + { + var hit = false + ScalaVersion.conditional( + "1.0.0", + "", + "hit = true" + ) + assert(hit) + } + + // Min bounds not hit + { + var hit = false + ScalaVersion.conditional( + "500.0.0", + "", + "hit = true" + ) + assert(!hit) + } + + // Max bounds hit + { + var hit = false + ScalaVersion.conditional( + "", + "500.0.0", + "hit = true" + ) + assert(hit) + } + + // Min bounds not hit + { + var hit = false + ScalaVersion.conditional( + "", + "0.0.0", + "hit = true" + ) + assert(!hit) + } + + // Min-max bound hit + { + var hit = false + ScalaVersion.conditional( + "0.0.0", + "500.0.0", + "hit = true" + ) + assert(hit) + } + + // Min-max bound not hit + { + var hit = false + ScalaVersion.conditional( + "500.0.0", + "1000.0.0", + "hit = true" + ) + assert(!hit) + } + } +} From d8e9765a2176e29599e329d378b9ef067ddb7525 Mon Sep 17 00:00:00 2001 From: Jamie Date: Sat, 1 Feb 2020 20:04:21 -0800 Subject: [PATCH 2/3] comments --- scala/private/rules/scala_library.bzl | 6 +- .../dependencyanalyzer/AstUsedJarFinder.scala | 4 +- .../dependencyanalyzer/ScalaVersion.scala | 75 ++++++++++++++----- .../dependencyanalyzer/ScalaVersionTest.scala | 45 ++++++----- 4 files changed, 83 insertions(+), 47 deletions(-) diff --git a/scala/private/rules/scala_library.bzl b/scala/private/rules/scala_library.bzl index 04bf15cdd..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) @@ -164,8 +166,6 @@ _scala_library_for_plugin_bootstrapping_attrs.update( common_attrs_for_plugin_bootstrapping, ) -_scala_library_for_plugin_bootstrapping_attrs["build_ijar"] = attr.bool(default = True) - def make_scala_library_for_plugin_bootstrapping(*extras): return rule( attrs = _dicts.add( 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 b76d93fa0..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 @@ -40,8 +40,8 @@ class AstUsedJarFinder( // We should examine OriginalTreeAttachment but that was only // added in 2.12.4, so include a version check ScalaVersion.conditional( - "2.12.4", - "", + Some("2.12.4"), + None, """ node.attachments .get[global.treeChecker.OriginalTreeAttachment] 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 index 9d6346047..911893eba 100644 --- 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 @@ -24,12 +24,17 @@ object ScalaVersion { * versions. The reason to use this rarely is the API's inflexibility * and the difficulty in debugging this code. * - * Each of minVersion and maxVersion can either be the empty string ("") - * to signify that there is no restriction on this bound. + * 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". * - * 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, no variables etc. i.e. + * 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(minVersion = "2.12.4", maxVersion = "", code = "foo()") @@ -37,8 +42,8 @@ object ScalaVersion { * conditional(minVersion = MinVersionForFoo, maxVersion = "", code = "foo()") */ def conditional( - minVersion: String, - maxVersion: String, + minVersionOpt: Option[String], + maxVersionOpt: Option[String], code: String ): Unit = macro conditionalImpl @@ -46,38 +51,70 @@ object ScalaVersion { def conditionalImpl( c: blackbox.Context )( - minVersion: c.Expr[String], - maxVersion: c.Expr[String], + minVersionOpt: c.Expr[Option[String]], + maxVersionOpt: c.Expr[Option[String]], code: c.Expr[String] ): c.Tree = { import c.{universe => u} - import u.Quasiquote - def extractString(expr: c.Expr[String]): String = { - expr.tree match { + + // 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)) => - s + 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 a string literal such as \"2.12.10\"") - "" + "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 minVersionStr = extractString(minVersion) - minVersionStr == "" || Current >= ScalaVersion(minVersionStr) + val minVersionOptValue = extractStringOption(minVersionOpt) + minVersionOptValue.forall(version => Current >= ScalaVersion(version)) } val meetsMaxVersionRequirement = { - val maxVersionStr = extractString(maxVersion) - maxVersionStr == "" || Current <= ScalaVersion(maxVersionStr) + val maxVersionOptValue = extractStringOption(maxVersionOpt) + maxVersionOptValue.forall(version => Current <= ScalaVersion(version)) } if (meetsMinVersionRequirement && meetsMaxVersionRequirement) { c.parse(extractString(code)) } else { - q"" + u.EmptyTree } } } 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 index 192f92678..c83535c1d 100644 --- 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 @@ -5,8 +5,8 @@ import third_party.dependency_analyzer.src.main.io.bazel.rulesscala.dependencyan class ScalaVersionTest extends FunSuite { test("version comparison works") { - // i.e. a>b - def testGreater(a: String, b: String): Unit = { + // 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) @@ -17,7 +17,6 @@ class ScalaVersionTest extends FunSuite { assert(va > vb) assert(va >= vb) - // Lesser versions assert(!(vb == va)) assert(vb != va) assert(vb < va) @@ -42,11 +41,11 @@ class ScalaVersionTest extends FunSuite { testEqual("1.2.3", "1.2.3") testEqual("30.20.10", "30.20.10") - testGreater("1.2.3", "1.0.0") - testGreater("1.2.1", "1.2.0") - testGreater("1.2.0", "1.1.9") - testGreater("2.12.12", "2.12.11") - testGreater("2.12.0", "2.1.50") + 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") { @@ -60,8 +59,8 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "", - "", + None, + None, "hit = true" ) assert(hit) @@ -71,8 +70,8 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "1.0.0", - "", + Some("1.0.0"), + None, "hit = true" ) assert(hit) @@ -82,8 +81,8 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "500.0.0", - "", + Some("500.0.0"), + None, "hit = true" ) assert(!hit) @@ -93,19 +92,19 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "", - "500.0.0", + None, + Some("500.0.0"), "hit = true" ) assert(hit) } - // Min bounds not hit + // Max bounds not hit { var hit = false ScalaVersion.conditional( - "", - "0.0.0", + None, + Some("1.0.0"), "hit = true" ) assert(!hit) @@ -115,8 +114,8 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "0.0.0", - "500.0.0", + Some("1.0.0"), + Some("500.0.0"), "hit = true" ) assert(hit) @@ -126,8 +125,8 @@ class ScalaVersionTest extends FunSuite { { var hit = false ScalaVersion.conditional( - "500.0.0", - "1000.0.0", + Some("500.0.0"), + Some("1000.0.0"), "hit = true" ) assert(!hit) From 17fa755d0b6f717e347d22660eff53f8b5dcdc69 Mon Sep 17 00:00:00 2001 From: Jamie Date: Sun, 2 Feb 2020 10:44:26 -0800 Subject: [PATCH 3/3] comments --- .../dependencyanalyzer/ScalaVersion.scala | 13 +++++++++++-- .../dependencyanalyzer/AstUsedJarFinderTest.scala | 12 +++++++++++- .../dependencyanalyzer/ScalaVersionTest.scala | 11 +++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) 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 index 911893eba..e7be56c5a 100644 --- 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 @@ -37,9 +37,9 @@ object ScalaVersion { * do not have a type attached. * * valid: - * conditional(minVersion = "2.12.4", maxVersion = "", code = "foo()") + * conditional(Some("2.12.4"), None, "foo()") * invalid: - * conditional(minVersion = MinVersionForFoo, maxVersion = "", code = "foo()") + * conditional(MinVersionForFoo, None, "foo()") */ def conditional( minVersionOpt: Option[String], @@ -103,11 +103,20 @@ object ScalaVersion { 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)) } 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 31a59218c..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 @@ -10,6 +10,10 @@ 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") @@ -401,7 +405,13 @@ class AstUsedJarFinderTest extends FunSuite { |} |""".stripMargin - // It is unclear why this only works with these versions but + // 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( 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 index c83535c1d..c3d46075c 100644 --- 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 @@ -55,6 +55,17 @@ class ScalaVersionTest extends FunSuite { // 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