Fix StackOverflowError in JavaTypeVisitor for circular type references#6492
Merged
sambsnyd merged 7 commits intoopenrewrite:mainfrom Jan 15, 2026
Merged
Conversation
This test demonstrates a StackOverflowError that occurs when JavaTypeVisitor
encounters circular type references in parameterized types, particularly
common in builder patterns like Builder<T extends Builder<T>>.
The test creates a circular type structure programmatically and attempts to
visit it with JavaTypeVisitor. Currently, this causes infinite recursion:
JavaTypeVisitor.visitParameterized()
-> visit()
-> ListUtils.map()
-> visit()
-> visitParameterized()
-> (infinite loop until StackOverflowError)
This issue affects real-world code, including:
- Builder patterns with recursive generic bounds
- OpenSearch Java client code
- Any fluent APIs using self-referential generics
The test currently fails with StackOverflowError. Once the fix is applied
(cycle detection in JavaTypeVisitor.visit()), this test should pass.
Test includes:
1. testCircularTypeReferencesCauseStackOverflow - Direct reproduction
2. testRealWorldBuilderPatternCausesStackOverflow - Integration test
Issue: Circular type references cause StackOverflowError
mcebanupgrade
force-pushed
the
fix/stackoverflow-circular-types
branch
from
dd71946 to
1c9c32b
Compare
Add visited type tracking to JavaTypeVisitor.visit() to prevent infinite recursion when encountering circular type references in parameterized types. The fix uses an IdentityHashMap-backed Set to track all types that have been visited. Unlike the previous approach which only tracked types on the current call stack, this approach: 1. Prevents infinite recursion (cycles) 2. Prevents exponential explosion from revisiting the same types via different paths in the type graph When a type is encountered that has already been visited, it is returned unchanged, breaking both cycles and redundant visits. Key changes: - Added 'visited' field to track all visited types - Check if type was visited before processing - Also track transformed types from preVisit() - Uses identity comparison (same object instance) not equals() This handles common patterns that cause StackOverflowError: - Builder patterns: Builder<T extends Builder<T>> - OpenSearch/Elasticsearch Java client code - Any fluent APIs with recursive generic bounds Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mcebanupgrade
changed the title
Member
|
Thanks for logging the issue and immediately proposing a fix! I can see JavaTypeVisitor is used in:
As such we're a little sensitive to performance changes with the new set introduced and evaluated for every type visited: @knutwannheden do you see any concerns or alternatives to be able to support visiting circular type references? |
sambsnyd
reviewed
Jan 7, 2026
Member
There was a problem hiding this comment.
We didn't build this visited set into the base class at the beginning on purpose. In case there were scenarios where it wouldn't be desirable or a different criteria might be used to determine when to keep visiting and when to stop.
I surveyed all the uses of JavaTypeVisitor in the OpenRewrite and Moderne codebases and most use this pattern. But some have variations on it such that I'd be a bit worried about regression.
So I recommend making this a subclass of JavaTypeVisitor to preserve the optionality of the cycle-avoidance criteria. Perhaps named StandardJavaTypeVisitor or UniqueJavaTypeVisitor
timtebeek
added a commit
that referenced
this pull request
Jan 7, 2026
As discovered when exploring #6492 usages of JavaTypeVisitor
Member
|
Marked as draft based on the above; perhaps it's also good to know what code path led you into JavaTypeVisitor, as there's a couple different options but none that stand out as taken by default. Is there a recipe you're executing that acts on these types? That way we can better judge if and where a subclass visitor would need to be applied. |
timtebeek
added a commit
that referenced
this pull request
Jan 7, 2026
As discovered when exploring #6492 usages of JavaTypeVisitor
Contributor
Author
|
Here's what I managed to extract from the stacktrace. |
Member
|
That helps, thanks! Here's a link to the relevant recipe: This could likely have been prevented had they used a precondition to limit which types get evaluated at all: Typically there would be some pattern like @Override
public TreeVisitor<?, ExecutionContext> getVisitor() {
return Preconditions.check(new UsesType<>(V1_S3_MODEL_PKG + "S3Object"), new Visitor());
}That way recipes get a nice performance boost, as they don't have to traverse files that have no relation whatsoever to the recipe, like in your case of a ElasticSearch builder being used presumably not immediately along side any S3Object usage. |
Member
|
I've gone ahead and reverted the unwanted changes, and based on your intended usage adapted the JavaTypeVisitor used in And since it sounds like you were already testing with a snapshot version, can you confirm this fixes the issue for you too? |
Contributor
Author
|
hi @timtebeek ! |
Member
|
Thanks for validating! @sambsnyd are you ok with the changes here to only change the usage in |
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
3 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
Fixes StackOverflowError when
JavaTypeVisitorencounters circular type references in parameterized types, such as builder patterns likeBuilder<T extends Builder<T>>.Problem
When visiting types with circular references,
JavaTypeVisitorwould infinitely recurse through:visitClass→getMethods()→visitMethod→getDeclaringType()→ back tovisitClassThis commonly occurs with:
interface Builder<T extends Builder<T>>Solution
Added visited type tracking using an
IdentityHashMap-backed Set inJavaTypeVisitor.visit(). When a type instance is encountered that has already been visited, it is returned unchanged.Key changes:
visitedfield to track all visited type instancespreVisit()This prevents both:
Test Plan
JavaTypeVisitorStackOverflowTestwith real-world builder pattern code*JavaTypeVisitor*tests pass*ChangeType*tests pass*UnsafeReplaceJavaType*tests pass🤖 Generated with Claude Code