Stricter unused dependency checking for plus one deps mode

[Jamie5]

When using plus one deps mode, many unused deps do get marked, but some deps can be left out because they are already the dep of another dep, and it also makes sense to leave them out because they never appear in the source code of the package being compiled, and the only reason the dep is needed is to make scalac happy. Would it be possible to have a unused deps mode for plus one deps mode where a dep is marked as an unused, unless it is explicitly referenced in the source code? (Not sure if this would end up with a number of false positives - not that familiar with scalac's needs)

This is now in progress, the below summarizes the status

• Anyone who wants to test this on their projects should do so

Known potential issues

• external deps such as https://github.com/anchlovi/unused-deps-specs2 - when +1 can't grab the deps of deps, deps and/or exports should be used on the external dependency to expose what's needed for its ijar
• Since +1 is a heuristic, not everything will work fine and unused deps may report issues. In our experience this has not happened but there are examples of problems, such as A->B->C->D subclassing and A's deps is only listed as B. There are ideas to fix this (and the previous issue as well) described below

Things to do

• Handle incoming issues discovered in the AST tracker
• Decide how/if to handle the above issues. Stricter unused dependency checking for plus one deps mode #867 (comment) discusses some solutions. It is unclear how often these issues happen if we export iface deps of external deps properly. Our (not speaking for rules_scala) plan is to use unused_dependency_checker_ignored_targets for such issues, as they are so rare we did not need any unused_dependency_checker_ignored_targets when enabling the new mode.
• Once the new AST mode is sufficiently validated [definition is 1 month since no more true errors [the last known one requiring a fix being https://github.com/Report macros as used #1008 ], 3 months after April 30 [which would be July 30], and also @ittaiz approves] do the following steps sequentially, with enough time in between them to allow people to migrate
  1. Remove dependency_tracking_method on toolchain and ast becomes the method for transitive/+1, while high-level becomes the method for direct
     a. At this point take another look at [WIP] Handle a unused_dependency_checker_ignored_targets #1034 and see if it is landable
  2. Change default for strict_deps to error on toolchain, and strict_deps=default is deprecated
  3. Remove strict_deps=default as an option

[ittaiz]

Thanks for the issue! TLDR: Unfortunately not. Longer: The current unused deps mode uses a heuristic which works sometimes (in Wix’s experience less, in Stripe’s experience more). It’s not source based but rather uses some internal accounting scalac has (which has both false positives and false negatives). Can something be done? 1. Develop a different unused deps mechanism which is source based as part of the build- I’m not sure how complicated this is to be honest. Java Bazel people have something in the Bazel repo which we looked at in the past but haven’t been able to get anywhere with. My initial plan with the +1 was to have something like this (as a separate action which will fail the build but doesn’t block the compilation graph). 2. Have an IntelliJ based unused deps source based mechanism- since IntelliJ does many of the heavy lifting AST wise then it seems easier to have some sort of IJ Inspection that looks at the entire target and the sources and removed deps. I think this won’t be cheap perf wise but is an interesting approach. 3. Drop and then recreate pattern- with buildozer you can drop all deps in one command and if you then have a tool which automatically adds dependencies you can use this pattern every X times to clean up. We (Wix) have one such tool which we plan on open sourcing (tentatively Q1) and are working on another one inside of our IJ plugin. Would love to hear your thoughts

…

On Fri, 1 Nov 2019 at 21:36 Jamie5 ***@***.***> wrote: When using plus one deps mode, many unused deps do get marked, but some deps can be left out because they are already the dep of another dep, and it also makes sense to leave them out because they never appear in the source code of the package being compiled, and the only reason the dep is needed is to make scalac happy. Would it be possible to have a unused deps mode for plus one deps mode where a dep is marked as an unused, unless it is explicitly referenced in the source code? (Not sure if this would end up with a number of false positives - not that familiar with scalac's needs) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF53MIXENSTDWEJSLRDQRSAKHANCNFSM4JH7D3JQ> .

[Jamie5]

Re 1, I don't think walking over the tree and finding used types would be very hard - the unused deps mechanism already does something similar (though they walk over different things). The tricky part may be to understand, when you see a type, should it be a direct dep? Though that might not be tricky at all and actually be really straightforward.

For 3, that seems useful though 1 feels more preferable. Also wouldn't this potentially remove direct dependencies that you also happen to depend on in a +1 situation, which would violate the desire of strict-deps?

[ittaiz]

Re 3- you nailed it on both problems this brings. Our existing solution is indeed not a very good one but better than letting build files rot. The new mechanism in the plugin won’t solve the need for user activation but will be source based so should solve the +1 issue. Re 1- how do you suggest to do this? Our main goal was to have a small to zero overhead for unused deps mechanism. Performing this iteration on the scalac action (as a plugin for example) was deemed costly by people more familiar with scalac than me. Performing this in a separate action has a big cost resources wise even though you might not increase the effective build time. This is the main reason we went with the current heuristic which capitalized on rough information scalac already collects. Another thought was to work with the zinc people to extract their analysis module to be more decoupled and then depend on that but it required bandwidth we didn’t have.

…

On Sat, 2 Nov 2019 at 21:23 Jamie5 ***@***.***> wrote: Re 1, I don't think walking over the tree and finding used types would be very hard - the unused deps mechanism already does something similar (though they walk over different things). The tricky part may be to understand, when you see a type, should it be a direct dep? Though that might not be tricky at all and actually be really straightforward. For 3, that seems useful though 1 feels more preferable. Also wouldn't this potentially remove direct dependencies that you also happen to depend on in a +1 situation, which would violate the desire of strict-deps? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF6362WZSHZWZBIU74DQRXHRHANCNFSM4JH7D3JQ> .

[Jamie5]

Yes, agreed that 3 is better than nothing, even with the problem of non-strict deps.

For 1, IME iterating over the tree as a plugin was not overly expensive (though non-zero). But certainly I'm not that experienced with scalac, nor the cost of the specific operations needed for this. From my understanding the existing strict-deps mechanism already iterates over the full AST and pays some reasonable amount of cost for it.

Actually is there a reason the existing strict-deps mechanism can't give the list of directly-referenced deps (from my understanding, it should be able to do that) which we can use to find the unneeded ones?

[ittaiz]

Have you read the code of the existing strict deps mechanism? It doesn’t do any iteration over the AST but rather just take the list of jars scalac needed to load. If you can do it via a plugin then maybe do it externally and measure the cost?

…

On Sat, 2 Nov 2019 at 22:38 Jamie5 ***@***.***> wrote: Yes, agreed that 3 is better than nothing, even with the problem of non-strict deps. For 1, IME iterating over the tree as a plugin was not overly expensive (though non-zero). But certainly I'm not that experienced with scalac, nor the cost of the specific operations needed for this. From my understanding the existing strict-deps mechanism already iterates over the full AST and pays some reasonable amount of cost for it. Actually is there a reason the existing strict-deps mechanism can't give the list of directly-referenced deps (from my understanding, it should be able to do that) which we can use to find the unneeded ones? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF4KQK3RYL2ETKVGRPLQRXQKXANCNFSM4JH7D3JQ> .

[Jamie5]

Ah I see, I misread the code. It does seem to use the native java strict_deps but only for java files. Which does appear to iterate over the AST. Now I see that the strict_deps for scala files does not do that.

Just to be sure, is https://github.com/bazelbuild/rules_scala/blob/master/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala what handles strict deps or am I misreading again? That one does appear to do very limited iteration over the AST in a way I don't fully follow.

Hmm maybe will try that out and see, it would hopefully answer the question easily enough. Is there a particular big bazel-ified codebase you would recommend?

[ittaiz]

None of the needed combo (OSS+Scala+Bazel). Do you work for a company that uses Bazel and Scala? Maybe you can time it on an internal codebase. If diff is small enough we can continue the discussion (we can time it on our codebase as well)

…

On Sat, 2 Nov 2019 at 23:19 Jamie5 ***@***.***> wrote: Ah I see, I misread the code. It does seem to use the native java strict_deps but only for java files. Which does appear to iterate over the AST. Now I see that the strict_deps for scala files does not do that. Just to be sure, is https://github.com/bazelbuild/rules_scala/blob/master/third_party/dependency_analyzer/src/main/io/bazel/rulesscala/dependencyanalyzer/DependencyAnalyzer.scala what handles strict deps or am I misreading again? That one does appear to do very limited iteration over the AST in a way I don't fully follow. Hmm maybe will try that out and see, it would hopefully answer the question easily enough. Is there a particular big bazel-ified codebase you would recommend? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF27DN2A3SVVH5F4DLTQRXVGHANCNFSM4JH7D3JQ> .

[Jamie5]

Ok Jamie5@cbba543 has a diff which is rather hacky (at least in terms of some plumbing) but does appear to do what we want.

Run on a 2.12.8 codebase, some of the rules testing old vs new unused dependency checker were as follows. Note that while testing methodology was probably fairly reasonable, it would be far from airtight. The results suggest that timing is not significantly different, but if you have good infra for timing it would probably have more reliable results.

Rule 1
New: 169.07, 166.10, 165.01 => 166.73
Old: 168.59, 160.43, 164.30 => 164.44

Rule 2
New: 22.06, 22.18, 25.65, 25.45 => 23.84
Old: 22.39, 21.79, 25.50, 25.84 => 23.88

Rule 3
New: 19.56, 19.56, 19.81, 19.28 => 19.55
Old: 19.80, 19.21, 19.85, 20.29 => 19.78

Some notes

• Due to constant folding, this has false positives. It appears to have a potential solution as noted in the code for certain versions of 2.12.x and up. In the test codebase, 3 ignores were needed to prevent this.
• There is some strange issue probably around exports where certain rules not listed in deps were still reported as unused. This might be due to how the list of direct jars is produced, but did not fully dig into it yet
• This might have additional false positives

[Jamie5]

@ittaiz did you get a chance to look at this?

[ittaiz]

No I'm sorry. This is really interesting to me but I'm a bit under capacity trying to wrap my head around the refactor PR. I'll do my best to get to it in the next few days, ok?

…

On Wed, Nov 13, 2019 at 8:18 PM Jamie5 ***@***.***> wrote: @ittaiz <https://github.com/ittaiz> did you get a chance to look at this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF7325EZLJRHJ52OGC3QTRAG5ANCNFSM4JH7D3JQ> .

[Jamie5]

Sounds good, no worries I just never know if something is in someone's queue or got lost in the notification void.

[ittaiz]

Fair enough, unfortunately we have so many notifications it does happen sometimes. Please feel free to ping me again mid of next week if I don't respond.

…

On Wed, Nov 13, 2019 at 11:27 PM Jamie5 ***@***.***> wrote: Sounds good, no worries I just never know if something is in someone's queue or got lost in the notification void. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#867>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQFZTCE66EGMA54GTPFLQTRWNZANCNFSM4JH7D3JQ> .

[Jamie5]

One other random thought, if this works could a similar mechanism be used to not do plus one deps but examine only the ijar and determine which deps are actually needed to compile the ijar, and propogate only those ones? (IIRC I read somewhere java rules did something similar like that but not sure about it).

And maybe strict deps can stem from this as well, the mechanisms seem similar at least.

[ittaiz]

Potentially. I think that there are more nuances in this space so work will need to be iterative but a strong enough tool can definitely help us improve things.
I encourage you to dive a bit more into how java rules work since they are very advanced in these areas.

[Jamie5]

@ittaiz did you get a chance to take a look?