Improve slow error prones: LambdaMethodReference and UnnecessarilyQualified

[ash211]

I ran with -PerrorProneTimings from palantir/suppressible-error-prone#38 on an internal repo and had a gradle module with these numbers:

Total errorprone time: 71.293000 seconds 
 
ErrorProneName: <time taken> (percentage of all errorprone time) 
 
LambdaMethodReference                            : 29.449000 seconds (41.31%) 
UnnecessarilyQualified                           : 12.412000 seconds (17.41%) 
IllegalSafeLoggingArgument                       : 5.425000 seconds  (7.61%) 
NullAway                                         : 4.582000 seconds  (6.43%) 
SafeLoggingPropagation                           : 1.283000 seconds  (1.80%) 
ParameterName                                    : 1.026000 seconds  (1.44%)

I'm surprised that LambdaMethodReference and UnnecessarilyQualified are so expensive. Especially compared to SafeLoggingPropagation, which seems like it'd be much more expensive to trace log safety through a codebase.

I've disabled these two slow error prones on a large internal repo and saw non-incremental compile speedup from 10m27s to 7m9s.

To re-enable these at some point in the future, is there anything we can do to improve performance and reduce runtime of these checks?

[pkoenig10]

I think UnnecessarilyQualified is so expensive because it matches on every MemberSelectTree and iterates through the imports. Member select trees are incredibly common. I imagine that we can somehow cache/precompute the import information per compilation tree in a way that only requires a constant time lookup.

[carterkozak]

I think there's some very low hanging fruit around UnnecessarilyQualified, I can take a quick crack at it shortly

[carterkozak]

See #2998

[carterkozak]

We should probably not run LambdaMethodReference by default -- I'd implemented it initially in order to clean up lambdas left by other refactors (e.g. OptionalOrElseGetMethodInvocation, see the example from the original PR: #763).

It's particularly expensive because it's difficult to determine whether generic type inference will work correctly, so we use the SuggestedFixes.compilesWithFix utility to attempt compilation with proposed changes. Compilation is expensive! Over time, the build will succeed with more and more of these sub-compilation-failures for lambdas that cannot be replaced, multiplying out the compilation times in some areas.

I like the idea of having the check around to clean up results from other checks, or perhaps running asynchronously via an excavator rather than enforcing for all compilations. Thoughts?

[ash211]

I had also considered doing LambdaMethodReference async as a run of a new "Excavator: Apply errorprone suggestions". This would be in the same style as we currently have https://github.com/palantir/assertj-automation configured to do. But as @pkoenig10 pointed out on an internal thread, it's not great to allow problems to merge into a repo and then try fix post-merge. Better to prevent them from merging in the first place, if possible.

So I think the preference is:

1. make LambdaMethodReference fast enough to run by default, or if that's infeasible
2. make it run async as part of an excavator

[carterkozak]

Making it faster is difficult because it requires us to model language features like generic type inference to avoid ambiguity/failing suggestions. That's a bit more complexity than I'd prefer to encode, and it can change over time with future java releases as well.

[ash211]

With LambdaMethodReference disabled by default and UnnecessarilyQualified dramatically improved, I think we're good to close?

I'm still a bit surprised that IllegalSafeLoggingArgument takes as long as NullAway -- tracing nullability seems way more complicated than checking if an unsafe arg is a SafeArg.of() parameter. Maybe I'm off and it's doing much more than that. But there's much less juice to squeeze now with the above changes in place:

Total errorprone time: 14.731000 seconds                                                                                                                                 
                                                                                                                                                                         
ErrorProneName: <time taken> (percentage of all errorprone time)                                                                                                         
                                                                                                                                                                         
IllegalSafeLoggingArgument                       : 2.643000 seconds (17.94%)                                                                                             
NullAway                                         : 2.065000 seconds (14.02%)                                                                                             
SafeLoggingPropagation                           : 851 millis       (5.78%)                                                                                              
ParameterName                                    : 338 millis       (2.30%)                                                                                              
ArgumentSelectionDefectChecker                   : 230 millis       (1.56%)                                                                                              
AlmostJavadoc                                    : 192 millis       (1.30%)                                                                                              
SecuritySchemaUpgrade                            : 183 millis       (1.25%)                                                                                              
DoNotCall                                        : 169 millis       (1.15%)                                                                                              
ThreadJoinLoop                                   : 167 millis       (1.13%)                                                                                              
SameNameButDifferent                             : 160 millis       (1.09%)                                                                                              
SecuritySchemaTestFixture                        : 156 millis       (1.06%)                                                                                              
DangerousIdentityKey                             : 139 millis       (0.95%)                                                                                              
Immutable                                        : 133 millis       (0.91%)                                                                                              
ForOverride                                      : 132 millis       (0.90%)                                                                                              
StrictCollectionIncompatibleType                 : 130 millis       (0.88%)                                                                                              
AlreadyChecked                                   : 114 millis       (0.78%)                                                                                              
UnnecessarilyQualified                           : 108 millis       (0.74%)                                                                                              
TruthIncompatibleType                            : 106 millis       (0.72%)                                                                                              
SecuritySchemaPrivateApi                         : 97 millis        (0.66%)