Fix ControlFlow crashes on multi-method anon classes and Kotlin booleans#96
Merged
Merged
Conversation
3 tasks
…and Kotlin booleans Three independent issues surfaced as `ControlFlowIllegalStateException`s thrown from the `ControlFlow` analyzer that powers `Dataflow`/`FindLocalFlowPaths`: 1. **"No current node!"** when an anonymous class body contained more than one method and the first one's body ended in a `return`. The default `JavaIsoVisitor` walked the inner method declaration as part of the enclosing flow, so `visitReturn` cleared `current`, and the *next* method's leading `@Override` annotation tripped `currentAsBasicBlock()`. Override `visitMethodDeclaration` to analyze each nested method body as its own sub-flow, the same way `visitLambda` already handles a lambda body. 2. **Reachability stopped at lambda/anonymous-class End nodes.** Once `visitNewClass` started recursing into anonymous class bodies, code that appeared *after* an anonymous class (or a block-body lambda) stopped being marked reachable, because `recurseComputeReachableBasicBlock` returned early on every `End`. Lambda-typed `End` nodes have a real successor in the surrounding flow; method-typed `End` nodes return an empty successor set, so just always traverse the successors. 3. **"Condition Node is not a guard!"** for Kotlin sources because `kotlin.Boolean` is modelled as a `JavaType.FullyQualified`, and `TypeUtils.isAssignableTo(Primitive.Boolean, kotlin.Boolean)` returns false. Recognise `kotlin.Boolean` as boolean-like in `Guard.from`, and consult the method-invocation return type for cases like `someStack.contains(x)`. Reproduced on flagship runs of the "OpenRewrite recipe best practices" recipe against `moderneinc/moderne-intellij-plugin`, `moderneinc/rewrite-sql`, and `openrewrite/rewrite-templating`, all surfaced through the `org.openrewrite.staticanalysis.ReplaceStackWithDeque` recipe.
122ad89 to
0b66e4f
Compare
3 tasks
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
1 participant
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.
Motivation
Three independent issues surfaced as
ControlFlowIllegalStateExceptions thrown from theControlFlowanalyzer that powersDataflow/FindLocalFlowPathsand, transitively, recipes likeorg.openrewrite.staticanalysis.ReplaceStackWithDeque. Reproduced on flagship runs of the "OpenRewrite recipe best practices" recipe (2026-05-05 and 2026-05-06) against:moderneinc/moderne-intellij-plugin(Kotlin) —Condition Node is not a guard!moderneinc/rewrite-sql(Java) —No current node!openrewrite/rewrite-templating(Java) —No current node!Each surfaces inside
ReplaceStackWithDeque.visitVariable→FindLocalFlowPaths.noneMatch→ControlFlow.startingAt(...).findControlFlow().Summary
No current node!when an anonymous class body contained more than one method and the first one's body ended in areturn. The defaultJavaIsoVisitorwalked the inner method declaration as part of the enclosing flow, sovisitReturnclearedcurrent, and the next method's leading@Overrideannotation trippedcurrentAsBasicBlock(). OverridevisitMethodDeclarationinControlFlowAnalysisto analyze each nested method body as its own sub-flow — the same wayvisitLambdaalready handles a lambda body.Endnodes. Once FixThisAccessto actually matchthisexpressions #95 started recursing into anonymous class bodies, code that appeared after an anonymous class (or a block-body lambda) stopped being marked reachable, becauserecurseComputeReachableBasicBlockreturned early on everyEnd.LAMBDA-typedEndnodes do have a successor in the surrounding flow;METHOD-typedEndnodes return an empty successor set anyway, so just always traverse the successors.Condition Node is not a guard!for Kotlin sources becausekotlin.Booleanis modelled as aJavaType.FullyQualifiedandTypeUtils.isAssignableTo(Primitive.Boolean, kotlin.Boolean)returns false. Recognizekotlin.Booleanas boolean-like inGuard.from, and consult the method-invocation return type for cases likesomeStack.contains(x).Test plan
anonymousClassWithMultipleAnnotatedMethodsinFindLocalFlowPathsAnonymousClassTest(reproduces case 1 verbatim, also exercises case 2 by checking that the capturedoreference inside the first method body is tracked).org.openrewrite.staticanalysis.ReplaceStackWithDequeagainst minimized versions of all three failing repos (separate PR inrewrite-static-analysis).rewrite-analysistest suite still passes (./gradlew test).