Skip to content

Fix RemoveUnusedLocalVariables to preserve assignments with side effects in try blocks #741

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 2 commits into
base: main
Choose a base branch
from
Draft

Fix RemoveUnusedLocalVariables to preserve assignments with side effects in try blocks #741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d57f52
Fix RemoveUnusedLocalVariables to preserve assignments with side effe…
timtebeek Sep 26, 2025
8e55184
Simplify
timtebeek Sep 26, 2025
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
90 changes: 85 additions & 5 deletions src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ private Cursor getCursorToParentScope(Cursor cursor) {
List<J> readReferences = VariableReferences.findRhsReferences(parentScope.getValue(), variable.getName());
if (readReferences.isEmpty()) {
List<Statement> assignmentReferences = VariableReferences.findLhsReferences(parentScope.getValue(), variable.getName());

// Check if any assignment might have side effects
if (assignmentsMightSideEffect(assignmentReferences)) {
return variable;
}

for (Statement ref : assignmentReferences) {
if (ref instanceof J.Assignment) {
doAfterVisit(new PruneAssignmentExpression((J.Assignment) ref));
Expand Down Expand Up @@ -203,14 +209,89 @@ private boolean initializerMightSideEffect(J.VariableDeclarations.NamedVariable
if (variable.getInitializer() == null) {
return false;
}
AtomicBoolean mightSideEffect = new AtomicBoolean(false);
new JavaIsoVisitor<AtomicBoolean>() {
return expressionMightSideEffect(variable.getInitializer());
}

private boolean assignmentsMightSideEffect(List<Statement> assignmentReferences) {
for (Statement ref : assignmentReferences) {
// Check if the assignment is within a try block and might throw an exception
if (isAssignmentInTryBlock(ref)) {
if (ref instanceof J.Assignment) {
J.Assignment assignment = (J.Assignment) ref;
if (expressionMightThrowException(assignment.getAssignment())) {
return true;
}
} else if (ref instanceof J.AssignmentOperation) {
J.AssignmentOperation assignOp = (J.AssignmentOperation) ref;
if (expressionMightThrowException(assignOp.getAssignment())) {
return true;
}
}
}
}
return false;
}

private boolean isAssignmentInTryBlock(Statement statement) {
// Find if this statement is contained within a try block
J parent = getCursorToParentScope(getCursor()).getValue();
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.Try.Resource visitTryResource(J.Try.Resource tryResource, AtomicBoolean result) {
// Skip try-with-resources
return tryResource;
}

@Override
public J.Try visitTry(J.Try tryStatement, AtomicBoolean result) {
// Check if the statement is in the try body
new JavaIsoVisitor<AtomicBoolean>() {
@Override
public Statement visitStatement(Statement s, AtomicBoolean found) {
if (s == statement) {
found.set(true);
return s;
}
return super.visitStatement(s, found);
}
}.visit(tryStatement.getBody(), result);
return tryStatement;
}
}.reduce(parent, new AtomicBoolean(false)).get();
}

private boolean expressionMightThrowException(Expression expression) {
// Only check for method invocations that might throw exceptions
// Constructors like "new Object()" typically don't throw checked exceptions
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation methodInvocation, AtomicBoolean result) {
if (SAFE_GETTER_METHODS.matches(methodInvocation)) {
return methodInvocation;
}
if (withSideEffects == null || !withSideEffects) {
result.set(true);
}
return methodInvocation;
}

@Override
public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean result) {
// Nested assignments might throw
result.set(true);
return assignment;
}
}.reduce(expression, new AtomicBoolean(false)).get();
}

private boolean expressionMightSideEffect(Expression expression) {
return new JavaIsoVisitor<AtomicBoolean>() {
@Override
public J.MethodInvocation visitMethodInvocation(J.MethodInvocation methodInvocation, AtomicBoolean result) {
if (SAFE_GETTER_METHODS.matches(methodInvocation)) {
return methodInvocation;
}
if (withSideEffects == null || Boolean.FALSE.equals(withSideEffects)) {
if (withSideEffects == null || !withSideEffects) {
result.set(true);
}
return methodInvocation;
Expand All @@ -227,8 +308,7 @@ public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean resul
result.set(true);
return assignment;
}
}.visit(variable.getInitializer(), mightSideEffect);
return mightSideEffect.get();
}.reduce(expression, new AtomicBoolean(false)).get();
}
};
}
Expand Down
23 changes: 23 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariablesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1214,6 +1214,29 @@ void method() {
);
}

@Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/740")
@Test
void doNotRemoveVariableAssignmentWithPotentialSideEffects() {
rewriteRun(
//language=java
java(
"""
class Test {
private String baz() {
String foo;
try {
foo = String.valueOf(1);
} catch (RuntimeException e) {
return "error";
}
return "ok";
}
}
"""
)
);
}

@Nested
class Kotlin {

Expand Down