Strict scala deps and unused_deps

[ittaiz]

So,
I met with @cgrushko a few days ago and apparently the way we (rules_scala) currently do strict deps (allowing a target to only use what it directly depends on) is very different from the way java rules do it and IMHO not as good.

Problem:
We currently pass to scalac only the direct jars and let scalac emit an error to the user.
These errors are quite often cryptic and unuseful for developers to be able to easily fix the problem.
Moreover because of how the scala compiler works there are many cases where it needs transitive dependencies even if they aren't mentioned in the source code of the unit we're compiling[1]. This leads to having to add many transitive targets without any reasoning just to appease the compiler or worse to start using exports loosely around the codebase.

We suffered from the above transitive dependencies so much that we're just adding the whole transitive closure to the deps of a target. This is in the onboarding of a project so we're ok with it but this solution can help greatly minimize it.

Proposed solution:
Pass direct and indirect jars to scalac's classpath (like java compilation does).
Add a scala compiler plugin which essentially will walk the AST after the type annotation phase, will stop at each 'type expression' and check that the jar from which the type was loaded is a direct_dependency. If not, it is an --indirect_dependency, and the plugin generates an error message. The message includes an 'add_dep' command to add the missing direct dep.

We will also support a lenient mode where this error is outputted as a warning.
add_dep is a command which buildozer can take and apply it to your BUILD file.
I think (not sure) that ibazel can pick up these warnings and automatically apply them to your BUILD file. Probably rebazel will be able to do this as well if they'll want to.

Value:
Separation of concerns (scalac won't enforce graph concerns)
Allows higher level messages and so call to actions
Might allow finer grain modeling of when indirect dependencies are ok (for an example where scalac "messes" up but we might be able to improve see this repo)
Better cycle for developers since they can run with lenient mode and iterate quickly and fix the deps before pushing (or use ibazel/rebazel). The caveat is obviously the fact people can have a local green build which fails on CI.

Open questions:
It's unclear how smart the compiler plugin can be given scalac's limitations and so unclear on how much noise will still be added to a target's deps.

Small implementation detail:
To do this we'll need to pass to the compiler plugin a list of direct dependencies and a list of indirect dependencies where at least the latter needs to have both jars and the targets so that the error/warning message can be useful (add //src/main/foo:baz to your deps)

[1]Improve user experience for builds that use only direct dependencies

[ittaiz]

cc some people who are/were involved with thinking about this at Wix (@ors @natans @nadavwe @shvar @dkomanov)

[johnynek]

this seems really nice.

note related post on bazel blog for java
https://blog.bazel.build/2017/06/28/sjd-unused_deps.html

The strategy here looks good to me. Some comments:

1. can we get the reliability so this is the default, or will we need to be opt in for a while (I imagine the latter).
2. should we have a version that just warns on unused dependencies first (kind of the reverse problem as this).
3. I'm surprised the strict dependencies became a show-stopper for you. Our dependencies are probably over-broad, but it hasn't been debilitating.

[ittaiz]

The blog post and the issue are both products of Carmi and me meeting a few days ago :) Some of the wording in the issue are copied from the explanation of how the plugin works. Re #1 I think having "dumb" strict mode via Bazel and not scalac should be easy. By "dumb" I mean we warn/error for every indirection. What might be more complicated is the "smarter" mode where we are able to identify which indirect usages are legitimate (compiler's limitations) and which aren't (you're just using that dep yourself) Re #2 honestly that's not the problem I need to solve now. Many of our generated BUILD files have ~100 dependencies in their "deps". If I get strict mode with warnings then I'm able to add that into the migration phase and output minimal BUILD files. Unused is for longer down the road where you want to keep BUILD files cleaner and IINM the current convention in Google is to move the unused dependencies to runtime under the assumption something might need it. Re #3 after I added around 30 transitive targets without understanding why the compiler is angry I decided to automate it. Note that it wasn't all of them. For this one repo I'd guess we're talking about several hundreds of noise dependencies.

…

On Thu, 29 Jun 2017 at 5:06 P. Oscar Boykin ***@***.***> wrote: this seems really nice. note related post on bazel blog for java https://blog.bazel.build/2017/06/28/sjd-unused_deps.html The strategy here looks good to me. Some comments: 1. can we get the reliability so this is the default, or will we need to be opt in for a while (I imagine the latter). 2. should we have a version that just warns on unused dependencies first (kind of the reverse problem as this). 3. I'm surprised the strict dependencies became a show-stopper for you. Our dependencies are probably over-broad, but it hasn't been debilitating. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#235 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF0si-kg_1lWXezRPcevNm5oLGICOks5sIwaJgaJpZM4OHkDK> .

[ittaiz]

@gkossakowski Since Oscar pulled you to the PR I thought I'd pull you to the issue and ask you if you have an intiuition or an idea on how "smart" will the plugin be able to be? What we really want to enable is to warn/error the user when their code uses a transitive dependency. This is to allow for upstream targets freedom in changing implementation decisions. I don't know how feasible this distinction (I use vs compiler needs for transitive reasons) is in the compiler/compiler-plugins.

Thanks!

[ittaiz]

@gkossakowski Do you have an intuition or an idea on how we can advance the "smart" plugin mode? What we really want to enable is to warn/error the user when their code uses a transitive dependency as opposed to warning the user for cases where the compiler needs this symbol lookup just for the its own needs. This is to allow for upstream targets freedom in changing implementation decisions.

Thanks!

[gkossakowski]

Yes, I mulled over this problem in the context of zinc's incremental compilation decision. In zinc, you want to approximate the dependencies scalac would ever need.

The short answer is: the dependencies introduced by reference do not require a transitive closure but the one introduced by inheritance does require a transitive closure. When a program mentions a symbol, e.g. val x: Foo then it introduces a reference dependency. When program mentions a dependency in the context of declaring a parent type for a class, e.g class X extends Y[Foo], then it introduces a dependency by inheritance.

You can capture both kinds of dependencies by traversing the trees and collecting the information in the right context. Zinc implements this: https://github.com/sbt/zinc/blob/1.x/internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala#L326

[ittaiz]

Are you sure?
I have an example which has a class A with a member B and B has a member C (which is a java interface) and you can't compile A without C. Tested with 2.11.7 and 2.12.1 at the time

[ittaiz]

Actually I think that my example above shows that there are more areas the plugin can improve in but what you wrote IIUC is that at least in that aspect (of inheritance) the plugin can be smarter and not warn/error. Did I understand you correctly?

[gkossakowski]

My comment is what should happen given Scala's typesystem design to shed light what directionally to expect. From there you can derive whether the behavior you're observing is a bug or a design choice you have to work with.

I just checked that what I said about dependencies by reference not forcing transitive dependencies is true for pure Scala:

[gkk@mbp ~/tmp/scala-tmp]$ echo "class A" > A.scala
[gkk@mbp ~/tmp/scala-tmp]$ echo "class B {
>     def a: A = new A
> }
> " > B.scala
[gkk@mbp ~/tmp/scala-tmp]$ echo "class C {
>     def b: B = new B
> }
> " > C.scala
[gkk@mbp ~/tmp/scala-tmp]$ mkdir outA outB outC
[gkk@mbp ~/tmp/scala-tmp]$ scalac -d outA A.scala
[gkk@mbp ~/tmp/scala-tmp]$ scalac -d outB -cp outA B.scala
[gkk@mbp ~/tmp/scala-tmp]$ scalac -d outC C.scala # skip -cp to show that classpaths are isolated
C.scala:2: error: not found: type B
    def b: B = new B
           ^
C.scala:2: error: not found: type B
    def b: B = new B
                   ^
two errors found
[gkk@mbp ~/tmp/scala-tmp]$ scalac -d outC -cp outB C.scala # the A class is not on the classpath

I'm not sure what's different about your example but I think it should work. Perhaps you can check whether you can reproduce the problem in pure Scala setting? I suspect that loading Java-originating symbols from a class file is overeager. It's a different code path than loading Scala symbols in the compiler.