Tracking implicit compiler dependencies #1459

liucijus · 2022-12-19T10:24:35Z

Ready for review

~~Opening this PR for visibility and feedback.~~

~~Warning: this is WIP and very experimental and probably contains bugs. Review comments are very welcome.~~

Why?

Currently dep tracking in Rules Scala is implemented as compiler plugins and do not get full information which dependencies compiler has used to resolve types. All symbols which are not detected by the dependency tracking plugin are reported as unused and can break compilation if corresponding targets will be removed.
Dependencies can be grouped into three categories:

Strict (S) - Deps which are detected by the plugin (used in the AST or source)
Compiler only (I) - Deps which are not mentioned in AST/source, but needed by compiler to resolve other types
Unused (U) - Deps are not used in AST/source and not needed by the compiler

Currently reported unused deps are I+U, which makes dependency tracking mostly unusable, and requires complex use of unused_dependency_checker_ignored_targets.

How?

Patch Scala compiler to report which jars it has opened. Aggregate and analyze compiler and plugin reported information in custom reporter implementation.

Outputs:

sdeps - similar to jdeps, but with more information, eg. jar labels. Why not jdeps? Because they break some tools like Intellij and lack precise label information. If there's a need for jdeps to be produced, it's easy to add toggle in the toolchain.
logs buildozer commands in WARN, ERROR or off for each group. Can be configured per group.

Testing:

I've done some testing on large codebase and it seems to be working without issues. It's annoying that there are so many I deps reported, but I guess that's expected. The pros are that I'm able to clean target deps in isolation without a need for external indexes or fiddling with unused_dependency_checker_ignored_targets.
Only tested with 2.12 ~~- patch may fail on 2.13~~.
sdeps can be used to build more sophisticated tools, eg. explain what are all these deps in my target.
I'm settling on S/error, I/warn, U/warn, which gives some room for changes not to break builds while large codebase is changing in multiple places.

Key design decisions

Patch scala/tools/nsc/symtab/SymbolLoaders.scala and prepend on the compiler classpath to get loaded jars reported.
Patch must be applied only if dependency tracking is enabled (scala_config(enable_compiler_dependency_tracking = True))
Produce sdeps (named after similar jdeps) with label, jar and usage info to be consumed by tools. Why not jdeps, because it breaks current Intellij support and does not contain label information. jdeps can be easily added later if there's a need.
Aggregation of compiler and analyzer plugins information and reporting happens in custom reporter

Things left out of the initial implementation

Silence log messages, but produce sdeps
Exclude/include patterns for compiler (I) deps
A tool to read sdeps for a target

liucijus · 2022-12-19T10:25:13Z

cc @ianoc

ianoc · 2023-01-02T19:10:36Z

This is pretty amazing/awesome @liucijus , thanks for sharing it. I'll try get some time soon to see how it works in our code base. Having reliable unused deps tracking unlocks a bunch of fun new possibilities!

ianoc · 2023-01-09T16:09:27Z

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

+        target = indirectJarToTarget.get(jar);
+      }
+
+      if (target.startsWith("Unknown")) {


I ended up with target being null here on scala 2.11

This seems to happen from jars from the jdk for me --
external/remote_jdk8_macos_aarch64/zulu-8.jdk/Contents/Home/jre/lib/rt.jar is triggering this, since its not a direct or indirect target

(i did go back now and just try confirm unused deps in ast mode works, its too buggy on 2.11 for us to use. But it does run. [this i believe is a known thing that its not great on 2.11 going back many years.... some day we'll get this codebase on 2.12....])

I think the null check just needs to be moved up a few lines, looks like you have one one right after this

Thanks, @ianoc, this is a good catch! I think I need to ignore jars which are not associated with labels as it is done in the analyzer plugin.

I hope 27f96d4 fixes the problem, but I haven't tested it yet

ianoc · 2023-01-09T19:42:01Z

I've been going through this a bit trying to use it, the ast code for 2.11 (really we just need to upgrade, so i think ignore this), isn't so reliable so i did a bit of surgery locally to try ensure i could focus purely on the results of the compiler plugin.

With that the results generally that i saw seemed correct, vs our BUILD file generation tooling i ran into some stuff like:

Foo.fieldWithAList.map(_.methodB)

Our ast stuff can't see the type of _ which is really a direct dependency. so that shows up a bunch in the delta here.

I did just spend a bunch of time tracking down (i think it might be worth expanding the error messages in here, like:

  private String reportDep(Dep dep, boolean add) {
    if(add) {
      String message = "error: Target '" + dep.target + "' (via jar: ' " + dep.jar +" ') is being used by " + ops.currentTarget + " but is is not specified as a dependency, please add it to the deps.\nYou can use the following buildozer command:\n";
      String command = "buildozer 'add deps " + dep.target + "' " + ops.currentTarget;
      return message + command + "\n";
    } else {
      String message = "error: Target '" + dep.target + "' (via jar: ' " + dep.jar +" ')  is specified as a dependency to " + ops.currentTarget + " but isn't used, please remove it from the deps.\nYou can use the following buildozer command:\n";
      String command = "buildozer 'remove deps " + dep.target + "' " + ops.currentTarget;
      return message + command + "\n";
    }
  }

I had wanted to make them look like the javac ones so some of our existing error correction tooling would just work -- but i've ran into really wanting that jar information. We are currently using 3rdparty being brought in as exports on the fields. So the target mapping information we wind up building here is kinda off.

e..g.

A - B - C - D
            \
               - D
             

With the meaning A depends on B
B depends on C and D
C also depends on D.


B depends on C via field `foo`, and d via field `bar`

In effect when accessing B.foo in A i was picking up a dependency on C. Though the dependency is actually on D. This looks to be happening because C is in the transitive dependencies of B, and it itself is exporting D.

So i think some of this is a reflection more of our our dependencies are structured too here that makes it tricky. The _ case above also would pose a challenge for us in generating build files too, though it is right

liucijus · 2023-01-11T08:33:00Z

  private String reportDep(Dep dep, boolean add) {
    if(add) {
      String message = "error: Target '" + dep.target + "' (via jar: ' " + dep.jar +" ') is being used by " + ops.currentTarget + " but is is not specified as a dependency, please add it to the deps.\nYou can use the following buildozer command:\n";
      String command = "buildozer 'add deps " + dep.target + "' " + ops.currentTarget;
      return message + command + "\n";
    } else {
      String message = "error: Target '" + dep.target + "' (via jar: ' " + dep.jar +" ')  is specified as a dependency to " + ops.currentTarget + " but isn't used, please remove it from the deps.\nYou can use the following buildozer command:\n";
      String command = "buildozer 'remove deps " + dep.target + "' " + ops.currentTarget;
      return message + command + "\n";
    }
  }

I had wanted to make them look like the javac ones so some of our existing error correction tooling would just work -- but i've ran into really wanting that jar information. We are currently using 3rdparty being brought in as exports on the fields. So the target mapping information we wind up building here is kinda off.

Updated to your version

liucijus · 2023-01-11T13:49:40Z

In effect when accessing B.foo in A i was picking up a dependency on C. Though the dependency is actually on D. This looks to be happening because C is in the transitive dependencies of B, and it itself is exporting D.

Does the same behavior reproduce with both compiler deps tracking and regular AST plugin? If they act differently in regards of exported deps, then it's a regression I have introduced. I've seen similar problems in our codebase, but I'm not sure if that's exactly the same dependency situation. If it reproduces with AST plugin, a separate issue, and if possible, a test or repro would be very appreciated.

liucijus · 2023-01-24T09:29:43Z

Status update. I'm testing this change in production with a large codebase and so far it works flawlessly.

My next step is to rebase this PR for review and hopefully get it merged.

Add manual test script to check if patches apply correctly. Don't support old Scala versions 2.11.x and 2.12.0

simuons · 2023-02-01T09:00:40Z

scala/private/dependency.bzl

        dependency_tracking_method):
    is_strict_deps_on = strict_deps_mode != "off"
    is_unused_deps_on = unused_deps_mode != "off"

    need_direct_jars = is_strict_deps_on or is_unused_deps_on
-    need_direct_targets = is_unused_deps_on
+    need_direct_targets = is_unused_deps_on or is_strict_deps_on


Looks like both need_direct_jars and need_direct_targets will always have same value. Maybe these needs to be squashed into single flag if it's needed

thanks for catching - simplified the code

simuons · 2023-02-01T09:09:32Z

scala/private/macros/scala_repositories.bzl

+    patch = "@io_bazel_rules_scala//dt_patches:dt_compiler_%s.patch" % SCALA_MAJOR_VERSION
+
+    if SCALA_MAJOR_VERSION == "2.12":
+        minor_version = int(SCALA_VERSION[SCALA_VERSION.find(".", 2) + 1:])


Maybe SCALA_VERSION.split(".")[2] would be cleaner? And maybe this can be moved to @io_bazel_rules_scala_config//:config.bzl to keep version parsing in single place.

simuons

Overall looks good to me. Some minor comments, commented code and clarifications I would like to get.

scala/private/macros/scala_repositories.bzl

simuons · 2023-02-01T09:24:15Z

scala/private/phases/phase_dependency.bzl

@@ -55,6 +56,11 @@ def _phase_dependency(
    else:
        strict_deps_mode = get_strict_deps_mode(ctx)

+    if strict_deps_always_off or not included_in_strict_deps_analysis:


Just to clarify. There is a dependency between strict_deps_mode and compiler_deps_mode
If strict_deps_mode is "off" then no matter what is specified for compiler_deps_mode it will be turned "off"
Maybe it would be better to express this dependency like compiler_deps_mode == get_compiler_deps_mode(ctx) if strict_deps_mode != "off" else "off" or something similar instead of having duplicate if condition.

I left it this way because I think we will need a separate flag to control compiler deps mode in the future. Because it's a breaking change and I'm not very sure about the need yet, I left it this way.

I think I'll squash strict and compiler modes under one control statement. Eg.:

if strict_deps_always_off or not included_in_strict_deps_analysis: strict_deps_mode = "off" compiler_deps_mode = "off" else: strict_deps_mode = get_strict_deps_mode(ctx) compiler_deps_mode = get_compiler_deps_mode(ctx)

scala/private/rule_impls.bzl

scala/scala_toolchain.bzl

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

simuons · 2023-02-06T09:39:15Z

test/shell/test_misc.sh

-$runner scala_test_test_filters
-$runner test_multi_service_manifest
-$runner test_override_javabin
+#$runner test_disappearing_class


Are these tests irrelevant?

fixed - were commented by mistake

simuons · 2023-02-06T09:51:51Z

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

+    }
+  }
+
+  private String reportDep(Dep dep, boolean add) {


Maybe just split this method into two instead of boolean flag. It would be easier to read code where it's used.

rearranged it a bit + fixed warning/error part of the message

simuons · 2023-02-06T09:56:58Z

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

+        try {
+          jarToTarget.put(ops.directJars[i], ops.directTargets[i]);
+        } catch (ArrayIndexOutOfBoundsException e) {
+          throw new RuntimeException(


Maybe throw IllegalArgumentException from else clause instead of catching ArrayIndexOutOfBoundsException

Fixed, thanks for catching

simuons · 2023-02-06T09:57:51Z

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

+      }
+    }
+
+    if (ops.indirectTargets.length == ops.indirectJars.length) {


Should this be guarded from situation when sizes doesn't match?

simuons · 2023-02-06T10:11:46Z

src/java/io/bazel/rulesscala/scalac/reporter/DepsTrackingReporter.java

+    protoReporter.writeTo(Paths.get(diagnosticsFile));
+  }
+
+  private static class Dep {


Is this class really needed. It looks like it maps one to one to Dependencies from proto. You could cut some conversion code. But I myself not convinced this would be proper approach.

Replaced with Dependencies

…ment

johnynek · 2023-02-14T01:06:29Z

Thanks for your work on this folks!

Side note: it would be really nice to get this upstreamed into the scala compiler this is a standard feature that other build tools could also use.

liucijus · 2023-02-21T08:39:23Z

Side note: it would be really nice to get this upstreamed into the scala compiler this is a standard feature that other build tools could also use.

On my todo list

ianoc reviewed Jan 9, 2023

View reviewed changes

liucijus added 23 commits January 25, 2023 10:05

poc

d185984

poc tested

cb6bb05

Add compiler patching

d39a406

Version with compiler patching

87477c8

Add compiler_deps_mode attribute to scala toolchain

3487746

Check if compiler deps need to be reported

e0a3a11

Fix after rebase

492f2fa

Add dep tracking compiler patch

a1933a3

Lint

9e9cd73

Check if mode is verbose log

1108088

Fixes

b7bfc52

Fix Scala 3 support

dedf5ce

Patch compiler only if compiler dt is enabled

250bdd3

Fix version tests

74bb915

Rename verbose-log to ast-plus

b5ccb9a

Add patches for other supported version

1e1e205

Add manual test script to check if patches apply correctly. Don't support old Scala versions 2.11.x and 2.12.0

Fix versions test by adding 2.11.12 support

a99ecaa

Expand dt messages to include jar information

b755a1b

Exclude jars without labels from dep reporting

de3538e

Add dependecy tracking method field to sdeps proto

f3b004d

Remove unused field

6838998

Fix test class after moving

391111d

Exclude jars without labels from dep reporting - fix for transitive jars

2db9aec

liucijus added 2 commits January 25, 2023 10:12

Refactor reporting code

9e717af

Add integration tests

1c88ea6

liucijus force-pushed the compiler-deps-tracking branch from 2ed3a13 to 1c88ea6 Compare January 25, 2023 08:17

liucijus marked this pull request as ready for review January 25, 2023 08:30

liucijus requested a review from simuons as a code owner January 25, 2023 08:30

liucijus added 2 commits January 25, 2023 11:56

Remove sdeps reader leftover

e810aa7

Remove unused symbol from load statement

ca13462

liucijus requested a review from ianoc January 27, 2023 14:38

simuons reviewed Feb 1, 2023

View reviewed changes

liucijus added 2 commits February 1, 2023 11:40

Remove redundant flags

4020140

Add scala minor version to scala config

aa9faa3

simuons requested changes Feb 6, 2023

View reviewed changes

liucijus added 8 commits February 6, 2023 16:33

Move strict and compiler mode resolution under the same control state…

2a3224e

…ment

Move strict and compiler mode resolution under the same control state…

4c5df85

…ment

Use simple equals

4ff757c

Validate and report if dep tracking targets/jars parameter can pair

42835b9

Replace Dep class with generated; distinguish between warning and error

0de9ffa

Uncomment tests commented by mistake

15dd54b

Restructure dep reporting message

a3a6070

Don't catch exception

f766a99

liucijus requested review from simuons and removed request for ianoc February 7, 2023 09:35

simuons approved these changes Feb 8, 2023

View reviewed changes

liucijus merged commit 56bfe4f into bazelbuild:master Feb 13, 2023

liucijus deleted the compiler-deps-tracking branch February 13, 2023 07:15

crt-31 mentioned this pull request Mar 27, 2023

New dep tracking not working on Windows due to 'unused dep' errors #1487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tracking implicit compiler dependencies #1459

Tracking implicit compiler dependencies #1459

liucijus commented Dec 19, 2022 •

edited

Loading

liucijus commented Dec 19, 2022

ianoc commented Jan 2, 2023

ianoc Jan 9, 2023

ianoc Jan 9, 2023

ianoc Jan 9, 2023

ianoc Jan 9, 2023

liucijus Jan 9, 2023

liucijus Jan 17, 2023

ianoc commented Jan 9, 2023

liucijus commented Jan 11, 2023

liucijus commented Jan 11, 2023

liucijus commented Jan 24, 2023

simuons Feb 1, 2023

liucijus Feb 1, 2023

simuons Feb 1, 2023

liucijus Feb 1, 2023

simuons left a comment

simuons Feb 1, 2023

liucijus Feb 6, 2023

simuons Feb 6, 2023

liucijus Feb 7, 2023

simuons Feb 6, 2023

liucijus Feb 7, 2023

simuons Feb 6, 2023

liucijus Feb 7, 2023

simuons Feb 6, 2023

liucijus Feb 7, 2023

simuons Feb 6, 2023

liucijus Feb 7, 2023

johnynek commented Feb 14, 2023

liucijus commented Feb 21, 2023

Tracking implicit compiler dependencies #1459

Tracking implicit compiler dependencies #1459

Conversation

liucijus commented Dec 19, 2022 • edited Loading

Why?

How?

Outputs:

Testing:

Key design decisions

Things left out of the initial implementation

liucijus commented Dec 19, 2022

ianoc commented Jan 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianoc commented Jan 9, 2023

liucijus commented Jan 11, 2023

liucijus commented Jan 11, 2023

liucijus commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simuons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Feb 14, 2023

liucijus commented Feb 21, 2023

liucijus commented Dec 19, 2022 •

edited

Loading