Add OpenRewrite code cleanup with an incremental in-house runner#6217
Open
vlsi wants to merge 4 commits into
Open
Add OpenRewrite code cleanup with an incremental in-house runner#6217vlsi wants to merge 4 commits into
vlsi wants to merge 4 commits into
Conversation
a75a074 to
d21c15b
Compare
Collaborator
Author
|
It looks like OpenRewrite does not support mixed Java-Kotlin projects, so it can't parse Java files that call Kotlin classes: |
e4cafe6 to
da50aaa
Compare
3bf9740 to
1366ed4
Compare
6 tasks
Rebuild the OpenRewrite integration from PR apache#6217 on top of current master. The 2024 attempt hit a hard blocker: with rewrite-gradle-plugin 6.6.3 the Java parser could not resolve Kotlin classes declared in the same module (e.g. JFactory.java -> JEditableCheckBox.kt failed with "cannot find symbol"). Plugin 7.35.0 on Gradle 9.2.1 no longer has that problem. The Java parser sees the module's compiled Kotlin output through compileClasspath, so mixed Java/Kotlin modules (jorphan, components, core) parse with full type information. FindMissingTypes reports zero missing types on the Java side. The old approach is reduced to a single convention plugin plus rewrite.yml: - build-logic.openrewrite applies org.openrewrite.rewrite and the static-analysis recipes, which run cleanly on both Java and Kotlin; - build-logic.style applies it to every non-root project (the plugin infects allprojects when applied to the root) and wires rewriteRun/rewriteDryRun into the style/styleCheck tasks; - a skipOpenrewrite build parameter gates the whole thing. Two residual issues, both narrower than the original blocker: - rewrite-kotlin still leaves some Kotlin idioms without type attribution (scope-function apply {} constructors, labelled return@); non-fatal. - the rewrite-testing-frameworks JUnit 5 recipes are Java-only and throw on Kotlin test sources, so they are gated to modules without src/test/kotlin. skipOpenrewrite defaults to true because the OpenRewrite plugin and recipe artifacts are not yet in gradle/verification-metadata.xml. Until they are, run with -PskipOpenrewrite=false --dependency-verification=lenient. Refs: apache#6217 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…runner The stock org.openrewrite.rewrite plugin re-parses a module on every rewriteDryRun (~19s for jorphan even with no changes) and, when applied to the root project, analyses the whole repository at once. Port the custom runner started in PR apache#6217 to current master and finish it: - build-logic/openrewrite runs each module's recipes in an isolated worker and declares the sources, compile classpath and config as typed task inputs, so rewriteDryRun is up-to-date (~2s) when nothing changed. - rewriteRun now writes the results: it creates, deletes, moves and updates files in place. The WIP only logged them. - rewriteDryRun writes a unified diff to build/reports/openrewrite and fails only when failOnDryRunResults is set. The worker loads recipes from META-INF/rewrite/*.yml on the recipe classpath instead of Environment.scanClassLoader, avoiding the ClassGraph scan that OOMs on large classpaths (openrewrite/rewrite#3899). Kotlin processing is off by default: rewrite-kotlin does not round-trip Kotlin (it prints `x as Foo` as `x asFoo`), so applying recipes corrupts untouched code, and the stock plugin has the same bug. The Java parser still resolves types declared in Kotlin because the module's compiled Kotlin classes are on its compile classpath, so Java cleanup stays correct and compiles. Refs: apache#6217 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two follow-ups to the in-house OpenRewrite runner.
disabledRecipes lets a single recipe be switched off by name, e.g.
openrewrite {
disabledRecipes.add("org.openrewrite.java.testing.junit5.AssertThrowsOnLastStatement")
}
The compiled composites (JUnit5BestPractices and friends) expose an immutable
recipeList, so the runner wraps the recipe tree in a FilteringRecipe that hides
the disabled names instead of mutating the list. This removes the need to fork a
recipe module or drop a whole composite; the JUnit 5 recipes are now enabled for
every test module and only AssertThrowsOnLastStatement is switched off, pending
openrewrite/rewrite-testing-frameworks#1048
rewriteDiagnose runs each active leaf recipe on its own and reports whether it
fails, would change files, or does nothing, so the problematic recipes can be
found without reading a full run. On jorphan: 183 leaf recipes, 0 fail in Java
mode, 21 would make changes. Note that a recipe which only fails as part of an
ordered composite (AssertThrowsOnLastStatement on Kotlin) passes in isolation, so
the per-leaf report is a lower bound; the composite run surfaces the rest as
non-fatal "Error while rewriting" warnings thanks to the lenient ExecutionContext.
-PopenrewriteKotlin=true flips Kotlin processing on from the command line for
experimenting with rewrite-kotlin.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add an additive -PopenrewriteDisable=<comma,separated> switch so recipes can be turned off from the command line without editing the build, and record which Java recipes corrupt Kotlin when -PopenrewriteKotlin=true is used. Applying the active recipes to Kotlin and compiling the result shows exactly three Java recipes that produce non-compiling Kotlin: ShortenFullyQualified- TypeReferences (drops a qualifier without the import), TypecastParenPad (`x as Foo` -> `x asFoo`, root cause in openrewrite/rewrite#8236) and OperatorWrap (leading `+` becomes a unary plus). All three are safe on Java, so they stay active; Kotlin stays off by default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add OpenRewrite-based code cleanup (static analysis + formatting), driven by an in-house runner in
build-logic/openrewriterather than the stockorg.openrewrite.rewriteGradle plugin. Recipes are declared inconfig/openrewrite/rewrite.yml;rewriteRunapplies them,rewriteDryRunreports a patch,rewriteDiagnoseruns each recipe on its own to find failures.Kotlin processing is off by default, so this is Java-only cleanup unless
-PopenrewriteKotlin=trueis passed.Why an in-house runner instead of the stock plugin
The stock plugin was tried first (see the git history of this branch) and rebuilt as a custom runner for three reasons:
rewriteDryRunre-parses the whole module on every invocation (~19s forjorphaneven with no changes). The custom task declares the sources, compile classpath and config as typed inputs, so a no-change run isUP-TO-DATEin ~2s.Environment.scanClassLoaderuses ClassGraph, which OOMs on large classpaths (Replace ClassGraph classpath scanning with ServiceLoader or something else openrewrite/rewrite#3899). The runner instead loads declarative recipes fromMETA-INF/rewrite/*.ymlon the recipe classpath and lets the referenced compiled recipes instantiate by name, and runs the work in an isolated worker.Mixed Java/Kotlin
The 2024 blocker — the Java parser could not resolve Kotlin classes declared in the same module (
JFactory.java→JEditableCheckBox.ktfailed withcannot find symbol) — is gone with rewrite-gradle-plugin 7.35.0 / rewrite 8.86: the module's compiled Kotlin output is oncompileClasspath, so Java sources parse with full type information.Applying recipes to Kotlin is a separate story. Three problems were found and reported upstream; two are already fixed:
AssertThrowsOnLastStatementthrows on Kotlin test sources — fixed in Support Kotlin sources inAssertThrowsOnLastStatementopenrewrite/rewrite-testing-frameworks#1048TypecastParenPadrewritesx as Foointox asFoo— root cause inSpacesVisitor, fixed in Do not apply Java typecast spacing to Kotlinascasts openrewrite/rewrite#8236ShortenFullyQualifiedTypeReferencesdrops a qualifier without adding the import, andOperatorWrapmoves a binary+to the start of the next line where Kotlin reads it as a unary plus — still openBecause of these, Kotlin stays off by default. With those recipes disabled the applied Kotlin compiles across the whole repository; individual recipes can be turned off by name with
openrewrite.disabledRecipesor-PopenrewriteDisable=<a,b,c>.How to run
skipOpenrewritedefaults totrueuntil the OpenRewrite recipe artifacts are added togradle/verification-metadata.xml; until then the tasks also need--dependency-verification=lenient.