Skip incompatible registered mixins in TreeVisitorAdapter#7718
Closed
timtebeek wants to merge 1 commit into
Closed
Skip incompatible registered mixins in TreeVisitorAdapter#7718timtebeek wants to merge 1 commit into
timtebeek wants to merge 1 commit into
Conversation
A base visitor like `RemoveAnnotationVisitor` can have mixins registered from multiple language modules — Kotlin's mixin extends `KotlinIsoVisitor`, a Groovy mixin would extend `GroovyVisitor`, etc. `discoverRegisteredMixin` returned the first registered entry unconditionally; if its language didn't match `adaptTo`, `adapt` then threw `Mixin ... is not assignable to ...`. Concretely: adapting `RemoveAnnotationVisitor` to `GroovyVisitor` (triggered by any Spring Boot upgrade recipe applied to a Gradle Groovy build script) crashed because `RemoveAnnotationKotlinMixin` was picked up. Filter mixin entries by `adaptTo.isAssignableFrom(mixinClass)` so the discovery loop can pass over Kotlin-only / Java-only / etc. mixins and find the one that matches the current target — or return `null` so dispatch falls through to the base visitor.
Contributor
|
Looks like we've both been working on the mixin downstream. I'll leave it up to you to determine which is the best solution |
Member
Author
|
Thanks! Yours is more succinct; let's go for that one. |
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
2 participants
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.
Summary
Filter registered mixins by language compatibility in
TreeVisitorAdapter.discoverRegisteredMixinso that adapting a base visitor to one language'sadaptTodoesn't pick up another language's mixin and crash.Background
RemoveAnnotationKotlinMixinis registered forRemoveAnnotationVisitorviaMETA-INF/rewrite/mixins/org.openrewrite.java.RemoveAnnotationVisitor. WhenRemoveAnnotationVisitorruns against a Groovy file,G.acceptadapts it toGroovyVisitor— butdiscoverRegisteredMixinreturned the Kotlin mixin unconditionally, and the assignability check at the top ofadaptthen threw:This affects any recipe that uses
RemoveAnnotation(or any other base visitor with a language-specific mixin registered) when it touches a Groovy / non-Kotlin source file. Surfaced viaSpringBootVersionUpgradeTest.upgradeSpringCloudAwsVersioninrewrite-spring, which exercises this on a Gradle Groovy build script.Fix
In
discoverRegisteredMixin, skip entries whose class isn't a subtype ofadaptTorather than returning them. A registry file may list multiple language-specific mixins for the same base — the discovery loop now passes over incompatible ones and finds the matching one (or returnsnullso dispatch falls through to the base).Test plan
crossLanguageRegisteredMixinSkippedinTreeVisitorAdapterTest— fails without the fix with the exactIllegalArgumentExceptionabove, passes with it.TreeVisitorAdapterTestcases still pass.SpringBootVersionUpgradeTest.upgradeSpringCloudAwsVersioninrewrite-spring— now green.