Removing error-prone from our build

[nedtwigg]

We have recently merged a ton of automated refactors, I believe some by OpenRewrite and some by error-prone. Right now we have:

• spotless
• spotbugs
• errorprone
• openrewrite

We just received an excellent PR tackling a very hard and long-requested feature:

• https://github.com/diffplug/spotless/pull/2744

It failed CI. I took a look and

• https://github.com/diffplug/spotless/actions/runs/19546227953/job/55965599249
• literally 3000 lines of warnings in the build (!!!)
• I clone it locally
• Looks like this is the failure

* What went wrong:
Execution failed for task ':lib:compileJavaParserJava'.
> Compilation failed; see the compiler output below.
  /Users/ntwigg/Documents/dev/spotless/lib/src/javaParser/java/com/diffplug/spotless/glue/javaParser/ExpandWildcardsFormatterFunc.java:95: error: [CollectorMutability] Avoid `Collectors.to{List,Map,Set}` in favor of collectors that emphasize (im)mutability
                                .collect(toMap(Function.identity(),
                                              ^
      (see https://error-prone.picnic.tech/bugpatterns/CollectorMutability)
    Did you mean '.collect(toImmutableMap(Function.identity(),' or 't -> new TreeSet<>(Comparator.comparing(ImportDeclaration::getNameAsString)), (a, b) -> { throw new IllegalStateException(); }, HashMap::new));'?
  1 error

I like immutability, but the warning above is not easy to fix. We don't have guava, toMap is a good choice here.

@Pankraz76 The feature in the PR is more important than the rule-check. When rule-checking is consuming more reviewer time than actual PRs, something is wrong. For this reason, I removed error-prone fom the build in the above-mentioned PR.

• https://github.com/diffplug/spotless/pull/2744/commits/06b33ff435ab94320ac9c632371cc60cbd72c99b

[Pankraz76]

I agree — it isn’t very pleasant or kind to use, or integrate, and sometimes it feels overwhelming and spam-like.

[Pankraz76]

• literally 3000 lines of warnings in the build (!!!)

-XepDisableAllWarnings might was the missing key and should be the cure of this madness.

Idk whats the point, beside the spamming yes, as warnings are just optional?

[jbduncan]

I haven't been paying attention to our ErrorProne setup, so I don't know which rules we had previously enabled.

Usually, the rules that come out-of-the-box and are enabled by default are a good baseline, because they're based on Google's internal lessons on which rules produce many false positives and which don't. Are we seeing problems from these rules, or any other rules that we've enabled, or both?

The way I see it, it would be worth reintroducing ErrorProne with just the enabled-by-default rules and slowly reintroduce others like the Picnic ones over time.

What do you think, both?

[Pankraz76]

thanks for your diplomatic support in guiding/guarding the overall quality goals.

-XepDisableAllWarnings might was the missing key and should be the cure of this madness.

This should fix the massiv output, also referring in:

• https://github.com/diffplug/spotless/pull/2754
  • https://github.com/diffplug/spotless/issues/2743

[Pankraz76]

zero warnings as requested:

• https://github.com/diffplug/spotless/pull/2766

[nedtwigg]

Here's an example (of rewrite, not error-prone)

• 17 hrs ago #2758 gets opened.
• 12 hrs ago I notice it exists. It's blocked on me because I need to approve CI to run.
  • I look at the code, great little feature, people have asked for it for years, tight implementation, updated changelog, looks like I can just "approve CI", "enable automerge" and the PR submitter and I are both done
• 10 hrs later I notice it hasn't merged. CI failed. Huh. sanityCheck failed
  • rewriteDryRun failed. There are 8,000 lines of warnings, hard for new contributor to tell what's going on, I know the answer is gradlew rewriteRun so I drop a comment
• 1 hr ago the author sees it, switches context, runs the command, pushes it up
  • https://github.com/diffplug/spotless/pull/2758/commits/de5469f9e4f1b6d4ca21786730a912b990d42ca3
• just one I notice he pushed, I approve CI again and enable automerge, and now the feature has landed. Huzzah!

I like what rewrite did here, it's better! In sum, for this particular PR:

• Benefit: slightly safer pattern for avoiding NPE
• Cost: 2 context switches for me, 1 context switch for the PR submitter

So the choice to keep errorProne and rewrite is about cost/benefit. Maybe we trigger rewriteRun so that it runs when spotlessApply runs? One less thing for contributors to learn? The trouble is it's pretty slow and throws so many warnings...

[jbduncan]

@nedtwigg That's fair. Despite my love for error-prone, I totally acknowledge that the current setup is consuming a lot of your time and this whole thing has been raising your hackles, so I'm happy enough to remove it and I'd totally understand if you want nothing to do with it. :)