Update dependency configuration options (bazelbuild#995)

* Update dependency configuration

* lint

* comments

* comment

* comments

README.md

@@ -158,31 +158,75 @@ replacements:
         "@io_bazel_rules_scala_scala_xml//:io_bazel_rules_scala_scala_xml"
 ```
 
-## [Experimental] Using strict-deps
-Bazel pushes towards explicit and minimal dependencies to keep BUILD file hygiene and allow for targets to refactor their dependencies without fear of downstream breaking.
-Currently rules_scala does this at the cost of having cryptic `scalac` errors when one mistakenly depends on a transitive dependency or, as more often the case for some, a transitive dependency is needed to [please scalac](https://github.com/scalacenter/advisoryboard/blob/master/proposals/009-improve-direct-dependency-experience.md) itself.
-To learn more about the motivation of strict-deps itself you can visit this Bazel blog [post](https://blog.bazel.build/2017/06/28/sjd-unused_deps.html) on the subject.
+## [Experimental] Dependency options
 
-To use it just add `--strict_java_deps=WARN|ERROR` to your `bazel` invocation.
-In both cases of `WARN` or `ERROR` you will get the following text in the event of a violation:
+There are a number of dependency options which can be set in the scala toolchain. These include `dependency_mode`, `strict_deps_mode`, `unused_dependency_checker_mode`, and `dependency_tracking_method`.
+
+### [Experimental] Recommended options
+
+We recommend one of the following sets of options
+
+**Option A**
+Accept the defaults, which might work well enough for you. The defaults are
 ```
-...
-Target '//some_package:transitive_dependency' is used but isn't explicitly declared, please add it to the deps.
-You can use the following buildozer command:
-buildozer 'add deps //some_package:transitive_dependency' //some_other_package:transitive_dependency_user
+  dependency_mode = "direct",
+  strict_deps_mode = "off",
+  unused_dependency_checker_mode = "off",
+  dependency_tracking_method = "high-level",
+```
+but you do not need to include this in the toolchain as they are the defaults.
+
+**Option B**
+```
+  dependency_mode = "plus-one",
+  strict_deps_mode = "error",
+  unused_dependency_checker_mode = "error",
+  dependency_tracking_method = "ast",
 ```
-Note that if you have `buildozer` installed you can just run the last line and have it automatically apply the fix for you.
 
-**Caveats:**
+Should the first option result in too much effort in handling build files and the like due to confusing dependencies and you becoming confused as to why some specific dependency is needed when the code being compiled never references it, consider this set of options. It will include both dependencies and dependencies of dependencies, which in practice is enough to stop almost all strange missing dependency errors at the cost of somewhat more incremental compile cost in certain cases.
+
+With these settings, we also will error on dependencies which are unneeded, and dependencies which should be included in `deps` due to be directly referenced in the code, but are not.
+
+The dependency tracking method `ast` is experimental but so far proves to be better than the default for computing the direct dependencies for `plus-one` mode code. In the future we hope to make this the default for `plus-one` mode and remove the option altogether.
+
+### [Experimental] Dependency mode
+
+There are three dependency modes. The reason for the multiple modes is that often `scalac` depends on jars which seem unnecessary at first glance. Hence, in order to reduce the need to please `scalac`, we provide the following options.
+- `dependency_mode = "direct"` - only include direct dependencies during compiliation; that is, those in the `deps` attribute
+- `dependency_mode = "plus-one"` - only include `deps` and `deps` of `deps` during compiliation.
+- `dependency_mode = "transitive"` - all transitive dependencies are included during compiliation. That is, `deps`, `deps` of `deps`, `deps` of `deps` of `deps`, and so on.
+
+Note when a dependency is included, that means its jars are included on the classpath, along with the jars of any targets that it exports.
+
+When using `direct` mode, there can be cryptic `scalac` errors when one mistakenly depends on a transitive dependency or, as more often the case for some, a transitive dependency is needed to [please scalac](https://github.com/scalacenter/advisoryboard/blob/master/proposals/009-improve-direct-dependency-experience.md) itself.
+
+As one goes down the list, more dependencies are included which helps reduce confusing requirements to add `deps`, at the cost of increased incremental builds due to a greater number of dependencies. In practice, using `plus-one` deps results in almost no confusing `deps` entries required while still being relatively small in terms of the number of total dependencies included.
+
+**Caveats for `plus_one` and `transitive`:**
 <ul>
-    <li>Extra builds- when strict-deps is on the transitive dependencies are inputs to the compilation action which means you can potentially have more build triggers for changes the cross the ijar boundary </li>
+    <li>Extra builds- Extra dependencies are inputs to the compilation action which means you can potentially have more build triggers for changes the cross the ijar boundary </li>
     <li>Label propagation- since label of targets are needed for the clear message and since it's not currently supported by JavaInfo from bazel we manually propagate it. This means that the error messages have a significantly lower grade if you don't use one of the scala rules or scala_import (since they don't propagate these labels)</li>
     <li>javac outputs incorrect targets due to a problem we're tracing down. Practically we've noticed it's pretty trivial to understand the correct target (i.e. it's almost a formatting problem) </li>
   </ul>
 
-Note: Currently strict-deps is protected by a feature toggle but we're strongly considering making it the default behavior as `java_*` rules do.
+Note: the last two issues are bugs which will be addressed by [https://github.com/bazelbuild/rules_scala/issues/839].
+
+### [Experimental] Strict deps mode
+We have a strict dependency checker which requires that any type referenced in the sources of a scala target should be included in that rule's deps. To learn about the motivation for this you can visit this Bazel blog [post](https://blog.bazel.build/2017/06/28/sjd-unused_deps.html) on the subject.
 
-## [Experimental] Unused dependency checking
+The option `strict_deps_mode` can be set to `off`, `warn`, or `error`. We highly recommend setting it to `error`.
+
+In both cases of `warn` or `error` you will get the following text in the event of a violation:
+```
+...
+Target '//some_package:transitive_dependency' is used but isn't explicitly declared, please add it to the deps.
+You can use the following buildozer command:
+buildozer 'add deps //some_package:transitive_dependency' //some_other_package:transitive_dependency_user
+```
+Note that if you have `buildozer` installed you can just run the last line and have it automatically apply the fix for you.
+
+### [Experimental] Unused dependency checking
 To allow for better caching and faster builds we want to minimize the direct dependencies of our targets. Unused dependency checking
 makes sure that all targets specified as direct dependencies are actually used. If `unused_dependency_checker_mode` is set to either
 `error` or `warn` you will get the following message for any dependencies that are not used:
@@ -192,12 +236,38 @@ You can use the following buildozer command:
 buildozer 'remove deps //some_package:unused_dep' //target:target
 ```
 
-Currently unused dependency checking and strict-deps can't be used simultaneously, if both are set only strict-deps will run.
-
 Unused dependency checking can either be enabled globally for all targets using a scala toolchain or for individual targets using the
-`unused_dependency_checker_mode` attribute. The feature is still experimental and there can thus be cases where it works incorrectly,
-in these cases you can enable unused dependency checking globally through a toolchain and override individual misbehaving targets
-using the attribute.
+`unused_dependency_checker_mode` attribute.
+
+The feature is still experimental and there can thus be cases where it works incorrectly, in these cases you can enable unused dependency checking globally through a toolchain and disable reports of individual misbehaving targets with `unused_dependency_checker_ignored_targets` which is a list of labels.
+
+### [Experimental] Dependency tracking method
+
+The strict dependency tracker and unused dependency tracker need to track the used dependencies of a scala compilation unit. This toggle allows one to pick which method of tracking to use.
+
+- `dependency_tracking_method = "high-level"` - This is the existing tracking method which has false positives and negatives but generally works reasonably well for `direct` dependency mode.
+- `dependency_tracking_method = "ast"` - This is a new tracking method which is being developed for `plus-one` and `transitive` dependency modes. It is still being developed and may have issues which need fixing. If you discover an issue, please submit a small repro of the problem.
+
+Note we intend to eventually remove this flag and use `high-level` as the method for `direct` dependency mode, and `ast` as the method for `plus-one` and `transitive` dependency modes.
+
+In the meantime, if you are using `plus-one` or `transitive` dependency modes, you can use `ast` dependency tracking mode and see how well it works for you.
+
+### [Experimental] Turning on strict_deps_mode/unused_dependency_checker_mode
+
+It can be daunting to turn on strict deps checking or unused dependency mode checking on a large codebase. However, it need not be so bad if this is done in phases
+
+1. Have a default scala toolchain `A` with the option of interest set to `off` (the starting state)
+2. Create a second scala toolchain `B` with the option of interest set to `warn` or `error`. Those who are working on enabling the flag can run with this toolchain as a command line argument to help identify issues and fix them.
+3. Once all issues are fixed, change `A` to have the option of interest set to `error` and delete `B`.
+
+We recommend turning on strict_deps_mode first, as rule `A` might have an entry `B` in its `deps`, and `B` in turn depends on `C`. Meanwhile, the code of `A` only uses `C` but not `B`. Hence, the unused dependency checker, if on, will request that `B` be removed from `A`'s deps. But this will lead to a compile error as `A` can no longer depend on `C`. However, if strict dependency checking was on, then `A`'s deps is guaranteed to have `C` in it.
+
+### [Experimental] Migrating from deprecated configurations
+
+There are a few deprecated configuration methods which we will be removing in the near future.
+
+- `plus_one_deps_mode = "on"` on the scala toolchain. Instead, set `dependency_mode = "plus-one"` on the scala toolchain. `plus_one_deps_mode` will be removed in the future.
+- The command line argument `--strict_java_deps=WARN/ERROR`. Instead, set `dependency_mode = "transitive"` on the scala toolchain, and if only a warning is desired set `strict_deps_mode = "warn"` on the toolchain. In the future, `strict_java_deps` will no longer affect how scala files are compiled. Note that `strict_java_deps` will still control java compilation.
 
 ## Advanced configurable rules
 To make the ruleset more flexible and configurable, we introduce a phase architecture. By using a phase architecture, where rule implementations are defined as a list of phases that are executed sequentially, functionality can easily be added (or modified) by adding (or swapping) phases.

scala/plusone.bzl

@@ -10,7 +10,7 @@ PlusOneDeps = provider(
 )
 
 def _collect_plus_one_deps_aspect_impl(target, ctx):
-    if (ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].plus_one_deps_mode == "off"):
+    if (ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].dependency_mode != "plus-one"):
         return []
     export_plus_one_deps = []
     for exported_dep in getattr(ctx.rule.attr, "exports", []):

scala/private/dependency.bzl

@@ -8,11 +8,6 @@ def new_dependency_info(
         strict_deps_mode,
         dependency_tracking_method):
     is_strict_deps_on = strict_deps_mode != "off"
-
-    # Currently we do not support running both strict and unused deps at the same time
-    if is_strict_deps_on:
-        unused_deps_mode = "off"
-
     is_unused_deps_on = unused_deps_mode != "off"
 
     need_direct_jars = is_strict_deps_on or is_unused_deps_on
@@ -24,7 +19,7 @@ def new_dependency_info(
         need_direct_jars = need_direct_jars,
         need_direct_targets = need_direct_targets,
         need_direct_info = need_direct_jars or need_direct_targets,
-        dependency_tracking_method = "high-level",
+        dependency_tracking_method = dependency_tracking_method,
         unused_deps_mode = unused_deps_mode,
         strict_deps_mode = strict_deps_mode,
         use_analyzer = is_strict_deps_on or is_unused_deps_on,
@@ -41,7 +36,7 @@ def legacy_unclear_dependency_info_for_protobuf_scrooge(ctx):
 
 # TODO(https://github.com/bazelbuild/rules_scala/issues/987): Clariy the situation
 def _legacy_unclear_dependency_mode_for_protobuf_scrooge(ctx):
-    if is_strict_deps_on(ctx):
+    if _is_strict_deps_on(ctx):
         return "transitive"
     else:
         return "direct"
@@ -50,11 +45,7 @@ def get_strict_deps_mode(ctx):
     if not hasattr(ctx.attr, "_dependency_analyzer_plugin"):
         return "off"
 
-    # when the strict deps FT is removed the "default" check
-    # will be removed since "default" will mean it's turned on
-    if ctx.fragments.java.strict_java_deps == "default":
-        return "off"
-    return ctx.fragments.java.strict_java_deps
+    return ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].strict_deps_mode
 
-def is_strict_deps_on(ctx):
+def _is_strict_deps_on(ctx):
     return get_strict_deps_mode(ctx) != "off"

scala/private/phases/phase_dependency.bzl

@@ -3,7 +3,6 @@
 load(
     "@io_bazel_rules_scala//scala/private:dependency.bzl",
     "get_strict_deps_mode",
-    "is_strict_deps_on",
     "new_dependency_info",
 )
 load(
@@ -35,6 +34,8 @@ def _phase_dependency(
         p,
         unused_deps_always_off,
         strict_deps_always_off):
+    toolchain = ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"]
+
     if strict_deps_always_off:
         strict_deps_mode = "off"
     else:
@@ -52,26 +53,12 @@ def _phase_dependency(
         unused_deps_mode = "off"
 
     return new_dependency_info(
-        _get_dependency_mode(ctx),
+        toolchain.dependency_mode,
         unused_deps_mode,
         strict_deps_mode,
-        "high-level",
+        toolchain.dependency_tracking_method,
     )
 
-def _is_plus_one_deps_on(ctx):
-    return ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].plus_one_deps_mode != "off"
-
-def _get_dependency_mode(ctx):
-    if is_strict_deps_on(ctx):
-        # all transitive dependencies are included
-        return "transitive"
-    elif _is_plus_one_deps_on(ctx):
-        # dependencies and dependencies of dependencies are included
-        return "plus-one"
-    else:
-        # only explicitly-specified dependencies are included
-        return "direct"
-
 def _get_unused_deps_mode(ctx):
     if ctx.attr.unused_dependency_checker_mode:
         return ctx.attr.unused_dependency_checker_mode

scala/scala_toolchain.bzl

@@ -3,12 +3,74 @@ load(
     _ScalacProvider = "ScalacProvider",
 )
 
+def _compute_dependency_mode(input_dependency_mode, input_plus_one_deps_mode):
+    if input_plus_one_deps_mode == "on":
+        return "plus-one"
+
+    if input_dependency_mode == "":
+        return "direct"
+
+    return input_dependency_mode
+
+def _compute_strict_deps_mode(input_strict_deps_mode, dependency_mode):
+    if dependency_mode == "direct":
+        return "off"
+    if input_strict_deps_mode == "default":
+        if dependency_mode == "transitive":
+            return "error"
+        else:
+            return "off"
+    return input_strict_deps_mode
+
+def _compute_dependency_tracking_method(input_dependency_tracking_method):
+    if input_dependency_tracking_method == "default":
+        return "high-level"
+    return input_dependency_tracking_method
+
 def _scala_toolchain_impl(ctx):
+    if ctx.attr.plus_one_deps_mode != "":
+        print(
+            "Setting plus_one_deps_mode on toolchain is deprecated." +
+            "Use 'dependency_mode = \"plus-one\"' instead",
+        )
+    if ctx.attr.dependency_mode != "" and ctx.attr.plus_one_deps_mode != "":
+        fail("Cannot set both dependency_mode and plus_one_deps_mode on toolchain")
+
+    if ctx.fragments.java.strict_java_deps != "default" and ctx.fragments.java.strict_java_deps != "off":
+        dependency_mode = "transitive"
+        strict_deps_mode = ctx.fragments.java.strict_java_deps
+        unused_dependency_checker_mode = "off"
+        dependency_tracking_method = "high-level"
+    else:
+        dependency_mode = _compute_dependency_mode(
+            ctx.attr.dependency_mode,
+            ctx.attr.plus_one_deps_mode,
+        )
+        strict_deps_mode = _compute_strict_deps_mode(
+            ctx.attr.strict_deps_mode,
+            dependency_mode,
+        )
+
+        unused_dependency_checker_mode = ctx.attr.unused_dependency_checker_mode
+        dependency_tracking_method = _compute_dependency_tracking_method(ctx.attr.dependency_tracking_method)
+
+    # Final quality checks to possibly detect buggy code above
+    if dependency_mode not in ("direct", "plus-one", "transitive"):
+        fail("Internal error: invalid dependency_mode " + dependency_mode)
+
+    if strict_deps_mode not in ("off", "warn", "error"):
+        fail("Internal error: invalid strict_deps_mode " + strict_deps_mode)
+
+    if dependency_tracking_method not in ("ast", "high-level"):
+        fail("Internal error: invalid dependency_tracking_method " + dependency_tracking_method)
+
     toolchain = platform_common.ToolchainInfo(
         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,
+        dependency_mode = dependency_mode,
+        strict_deps_mode = strict_deps_mode,
+        unused_dependency_checker_mode = unused_dependency_checker_mode,
+        dependency_tracking_method = dependency_tracking_method,
         enable_code_coverage_aspect = ctx.attr.enable_code_coverage_aspect,
         scalac_jvm_flags = ctx.attr.scalac_jvm_flags,
         scala_test_jvm_flags = ctx.attr.scala_test_jvm_flags,
@@ -23,13 +85,23 @@ scala_toolchain = rule(
             default = "@io_bazel_rules_scala//scala:scalac_default",
             providers = [_ScalacProvider],
         ),
+        "dependency_mode": attr.string(
+            values = ["direct", "plus-one", "transitive", ""],
+        ),
+        "strict_deps_mode": attr.string(
+            default = "default",
+            values = ["off", "warn", "error", "default"],
+        ),
         "unused_dependency_checker_mode": attr.string(
             default = "off",
             values = ["off", "warn", "error"],
         ),
+        "dependency_tracking_method": attr.string(
+            default = "default",
+            values = ["ast", "high-level", "default"],
+        ),
         "plus_one_deps_mode": attr.string(
-            default = "off",
-            values = ["off", "on"],
+            values = ["off", "on", ""],
         ),
         "enable_code_coverage_aspect": attr.string(
             default = "off",
@@ -38,4 +110,5 @@ scala_toolchain = rule(
         "scalac_jvm_flags": attr.string_list(),
         "scala_test_jvm_flags": attr.string_list(),
     },
+    fragments = ["java"],
 )

test/scala_test/A.scala

@@ -0,0 +1,10 @@
+
+import org.scalatest._
+
+abstract class A extends FunSuite {
+	val Number: Int
+
+	test("number is positive") {
+		assert(Number > 0)
+	}
+}

test/scala_test/B.scala

@@ -0,0 +1,4 @@
+
+class B extends A {
+	override val Number: Int = 12
+}

test/scala_test/BUILD

@@ -0,0 +1,12 @@
+load("@io_bazel_rules_scala//scala:scala.bzl", "scala_test")
+
+scala_test(
+    name = "a",
+    srcs = ["A.scala"],
+)
+
+scala_test(
+    name = "b",
+    srcs = ["B.scala"],
+    deps = [":a"],
+)