add plus one deps transitive mode for compilation (#698)

* add plus one deps transitive mode for compilation
tries to balance between sterness of strict-deps off and strict-deps on cache issues and false positives

* Update .travis.yml

* move plusone provider and aspect to separate file

* try 17 again after moving out provider

* move configuration from `define` to attribute on toolchain

* added documentation for the plusone provider

* remove trailing 9

Author: ittaiz

WORKSPACE

@@ -171,4 +171,30 @@ git_repository(
     name = "bazel_skylib",
     remote = "https://github.com/bazelbuild/bazel-skylib.git",
     tag = "0.6.0",
-)
+)
+
+## deps for tests of limited deps support
+scala_maven_import_external(
+    name = "org_springframework_spring_core",
+    artifact = "org.springframework:spring-core:5.1.5.RELEASE",
+    jar_sha256 = "f771b605019eb9d2cf8f60c25c050233e39487ff54d74c93d687ea8de8b7285a",
+    licenses = ["notice"],  # Apache 2.0
+    server_urls = [
+        "https://repo1.maven.org/maven2/",
+        "https://mirror.bazel.build/repo1.maven.org/maven2",
+        ],
+)
+
+scala_maven_import_external(
+    name = "org_springframework_spring_tx",
+    artifact = "org.springframework:spring-tx:5.1.5.RELEASE",
+    jar_sha256 = "666f72b73c7e6b34e5bb92a0d77a14cdeef491c00fcb07a1e89eb62b08500135",
+    licenses = ["notice"],  # Apache 2.0
+    server_urls = [
+        "https://repo1.maven.org/maven2/",
+        "https://mirror.bazel.build/repo1.maven.org/maven2",
+        ],
+    deps = [
+        "@org_springframework_spring_core"
+    ]
+)

scala/plusone.bzl

@@ -0,0 +1,17 @@
+"""
+Keeps direct compile dependencies of targets.
+This enables targets to pass the to compiler the plus one dependencies in addition to the direct ones.
+For motivation of plus one see the e2e tests
+"""
+PlusOneDeps = provider(
+    fields = {
+        'direct_deps' : 'list of direct compile dependencies of a target',
+    }
+)
+
+def _collect_plus_one_deps_aspect_impl(target, ctx):
+    return [PlusOneDeps(direct_deps = getattr(ctx.rule.attr,'deps',[]))]
+
+collect_plus_one_deps_aspect = aspect(implementation = _collect_plus_one_deps_aspect_impl,
+    attr_aspects = ['deps'],
+)

scala/private/common.bzl

@@ -1,4 +1,5 @@
 load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo")
+load("@io_bazel_rules_scala//scala:plusone.bzl", "PlusOneDeps")
 
 def write_manifest(ctx):
     main_class = getattr(ctx.attr, "main_class", None)
@@ -22,21 +23,25 @@ def collect_srcjars(targets):
 def collect_jars(
         dep_targets,
         dependency_analyzer_is_off = True,
-        unused_dependency_checker_is_off = True):
+        unused_dependency_checker_is_off = True,
+        plus_one_deps_is_off = True):
     """Compute the runtime and compile-time dependencies from the given targets"""  # noqa
 
     if dependency_analyzer_is_off:
         return _collect_jars_when_dependency_analyzer_is_off(
             dep_targets,
             unused_dependency_checker_is_off,
+            plus_one_deps_is_off,
         )
     else:
         return _collect_jars_when_dependency_analyzer_is_on(dep_targets)
 
 def _collect_jars_when_dependency_analyzer_is_off(
         dep_targets,
-        unused_dependency_checker_is_off):
+        unused_dependency_checker_is_off,
+        plus_one_deps_is_off):
     compile_jars = []
+    plus_one_deps_compile_jars = []
     runtime_jars = []
     jars2labels = {}
 
@@ -58,11 +63,16 @@ def _collect_jars_when_dependency_analyzer_is_off(
         else:
             print("ignored dependency, has no JavaInfo: " + str(dep_target))
 
+        if (not plus_one_deps_is_off) and (PlusOneDeps in dep_target):
+            plus_one_deps_compile_jars.append(
+                depset(transitive = [dep[JavaInfo].compile_jars for dep in dep_target[PlusOneDeps].direct_deps if JavaInfo in dep ])
+            )
+
     return struct(
         compile_jars = depset(transitive = compile_jars),
         transitive_runtime_jars = depset(transitive = runtime_jars),
         jars2labels = JarsToLabelsInfo(jars_to_labels = jars2labels),
-        transitive_compile_jars = depset(),
+        transitive_compile_jars = depset(transitive = compile_jars + plus_one_deps_compile_jars),
     )
 
 def _collect_jars_when_dependency_analyzer_is_on(dep_targets):

scala/private/rule_impls.bzl

@@ -220,6 +220,8 @@ CurrentTarget: {current_target}
             ignored_targets = ignored_targets,
             current_target = current_target,
         )
+    if is_dependency_analyzer_off(ctx) and not _is_plus_one_deps_off(ctx):
+       compiler_classpath_jars = transitive_compile_jars
 
     plugins_list = plugins.to_list()
     plugin_arg = _join_path(plugins_list)
@@ -616,6 +618,9 @@ def is_dependency_analyzer_on(ctx):
 def is_dependency_analyzer_off(ctx):
     return not is_dependency_analyzer_on(ctx)
 
+def _is_plus_one_deps_off(ctx):
+    return ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].plus_one_deps_mode == "off"
+
 # Extract very common code out from dependency analysis into single place
 # automatically adds dependency on scala-library and scala-reflect
 # collects jars from deps, runtime jars from runtime_deps, and
@@ -631,6 +636,7 @@ def _collect_jars_from_common_ctx(
         ctx.attr.deps + extra_deps + base_classpath,
         dependency_analyzer_is_off,
         unused_dependency_checker_is_off,
+        _is_plus_one_deps_off(ctx),
     )
 
     (
@@ -985,9 +991,10 @@ def scala_test_impl(ctx):
     ]
     unused_dependency_checker_is_off = unused_dependency_checker_mode == "off"
 
+    scalatest_base_classpath = scalac_provider.default_classpath + [ctx.attr._scalatest]
     jars = _collect_jars_from_common_ctx(
         ctx,
-        scalac_provider.default_classpath,
+        scalatest_base_classpath,
         extra_runtime_deps = [
             ctx.attr._scalatest_reporter,
             ctx.attr._scalatest_runner,
@@ -1006,36 +1013,6 @@ def scala_test_impl(ctx):
         jars.jars2labels,
     )
 
-    # _scalatest is an http_jar, so its compile jar is run through ijar
-    # however, contains macros, so need to handle separately
-    scalatest_jars = collect_jars([ctx.attr._scalatest]).transitive_runtime_jars
-    cjars = depset(transitive = [cjars, scalatest_jars])
-    transitive_rjars = depset(transitive = [transitive_rjars, scalatest_jars])
-
-    if is_dependency_analyzer_on(ctx):
-        transitive_compile_jars = depset(
-            transitive = [scalatest_jars, transitive_compile_jars],
-        )
-        scalatest_jars_list = scalatest_jars.to_list()
-        j2l = jars_to_labels.jars_to_labels
-        add_labels_of_jars_to(
-            j2l,
-            ctx.attr._scalatest,
-            scalatest_jars_list,
-            scalatest_jars_list,
-        )
-        jars_to_labels = JarsToLabelsInfo(jars_to_labels = j2l)
-
-    elif not unused_dependency_checker_is_off:
-        j2l = jars_to_labels.jars_to_labels
-        add_labels_of_jars_to(
-            j2l,
-            ctx.attr._scalatest,
-            [],
-            scalatest_jars.to_list(),
-        )
-        jars_to_labels = JarsToLabelsInfo(jars_to_labels = j2l)
-
     args = " ".join([
         "-R \"{path}\"".format(path = ctx.outputs.jar.short_path),
         _scala_test_flags(ctx),

scala/scala.bzl

@@ -24,6 +24,10 @@ load(
     "@io_bazel_rules_scala//specs2:specs2_junit.bzl",
     _specs2_junit_dependencies = "specs2_junit_dependencies",
 )
+load(
+    "@io_bazel_rules_scala//scala:plusone.bzl",
+    _collect_plus_one_deps_aspect = "collect_plus_one_deps_aspect",
+)
 
 _launcher_template = {
     "_java_stub_template": attr.label(
@@ -105,7 +109,7 @@ _junit_resolve_deps = {
 # Common attributes reused across multiple rules.
 _common_attrs_for_plugin_bootstrapping = {
     "srcs": attr.label_list(allow_files = [".scala", ".srcjar", ".java"]),
-    "deps": attr.label_list(),
+    "deps": attr.label_list(aspects = [_collect_plus_one_deps_aspect]),
     "plugins": attr.label_list(allow_files = [".jar"]),
     "runtime_deps": attr.label_list(providers = [[JavaInfo]]),
     "data": attr.label_list(allow_files = True),

scala/scala_toolchain.bzl

@@ -8,6 +8,7 @@ def _scala_toolchain_impl(ctx):
         scalacopts = ctx.attr.scalacopts,
         scalac_provider_attr = ctx.attr.scalac_provider_attr,
         unused_dependency_checker_mode = ctx.attr.unused_dependency_checker_mode,
+        plus_one_deps_mode = ctx.attr.plus_one_deps_mode,
     )
     return [toolchain]
 
@@ -23,5 +24,9 @@ scala_toolchain = rule(
             default = "off",
             values = ["off", "warn", "error"],
         ),
+        "plus_one_deps_mode": attr.string(
+            default = "off",
+            values = ["off", "on"],
+        ),
     },
 )

test_expect_failure/plus_one_deps/BUILD.bazel

@@ -0,0 +1,25 @@
+load("//scala:scala_toolchain.bzl", "scala_toolchain")
+scala_toolchain(
+    name = "plus_one_deps_impl",
+    plus_one_deps_mode = "on",
+    visibility = ["//visibility:public"],
+)
+toolchain(
+    name = "plus_one_deps",
+    toolchain = "plus_one_deps_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+scala_toolchain(
+    name = "plus_one_deps_with_unused_error_impl",
+    unused_dependency_checker_mode = "error",
+    plus_one_deps_mode = "on",
+    visibility = ["//visibility:public"],
+)
+toolchain(
+    name = "plus_one_deps_with_unused_error",
+    toolchain = "plus_one_deps_with_unused_error_impl",
+    toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
+    visibility = ["//visibility:public"],
+)
+

test_expect_failure/plus_one_deps/exports_deps/A.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class A {
+  println(new B().hi)
+}

test_expect_failure/plus_one_deps/exports_deps/B.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class B extends C {
+  def hi: String = "hi"
+}

test_expect_failure/plus_one_deps/exports_deps/BUILD.bazel

@@ -0,0 +1,21 @@
+load("//scala:scala.bzl", "scala_library")
+scala_library(
+    name = "a",
+    srcs = ["A.scala"],
+    deps = [":b"],
+)
+scala_library(
+    name = "b",
+    srcs = ["B.scala"],
+    deps = [":c"],
+)
+scala_library(
+    name = "c",
+    srcs = ["C.scala"],
+    deps = [":d"],
+    exports = ["d"],
+)
+scala_library(
+    name = "d",
+    srcs = ["D.scala"],
+)

test_expect_failure/plus_one_deps/exports_deps/C.scala

@@ -0,0 +1,3 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class C extends D

test_expect_failure/plus_one_deps/exports_deps/D.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class D {
+
+}

test_expect_failure/plus_one_deps/external_deps/A.scala

@@ -0,0 +1,6 @@
+package scalarules.test_expect_failure.plus_one_deps.external_deps
+import org.springframework.dao.DataAccessException
+
+class A {
+  println(classOf[DataAccessException])
+}

test_expect_failure/plus_one_deps/external_deps/BUILD.bazel

@@ -0,0 +1,7 @@
+load("//scala:scala.bzl", "scala_library")
+scala_library(
+    name = "a",
+    srcs = ["A.scala"],
+    deps = ["@org_springframework_spring_tx"]
+
+)

test_expect_failure/plus_one_deps/internal_deps/A.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class A {
+  println(new B().hi)
+}

test_expect_failure/plus_one_deps/internal_deps/B.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class B extends C {
+  def hi: String = "hi"
+}

test_expect_failure/plus_one_deps/internal_deps/BUILD.bazel

@@ -0,0 +1,15 @@
+load("//scala:scala.bzl", "scala_library")
+scala_library(
+    name = "a",
+    srcs = ["A.scala"],
+    deps = [":b"],
+)
+scala_library(
+    name = "b",
+    srcs = ["B.scala"],
+    deps = [":c"],
+)
+scala_library(
+    name = "c",
+    srcs = ["C.scala"],
+)

test_expect_failure/plus_one_deps/internal_deps/C.scala

@@ -0,0 +1,3 @@
+package scalarules.test_expect_failure.plus_one_deps.internal_deps
+
+class C

test_expect_failure/plus_one_deps/with_unused_deps/A.scala

@@ -0,0 +1,5 @@
+package scalarules.test_expect_failure.plus_one_deps.with_unused_deps
+
+class A {
+  println(new B().hi)
+}

test_expect_failure/plus_one_deps/with_unused_deps/B.scala

@@ -0,0 +1,8 @@
+package scalarules.test_expect_failure.plus_one_deps.with_unused_deps
+
+class B {
+  def hi: String = {
+    println(classOf[C])
+    "hi"
+  }
+}

test_expect_failure/plus_one_deps/with_unused_deps/BUILD.bazel

@@ -0,0 +1,15 @@
+load("//scala:scala.bzl", "scala_library")
+scala_library(
+    name = "a",
+    srcs = ["A.scala"],
+    deps = [":b",":c"],
+)
+scala_library(
+    name = "b",
+    srcs = ["B.scala"],
+    deps = [":c"],
+)
+scala_library(
+    name = "c",
+    srcs = ["C.scala"],
+)

test_expect_failure/plus_one_deps/with_unused_deps/C.scala

@@ -0,0 +1,3 @@
+package scalarules.test_expect_failure.plus_one_deps.with_unused_deps
+
+class C

test_expect_failure/scala_import/ScalaImportPropagatesCompileDepsTest.scala

@@ -7,7 +7,7 @@ import org.specs2.mutable.SpecificationWithJUnit
 class ScalaImportPropagatesCompileDepsTest extends SpecificationWithJUnit {
 
   "scala_import" should {
-    "propagate runtime deps" in {
+    "propagate compile time deps" in {
       println(classOf[Cache[String, String]])
       println(classOf[ArrayUtils])
       println(classOf[cats.Applicative[Any]])