[WIP] POC - Decouple dependency phase

[natansil]

POC - Decouple dependency phase so other build tools would be able to use it and code will not be duplicated and maintained several times.
e.g. bazelbuild/rules_scala strict_deps feature.

changes:

1. CallbackGlobal can not be part of the dependency analysis phase as it is zinc-specific, so the DependencyAnalyzer logic should only know of Global.

2. AnalysisCallback interface should be split in two, so that there will be an interface that only declare dependency related methods

pending issues:

1. LocalToNonLocalClass cache should somehow still be shared with other phases (this branch just creates a standalone instance of the cache for the DependencyAnalyzer due to technical issue relating Symbol type conflicts).

@gkossakowski @stuhood @ittaiz @johnynek
WDYT?

[lightbend-cla-validator]

Hi @natansil,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

[natansil]

also, please note that I've managed to successfully use the dependency analysis logic in rules_scala by copying the code:
bazel-contrib/rules_scala#439
(all tests pass)

[gkossakowski]

I think decoupling callbacks for the Dependency phase makes sense. What's your long-term idea to use that code from zinc? Depend on a zinc jar and use the Depenendcy phase yourself (skipping the rest of zinc) or are you thinking about some more high-level api?

[jvican]

I suspect we need to be creative here, instead of commenting this code.

[natansil]

absolutely.
Seems like one solution can be to split LocalToNonLocalClass to CallbackLocalToNonLocalClass which handles the zinc-specific logic (and knows of CallbackGlobal ) and InternalLocalToNonLocalClass which handles the caching logic (and only knows of Global). CallbackLocalToNonLocalClass can be a wrapper for InternalLocalToNonLocalClass.

of-course maybe other refactoring options for the shared cache issues can be more elegant...

[jvican]

What is the exact use case for this decoupling? I'm interested to hear more about how you plan on improving support for strict dependencies.

[natansil]

@gkossakowski, for bazelbuild/rules_scala, pipelining plugin phases (be it from external sources or internal ones) seems like an interesting idea (I can think of at least two plugin phases - strict deps (already exists) and unused deps)
If zinc's Dependency Phase can become standalone (i.e. not use CallbackGlobal and solve the LocalToNonLocalClass cache), then rules_scala can depend on it directly.

Regarding higher-level apis, I'm not sure what you mean. In this PR, iv'e tried to split the actual dependency analysis logic from the Plugin boiler-plate, but the way Global object is structured (huge class that has many many mixins that introduce many inner classes) is very rigid and makes it hard to access the CompilationUnit and syntax tree without passing the entire global object. So I'm not sure how much abstraction can happen without using Global

[natansil]

@jvican regarding support for strict deps in bazelbuild/rules_scala,
Currently there is a solution based on scalacenter/classpath-shrinker where the dependency analysis part collects all jars loaded by the compiler.
The disadvantages here are:

1. over-approximation of actually used jars (due to compiler's aggressive loading)
2. no mapping between the source code dependency (file + line number) and the commensurate jar dependency.

The rules_scala plugin also receives information on the jars from Bazel - for this unit (target), which jars are declared directly, and which ones are declared indirectly (transitively)
Then it simply alerts on all loaded jars that are in the indirect category.

We would like to change the dependency analysis to work on the AST itself, which should mitigate the two disadvantages I described above
@ittaiz, anything you would like to add?

[jvican]

over-approximation of actually used jars (due to compiler's aggressive loading)

Is this just a guess or do you have data to back up that the loading is too agressive? I know for sure that the compiler is aggresive on loading symbols from the Java namespace (because of macros), but I think this is not the case for libraries code.

In any case, if you want to avoid that, you need to improve the class loaders in the compiler. If you try to remove from the classpath things that would be loaded by the compiler, wouldn't compilation fail? I guess I'll need more details on this topic to be able to make an educated guess of what's going on.

no mapping between the source code dependency (file + line number) and the commensurate jar dependency.

So is this a disadvantage of the way you want to work around the fact that the compiler loads aggressively?

[natansil]

1. On over-approximation:
   I've also heard of macros causing this, and also on transitive dependencies introduced by traits for example. but haven't seen any concrete example by myself. maybe @gkossakowski can shed more light here on the motivation.

I don't think compilation will fail, because the fixes that will be introduced by strict deps adherence will not remove any dependency from the transitive closure. just add jars to be direct deps.
on the other hand this could happen for unused deps feature.

2. Regarding lack of mapping between jar and source code disadvantage, what I mean is that the current solution that exists in rules_scala that aggregates all loaded jars does not have any fine grained source code data for the error shown to the user. e.g. (from bazel's java strict deps):

A.java:6: error: [strict] Using type C from an indirect dependency (TOOL_INFO: "//:C"). See command below **
  C getC() {
  ^
** Please add the following dependencies:
  //:C  to //:A

[jvican]

I don't think compilation will fail, because the fixes that will be introduced by strict deps adherence will not remove any dependency from the transitive closure. just add jars to be direct deps. on the other hand this could happen for unused deps feature.

I think the fix in https://github.com/scalacenter/classpath-shrinker tells you everything that was loaded intentionally by the compiler for a given compilation unit; if you try to be smarter and remove some of the symbols that were loaded, without changing the compiler, I would expect the symbol loaders in the compiler to fail compilation. (I'm not an expert in this area, so take my word with a pinch of salt, I'm just trying to provide useful information in this discussion. I suggest you have concrete examples to work on and you prove that there is too much unnecessary loading going on and that you can avoid it by removing the jars from the classpath.)

rules_scala that aggregates all loaded jars does not have any fine grained source code data for the error shown to the user

Aha, I see. Have you looked into other ways of getting that information that does not require adding this decoupling? I think doing the Dependency decoupling is a good idea, not only for your use case in general but for other tools like scala-sculpt, but I suspect you may be better off getting that information alone in another way.

[jvican]

This has been sitting in the queue for quite a while. Let's reopen if activity in the code or the comments restarts 😄