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.

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

Conversation

JohannisK
Copy link
Contributor

What's changed?

Added test showing incompatibility between ChangeType and RenameVariable

What's your motivation?

When using ChangeType, change type tries to rename the variable through RenameVariable. Normally RenameVariable renames all relevant variables, but when invoked through ChangeType it seems to only rename the variable where it is declared.

@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite Jun 2, 2025
@JohannisK JohannisK added the bug Something isn't working label Jun 2, 2025
@timtebeek timtebeek marked this pull request as draft June 2, 2025 11:11
Comment on lines +1476 to +1480
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;
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Tests for ReplaceStackWithDeque fail as it produces invalid code
2 participants