From 7d57f5244cc922a9c94a2734a4bd4fe7bd81c227 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 26 Sep 2025 19:16:37 +0200 Subject: [PATCH 1/2] Fix RemoveUnusedLocalVariables to preserve assignments with side effects in try blocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The recipe was incorrectly removing variable assignments in try blocks even when those assignments could throw exceptions and change program behavior. This fix ensures that method invocations within try blocks are preserved since they may throw exceptions that affect control flow. Fixes #740 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../RemoveUnusedLocalVariables.java | 83 ++++++++++++++++++- .../RemoveUnusedLocalVariablesTest.java | 23 +++++ 2 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java b/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java index 91ad8ea1b8..c63345ce62 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java @@ -162,6 +162,12 @@ private Cursor getCursorToParentScope(Cursor cursor) { List readReferences = VariableReferences.findRhsReferences(parentScope.getValue(), variable.getName()); if (readReferences.isEmpty()) { List 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)); @@ -203,6 +209,81 @@ private boolean initializerMightSideEffect(J.VariableDeclarations.NamedVariable if (variable.getInitializer() == null) { return false; } + return expressionMightSideEffect(variable.getInitializer()); + } + + private boolean assignmentsMightSideEffect(List 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 + return new JavaIsoVisitor() { + @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() { + @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((J) getCursorToParentScope(getCursor()).getValue(), 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() { + @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)) { + 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) { AtomicBoolean mightSideEffect = new AtomicBoolean(false); new JavaIsoVisitor() { @Override @@ -227,7 +308,7 @@ public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean resul result.set(true); return assignment; } - }.visit(variable.getInitializer(), mightSideEffect); + }.visit(expression, mightSideEffect); return mightSideEffect.get(); } }; diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariablesTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariablesTest.java index a57c0b6da6..846e454ba4 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariablesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariablesTest.java @@ -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 { From 8e551842e705f1723bdbb162cfaa63d4fafc5666 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Fri, 26 Sep 2025 19:25:01 +0200 Subject: [PATCH 2/2] Simplify --- .../staticanalysis/RemoveUnusedLocalVariables.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java b/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java index c63345ce62..c4977160fd 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveUnusedLocalVariables.java @@ -234,6 +234,7 @@ private boolean assignmentsMightSideEffect(List assignmentReferences) private boolean isAssignmentInTryBlock(Statement statement) { // Find if this statement is contained within a try block + J parent = getCursorToParentScope(getCursor()).getValue(); return new JavaIsoVisitor() { @Override public J.Try.Resource visitTryResource(J.Try.Resource tryResource, AtomicBoolean result) { @@ -256,7 +257,7 @@ public Statement visitStatement(Statement s, AtomicBoolean found) { }.visit(tryStatement.getBody(), result); return tryStatement; } - }.reduce((J) getCursorToParentScope(getCursor()).getValue(), new AtomicBoolean(false)).get(); + }.reduce(parent, new AtomicBoolean(false)).get(); } private boolean expressionMightThrowException(Expression expression) { @@ -268,7 +269,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation methodInvocat if (SAFE_GETTER_METHODS.matches(methodInvocation)) { return methodInvocation; } - if (withSideEffects == null || Boolean.FALSE.equals(withSideEffects)) { + if (withSideEffects == null || !withSideEffects) { result.set(true); } return methodInvocation; @@ -284,14 +285,13 @@ public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean resul } private boolean expressionMightSideEffect(Expression expression) { - AtomicBoolean mightSideEffect = new AtomicBoolean(false); - new JavaIsoVisitor() { + return new JavaIsoVisitor() { @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; @@ -308,8 +308,7 @@ public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean resul result.set(true); return assignment; } - }.visit(expression, mightSideEffect); - return mightSideEffect.get(); + }.reduce(expression, new AtomicBoolean(false)).get(); } }; }