-
Notifications
You must be signed in to change notification settings - Fork 359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StackOverflowError
on @java.lang.annotation.Documented
due to self reference
#4161
Comments
I can't see my uploaded zip containing a small test case for reproduction, but I hope you have received it. |
Oh wow, thanks for not including the full stacktrace in the case of a stack overflow. 😅 Weird to see that self referencing documented annotation causing issues here; definitely something we should fix in our parser. /cc @knutwannheden I suspect we might even be able to trim down the reproduction test even further, just looking at how that's defined. Thanks a lot for the level of detail in your report; much appreciated that we immediately have all the details visible here. |
StackOverflowError
on @java.lang.annotation.Documented
due to self reference
Hmm; I've tried to replicate this with a smaller unit test added to JavaParserTest, but no luck so far with @Test
@Issue("https://github.com/openrewrite/rewrite/issues/4161")
void documentedSelfReference() {
rewriteRun(
java(
"""
import javax.swing.JFrame;
class A extends JFrame {
}
"""
)
);
} The reason I'm trying to replicate with a smaller sample is because I don't have access to the |
Sorry for not responding right away. I was on vacation for a few days. Yes, I am able to reproduce without external dependencies. For your convenience I have added an updated test case: OpenRewriteProblem-2.zip The reason why your test is not reproducing the problem is probably because you are not trying to apply the template Templates.implementCloneableTemplate included in the test case. The problem happens during template application.
|
Looking over the zip I'm wondering if it'd be possible to replicate this with a self contained unit test; now it still refers to an external recipe, two visitors and a utility class. Ideally it'd be a a single unit test using |
I can replicate this as well with a nonsense example (excluding any preconditions and just mutating the class declaration directly): package com.sample.test;
import org.junit.jupiter.api.Test;
import org.openrewrite.ExecutionContext;
import org.openrewrite.java.JavaIsoVisitor;
import org.openrewrite.java.JavaParser;
import org.openrewrite.java.JavaTemplate;
import org.openrewrite.java.tree.J;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import static org.openrewrite.java.Assertions.java;
public class StackOverFlowErrorOnAnnotation implements RewriteTest {
@Override
public void defaults(RecipeSpec spec) {
spec.recipe(RewriteTest.toRecipe(() -> {
return new JavaIsoVisitor<>() {
@Override
public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext executionContext) {
J.ClassDeclaration classDeclaration = super.visitClassDeclaration(classDecl, executionContext);
classDeclaration = JavaTemplate
.builder("Object")
.imports("java.lang.Object")
.javaParser(JavaParser.fromJavaVersion())
.build()
.apply(getCursor(), classDeclaration.getCoordinates().addImplementsClause());
return classDeclaration;
}
};
}))
.parser(JavaParser.fromJavaVersion()
.classpath("junit-jupiter-api"));
}
@Test
void stackOverflowOnImplements() {
rewriteRun(
// language=java
java(
"""
package com.sample.test;
import org.junit.jupiter.api.DisplayName;
@DisplayName("GIVEN a test with a '@DisplayName' annotation")
abstract class MyClassWithDisplayNameAnnotation {}
""",
"""
package com.sample.test;
import org.junit.jupiter.api.DisplayName;
@DisplayName("GIVEN a test with a '@DisplayName' annotation")
abstract class MyClassWithDisplayNameAnnotation implements Object {}
"""
)
);
}
} The DisplayName annotation is also annotated with the self referencing Documented annotation. |
Thanks a lot @darthblonderss ; great to have a self-contained example. Did you already briefly explore a cause or fix? |
The use of `JavaTypeVisitor` in `JavaTemplateJavaExtension` is what caused the `StackOverflowError`, as the `JavaType` graphs can contain cycles and don't provide any mechanism to guard against that. While I understand the reason for using a `JavaTypeVisitor` to also correct the references to that type in the graph, it won't fix all references in any case, so it seems a bit futile. Until OpenRewrite provides a mechanism to "globally" update a type attribution, I feel like the best option here is to do what other recipes typically do: Just update the reference at hand. The other alternative here would be to use the internal method `unsafeSet()`. - Fixes: #4161
What version of OpenRewrite are you using?
I am using
rewrite-java 8.23.1 through importing 2.9.0 rewrite-recipe-bom
How are you running OpenRewrite?
In production we plan to use the Maven plugin, and projects there are usually multi module projects,
however I can reproduce the error using a junit test in a single module setup
What is the smallest, simplest way to reproduce the problem?
The following produces a stackoverflow when used with MyRecipe. For details about MyRecipe see the attached maven project, which contains a downsized version of our original recipe.
What did you expect to see?
I did expect to not encounter a StackOverflowError caused by the self referencing @documented annotation on JavaBean annotation on JFrame
The self reference from Documented to Documented causes the StackOverflowError
What did you see instead?
I see a StackOverflowError when replacing interface implementations
What is the full stack trace of any errors you encountered?
too long, but includes repetitions of the following till StackOverflow is encountered
Are you interested in contributing a fix to OpenRewrite?
Would have to ask my employer.
The text was updated successfully, but these errors were encountered: