Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle inlined literals in AST walker #985

Merged
merged 4 commits into from
Feb 2, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions scala/private/phases/phase_compile.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
4 changes: 3 additions & 1 deletion scala/private/rules/scala_library.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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),
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
}

_scala_library_for_plugin_bootstrapping_attrs.update(implicit_deps)

Expand Down
16 changes: 16 additions & 0 deletions third_party/dependency_analyzer/src/main/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand All @@ -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",
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
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.
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
*
* 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(minVersion = "2.12.4", maxVersion = "", code = "foo()")
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
* invalid:
* conditional(minVersion = MinVersionForFoo, maxVersion = "", code = "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)
minVersionOptValue.forall(version => Current >= ScalaVersion(version))
}

val meetsMaxVersionRequirement = {
val maxVersionOptValue = extractStringOption(maxVersionOpt)
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"
}
}
16 changes: 14 additions & 2 deletions third_party/dependency_analyzer/src/test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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`.
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
// (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",
Expand Down Expand Up @@ -331,4 +386,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 = {}
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
| 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")
)
}
}
}
}
Loading