error-prone icon indicating copy to clipboard operation
error-prone copied to clipboard

Add `rewrite` support for `errorprone.refasterrules`

Open Pankraz76 opened this issue 4 months ago • 10 comments

1. What are you trying to do?

Add rewrite support for errorprone.refasterrules

I’d like to propose integrating Google’s Error Prone and its Picnic extension (demonstrated in Automating Away Bugs with Error Prone | PlatformCon 2023) to enable automated bug fixes via rewrite rules. This would complement Checkstyle’s static analysis capabilities by addressing semantic bugs rather than stylistic issues.

Motivation

Error Prone’s refaster/rewrite rules can automatically:

  • Fix common bug patterns (e.g., String.equals() misuse)
  • Modernize code (e.g., JDK migration helpers)
  • Enforce best practices (e.g., null-check improvements)

Real-world adoptions show tangible benefits:

  • https://github.com/checkstyle/checkstyle/pull/17545
  • https://github.com/pmd/pmd/pull/5956
  • https://github.com/opensearch-project/OpenSearch/pull/18791
  • https://github.com/diffplug/spotless/pull/2576
  • https://github.com/apache/kafka/pull/20219

Proposal

  1. Add support for errorprone.refasterrules-based rewrites
  2. Implement with opt-in adoption (no breaking changes)
  3. Include suppression mechanisms for API constraints

Discussion Points

  • Need consensus on:
  • Scope of auto-fixes
  • Preferred suppression strategy
  • Integration approach

Next Steps

I’m happy to:

  • Prepare a PoC demonstrating the value
  • Collaborate on implementation strategy
  • Address any concerns about compatibility

relates to:

  • https://github.com/checkstyle/checkstyle/issues/16543

Pankraz76 avatar Aug 09 '25 10:08 Pankraz76

Hi, I'm not sure I understand what the proposal is. This is the repository for Error Prone. We already run Error Prone checks on this codebase.

Can you clarify what the proposal is here? Do you want to integrate Error Prone into something? Or add support for a specific feature to Error Prone? Or something else?

cushon avatar Nov 06 '25 08:11 cushon

We already run

was not aware about this, so its fixed already thanks. Any more changes are then individual approaches.

Thanks.

Pankraz76 avatar Nov 06 '25 10:11 Pankraz76

this is one way to integrate. i could not find picnic so i tried this way:

  • https://github.com/google/error-prone/pull/5328

Pankraz76 avatar Nov 08 '25 13:11 Pankraz76

Can you clarify what the proposal is here? Do you want to integrate Error Prone into something? Or add support for a specific feature to Error Prone? Or something else?

I'm still not sure exactly what the proposal is here, can you help me understand? Is the idea to add an openrewrite config (the rewrite.yml in the PR) and use it to make some cleanups to Error Prone?

cushon avatar Nov 10 '25 11:11 cushon

Yes exact. Its "just another" tool, allowing to patch, but on a whole different kind of scope. Promoting from pattern to structue.

We could even create our own rewrite recipes if needed — similar to how Checkstyle provides custom recipes to handle the burden of certain checks:

  • https://github.com/checkstyle/checkstyle-openrewrite-recipes

This approach could integrate some Picnic rules in a much more user-friendly and versatile way than using the Prone configuration directly.

  • https://github.com/Pankraz76/junit5/pull/13

Pankraz76 avatar Nov 10 '25 12:11 Pankraz76

@Pankraz76 I suspect you might be missing some context here.

Refaster is created by Google and the implementation of it actually lives within this repository. Error Prone Support is a repository that contains extensions to both Error Prone and Refaster.

Instead of adding a new tool which has an impact on buildtime, I suspect adding the Refaster rules directly could make more sense. With the side note that I'm not fully aware of how within Google these checks are being applied. Sometimes there is a self-check that's being applied, so the ideal setup would be to add the desired Refaster rules there 🤔.


So, in the description it mentions:

I’d like to propose integrating Google’s Error Prone and its Picnic extension

But we are exactly in the official repository of Error Prone already. Also, important to note is that Error Prone itself has a lot of checks that have overlap with what is provided by OpenRewrite (see a full list).

rickie avatar Nov 10 '25 13:11 rickie

As best I understand the suggest here it is to perform a code cleanup of Error Prone's own source code, using openrewrite rules, including ones that run Error Prone Support refaster templates. Is that right?

I'm open to making code cleanups to Error Prone, but we probably would want to review and discuss a bit which ones, there are a bunch of difference changes in the draft PR and I think some of them may be more valuable than others for us.

cushon avatar Nov 10 '25 13:11 cushon

Refaster rules directly could make more sense

If we only would talk about refaster exclusifly, im fine with going by that argumentation, not having the need for rewrite.

As we are aiming beyond picnic integration, this change is also about laying the foundation for rewrite.

Bumping maven version, applying security fixes and common static analysis recipes.

Rewrite offers a wide range, making it an tryout candidate.

but we probably would want to review and discuss a bit which ones, there are a bunch of difference changes in the draft PR and I think some of them may be more valuable than others for us.

Yes this is something the core team needs to distinguish. First its about including some trivial stuff, then going into the details later on.

Thanks for the feedback so far.

Pankraz76 avatar Nov 10 '25 13:11 Pankraz76

I cleaned up some of the occurrences of !optional.isPresent() -> optional.isEmpty() using Error Prone's version of that refactoring in https://github.com/google/error-prone/commit/22c7428b75b8177e7b6d0433b65197e80e8e6cdb

cushon avatar Nov 12 '25 12:11 cushon

thanks for the fixup.

rebase:

[INFO] Running recipe(s)...
[INFO] Printing available datatables to: target/rewrite/datatables/2025-11-12_13-25-51-096
[WARNING] The recipe produced 5 warning(s). Please report this to the recipe author.
[WARNING] Changes have been made to check_api/pom.xml by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.javax.AddInjectDependencies
[WARNING]             org.openrewrite.java.dependencies.AddDependency: {groupId=jakarta.inject, artifactId=jakarta.inject-api, version=1.0.3, onlyIfUsing=javax.inject.*, acceptTransitive=true}
[WARNING]         org.openrewrite.java.migrate.javax.AddCommonAnnotationsDependencies
[WARNING]             org.openrewrite.java.dependencies.AddDependency: {groupId=jakarta.annotation, artifactId=jakarta.annotation-api, version=1.3.x, onlyIfUsing=javax.annotation..*, scope=provided, acceptTransitive=true}
[WARNING] Changes have been made to check_api/src/main/java/com/google/errorprone/apply/DescriptionBasedDiff.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Changes have been made to core/pom.xml by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.javax.AddInjectDependencies
[WARNING]             org.openrewrite.java.dependencies.AddDependency: {groupId=jakarta.inject, artifactId=jakarta.inject-api, version=1.0.3, onlyIfUsing=javax.inject.*, acceptTransitive=true}
[WARNING]         org.openrewrite.java.migrate.javax.AddCommonAnnotationsDependencies
[WARNING]             org.openrewrite.java.dependencies.AddDependency: {groupId=jakarta.annotation, artifactId=jakarta.annotation-api, version=1.3.x, onlyIfUsing=javax.annotation..*, scope=provided, acceptTransitive=true}
[WARNING] Changes have been made to core/src/main/java/com/google/errorprone/bugpatterns/checkreturnvalue/ExternalCanIgnoreReturnValue.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Changes have been made to core/src/test/java/com/google/errorprone/ErrorProneJavacPluginTest.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Changes have been made to core/src/test/java/com/google/errorprone/bugpatterns/apidiff/CompilationBuilderHelpers.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Changes have been made to docgen_processor/pom.xml by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.javax.AddCommonAnnotationsDependencies
[WARNING]             org.openrewrite.java.dependencies.AddDependency: {groupId=jakarta.annotation, artifactId=jakarta.annotation-api, version=1.3.x, onlyIfUsing=javax.annotation..*, scope=provided, acceptTransitive=true}
[WARNING] Changes have been made to docgen/src/main/java/com/google/errorprone/DocGenTool.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Changes have been made to docgen/src/main/java/com/google/errorprone/BugPatternFileGenerator.java by:
[WARNING]     org.openrewrite.java.migrate.Java8toJava11
[WARNING]         org.openrewrite.java.migrate.nio.file.PathsGetToPathOf
[WARNING]             org.openrewrite.java.ChangeMethodTargetToStatic: {methodPattern=java.nio.file.Paths get(..), fullyQualifiedTargetTypeName=java.nio.file.Path}
[WARNING]             org.openrewrite.java.ChangeMethodName: {methodPattern=java.nio.file.Path get(..), newMethodName=of}
[WARNING] Please review and commit the results.
[WARNING] Estimate time saved: 45m
[INFO] ------------------------------------------------------------------------

Pankraz76 avatar Nov 12 '25 12:11 Pankraz76