[prone] apply -XepDisableAllWarnings

[Pankraz76]

• Removing error-prone from our build #2745
• picnic breaking -XepDisableAllWarnings google/error-prone#5365

Please DO NOT FORCE PUSH. Don't worry about messy history, it's easier to do code review if we can tell what happened after the review, and force pushing breaks that.

Please make sure that your PR allows edits from maintainers. Sometimes it's faster for us to just fix something than it is to describe how to fix it.

After creating the PR, please add a commit that adds a bullet-point under the [Unreleased] section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:

• a summary of the change
• either
  • a link to the issue you are resolving (for small changes)
  • a link to the PR you just created (for big changes likely to have discussion)

If your change only affects a build plugin, and not the lib, then you only need to update the plugin-foo/CHANGES.md for that plugin.

If your change affects lib in an end-user-visible way (fixing a bug, updating a version) then you need to update CHANGES.md for both the lib and all build plugins. Users of a build plugin shouldn't have to refer to lib to see changes that affect them.

This makes it easier for the maintainers to quickly release your changes :)

[nedtwigg]

I prefer to just remove it. It provided us a huge migration, great, but all that churn didn't reveal any bugs. Errorprone is a great project, but Spotless is a weird case, I don't think it's a good fit for all the weird glue code that makes Spotless work.

[Pankraz76]

reveal any bugs

Usability, accessibility, and readability are also considered a form of “bug” and or "issue", at least within the context of this project.

With tools like this, it’s also about preventing further issues from occurring.

I understand you’re against it, but “error-prone” is already well-established throughout Gradle and similar ecosystems. It makes me wonder why this Java codebase should be treated differently from others.

[nedtwigg]

Spotless has a ton of stuff in it. As a projects get large, they tend to get harder to contribute to, such that eventually it's so hard that new stuff doesn't make it in at all.

Right now there are two PRs adding new features that I think are quite useful

• Allow foreign executables to explicitly specify "use any found version" with '*' #2758
• new step to expand java wildcard imports #2744

And for both PRs, I have spent more time on quality checks for nits that I think are immaterial. Quality matters for sure, but I think Spotbugs has a better signal-to-noise ratio. It stays quiet unless it's a biggish deal.

I'm open to being wrong, and for Spotless I try to lean towards "let people do what they want if it doesn't break other people's stuff". Right now I think I've let it swing too far towards strict checks, and it's too hard to get an actual feature or bugfix into the codebase because it gets mired in strict automated rules.

A good barometer for this is

• with all of the changes merged by the automated fixes (thanks for those btw, great cleanup!)
• how many bugs did we find?

I believe we found 0. It's cleaner now, sure, but it's mostly stylistic. I'm fine with style rules, I'm not okay with the project being hard to contribute to.

[Pankraz76]

but all that churn didn't reveal any bugs

This seems like a questionable conclusion, since we had to suppress three real Error Prone findings:

• FunctionalInterfaceMethodChanged
• OverridesJavaxInjectableMethod
• JavaxInjectOnAbstractMethod

I would suggest fixing the issues you’ve encountered, because it really looks like something is misconfigured or mis-implemented. Suppressing them doesn’t seem like a healthy long-term approach.

spent more time on quality checks for nits that I think are immaterial

That really shouldn’t be the case. If it is, then IN_PLACE exists specifically to automate the annoying parts — for example, static imports. Doing those manually is tedious, so I understand your frustration.

I generally have no issues writing code that Error Prone doesn’t complain about, so I would genuinely appreciate seeing the exact issues you ran into.

Normally, Error Prone and similar tools are designed to only flag odd or suspicious patterns — the kinds of things that are worth reviewing because they often hide real problems. If not its a problem in the check itself.

Thanks for the update.

[nedtwigg]

I would genuinely appreciate seeing the exact issues you ran into.

• In Allow foreign executables to explicitly specify "use any found version" with '*' #2758, the sanity check is failing after 8,000 lines (!!!) of warnings
• In new step to expand java wildcard imports #2744, the build was failing with 3,000 lines of warnings that end in this error:

Execution failed for task ':lib:compileJavaParserJava'.
See https://docs.gradle.org/9.2.1/userguide/configuration_cache_requirements.html#config_cache:requirements:disallowed_types
Compilation failed; see the compiler output below.
/home/runner/work/spotless/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(); },

It wants the author to use toImmutableMap, which I believe is a Guava thing that we don't have. I thought the code looked fine, and I'm grateful that anyone was contributing PRs at all while the build was throwing thousands of lines of warnings lol.

Quality is good, but the goal is delivering value. People deliver value with shit codebases all the time. It's not my goal to have a shit codebase, haha, but it's more important to deliver value and accept a little shit than to have a pristince cleanroom that nobody can land a new feature in.

[Pankraz76]

value with shit codebases all the time

Yes, that’s the hard reality (humans are error prone) — all day, every day.

In my opinion, we’re actually on the exact same page. We’re making the same arguments from slightly different angles, and we understand and relate to each other’s perspective.

Please let me fix Error Prone — that’s the actual issue here, not Error Prone itself.

It worked “perfectly fine” for at least 20 days.

But now that we’ve introduced a lot of new code, it naturally has the potential to reveal many new findings.
That shouldn’t be a problem; it should simply highlight what needs attention and either be fixed or suppressed — exactly the way Gradle handles it.
If a project 10 times the size of this one can handle this plugin, I’m sure Spotless can as well.

For example:

• [prone] fix NPE apply UndefinedEquals gradle/gradle#35662
• [prone] Apply UndefinedEquals gradle/gradle#35609

Having thousands of unrelated warnings is totally unacceptable, and it strongly motivates fixing the tool configuration so we can get clean, meaningful feedback again.

@cushon Would it be sufficient to attach the commit as a zipped reproducer?
I can’t really imagine how to reproduce these issues from an external standpoint.
In my opinion, we may need to use the Spotless project itself to reproduce the problem.