Use a scala compiler plugin to throw errors on missing direct-deps (#243

)

First step (experimental) in the introduction of strict scala deps feature (see issue #235).

Currently this feature is turned off (due to noticeable impact on performance)

switching ‘dependency_analyzer_mode_soon_to_be_removed’ to “on” means that ANY API change in a target’s transitive dependencies will trigger a recompilation of that target, on the other hand any internal change (i.e. on code that ijar omits) WON’T trigger recompilation by transitive dependencies

A few implementation  details:
A new scalac plugin informs the user on every target that has missing transitive dependencies that should be added as direct dependencies. For convenience a buildozer command is provided to make the change in the BUILD file easily.

before the compilation, transitive compile time jars are collected along with a mapping to the originating target label.

.gitignore

@@ -3,3 +3,4 @@ bazel-*
 *.idea
 hash1
 hash2
+.DS_store

CONTRIBUTORS

@@ -19,3 +19,5 @@ Laurent Le Brun <[email protected]>
 Nathan Harmata <[email protected]>
 Oscar Boykin <[email protected]>
 Justine Alexandra Roberts Tunney <[email protected]>
+Natan Silnitsky <[email protected]>
+Nadav Wexler <[email protected]>

WORKSPACE

@@ -1,5 +1,7 @@
 workspace(name = "io_bazel_rules_scala")
 
+
+
 load("//scala:scala.bzl", "scala_repositories", "scala_mvn_artifact")
 scala_repositories()
 
@@ -28,4 +30,15 @@ maven_jar(
   artifact = scala_mvn_artifact("org.psywerx.hairyfotr:linter:0.1.13"),
   sha1 = "e5b3e2753d0817b622c32aedcb888bcf39e275b4")
 
+# test of strict deps (scalac plugin UT + E2E)
+maven_jar(
+    name = "com_google_guava_guava_21_0",
+    artifact = "com.google.guava:guava:21.0",
+    sha1 = "3a3d111be1be1b745edfa7d91678a12d7ed38709"
+)
 
+maven_jar(
+    name = "org_apache_commons_commons_lang_3_5",
+    artifact = "org.apache.commons:commons-lang3:3.5",
+    sha1 = "6c6c702c89bfff3cd9e80b04d668c5e190d588c6"
+)

src/java/io/bazel/rulesscala/scalac/CompileOptions.java

@@ -22,6 +22,11 @@ public class CompileOptions {
   final public Map<String, String> resourceFiles;
   final public String resourceStripPrefix;
   final public String[] resourceJars;
+  final public String[] directJars;
+  final public String[] indirectJars;
+  final public String[] indirectTargets;
+  final public String dependencyAnalyzerMode;
+  final public String currentTarget;
 
   public CompileOptions(List<String> args) {
     Map<String, String> argMap = buildArgMap(args);
@@ -52,6 +57,13 @@ public CompileOptions(List<String> args) {
     resourceFiles = getResources(argMap);
     resourceStripPrefix = getOrEmpty(argMap, "ResourceStripPrefix");
     resourceJars = getCommaList(argMap, "ResourceJars");
+
+    directJars = getCommaList(argMap, "DirectJars");
+    indirectJars = getCommaList(argMap, "IndirectJars");
+    indirectTargets = getCommaList(argMap, "IndirectTargets");
+
+    dependencyAnalyzerMode = getOrElse(argMap, "DependencyAnalyzerMode", "off");
+    currentTarget = getOrElse(argMap, "CurrentTarget", "NA");
   }
 
   private static Map<String, String> getResources(Map<String, String> args) {
@@ -96,10 +108,14 @@ private static String[] getCommaList(Map<String, String> m, String k) {
   }
 
   private static String getOrEmpty(Map<String, String> m, String k) {
-    if(m.containsKey(k)) {
-      return m.get(k);
+    return getOrElse(m, k, "");
+  }
+
+  private static String getOrElse(Map<String, String> attrs, String key, String defaultValue) {
+    if(attrs.containsKey(key)) {
+      return attrs.get(key);
     } else {
-      return "";
+      return defaultValue;
     }
   }
 

src/java/io/bazel/rulesscala/scalac/ScalacProcessor.java

@@ -17,11 +17,7 @@
 import java.nio.file.Paths;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Enumeration;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.Map.Entry;
 import java.util.jar.JarEntry;
 import java.util.jar.JarFile;
@@ -187,7 +183,45 @@ private static boolean matchesFileExtensions(String fileName, String[] extension
     return false;
   }
 
+  private static String[] encodeBazelTargets(String[] targets) {
+    return Arrays.stream(targets)
+            .map(ScalacProcessor::encodeBazelTarget)
+            .toArray(String[]::new);
+  }
+
+  private static String encodeBazelTarget(String target) {
+    return target.replace(":", ";");
+  }
+
+  private static boolean isModeEnabled(String mode) {
+    return !"off".equals(mode);
+  }
+
+  private static String[] getPluginParamsFrom(CompileOptions ops) {
+    String[] pluginParams;
+
+    if (isModeEnabled(ops.dependencyAnalyzerMode)) {
+      String[] targets = encodeBazelTargets(ops.indirectTargets);
+      String currentTarget = encodeBazelTarget(ops.currentTarget);
+
+      String[] pluginParamsInUse = {
+              "-P:dependency-analyzer:direct-jars:" + String.join(":", ops.directJars),
+              "-P:dependency-analyzer:indirect-jars:" + String.join(":", ops.indirectJars),
+              "-P:dependency-analyzer:indirect-targets:" + String.join(":", targets),
+              "-P:dependency-analyzer:mode:" + ops.dependencyAnalyzerMode,
+              "-P:dependency-analyzer:current-target:" + currentTarget,
+      };
+      pluginParams = pluginParamsInUse;
+    } else {
+      pluginParams = new String[0];
+    }
+    return pluginParams;
+  }
+
   private static void compileScalaSources(CompileOptions ops, String[] scalaSources, Path tmpPath) throws IllegalAccessException {
+
+    String[] pluginParams = getPluginParamsFrom(ops);
+
     String[] constParams = {
       "-classpath",
       ops.classpath,
@@ -199,6 +233,7 @@ private static void compileScalaSources(CompileOptions ops, String[] scalaSource
       ops.scalaOpts,
       ops.pluginArgs,
       constParams,
+      pluginParams,
       scalaSources);
 
     MainClass comp = new MainClass();

test/src/main/scala/scala/test/strict_deps/no_recompilation/A.scala

@@ -0,0 +1,7 @@
+package scala.test.strict_deps.no_recompilation;
+
+object A {
+	def foo = {
+		B.foo
+	}
+}

test/src/main/scala/scala/test/strict_deps/no_recompilation/B.scala

@@ -0,0 +1,7 @@
+package scala.test.strict_deps.no_recompilation;
+
+object B {
+	def foo = {
+		C.foo
+	}
+}

test/src/main/scala/scala/test/strict_deps/no_recompilation/BUILD

@@ -0,0 +1,28 @@
+package(default_visibility = ["//visibility:public"])
+load("//scala:scala.bzl", "scala_library", "scala_test", "scala_binary")
+
+scala_library(
+    name="transitive_dependency_user",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["direct_dependency"],    
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)
+
+scala_library(
+    name="direct_dependency",
+    srcs=[
+        "B.scala",
+        ],
+    deps = ["transitive_dependency"],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)
+
+scala_library(
+    name="transitive_dependency",
+    srcs=[
+        "C.scala",
+        ],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)

test/src/main/scala/scala/test/strict_deps/no_recompilation/C.scala

@@ -0,0 +1,7 @@
+package scala.test.strict_deps.no_recompilation;
+
+object C {
+	def foo = {
+		println("orig")
+	}
+}

test_expect_failure/dep_analyzer_modes/A.scala

@@ -0,0 +1,8 @@
+package test_expect_failure.dep_analyzer_modes
+
+object A {
+  def foo = {
+    B.foo
+    C.foo
+  }
+}

test_expect_failure/dep_analyzer_modes/B.scala

@@ -0,0 +1,7 @@
+package test_expect_failure.dep_analyzer_modes
+
+object B {
+  def foo = {
+    C.foo
+  }
+}

test_expect_failure/dep_analyzer_modes/BUILD

@@ -0,0 +1,56 @@
+package(default_visibility = ["//visibility:public"])
+load("//scala:scala.bzl", "scala_library", "scala_binary", "scala_test")
+
+scala_library(
+    name="error_mode",
+    dependency_analyzer_mode_soon_to_be_removed="error",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["direct_dependency"],
+)
+
+scala_library(
+    name="warn_mode",
+    dependency_analyzer_mode_soon_to_be_removed="warn",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["direct_dependency"],
+)
+
+scala_library(
+    name="weird_mode",
+    dependency_analyzer_mode_soon_to_be_removed="kuki_buki",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["direct_dependency"],
+)
+
+scala_library(
+    name="off_mode",
+    dependency_analyzer_mode_soon_to_be_removed="off",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["direct_dependency"],
+)
+
+
+scala_library(
+    name="direct_dependency",
+    dependency_analyzer_mode_soon_to_be_removed="error",
+    srcs=[
+        "B.scala",
+        ],
+    deps = ["transitive_dependency"],
+)
+
+scala_library(
+    name="transitive_dependency",
+    dependency_analyzer_mode_soon_to_be_removed="error",
+    srcs=[
+        "C.scala",
+        ],
+)

test_expect_failure/dep_analyzer_modes/C.scala

@@ -0,0 +1,7 @@
+package test_expect_failure.dep_analyzer_modes
+
+object C {
+  def foo = {
+    println("in C")
+  }
+}

test_expect_failure/missing_direct_deps/external_deps/A.scala

@@ -0,0 +1,8 @@
+package test_expect_failure.missing_direct_deps.external_deps
+
+object A {
+	def foo = {
+		B.foo
+		com.google.common.base.Strings.commonPrefix("abc", "abcd")
+	}
+}

test_expect_failure/missing_direct_deps/external_deps/B.scala

@@ -0,0 +1,8 @@
+package test_expect_failure.missing_direct_deps.external_deps
+
+object B {
+	def foo = {
+		println("in B")
+		com.google.common.base.Strings.commonPrefix("abc", "abcd")
+	}
+}

test_expect_failure/missing_direct_deps/external_deps/BUILD

@@ -0,0 +1,20 @@
+package(default_visibility = ["//visibility:public"])
+load("//scala:scala.bzl", "scala_library", "scala_test")
+
+scala_library(
+    name="transitive_external_dependency_user",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["external_dependency_user"],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)
+
+scala_library(
+    name="external_dependency_user",
+    srcs=[
+        "B.scala",
+        ],
+    deps = ["@com_google_guava_guava_21_0//jar"],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)

test_expect_failure/missing_direct_deps/external_deps_file_group/A.scala

@@ -0,0 +1,8 @@
+package test_expect_failure.missing_direct_deps.external_deps_file_group
+
+object A {
+	def foo = {
+		B.foo
+		com.google.common.base.Strings.commonPrefix("abc", "abcd")
+	}
+}

test_expect_failure/missing_direct_deps/external_deps_file_group/B.scala

@@ -0,0 +1,8 @@
+package test_expect_failure.missing_direct_deps.external_deps_file_group
+
+object B {
+	def foo = {
+		println("in B")
+		com.google.common.base.Strings.commonPrefix("abc", "abcd")
+	}
+}

test_expect_failure/missing_direct_deps/external_deps_file_group/BUILD

@@ -0,0 +1,20 @@
+package(default_visibility = ["//visibility:public"])
+load("//scala:scala.bzl", "scala_library", "scala_test")
+
+scala_library(
+    name="transitive_external_dependency_user",
+    srcs=[
+        "A.scala",
+        ],
+    deps = ["external_dependency_user"],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)
+
+scala_library(
+    name="external_dependency_user",
+    srcs=[
+        "B.scala",
+        ],
+    deps = ["@com_google_guava_guava_21_0//jar:file"],
+    dependency_analyzer_mode_soon_to_be_removed="error",
+)

test_expect_failure/missing_direct_deps/internal_deps/A.scala

@@ -0,0 +1,10 @@
+package test_expect_failure.missing_direct_deps.internal_deps;
+
+object A {
+	def foo = {
+		B.foo
+		C.foo
+	}
+
+	def main = foo
+}

test_expect_failure/missing_direct_deps/internal_deps/B.scala

@@ -0,0 +1,7 @@
+package test_expect_failure.missing_direct_deps.internal_deps;
+
+object B {
+	def foo = {
+		C.foo
+	}
+}