Only run data flow analysis on Stack variables in ReplaceStackWithDeque - #1030
Merged
Merged
Conversation
…Deque` `visitVariable` ran `FindLocalFlowPaths` for every initialized variable in any file that mentions `java.util.Stack`, then applied `ChangeType` to the initializer -- a no-op for variables that are not a `Stack`. Applying `ChangeType` first and bailing out when it leaves the initializer untouched is equivalent, and confines the analysis to the variables the recipe can actually rewrite. That also avoids openrewrite/rewrite-analysis#113, which is how this surfaced: `ForwardFlow.computeVariableAssignment` recurses between its "argument to select" and "select to argument" steps without a visited set, so an expression like `Optional.ofNullable(a).orElse(opt.get())` -- where both the select and an argument are themselves calls, and the outer method is in both the `Argument[k] -> ReturnValue` and `Argument[this] -> ReturnValue` model groups -- overflows the stack. The `Stack` in the regression test only satisfies the recipe's `UsesType` precondition; the overflow came from an unrelated variable, as it did on `spring-data-mongodb`'s `MongoExampleMapper`. The engine-level cycle remains, so a `Stack` whose flow reaches such an expression can still overflow; that needs the upstream fix.
Contributor
|
Why is this a problem specifically for stacks? This seems like a more general problem, or like we are missing more general recursion detection that would break a recursive check |
Contributor
|
This doesn't seem like the right way of solving this problem. This seems like a more general recursion problem where we aren't checking the graph being built correctly. Can we reconsider how we're fixing this and build something more resilient? |
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.
What's changed
ReplaceStackWithDeque.visitVariableranFindLocalFlowPathsfor every initialized variable in any file that mentionsjava.util.Stack, and only then appliedChangeTypeto the initializer — a no-op for variables that aren't aStack. Swapping the order (applyChangeTypefirst, bail out when it leaves the initializer untouched) is equivalent and confines the analysis to the variables the recipe can actually rewrite.Why
This is how the crash surfaced.
org.openrewrite.staticanalysis.ReplaceStackWithDequehas been failing withjava.lang.StackOverflowErroronspring-projects/spring-data-mongodb, filespring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/MongoExampleMapper.java, recurring since at least 2026-08-17 in the "Java best practices" and "Common static analysis issues" flagship runs ondev(ALL,Open Source,Netflix + Spring + Apache,Netflix + Spring). Confirmed in run20260825041417-SSuzc; absent on 2026-08-26, so it is input/attribution dependent and a clean run isn't evidence of a fix.The
SourcesFileErrorsdata table for that run gives ~1000 frames alternating between exactly two call sites:ForwardFlow.computeVariableAssignmentrecurses from an argument into the call's select (:466) and from the select back into an argument (:495) with no visited set, so whenisFlowStep(..)holds in both directions it never terminates. Root cause filed as StackOverflowError: unbounded mutual recursion between the argument→select and select→argument steps in ForwardFlow.computeVariableAssignment rewrite-analysis#113 with the analysis of which model groups produce the two directions.The added test reproduces the production stack trace exactly (502 frames at
:466, 501 at:495) onmain, and passes here. TheStackin it only satisfies the recipe'sUsesTypeprecondition — the overflow comes from the unrelatedString a, which is also what happens inMongoExampleMapper.Scope
This narrows the blast radius; it is not the engine fix. A
Stackwhose flow reaches such an expression can still overflow, and any other recipe usingFindLocalFlowPathsremains exposed until StackOverflowError: unbounded mutual recursion between the argument→select and select→argument steps in ForwardFlow.computeVariableAssignment rewrite-analysis#113 is fixed. A caller can't work around it themselves —DataFlowSpec.isFlowStepisfinal, so a spec cannot suppress the external models that create the cycle.ReplaceStackWithDeque sometimes fails with an exception #87 names this recipe but is a different defect (
ControlFlowIllegalStateExceptionout ofControlFlow.createFakeIteratorVariableDeclarations, notForwardFlow), so it is not the tracking issue for this and is not closed by this PR.