consider new PatchLocation mode `IN_PLACE_INCLUSIVE`

[Pankraz76]

Hello folks,

Thanks for the great tool and the opportunity to contribute to JUnit and other projects, as seen in:

• Add error-prone.picnic.tech featuring RedundantStringConversion junit-team/junit-framework#5006
• Apply RedundantStringConversion gradle/gradle#35278
• Issue #14194: Add LexicographicalAnnotationListing checkstyle/checkstyle#17892

All of this originated from and revolves around this tool.

While adapting it repeatedly, I've noticed the IN_PLACE behavior can be inconsistent. Sometimes it works without the required counterpart XepPatchChecks (which is another story), as encountered in:

• Apply errorprone suggestions using PatchLocation:IN_PLACE for DefaultCharset gradle/gradle#35107

I have experienced that IN_PLACE is exclusive, meaning it only runs the checks passed through the other variable. Please consider a new hybrid mode, perhaps IN_PLACE_INCLUSIVE, which would apply the rewrite approach—meaning it applies all suggested fixes. Some checks will have fixes, others won't. For simple stuff like ordering, this is about avoiding annoyance and improving overall performance.

if (!getenv().containsKey('CI') && getenv('IN_PLACE')?.toBoolean()) {
  errorproneArgs.addAll(
          '-XepPatchLocation:IN_PLACE',
          '-XepPatchChecks:RedundantStringConversion'
  )
}

While patching is considered experimental, others consider it good enough, and this idea would support that argument.

Origination from:

• Add error-prone.picnic.tech featuring RedundantStringConversion junit-team/junit-framework#5006 (comment)

---

Explanation

Sorry for the confusion, and thanks for considering.

This is just a new error-prone check, originating from Picnic, and it behaves the same as other rules in all builds. If the check finds an issue, it will fail the build and show a suggested fix, just like the current ones.

We can’t use the optional patching mode (IN_PLACE) because it overrides the rule settings when activated. While this could work locally, it would require significant overhead—like maintaining an explicit whitelist to keep the ruleset in sync. Otherwise, a locally green build might fail in CI, as some checks aren't included, creating a rule delta between local and CI builds. This is not ideal and would lead to manual intervention, which patching is meant to avoid (similar to what's done with rewrite).

This means only the specified checks would run locally, leading to a successful local build but a failed CI build, since CI does not patch and still enforces all rules for quality control.

To have patching, we would either need to:

• Adapt Error Prone to support this as a new feature, or
• Create a whitelist approach similar to Spotless.

Both options are unrealistic, so we should move forward with the current setup and leave the patching topic aside for now—to stop staring and start finishing. This can be revisited later, along with considerations like StaticImport.

Currently, Spotless uses a fully configured ruleset, which checks only the configured rules instead of following the convention. While this allows auto-fix capability, it’s not ideal for maintaining consistency with conventions.

Given that the required changes here are minimal, keeping our current setup is the better approach.

Related:

• https://github.com/diffplug/spotless/pull/2664/files#r2416889516
• Apply errorprone suggestions using PatchLocation:IN_PLACE for DefaultCharset gradle/gradle#35107
• Add error-prone.picnic.tech featuring RedundantStringConversion apache/kafka#20688
• Add error-prone.picnic.tech featuring RedundantStringConversion diffplug/spotless#2664

[rickie]

@Pankraz76 Have you tried the empty string for patchChecks. IIUC that will achieve the behavior you're looking for.

In Error Prone Support we have this script that does apply all the violations there are.
I validated that without -Perror-prone-fork this also works, so it is also possible with mainline Error Prone.

[tbroyer]

This will run disabled checks for now though: 157ce44