Skip to content

Added test showing incompatibility between ChangeType and RenameVariable #5540

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Added test showing incompatibility between ChangeType and RenameVariable #5540

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 85 additions & 4 deletions rewrite-java-test/src/test/java/org/openrewrite/java/RenameVariableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
*/
package org.openrewrite.java;

import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.ExecutionContext;
import org.openrewrite.Issue;
import org.openrewrite.Recipe;
import org.openrewrite.*;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.Statement;
import org.openrewrite.test.RewriteTest;
Expand Down Expand Up @@ -1427,4 +1425,87 @@ public void blocks() {
)
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/582")
@Nested
class RenameVariableCombinedWithChangeTypeHasUnexpectedBehavior implements RewriteTest {

@Test
void replacingTheNameOfTheVariableSucceeds() {
rewriteRun(
spec -> spec.recipe(toRecipe(() -> new JavaVisitor<>() {
@Override
public J visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext executionContext) {
doAfterVisit(new RenameVariable<>(multiVariable.getVariables().getFirst(), "arrayDeque"));
return super.visitVariableDeclarations(multiVariable, executionContext);
}
})),
//language=java
java(
"""
import java.util.Stack;

class Test {
void test() {
Stack<Integer> stack = new Stack<>();
stack.add(1);
stack.add(2);
}
}
""",
"""
import java.util.Stack;

class Test {
void test() {
Stack<Integer> arrayDeque = new Stack<>();
arrayDeque.add(1);
arrayDeque.add(2);
}
}
"""
)
);
}

@Test
void replacingTheNameAsAnEffectOfChangeTypeFails() {
rewriteRun(
spec -> spec.recipe(toRecipe(() -> new JavaVisitor<>() {
@Override
public J visit(Tree tree, ExecutionContext ctx) {
J t = super.visit(tree, ctx);
Tree visited = new ChangeType("java.util.Stack", "java.util.ArrayDeque", false)
.getVisitor().visit(t, ctx, getCursor());
return (J) visited;
Comment on lines +1476 to +1480
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's visit(Tree) method, this test case causes the ChangeType recipe to be called several times with various scopes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oeps... yeah.

I've changed it to test a few specific scopes, and you clearly see the issue.
On one hand it's good to be able to rename variables in an isolated scope, but I think the rename variables recipe should broaden it's scope to make all the necessary "transitive" changes outside the provided scope.

Right now it just operates in the provided scope.

}
})),
//language=java
java(
"""
import java.util.Stack;

class Test {
void test() {
Stack<Integer> stack = new Stack<>();
stack.add(1);
stack.add(2);
}
}
""",
"""
import java.util.ArrayDeque;

class Test {
void test() {
ArrayDeque<Integer> arrayDeque = new ArrayDeque<>();
arrayDeque.add(1);
arrayDeque.add(2);
}
}
"""
)
);
}
}
}
Loading