Add `rewrite-templating` annotation processor to also produce OpenRewrite recipes #925

timtebeek · 2023-12-17T16:43:53Z

Hi all! As discussed both offline and on openrewrite/rewrite-templating#5 here's a PR that adds the rewrite-templating annotation processor. This generates OpenRewrite recipes from the same Refaster rules in error-prone-contrib, which can then be run through OpenRewrite and Moderne as well using the same jar.

Increase in file size

There's an increase in the file size after adding this annotation processor. Nothing shocking in absolute sense, but figured point it out here for review in case anyone is sensitive to that. All dependencies I've added as <scope>provided</scope>

- -rw-rw-r-- 1 tim tim 2,9M dec 17 17:26 error-prone-contrib-0.14.1-SNAPSHOT.jar
+ -rw-rw-r-- 1 tim tim 3,4M dec 17 16:59 error-prone-contrib-0.14.1-SNAPSHOT.jar

To do here

Go over "forbidden" licenses used
Run through any additional checks
Potentially leverage more of the existing tests, depending on generated recipe coverage

Future work in rewrite-templating

rewrite-templating doesn't yet support all features of Refaster used in error-prone-support; here's a quick link to a few open issues. Well possible that we discover more as we go, but figured we can start somewhere and iterate.

Fixes openrewrite/rewrite-templating#5

timtebeek · 2023-12-17T16:49:39Z

As for the forbidden licenses; these new warnings are logged from mvnd -U clean package -Dverification.warn

[WARNING] There are 7 forbidden licenses used:

'BSD 3-Clause "New" or "Revised" License (BSD-3-Clause)' used by 1 dependencies:
- abego TreeLayout Core (org.abego.treelayout:org.abego.treelayout.core:1.0.3 - http://treelayout.sourceforge.net)
'The MIT License (MIT)' used by 1 dependencies:
- ClassGraph (io.github.classgraph:classgraph:4.8.165 - https://github.com/classgraph/classgraph)
'Public Domain, per Creative Commons CC0' used by 2 dependencies:
- HdrHistogram (org.hdrhistogram:HdrHistogram:2.1.12 - http://hdrhistogram.github.io/HdrHistogram/)
- LatencyUtils (org.latencyutils:LatencyUtils:2.0.3 - http://latencyutils.github.io/LatencyUtils/)
'The BSD License' used by 1 dependencies:
- StringTemplate 4 (org.antlr:ST4:4.3.4 - http://nexus.sonatype.org/oss-repository-hosting.html/ST4)
'BSD-2-Clause' used by 1 dependencies:
- HdrHistogram (org.hdrhistogram:HdrHistogram:2.1.12 - http://hdrhistogram.github.io/HdrHistogram/)
'Unicode/ICU License' used by 1 dependencies:
- ICU4J (com.ibm.icu:icu4j:71.1 - https://icu.unicode.org/)
'BSD licence' used by 1 dependencies:
- ANTLR 3 Runtime (org.antlr:antlr-runtime:3.5.3 - http://www.antlr.org)

Some can likely be merged, although I've only briefly looked into this as I'm not a lawyer to know the exact differences between the number of clauses for instance.

error-prone-support/pom.xml

Lines 1238 to 1299 in 94023ed

    
           <licenseMerge> 
        
               <!-- --> 
        
               Apache-2.0 
        
               | Apache 2 
        
               | Apache 2.0 
        
               | Apache License 2.0 
        
               | Apache License, Version 2.0 
        
               | The Apache License, Version 2.0 
        
               | The Apache Software License, Version 2.0 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               BSD-3-Clause 
        
               | 3-Clause BSD License 
        
               | BSD 3-clause 
        
               | BSD License 3 
        
               | Eclipse Distribution License (New BSD License) 
        
               | New BSD License 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               CC0-1.0 
        
               | CC0 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               CDDL-1.1 
        
               | CDDL 1.1 
        
               | Dual license consisting of the CDDL v1.1 and GPL v2 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               EPL-1.0 
        
               | Eclipse Public License 1.0 
        
               | Eclipse Public License - v 1.0 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               EPL-2.0 
        
               | Eclipse Public License - v 2.0 
        
               | Eclipse Public License v2.0 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               GPL-2.0-with-classpath-exception 
        
               | CDDL/GPLv2+CE 
        
               | GNU General Public License, version 2 (GPL2), with the classpath exception 
        
               | GNU General Public License, version 2, with the Classpath Exception 
        
               | GPL2 w/ CPE 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               LGPL-3.0+ 
        
               | GNU Lesser Public License 
        
           </licenseMerge> 
        
           <licenseMerge> 
        
               <!-- --> 
        
               MIT 
        
               | MIT license 
        
               | MIT License 
        
               | The MIT License 
        
           </licenseMerge>

The first three for instance would likely need something like this

diff --git a/pom.xml b/pom.xml
index d301c084..39170056 100644
--- a/pom.xml
+++ b/pom.xml
@@ -1253,6 +1253,7 @@
                                 | BSD License 3
                                 | Eclipse Distribution License (New BSD License)
                                 | New BSD License
+                                | BSD 3-Clause "New" or "Revised" License (BSD-3-Clause)
                             </licenseMerge>
                             <licenseMerge>
                                 <!-- -->
@@ -1296,6 +1297,12 @@
                                 | MIT license
                                 | MIT License
                                 | The MIT License
+                                | The MIT License (MIT)
+                            </licenseMerge>
+                            <licenseMerge>
+                                <!-- -->
+                                Public Domain
+                                | Public Domain, per Creative Commons CC0
                             </licenseMerge>
                         </licenseMerges>
                         <!-- Nearly no projects ship a "missing third party

Stephan202 · 2023-12-17T17:06:42Z

The license aliases are all safe; I've added a commit to suppress then. (The formatting, classification and comments may look a bit odd, but this is fully sync'ed from our internal Maven Parent.)

Stephan202 · 2023-12-17T17:14:07Z

Added one more commit; build should now pass 👀.

The increase in artifact size isn't an issue I think. ✔️

I'll need to play a bit more with the PR before finalizing the review (which may not happen this evening), but let me say: very cool and thanks for pushing this forward! 🚀

github-actions · 2023-12-17T17:15:05Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-17T17:18:31Z

Hmm, the SonarCloud analysis fails with:

Project not found. Please check the 'sonar.projectKey' and 'sonar.organization' properties, the 'SONAR_TOKEN' environment variable, or contact the project administrator

That's a setup issue on our side; will have to check the docs on that one. (Most PRs we get are from Picnic colleagues, who push a branch to this repo, rather than creating a separate fork.)

The JDK 21 build also fails when analyzing the generated sources. If OpenRewrite can be tweaked to generate compliant files: great! Otherwise we'll have to check how to suppress those warnings. (But that may then require a separate compilation phase, which wouldn't be nice. 🤔.)

timtebeek · 2023-12-17T18:00:26Z

Thanks for the quick feedback! Looking at the Java 21 build failures it seems to stumble over new warnings mostly:

[WARNING] /home/runner/work/error-prone-support/error-prone-support/error-prone-contrib/target/generated-sources/annotations/tech/picnic/errorprone/refasterrules/PrimitiveRulesRecipes$LessThanRecipe$1_before.java:[7,8] use of default constructor, which does not provide a comment
[WARNING] /home/runner/work/error-prone-support/error-prone-support/error-prone-contrib/target/generated-sources/annotations/tech/picnic/errorprone/refasterrules/PrimitiveRulesRecipes$LessThanRecipe$1_before.java:[9,8] no main description
[WARNING] /home/runner/work/error-prone-support/error-prone-support/error-prone-contrib/target/generated-sources/annotations/tech/picnic/errorprone/refasterrules/PrimitiveRulesRecipes$LessThanRecipe$1_after.java:[7,8] use of default constructor, which does not provide a comment
[WARNING] /home/runner/work/error-prone-support/error-prone-support/error-prone-contrib/target/generated-sources/annotations/tech/picnic/errorprone/refasterrules/PrimitiveRulesRecipes$LessThanRecipe$1_after.java:[9,8] no main description
[WARNING] /home/runner/work/error-prone-support/error-prone-support/error-prone-contrib/target/generated-sources/annotations/tech/picnic/errorprone/refasterrules/PrimitiveRulesRecipes$LessThanOrEqualToRecipe$1_before.java:[7,8] use of default constructor, which does not provide a comment

What's the magic comment I need to add to the generated template files to make that warning go away?

As for the SonarCloud issue: by default forks don't have (and shouldn't have) access to secrets I believe, which might cause your issues here. Not quite sure about what the best way forward is there, since you also don't want untrusted forks from running away with your Sonar and other secrets. Fun times.

Stephan202 · 2023-12-17T18:10:12Z

use of default constructor, which does not provide a comment

This is one we've also seen on other code; we work around it by making the constructor explicit and adding a comment, like this. Seems a bit artificial, but okay.

no main description

Haven't seen that one before, but based on this thread I assume it's about making sure that the Javadoc has an initial summary line.

W.r.t. Sonarcloud: there's this feature request. Perhaps it'd be secure if the code on the target branch would be executed. But if we can't find a fix, we can look into making the analysis conditional. (Which would kind of defeat the purpose, but alas.)

github-actions · 2023-12-17T19:25:29Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-17T19:47:07Z

I added a commit to try and skip the SonarCloud analysis; that worked. Opened a separate PR for that improvement (also to test the "other side" of the equation): #926.

github-actions · 2023-12-18T05:28:27Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

I added a commit that makes the JDK 11, 17 and 21 builds fully pass, at least locally. Let's see what GHA thinks 👀

.mvn/maven.config

.github/workflows/sonarcloud.yml

error-prone-contrib/pom.xml

...prone-contrib/src/test/java/tech/picnic/errorprone/refasterrules/StringRulesRecipesTest.java

github-actions · 2023-12-18T07:07:02Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T07:30:26Z

I added another commit to deal with some dual-licensed dependencies; by whitelisting more entries we avoid some redundant build output.

Two open points:

The GitHub Actions Windows build still fails. I have a suspicion that this is due to -XepExcludedPaths:\Q${project.build.directory}${file.separator}\E.* not working with Windows paths. OTOH, IIRC that used to work before, so not sure.
- Though we should better understand this ^, one suggestion: @timtebeek WDYT of adding an @Generated annotation to the generated recipe classes? Then they'll (also) be covered by -XepDisableWarningsInGeneratedCode and similar logic for Checkstyle.
Build time seems to be increased by ~15 seconds. Not a deal breaker, but it may be nice to understand why. (Solving point (1) may largely also solve this, though.)

Not sure I'll have (much) more time to look into this PR today, so next update may be a while.

github-actions · 2023-12-18T07:34:23Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T07:42:33Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T07:56:28Z

Added a commit to see whether indeed only the Windows build tries to run Error Prone against the generated recipes, and the answer turns out to be "yes", because now the other (previously cancelled) builds pass. This'll require more investigation (likely EOD earliest).

Will revert the test commit.

github-actions · 2023-12-18T07:58:13Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T08:17:01Z

Seems something got stuck. Since there's now also a conflict with the target branch, I'll retrigger by rebasing.

Edit: YAML sync issue, of course 🤦

github-actions · 2023-12-18T08:21:05Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T08:23:15Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T08:29:09Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-12-18T08:43:53Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-12-18T08:49:17Z

Right, so for Windows we have -XepExcludedPaths:\QD:\a\error-prone-support\error-prone-support\refaster-runner\target\\E.*. The double-backslash looks odd, but should work:

jshell> Pattern.compile("\\QD:\\a\\error-prone-support\\error-prone-support\\target\\\\E.*").matcher("D:\\a\\error-prone-support\\error-prone-support\\target\\x").matches()
$1 ==> true

This matches most paths as printed in the logs. However, Error Prone (Javac?) output uses a different path format:

[WARNING] /D:/a/error-prone-support/error-prone-support/refaster-runner/target/generated-test-sources/test-annotations/tech/picnic/errorprone/refaster/runner/FooRulesRecipes.java:[11,1] [RemoveUnusedImports] Unused imports: org.openrewrite.java.template.Primitive

Perhaps we should just match against .*/target/.* then (to be tested), but that does seem unfortunate. Anyway, meetings now :)

github-actions · 2023-12-24T18:19:48Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Merged master once more into this branch; the remaining changes belong to this PR proper.

W.r.t. the increased build time: ~5 seconds is due to the new StringRulesRecipesTest. The remainder was less easy to pinpoint, but at least part of it is of course due to source generation and the subsequent compilation. Anyway, not a big issue.

Suggested commit message:

Derive OpenRewrite recipes from a subset of Refaster rules (#925)

Using OpenRewrite's `rewrite-templating` annotation processor, Refaster rules
are now converted into matching recipes and bundled as part of the 
`error-prone-contrib` artifact. Note that not all rules are supported yet.

We should likely also communicate the new functionality in the README and on the website (possibly with a marker on documented Refaster rules for those that support it), but that's for a separate PR. (Especially the website setup is a bit in limbo right now.)

Stephan202 · 2023-12-24T19:04:53Z

NB: It may be that #515 is merged before this PR. In that case I can take care of the resultant conflict.

Stephan202 · 2023-12-26T18:08:40Z

NB: It may be that #515 is merged before this PR. In that case I can take care of the resultant conflict.

Done!

github-actions · 2023-12-26T18:12:33Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Enjoyed reading all the progress in the rewrite-templating repository. This is really cool stuff and nice to see this milestone 🚀!

rickie · 2023-12-31T14:33:25Z

Forgot to say; the only thing I wasn't yet able to fix is running the new test in my IntelliJ 🤔. Not sure that works for you guys?

timtebeek · 2023-12-31T14:37:28Z

Forgot to say; the only thing I wasn't yet able to fix is running the new test in my IntelliJ 🤔. Not sure that works for you guys?

Did you mark the generated folder as sources in your IDE?

github-actions · 2023-12-31T14:37:43Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2024-01-01T12:22:16Z

Forgot to say; the only thing I wasn't yet able to fix is running the new test in my IntelliJ 🤔. Not sure that works for you guys?

Did you mark the generated folder as sources in your IDE?

I hadn't tried until now (I think), but (IIUC) for me IDEA fails during the build step that precedes the test run. The error I get is:

java: java.lang.LinkageError: Tried to instantiate an instance of the annotation com.google.errorprone.annotations.DoNotCall while processing after, but the annotation class file was not present on the classpath.

IDEA did (automatically) mark the directory as a source folder. I tried to add com.google.errorprone:error_prone_annotations to the same annotationProcessorPaths declaration as rewrite-templating, but that didn't help.

One thing that's interesting, is that OpenRewrite indeed copies over the annotation to the recipe, even though that won't have any functional impact (see e.g. javap -v 'error-prone-contrib/target/classes/tech/picnic/errorprone/refasterrules/TestNGToAssertJRules$Fail.class'). Perhaps the annotation processor should discard such annotations instead, which would also yield smaller recipe class files? 🤔

timtebeek · 2024-01-02T17:47:50Z

Indeed sounds like we ought not to pass that annotation into the recipe; thanks for pointing that out! On the (long) list here to fix, possibly fixed as side effect of

Inline Javatemplate in RefasterTemplateProcessor openrewrite/rewrite-templating#57

Stephan202 · 2024-01-04T09:50:11Z

Top! I think that this isn't a blocker for merging the PR. I synced the branch; will let @rickie merge if he agrees.

github-actions · 2024-01-04T09:52:40Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2024-01-05T20:58:40Z

No blocker, lets merge! Sorry for my late response, I am (still) recovering from being sick..

timtebeek · 2024-01-05T21:09:02Z

Awesome! Look forward expanding this further in the near future and trying out the next release!

Stephan202 · 2024-01-10T18:47:20Z

The IDEA issue described in this comment turns out to be IDEA-342187, unrelated to OpenRewrite; I filed #958.

This was referenced Dec 17, 2023

Generate default constructor with JavaDoc for Xdoclint openrewrite/rewrite-templating#50

Merged

Extract display name and description from JavaDoc openrewrite/rewrite-templating#51

Merged

Stephan202 mentioned this pull request Dec 17, 2023

Skip SonarCloud analysis of PRs from forked repositories #926

Merged

timtebeek marked this pull request as ready for review December 17, 2023 21:39

Stephan202 reviewed Dec 18, 2023

View reviewed changes

Stephan202 force-pushed the add-rewite-templating branch from 08246cf to 93e5cc0 Compare December 18, 2023 07:41

Stephan202 force-pushed the add-rewite-templating branch from ae8e746 to f5a73a2 Compare December 18, 2023 08:19

Stephan202 force-pushed the add-rewite-templating branch from f5a73a2 to 82f357c Compare December 18, 2023 08:27

timtebeek and others added 4 commits December 20, 2023 12:21

rewrite-templating 1.3.13 to suppress all warnings

477d626

rewrite-templating 1.3.14 & rewrite-recipe-bom 2.5.4

6872eb8

Merge branch 'master' into add-rewite-templating

025eb5e

Merge branch 'master' into add-rewite-templating

9d972f4

Stephan202 added this to the 0.15.0 milestone Dec 24, 2023

Stephan202 added the new feature label Dec 24, 2023

Stephan202 approved these changes Dec 24, 2023

View reviewed changes

Stephan202 requested a review from rickie December 25, 2023 10:42

Merge branch 'master' into add-rewite-templating

9c57f7d

rickie approved these changes Dec 31, 2023

View reviewed changes

Merge branch 'master' into add-rewite-templating

36736e6

Merge branch 'master' into add-rewite-templating

ffe6fb5

rickie merged commit a2f44f8 into PicnicSupermarket:master Jan 5, 2024
15 checks passed

timtebeek deleted the add-rewite-templating branch January 5, 2024 21:07

This was referenced Jan 10, 2024

Introduce RefasterMethodParameterOrder check #775

Merged

Work around IntelliJ IDEA bug IDEA-342187 #958

Merged

Add rewrite-templating annotation processor to also produce OpenRewrite recipes #925

Add rewrite-templating annotation processor to also produce OpenRewrite recipes #925

Conversation

timtebeek commented Dec 17, 2023 • edited Loading

Increase in file size

To do here

Future work in rewrite-templating

timtebeek commented Dec 17, 2023 • edited Loading

Stephan202 commented Dec 17, 2023

Stephan202 commented Dec 17, 2023

github-actions bot commented Dec 17, 2023

Stephan202 commented Dec 17, 2023

timtebeek commented Dec 17, 2023

Stephan202 commented Dec 17, 2023

github-actions bot commented Dec 17, 2023

Stephan202 commented Dec 17, 2023

github-actions bot commented Dec 18, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023 • edited Loading

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

Stephan202 commented Dec 18, 2023

github-actions bot commented Dec 24, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 commented Dec 24, 2023

Stephan202 commented Dec 26, 2023

github-actions bot commented Dec 26, 2023

rickie left a comment

Choose a reason for hiding this comment

rickie commented Dec 31, 2023

timtebeek commented Dec 31, 2023

github-actions bot commented Dec 31, 2023

Stephan202 commented Jan 1, 2024

timtebeek commented Jan 2, 2024

Stephan202 commented Jan 4, 2024

github-actions bot commented Jan 4, 2024

rickie commented Jan 5, 2024

timtebeek commented Jan 5, 2024

Stephan202 commented Jan 10, 2024

Add `rewrite-templating` annotation processor to also produce OpenRewrite recipes #925

Add `rewrite-templating` annotation processor to also produce OpenRewrite recipes #925

timtebeek commented Dec 17, 2023 •

edited

Loading

timtebeek commented Dec 17, 2023 •

edited

Loading

Stephan202 commented Dec 18, 2023 •

edited

Loading