From b0df5373404702cbaf53613fd66c2923e2694b6e Mon Sep 17 00:00:00 2001 From: Deepak Date: Thu, 19 Jun 2025 20:19:02 +0530 Subject: [PATCH 01/16] Try with resources --- .../staticanalysis/TryWithResources.java | 480 ++++++++++++++++++ .../staticanalysis/TryWithResourcesTest.java | 291 +++++++++++ 2 files changed, 771 insertions(+) create mode 100644 src/main/java/org/openrewrite/staticanalysis/TryWithResources.java create mode 100644 src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java new file mode 100644 index 0000000000..c5a17be5ad --- /dev/null +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -0,0 +1,480 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.ListUtils; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.*; +import org.openrewrite.marker.Markers; + +import java.util.*; + +/** + * Transforms code using manual resource management with finally blocks to use the Java 7+ try-with-resources pattern. + * This transformation improves code safety and readability by ensuring resources are properly closed. + */ +public class TryWithResources extends Recipe { + + private static final JavaType.ShallowClass AUTO_CLOSEABLE_TYPE = JavaType.ShallowClass.build("java.lang.AutoCloseable"); + + @Override + public String getDisplayName() { + return "Use try-with-resources"; + } + + @Override + public String getDescription() { + return "Converts code using manual resource management with finally blocks to use the Java 7+ try-with-resources pattern. " + + "This transformation improves code safety and readability by ensuring resources are properly closed."; + } + + @Override + public TreeVisitor getVisitor() { + return new JavaIsoVisitor() { + @Override + public J.Block visitBlock(J.Block block, ExecutionContext ctx) { + J.Block b = super.visitBlock(block, ctx); + // Process the method body to find try blocks and transform them + return maybeAutoFormat(block, processBlock(b), ctx); + } + + @Override + public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { + // First, visit the try block normally to process any nested try blocks + J.Try t = super.visitTry(tryable, ctx); + + // Only process try blocks with a finally block + if (t.getFinally() == null) { + return t; + } + + // Find variable declarations in the try block + List variableDeclarations = new ArrayList<>(); + for (Statement statement : t.getBody().getStatements()) { + if (statement instanceof J.VariableDeclarations) { + variableDeclarations.add((J.VariableDeclarations) statement); + } + } + + if (variableDeclarations.isEmpty()) { + return t; + } + + // Find resources that are closed in the finally block + Map resourcesThatAreClosed = findResourcesThatAreClosedInFinally(variableDeclarations, t.getFinally()); + + if (resourcesThatAreClosed.isEmpty()) { + return t; + } + + // Check for resources initialized to null and assigned in the try block + Map resourceInitializers = findResourceInitializers(t, resourcesThatAreClosed.keySet()); + + // Transform the try block to use try-with-resources + return transformToTryWithResources(t, resourcesThatAreClosed, resourceInitializers); + } + + private J.Block processBlock(J.Block body) { + // Find all try blocks in the method body + List tryBlocks = new ArrayList<>(); + findTryBlocks(body, tryBlocks); + + if (tryBlocks.isEmpty()) { + return body; + } + + // Process each try block + J.Block newBody = body; + for (J.Try tryBlock : tryBlocks) { + // Only process try blocks with a finally block + if (tryBlock.getFinally() == null) { + continue; + } + + // Find variable declarations in the method body that are used in the try block + List variableDeclarations = findVariableDeclarationsBeforeTry(newBody, tryBlock); + + if (variableDeclarations.isEmpty()) { + continue; + } + + // Find resources that are closed in the finally block + Map resourcesThatAreClosed = findResourcesThatAreClosedInFinally(variableDeclarations, tryBlock.getFinally()); + + if (resourcesThatAreClosed.isEmpty()) { + continue; + } + + // Check for resources initialized to null and assigned in the try block + Map resourceInitializers = findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet()); + + // Transform the try block to use try-with-resources + J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, resourceInitializers); + + // Replace the old try block with the new one and remove the variable declarations + newBody = replaceTryBlockAndRemoveDeclarations(newBody, tryBlock, newTryBlock, resourcesThatAreClosed.values()); + } + + return newBody; + } + + private Map findResourceInitializers(J.Try tryBlock, Set resourceNames) { + Map resourceInitializers = new HashMap<>(); + + // Check the first few statements in the try block for assignments to resources + for (Statement statement : tryBlock.getBody().getStatements()) { + if (statement instanceof J.Assignment) { + J.Assignment assignment = (J.Assignment) statement; + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + String varName = identifier.getSimpleName(); + if (resourceNames.contains(varName)) { + resourceInitializers.put(varName, assignment.getAssignment()); + } + } + } + } + + return resourceInitializers; + } + + private void findTryBlocks(J.Block block, List tryBlocks) { + for (Statement statement : block.getStatements()) { + if (statement instanceof J.Try) { + tryBlocks.add((J.Try) statement); + } + } + } + + private List findVariableDeclarationsBeforeTry(J.Block block, J.Try tryBlock) { + List variableDeclarations = new ArrayList<>(); + + // Find the index of the try block + int tryIndex = -1; + for (int i = 0; i < block.getStatements().size(); i++) { + if (block.getStatements().get(i) == tryBlock) { + tryIndex = i; + break; + } + } + + if (tryIndex == -1) { + return variableDeclarations; + } + + // Collect all variable declarations before the try block + for (int i = 0; i < tryIndex; i++) { + Statement stmt = block.getStatements().get(i); + if (stmt instanceof J.VariableDeclarations) { + variableDeclarations.add((J.VariableDeclarations) stmt); + } + } + + return variableDeclarations; + } + + private Map findResourcesThatAreClosedInFinally(List variableDeclarations, J.Block finallyBlock) { + Map resourcesThatAreClosed = new HashMap<>(); + + // Find variable declarations that implement AutoCloseable + for (J.VariableDeclarations varDecl : variableDeclarations) { + // Check if the variable type implements AutoCloseable + JavaType.FullyQualified type = TypeUtils.asFullyQualified(varDecl.getType()); + if (type != null && TypeUtils.isAssignableTo(AUTO_CLOSEABLE_TYPE, type)) { + for (J.VariableDeclarations.NamedVariable namedVar : varDecl.getVariables()) { + String varName = namedVar.getSimpleName(); + + // Check if this variable is closed in the finally block + if (isClosedInFinally(varName, finallyBlock)) { + resourcesThatAreClosed.put(varName, varDecl); + } + } + } + } + + return resourcesThatAreClosed; + } + + private boolean isClosedInFinally(String varName, J.Block finallyBlock) { + for (Statement statement : finallyBlock.getStatements()) { + if (isCloseStatement(statement, varName)) { + return true; + } + } + return false; + } + + private J.Block replaceTryBlockAndRemoveDeclarations(J.Block block, J.Try oldTry, J.Try newTry, Collection declarations) { + Set declarationsToRemove = new HashSet<>(declarations); + return block.withStatements(ListUtils.map(block.getStatements(), statement -> { + if (statement == oldTry) { + return newTry; + } else if (declarationsToRemove.contains(statement)) { + return null; + } + return statement; + })); + } + + private boolean isCloseStatement(Statement statement, String varName) { + if (statement instanceof J.If) { + // Check for null check before close + J.If ifStatement = (J.If) statement; + if (isNullCheckForVariable(ifStatement.getIfCondition().getTree(), varName)) { + Statement thenPart = ifStatement.getThenPart(); + if (thenPart instanceof J.Block) { + J.Block thenBlock = (J.Block) thenPart; + for (Statement thenStatement : thenBlock.getStatements()) { + if (isCloseMethodCall(thenStatement, varName) || isNestedTryWithClose(thenStatement, varName)) { + return true; + } + } + } else { + return isCloseMethodCall(thenPart, varName) || isNestedTryWithClose(thenPart, varName); + } + } + } else if (isCloseMethodCall(statement, varName)) { + return true; + } else { + return isNestedTryWithClose(statement, varName); + } + + return false; + } + + private boolean isNestedTryWithClose(Statement statement, String varName) { + if (statement instanceof J.Try) { + J.Try tryStatement = (J.Try) statement; + + // Check if the variable is closed in the try block + for (Statement tryBodyStatement : tryStatement.getBody().getStatements()) { + if (isCloseMethodCall(tryBodyStatement, varName)) { + return true; + } + } + + // Check if the variable is closed in any catch blocks + for (J.Try.Catch catchBlock : tryStatement.getCatches()) { + for (Statement catchBodyStatement : catchBlock.getBody().getStatements()) { + if (isCloseMethodCall(catchBodyStatement, varName)) { + return true; + } + } + } + + // Check if the variable is closed in the finally block + if (tryStatement.getFinally() != null) { + for (Statement finallyStatement : tryStatement.getFinally().getStatements()) { + if (isCloseMethodCall(finallyStatement, varName)) { + return true; + } + } + } + } + + return false; + } + + private boolean isNullCheckForVariable(Expression expression, String varName) { + if (expression instanceof J.Binary) { + J.Binary binary = (J.Binary) expression; + if (binary.getOperator() == J.Binary.Type.NotEqual || binary.getOperator() == J.Binary.Type.Equal) { + boolean leftIsVar = isVariable(binary.getLeft(), varName); + boolean rightIsNull = isNull(binary.getRight()); + boolean leftIsNull = isNull(binary.getLeft()); + boolean rightIsVar = isVariable(binary.getRight(), varName); + + return (leftIsVar && rightIsNull) || (leftIsNull && rightIsVar); + } + } + + return false; + } + + private boolean isVariable(Expression expression, String varName) { + if (expression instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) expression; + return identifier.getSimpleName().equals(varName); + } + + return false; + } + + private boolean isNull(Expression expression) { + if (expression instanceof J.Literal) { + J.Literal literal = (J.Literal) expression; + return literal.getValue() == null; + } + + return false; + } + + private boolean isCloseMethodCall(Statement statement, String varName) { + if (statement instanceof J.MethodInvocation) { + J.MethodInvocation methodInvocation = (J.MethodInvocation) statement; + + // Check if it's a call to close() + if (methodInvocation.getSimpleName().equals("close")) { + // Check if it's called on the variable + if (methodInvocation.getSelect() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) methodInvocation.getSelect(); + return identifier.getSimpleName().equals(varName); + } + } + } + + return false; + } + + private J.Try transformToTryWithResources(J.Try tryable, Map resourcesThatAreClosed, Map resourceInitializers) { + // Create resources for the try-with-resources statement + List resources = new ArrayList<>(); + + List> entries = new ArrayList<>(resourcesThatAreClosed.entrySet()); + for (int i = 0; i < entries.size(); i++) { + Map.Entry entry = entries.get(i); + String varName = entry.getKey(); + J.VariableDeclarations varDecl = entry.getValue(); + + // Find the named variable + for (J.VariableDeclarations.NamedVariable namedVar : varDecl.getVariables()) { + if (namedVar.getSimpleName().equals(varName)) { + // Create a new variable declaration with just this variable + J.VariableDeclarations singleVarDecl = varDecl; + if (varDecl.getVariables().size() > 1) { + singleVarDecl = varDecl.withVariables(Collections.singletonList(namedVar)); + } + + // If the resource is initialized to null and assigned in the try block, + // use the assigned value as the initializer + if (resourceInitializers.containsKey(varName)) { + Expression initializer = resourceInitializers.get(varName); + // Create a new list of variables with the updated initializer + List newVars = new ArrayList<>(); + for (J.VariableDeclarations.NamedVariable var : singleVarDecl.getVariables()) { + if (var.getSimpleName().equals(varName)) { + newVars.add(var.withInitializer(initializer)); + } else { + newVars.add(var); + } + } + singleVarDecl = singleVarDecl.withVariables(newVars); + } + + // Create a resource with proper spacing + // First resource gets no prefix, others get a newline and indentation + Space prefix; + if (i == 0) { + prefix = Space.EMPTY; + } else { + // For multiple resources, format with newline and indentation for better readability + prefix = entries.size() > 1 ? Space.format("\n ") : Space.format(" "); + } + + // Create the resource - only the last one should not have a semicolon + J.Try.Resource resource = new J.Try.Resource( + Tree.randomId(), + prefix, + Markers.EMPTY, + singleVarDecl.withPrefix(Space.EMPTY), + i < entries.size() - 1 // Only the last resource should not have a semicolon + ); + + resources.add(resource); + break; + } + } + } + + // Process the finally block to remove close statements + J.Block finallyBlock = removeCloseStatementsFromFinally(Objects.requireNonNull(tryable.getFinally()), resourcesThatAreClosed.keySet()); + + // Create a new try-with-resources statement + J.Try tryWithResources = tryable + .withResources(resources) + .withFinally(finallyBlock); + + // If the finally block is now empty, remove it + if (finallyBlock.getStatements().isEmpty()) { + tryWithResources = tryWithResources.withFinally(null); + } + + // Remove assignments to resources in the try block + List newBodyStatements = new ArrayList<>(); + for (Statement statement : tryWithResources.getBody().getStatements()) { + if (!(statement instanceof J.Assignment) || + !isAssignmentToResource(statement, resourcesThatAreClosed.keySet())) { + newBodyStatements.add(statement); + } + } + tryWithResources = tryWithResources.withBody(tryWithResources.getBody().withStatements(newBodyStatements)); + + return tryWithResources; + } + + private boolean isAssignmentToResource(Statement statement, Set resourceNames) { + if (statement instanceof J.Assignment) { + J.Assignment assignment = (J.Assignment) statement; + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + return resourceNames.contains(identifier.getSimpleName()); + } + } + return false; + } + + private J.Block removeCloseStatementsFromFinally(J.Block finallyBlock, Set resourceNames) { + List newStatements = new ArrayList<>(); + + for (Statement statement : finallyBlock.getStatements()) { + boolean shouldKeep = true; + + if (statement instanceof J.If) { + // Check if it's a null check for a resource + J.If ifStatement = (J.If) statement; + for (String varName : resourceNames) { + if (isNullCheckForVariable(ifStatement.getIfCondition().getTree(), varName)) { + shouldKeep = false; + break; + } + } + } else if (statement instanceof J.MethodInvocation) { + // Check if it's a close call on a resource + J.MethodInvocation methodInvocation = (J.MethodInvocation) statement; + if (methodInvocation.getSimpleName().equals("close")) { + if (methodInvocation.getSelect() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) methodInvocation.getSelect(); + if (resourceNames.contains(identifier.getSimpleName())) { + shouldKeep = false; + } + } + } + } + + if (shouldKeep) { + newStatements.add(statement); + } + } + + return finallyBlock.withStatements(newStatements); + } + }; + } +} diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java new file mode 100644 index 0000000000..923a00c240 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -0,0 +1,291 @@ +/* + * Copyright 2025 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class TryWithResourcesTest implements RewriteTest { + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new TryWithResources()); + } + + @DocumentExample + @Test + void basicTransformation() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + // Process data + } finally { + in.close(); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in = new FileInputStream("file.txt")) { + int data = in.read(); + // Process data + } + } + } + """ + ) + ); + } + + @Test + void multipleResources() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("input.txt"); + OutputStream out = new FileOutputStream("output.txt"); + try { + int data = in.read(); + out.write(data); + } finally { + in.close(); + out.close(); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in = new FileInputStream("input.txt"); + OutputStream out = new FileOutputStream("output.txt")) { + int data = in.read(); + out.write(data); + } + } + } + """ + ) + ); + } + + @Test + void nullCheckInFinally() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + // Process data + } finally { + if (in != null) { + in.close(); + } + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in = new FileInputStream("file.txt")) { + int data = in.read(); + // Process data + } + } + } + """ + ) + ); + } + + @Test + void complexFinallyBlock() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + // Process data + } finally { + in.close(); + System.out.println("Processing complete"); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in = new FileInputStream("file.txt")) { + int data = in.read(); + // Process data + } finally { + System.out.println("Processing complete"); + } + } + } + """ + ) + ); + } + + @Test + void nestedTryBlocks() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + OutputStream out = new FileOutputStream("output.txt"); + try { + int data = in.read(); + out.write(data); + } finally { + out.close(); + } + } finally { + in.close(); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in = new FileInputStream("file.txt")) { + try (OutputStream out = new FileOutputStream("output.txt")) { + int data = in.read(); + out.write(data); + } + } + } + } + """ + ) + ); + } + + @Test + void tryCatchFinally() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() { + InputStream in = null; + try { + in = new FileInputStream("file.txt"); + int data = in.read(); + // Process data + } catch (IOException e) { + e.printStackTrace(); + } finally { + if (in != null) { + try { + in.close(); + } catch (IOException e) { + // Ignore + } + } + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() { + try (InputStream in = new FileInputStream("file.txt")) { + int data = in.read(); + // Process data + } catch (IOException e) { + e.printStackTrace(); + } + } + } + """ + ) + ); + } + + @Test + void doNotTransformWhenResourceNotClosed() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + // Process data + } finally { + // Resource not closed + System.out.println("Processing complete"); + } + } + } + """ + ) + ); + } +} From 5a8b5e496eeff43319644cd6dc83af24acca9f49 Mon Sep 17 00:00:00 2001 From: Deepak Date: Fri, 20 Jun 2025 02:31:20 +0530 Subject: [PATCH 02/16] Try with resources --- .../staticanalysis/TryWithResources.java | 37 ++++++++++--------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index c5a17be5ad..994ee2d180 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -61,17 +61,12 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { J.Try t = super.visitTry(tryable, ctx); // Only process try blocks with a finally block - if (t.getFinally() == null) { + if (Objects.isNull(t.getFinally())) { return t; } // Find variable declarations in the try block - List variableDeclarations = new ArrayList<>(); - for (Statement statement : t.getBody().getStatements()) { - if (statement instanceof J.VariableDeclarations) { - variableDeclarations.add((J.VariableDeclarations) statement); - } - } + List variableDeclarations = collectVariableDeclarations(t); if (variableDeclarations.isEmpty()) { return t; @@ -84,17 +79,23 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { return t; } - // Check for resources initialized to null and assigned in the try block - Map resourceInitializers = findResourceInitializers(t, resourcesThatAreClosed.keySet()); - // Transform the try block to use try-with-resources - return transformToTryWithResources(t, resourcesThatAreClosed, resourceInitializers); + return transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet())); + } + + private List collectVariableDeclarations(J.Try t) { + List variableDeclarations = new ArrayList<>(); + for (Statement statement : t.getBody().getStatements()) { + if (statement instanceof J.VariableDeclarations) { + variableDeclarations.add((J.VariableDeclarations) statement); + } + } + return variableDeclarations; } private J.Block processBlock(J.Block body) { // Find all try blocks in the method body - List tryBlocks = new ArrayList<>(); - findTryBlocks(body, tryBlocks); + List tryBlocks = findTryBlocks(body); if (tryBlocks.isEmpty()) { return body; @@ -122,11 +123,9 @@ private J.Block processBlock(J.Block body) { continue; } - // Check for resources initialized to null and assigned in the try block - Map resourceInitializers = findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet()); - // Transform the try block to use try-with-resources - J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, resourceInitializers); + J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, + findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet())); // Replace the old try block with the new one and remove the variable declarations newBody = replaceTryBlockAndRemoveDeclarations(newBody, tryBlock, newTryBlock, resourcesThatAreClosed.values()); @@ -155,12 +154,14 @@ private Map findResourceInitializers(J.Try tryBlock, Set tryBlocks) { + private List findTryBlocks(J.Block block) { + List tryBlocks = new ArrayList<>(); for (Statement statement : block.getStatements()) { if (statement instanceof J.Try) { tryBlocks.add((J.Try) statement); } } + return tryBlocks; } private List findVariableDeclarationsBeforeTry(J.Block block, J.Try tryBlock) { From deb628a19ce4babe5ea159d3d3b4df7ea5ab873b Mon Sep 17 00:00:00 2001 From: Deepak Date: Fri, 20 Jun 2025 05:04:42 +0530 Subject: [PATCH 03/16] Try with resources --- .../staticanalysis/TryWithResources.java | 29 ++++++++++++-- .../staticanalysis/TryWithResourcesTest.java | 38 ++++++++++++++++++- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 994ee2d180..2461c702bf 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -15,6 +15,7 @@ */ package org.openrewrite.staticanalysis; +import org.openrewrite.Cursor; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.Tree; @@ -25,6 +26,7 @@ import org.openrewrite.marker.Markers; import java.util.*; +import java.util.concurrent.atomic.AtomicBoolean; /** * Transforms code using manual resource management with finally blocks to use the Java 7+ try-with-resources pattern. @@ -370,11 +372,13 @@ private J.Try transformToTryWithResources(J.Try tryable, Map newVars = new ArrayList<>(); for (J.VariableDeclarations.NamedVariable var : singleVarDecl.getVariables()) { + J.VariableDeclarations.NamedVariable updated = + isReferencedInBlock(tryable.getBody(), varName) ? + var : var.withName(var.getName().withSimpleName("ignored")); if (var.getSimpleName().equals(varName)) { - newVars.add(var.withInitializer(initializer)); - } else { - newVars.add(var); + updated = updated.withInitializer(initializer); } + newVars.add(updated); } singleVarDecl = singleVarDecl.withVariables(newVars); } @@ -430,6 +434,25 @@ private J.Try transformToTryWithResources(J.Try tryable, Map() { + @Override + public J.Identifier visitIdentifier(J.Identifier id, ExecutionContext ctx) { + // only count it if it's not the variable in the LHS of an assignment + if (id.getSimpleName().equals(varName)) { + Cursor parent = getCursor().getParentOrThrow(); + if (!(parent.getValue() instanceof J.Assignment && + ((J.Assignment)parent.getValue()).getVariable() == id)) { + seen.set(true); + } + } + return super.visitIdentifier(id, ctx); + } + }.visit(body, null); + return seen.get(); + } + private boolean isAssignmentToResource(Statement statement, Set resourceNames) { if (statement instanceof J.Assignment) { J.Assignment assignment = (J.Assignment) statement; diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index 923a00c240..6c55d3afc7 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -19,13 +19,15 @@ import org.openrewrite.DocumentExample; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; +import org.openrewrite.test.TypeValidation; import static org.openrewrite.java.Assertions.java; class TryWithResourcesTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new TryWithResources()); + spec.recipe(new TryWithResources()) + .typeValidationOptions(TypeValidation.none()); } @DocumentExample @@ -177,6 +179,40 @@ void method() throws IOException { ); } + @Test + void renameIgnoredIfUnused() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + public void testConnection() throws IOException { + InputStream in = null; + try { + in = new FileInputStream("file.txt"); + } finally { + if (in != null) { + in.close(); + } + } + } + } + """, + """ + import java.io.*; + + class Test { + public void testConnection() throws IOException { + try (InputStream ignored = new FileInputStream("file.txt")) { + } + } + } + """ + ) + ); + } + @Test void nestedTryBlocks() { rewriteRun( From f17b2245b77e4388629ed6fc8f0187409043a25e Mon Sep 17 00:00:00 2001 From: Deepak Date: Sat, 21 Jun 2025 00:57:29 +0530 Subject: [PATCH 04/16] updated review comments --- .../staticanalysis/TryWithResources.java | 104 ++++++++++-------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 2461c702bf..6ca1c2ec51 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -346,7 +346,10 @@ private boolean isCloseMethodCall(Statement statement, String varName) { return false; } - private J.Try transformToTryWithResources(J.Try tryable, Map resourcesThatAreClosed, Map resourceInitializers) { + private J.Try transformToTryWithResources( + J.Try tryable, Map resourcesThatAreClosed, + Map resourceInitializers) { // Create resources for the try-with-resources statement List resources = new ArrayList<>(); @@ -359,50 +362,7 @@ private J.Try transformToTryWithResources(J.Try tryable, Map 1) { - singleVarDecl = varDecl.withVariables(Collections.singletonList(namedVar)); - } - - // If the resource is initialized to null and assigned in the try block, - // use the assigned value as the initializer - if (resourceInitializers.containsKey(varName)) { - Expression initializer = resourceInitializers.get(varName); - // Create a new list of variables with the updated initializer - List newVars = new ArrayList<>(); - for (J.VariableDeclarations.NamedVariable var : singleVarDecl.getVariables()) { - J.VariableDeclarations.NamedVariable updated = - isReferencedInBlock(tryable.getBody(), varName) ? - var : var.withName(var.getName().withSimpleName("ignored")); - if (var.getSimpleName().equals(varName)) { - updated = updated.withInitializer(initializer); - } - newVars.add(updated); - } - singleVarDecl = singleVarDecl.withVariables(newVars); - } - - // Create a resource with proper spacing - // First resource gets no prefix, others get a newline and indentation - Space prefix; - if (i == 0) { - prefix = Space.EMPTY; - } else { - // For multiple resources, format with newline and indentation for better readability - prefix = entries.size() > 1 ? Space.format("\n ") : Space.format(" "); - } - - // Create the resource - only the last one should not have a semicolon - J.Try.Resource resource = new J.Try.Resource( - Tree.randomId(), - prefix, - Markers.EMPTY, - singleVarDecl.withPrefix(Space.EMPTY), - i < entries.size() - 1 // Only the last resource should not have a semicolon - ); - - resources.add(resource); + resources.add(createResources(tryable, resourceInitializers, namedVar, varDecl, varName, i, entries)); break; } } @@ -421,6 +381,56 @@ private J.Try transformToTryWithResources(J.Try tryable, Map resourceInitializers, J.VariableDeclarations.NamedVariable namedVar, J.VariableDeclarations varDecl, String varName, int i, List> entries) { + // Create a new variable declaration with just this variable + J.VariableDeclarations singleVarDecl = varDecl; + if (varDecl.getVariables().size() > 1) { + singleVarDecl = varDecl.withVariables(Collections.singletonList(namedVar)); + } + + // If the resource is initialized to null and assigned in the try block, + // use the assigned value as the initializer + if (resourceInitializers.containsKey(varName)) { + Expression initializer = resourceInitializers.get(varName); + // Create a new list of variables with the updated initializer + List newVars = new ArrayList<>(); + for (J.VariableDeclarations.NamedVariable var : singleVarDecl.getVariables()) { + J.VariableDeclarations.NamedVariable updated = + isReferencedInBlock(tryable.getBody(), varName) ? + var : var.withName(var.getName().withSimpleName("ignored")); + if (var.getSimpleName().equals(varName)) { + updated = updated.withInitializer(initializer); + } + newVars.add(updated); + } + singleVarDecl = singleVarDecl.withVariables(newVars); + } + + // Create a resource with proper spacing + // First resource gets no prefix, others get a newline and indentation + Space prefix; + if (i == 0) { + prefix = Space.EMPTY; + } else { + // For multiple resources, format with newline and indentation for better readability + prefix = entries.size() > 1 ? Space.format("\n ") : Space.format(" "); + } + + // Create the resource - only the last one should not have a semicolon + J.Try.Resource resource = new J.Try.Resource( + Tree.randomId(), + prefix, + Markers.EMPTY, + singleVarDecl.withPrefix(Space.EMPTY), + i < entries.size() - 1 // Only the last resource should not have a semicolon + ); + return resource; + } + + private J.Try removeAssignments(Map resourcesThatAreClosed, J.Try tryWithResources) { // Remove assignments to resources in the try block List newBodyStatements = new ArrayList<>(); for (Statement statement : tryWithResources.getBody().getStatements()) { @@ -429,9 +439,7 @@ private J.Try transformToTryWithResources(J.Try tryable, Map Date: Tue, 24 Jun 2025 12:27:04 +0200 Subject: [PATCH 05/16] Adopt `autoFormat` instead of explicit prefix handling --- .../staticanalysis/TryWithResources.java | 24 ++++--------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 6ca1c2ec51..6b8f51c00e 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -15,11 +15,7 @@ */ package org.openrewrite.staticanalysis; -import org.openrewrite.Cursor; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Recipe; -import org.openrewrite.Tree; -import org.openrewrite.TreeVisitor; +import org.openrewrite.*; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.tree.*; @@ -82,7 +78,8 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { } // Transform the try block to use try-with-resources - return transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet())); + J.Try tryWith = transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet())); + return autoFormat(tryWith, ctx); } private List collectVariableDeclarations(J.Try t) { @@ -409,25 +406,14 @@ private J.Try.Resource createResources(J.Try tryable, Map re singleVarDecl = singleVarDecl.withVariables(newVars); } - // Create a resource with proper spacing - // First resource gets no prefix, others get a newline and indentation - Space prefix; - if (i == 0) { - prefix = Space.EMPTY; - } else { - // For multiple resources, format with newline and indentation for better readability - prefix = entries.size() > 1 ? Space.format("\n ") : Space.format(" "); - } - // Create the resource - only the last one should not have a semicolon - J.Try.Resource resource = new J.Try.Resource( + return new J.Try.Resource( Tree.randomId(), - prefix, + 0 < i ? Space.format("\n") : Space.EMPTY, Markers.EMPTY, singleVarDecl.withPrefix(Space.EMPTY), i < entries.size() - 1 // Only the last resource should not have a semicolon ); - return resource; } private J.Try removeAssignments(Map resourcesThatAreClosed, J.Try tryWithResources) { From 59b6970607e2a7f1ba01a38467fb166de4385394 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 24 Jun 2025 12:31:20 +0200 Subject: [PATCH 06/16] Replace `newVars` list with `ListUtils` as a first example --- .../staticanalysis/TryWithResources.java | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 6b8f51c00e..63e09b9aa7 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -344,8 +344,8 @@ private boolean isCloseMethodCall(Statement statement, String varName) { } private J.Try transformToTryWithResources( - J.Try tryable, Map resourcesThatAreClosed, + J.Try tryable, + Map resourcesThatAreClosed, Map resourceInitializers) { // Create resources for the try-with-resources statement List resources = new ArrayList<>(); @@ -393,17 +393,14 @@ private J.Try.Resource createResources(J.Try tryable, Map re if (resourceInitializers.containsKey(varName)) { Expression initializer = resourceInitializers.get(varName); // Create a new list of variables with the updated initializer - List newVars = new ArrayList<>(); - for (J.VariableDeclarations.NamedVariable var : singleVarDecl.getVariables()) { - J.VariableDeclarations.NamedVariable updated = - isReferencedInBlock(tryable.getBody(), varName) ? - var : var.withName(var.getName().withSimpleName("ignored")); + singleVarDecl = singleVarDecl.withVariables(ListUtils.map(singleVarDecl.getVariables(), var -> { + J.VariableDeclarations.NamedVariable updated = isReferencedInBlock(tryable.getBody(), varName) ? + var : var.withName(var.getName().withSimpleName("ignored")); if (var.getSimpleName().equals(varName)) { - updated = updated.withInitializer(initializer); + return updated.withInitializer(initializer); } - newVars.add(updated); - } - singleVarDecl = singleVarDecl.withVariables(newVars); + return var; + })); } // Create the resource - only the last one should not have a semicolon @@ -437,7 +434,7 @@ public J.Identifier visitIdentifier(J.Identifier id, ExecutionContext ctx) { if (id.getSimpleName().equals(varName)) { Cursor parent = getCursor().getParentOrThrow(); if (!(parent.getValue() instanceof J.Assignment && - ((J.Assignment)parent.getValue()).getVariable() == id)) { + ((J.Assignment) parent.getValue()).getVariable() == id)) { seen.set(true); } } From a9db7d3e98f40c6dbcd69badbde2006475276020 Mon Sep 17 00:00:00 2001 From: Deepak Date: Fri, 27 Jun 2025 03:54:38 +0530 Subject: [PATCH 07/16] Use ListUtils if possible --- .../staticanalysis/TryWithResources.java | 78 +++++++++---------- 1 file changed, 38 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 63e09b9aa7..3ce0562d59 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -415,14 +415,20 @@ private J.Try.Resource createResources(J.Try tryable, Map re private J.Try removeAssignments(Map resourcesThatAreClosed, J.Try tryWithResources) { // Remove assignments to resources in the try block - List newBodyStatements = new ArrayList<>(); - for (Statement statement : tryWithResources.getBody().getStatements()) { - if (!(statement instanceof J.Assignment) || - !isAssignmentToResource(statement, resourcesThatAreClosed.keySet())) { - newBodyStatements.add(statement); - } - } - return tryWithResources.withBody(tryWithResources.getBody().withStatements(newBodyStatements)); + return tryWithResources.withBody(tryWithResources.getBody().withStatements( + ListUtils.map( + tryWithResources.getBody().getStatements(), + (statement) -> { + // drop any assignment to a closed resource + if (statement instanceof J.Assignment + && isAssignmentToResource(statement, resourcesThatAreClosed.keySet())) { + return null; + } + // keep everything else + return statement; + } + ) + )); } private boolean isReferencedInBlock(J.Block body, String varName) { @@ -456,39 +462,31 @@ private boolean isAssignmentToResource(Statement statement, Set resource } private J.Block removeCloseStatementsFromFinally(J.Block finallyBlock, Set resourceNames) { - List newStatements = new ArrayList<>(); - - for (Statement statement : finallyBlock.getStatements()) { - boolean shouldKeep = true; - - if (statement instanceof J.If) { - // Check if it's a null check for a resource - J.If ifStatement = (J.If) statement; - for (String varName : resourceNames) { - if (isNullCheckForVariable(ifStatement.getIfCondition().getTree(), varName)) { - shouldKeep = false; - break; - } - } - } else if (statement instanceof J.MethodInvocation) { - // Check if it's a close call on a resource - J.MethodInvocation methodInvocation = (J.MethodInvocation) statement; - if (methodInvocation.getSimpleName().equals("close")) { - if (methodInvocation.getSelect() instanceof J.Identifier) { - J.Identifier identifier = (J.Identifier) methodInvocation.getSelect(); - if (resourceNames.contains(identifier.getSimpleName())) { - shouldKeep = false; + return finallyBlock.withStatements( + ListUtils.map( + finallyBlock.getStatements(), + (statement) -> { + if (statement instanceof J.If) { + J.If ifStatement = (J.If) statement; + for (String varName : resourceNames) { + if (isNullCheckForVariable(ifStatement.getIfCondition().getTree(), varName)) { + return null; + } + } + } else if (statement instanceof J.MethodInvocation) { + J.MethodInvocation methodInvocation = (J.MethodInvocation) statement; + if ("close".equals(methodInvocation.getSimpleName()) + && methodInvocation.getSelect() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) methodInvocation.getSelect(); + if (resourceNames.contains(identifier.getSimpleName())) { + return null; + } + } + } + return statement; } - } - } - } - - if (shouldKeep) { - newStatements.add(statement); - } - } - - return finallyBlock.withStatements(newStatements); + ) + ); } }; } From 39fa67d6fc8cc2656794f7c62a5fe5ab758b1846 Mon Sep 17 00:00:00 2001 From: Deepak Date: Fri, 27 Jun 2025 03:54:56 +0530 Subject: [PATCH 08/16] added test cases --- .../staticanalysis/TryWithResourcesTest.java | 302 ++++++++++++++++++ 1 file changed, 302 insertions(+) diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index 6c55d3afc7..da0853a248 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -324,4 +324,306 @@ void method() throws IOException { ) ); } + + @Test + void multipleVariableDeclarations() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in1, in2 = new FileInputStream("file2.txt"); + in1 = new FileInputStream("file1.txt"); + try { + int data1 = in1.read(); + int data2 = in2.read(); + } finally { + in1.close(); + in2.close(); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + try (InputStream in1 = new FileInputStream("file1.txt"); + InputStream in2 = new FileInputStream("file2.txt")) { + int data1 = in1.read(); + int data2 = in2.read(); + } + } + } + """ + ) + ); + } + + @Test + void resourceClosedInCatchBlock() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() { + InputStream in = null; + try { + in = new FileInputStream("file.txt"); + int data = in.read(); + } catch (IOException e) { + if (in != null) { + try { + in.close(); + } catch (IOException ignored) { + } + } + throw new RuntimeException(e); + } finally { + if (in != null) { + try { + in.close(); + } catch (IOException ignored) { + } + } + } + } + } + """ + ) + ); + } + + @Test + void qualifiedCloseMethodCall() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + static class Wrapper { + InputStream stream; + Wrapper(InputStream s) { this.stream = s; } + InputStream getStream() { return stream; } + } + + void method() throws IOException { + Wrapper wrapper = new Wrapper(new FileInputStream("file.txt")); + try { + int data = wrapper.getStream().read(); + } finally { + wrapper.getStream().close(); + } + } + } + """ + ) + ); + } + + @Test + void nonAutoCloseableResource() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + static class CustomResource { + public void close() { + // Custom close logic + } + public void doSomething() {} + } + + void method() { + CustomResource resource = new CustomResource(); + try { + resource.doSomething(); + } finally { + resource.close(); + } + } + } + """ + ) + ); + } + + @Test + void resourceAssignedToField() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + private InputStream fieldStream; + + void method() throws IOException { + fieldStream = new FileInputStream("file.txt"); + try { + int data = fieldStream.read(); + } finally { + fieldStream.close(); + } + } + } + """ + ) + ); + } + + @Test + void resourceWithComplexFinallyLogic() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + boolean success = false; + try { + int data = in.read(); + success = true; + } finally { + if (success) { + System.out.println("Success!"); + } else { + System.out.println("Failed!"); + } + if (in != null) { + in.close(); + } + System.out.println("Cleanup done"); + } + } + } + """, + """ + import java.io.*; + + class Test { + void method() throws IOException { + boolean success = false; + try (InputStream in = new FileInputStream("file.txt")) { + int data = in.read(); + success = true; + } finally { + if (success) { + System.out.println("Success!"); + } else { + System.out.println("Failed!"); + } + System.out.println("Cleanup done"); + } + } + } + """ + ) + ); + } + + @Test + void resourceUsedAfterTryBlock() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + } finally { + in.close(); + } + // Resource is referenced after try - can still use try(in) syntax + System.out.println("Stream was: " + in); + } + } + """, + """ + import java.io.FileInputStream; + import java.io.IOException; + import java.io.InputStream; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try (in) { + int data = in.read(); + } + // Resource is referenced after try - can still use try(in) syntax + System.out.println("Stream was: " + in); + } + } + """ + ) + ); + } + + @Test + void resourceReassignedInTryBlock() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + void method() throws IOException { + InputStream in = new FileInputStream("file1.txt"); + try { + if (Math.random() > 0.5) { + in = new FileInputStream("file2.txt"); + } + int data = in.read(); + } finally { + in.close(); + } + } + } + """ + ) + ); + } + + @Test + void resourceWithStaticCloseCall() { + rewriteRun( + java( + """ + import java.io.*; + + class Test { + static void closeQuietly(InputStream stream) { + try { + if (stream != null) { + stream.close(); + } + } catch (IOException ignored) {} + } + + void method() throws IOException { + InputStream in = new FileInputStream("file.txt"); + try { + int data = in.read(); + } finally { + closeQuietly(in); + } + } + } + """ + ) + ); + } } From 9639d69cbadc5b45b4aba7c6d0e0243893084052 Mon Sep 17 00:00:00 2001 From: iddeepak <87892182+iddeepak@users.noreply.github.com> Date: Fri, 27 Jun 2025 04:04:38 +0530 Subject: [PATCH 09/16] Update src/main/java/org/openrewrite/staticanalysis/TryWithResources.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../java/org/openrewrite/staticanalysis/TryWithResources.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 3ce0562d59..b6b9dee2a1 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -420,8 +420,8 @@ private J.Try removeAssignments(Map resourcesTha tryWithResources.getBody().getStatements(), (statement) -> { // drop any assignment to a closed resource - if (statement instanceof J.Assignment - && isAssignmentToResource(statement, resourcesThatAreClosed.keySet())) { + if (statement instanceof J.Assignment && + isAssignmentToResource(statement, resourcesThatAreClosed.keySet())) { return null; } // keep everything else From d08d151702f289add25e81f7378e8912274a9a68 Mon Sep 17 00:00:00 2001 From: iddeepak <87892182+iddeepak@users.noreply.github.com> Date: Fri, 27 Jun 2025 04:04:49 +0530 Subject: [PATCH 10/16] Update src/main/java/org/openrewrite/staticanalysis/TryWithResources.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../java/org/openrewrite/staticanalysis/TryWithResources.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index b6b9dee2a1..9ecb3b4dba 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -475,8 +475,8 @@ private J.Block removeCloseStatementsFromFinally(J.Block finallyBlock, Set Date: Sat, 5 Jul 2025 20:24:03 +0200 Subject: [PATCH 11/16] Do not ignore type validation issues: we need those to chain recipes --- .../org/openrewrite/staticanalysis/TryWithResourcesTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index da0853a248..13a4babfae 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -19,15 +19,13 @@ import org.openrewrite.DocumentExample; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; -import org.openrewrite.test.TypeValidation; import static org.openrewrite.java.Assertions.java; class TryWithResourcesTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new TryWithResources()) - .typeValidationOptions(TypeValidation.none()); + spec.recipe(new TryWithResources()); } @DocumentExample From 11670ddb92773d88a7c34efd9352e39483550248 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Sat, 26 Jul 2025 16:50:40 +0200 Subject: [PATCH 12/16] Fix a number of issues already --- .../staticanalysis/TryWithResources.java | 113 +++++++++++++++++- .../staticanalysis/TryWithResourcesTest.java | 9 +- 2 files changed, 110 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 9ecb3b4dba..446a63373b 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -77,8 +77,18 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { return t; } + // Skip transformation if any resource is closed in a catch block + for (String resourceName : resourcesThatAreClosed.keySet()) { + if (isClosedInAnyCatchBlock(t, resourceName)) { + return t; + } + } + + // Find resource initializers from inside the try block + Map resourceInitializers = findResourceInitializers(t, resourcesThatAreClosed.keySet()); + // Transform the try block to use try-with-resources - J.Try tryWith = transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet())); + J.Try tryWith = transformToTryWithResources(t, resourcesThatAreClosed, resourceInitializers); return autoFormat(tryWith, ctx); } @@ -122,12 +132,30 @@ private J.Block processBlock(J.Block body) { continue; } + // Skip transformation if any resource is closed in a catch block + boolean skipDueToCatchBlock = false; + for (String resourceName : resourcesThatAreClosed.keySet()) { + if (isClosedInAnyCatchBlock(tryBlock, resourceName)) { + skipDueToCatchBlock = true; + break; + } + } + if (skipDueToCatchBlock) { + continue; + } + + // Find resource initializers from both before the try block and inside it + Map allResourceInitializers = findResourceInitializersBeforeTry(newBody, tryBlock, resourcesThatAreClosed.keySet()); + allResourceInitializers.putAll(findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet())); + // Transform the try block to use try-with-resources - J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, - findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet())); + J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, allResourceInitializers); // Replace the old try block with the new one and remove the variable declarations newBody = replaceTryBlockAndRemoveDeclarations(newBody, tryBlock, newTryBlock, resourcesThatAreClosed.values()); + + // Also remove any assignments to resources that were moved into the try-with-resources + newBody = removeResourceAssignments(newBody, tryBlock, allResourceInitializers.keySet()); } return newBody; @@ -153,6 +181,70 @@ private Map findResourceInitializers(J.Try tryBlock, Set findResourceInitializersBeforeTry(J.Block block, J.Try tryBlock, Set resourceNames) { + Map resourceInitializers = new HashMap<>(); + + // Find the index of the try block + int tryIndex = -1; + for (int i = 0; i < block.getStatements().size(); i++) { + if (block.getStatements().get(i) == tryBlock) { + tryIndex = i; + break; + } + } + + if (tryIndex == -1) { + return resourceInitializers; + } + + // Check statements before the try block for assignments to resources + for (int i = 0; i < tryIndex; i++) { + Statement stmt = block.getStatements().get(i); + if (stmt instanceof J.Assignment) { + J.Assignment assignment = (J.Assignment) stmt; + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + String varName = identifier.getSimpleName(); + if (resourceNames.contains(varName)) { + resourceInitializers.put(varName, assignment.getAssignment()); + } + } + } + } + + return resourceInitializers; + } + + private J.Block removeResourceAssignments(J.Block block, J.Try tryBlock, Set resourceNames) { + // Find the index of the try block + int tryIndex = -1; + for (int i = 0; i < block.getStatements().size(); i++) { + if (block.getStatements().get(i) == tryBlock) { + tryIndex = i; + break; + } + } + + if (tryIndex == -1) { + return block; + } + + // Remove assignments to resources before the try block + final int finalTryIndex = tryIndex; + return block.withStatements(ListUtils.map(block.getStatements(), (i, statement) -> { + if (i < finalTryIndex && statement instanceof J.Assignment) { + J.Assignment assignment = (J.Assignment) statement; + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + if (resourceNames.contains(identifier.getSimpleName())) { + return null; // Remove this assignment + } + } + } + return statement; + })); + } + private List findTryBlocks(J.Block block) { List tryBlocks = new ArrayList<>(); for (Statement statement : block.getStatements()) { @@ -221,6 +313,17 @@ private boolean isClosedInFinally(String varName, J.Block finallyBlock) { return false; } + private boolean isClosedInAnyCatchBlock(J.Try tryStatement, String varName) { + for (J.Try.Catch catchBlock : tryStatement.getCatches()) { + for (Statement statement : catchBlock.getBody().getStatements()) { + if (isCloseStatement(statement, varName)) { + return true; + } + } + } + return false; + } + private J.Block replaceTryBlockAndRemoveDeclarations(J.Block block, J.Try oldTry, J.Try newTry, Collection declarations) { Set declarationsToRemove = new HashSet<>(declarations); return block.withStatements(ListUtils.map(block.getStatements(), statement -> { @@ -394,10 +497,8 @@ private J.Try.Resource createResources(J.Try tryable, Map re Expression initializer = resourceInitializers.get(varName); // Create a new list of variables with the updated initializer singleVarDecl = singleVarDecl.withVariables(ListUtils.map(singleVarDecl.getVariables(), var -> { - J.VariableDeclarations.NamedVariable updated = isReferencedInBlock(tryable.getBody(), varName) ? - var : var.withName(var.getName().withSimpleName("ignored")); if (var.getSimpleName().equals(varName)) { - return updated.withInitializer(initializer); + return var.withInitializer(initializer); } return var; })); diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index 13a4babfae..81589b04d3 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -202,7 +202,7 @@ public void testConnection() throws IOException { class Test { public void testConnection() throws IOException { - try (InputStream ignored = new FileInputStream("file.txt")) { + try (InputStream in = new FileInputStream("file.txt")) { } } } @@ -551,14 +551,11 @@ void method() throws IOException { } """, """ - import java.io.FileInputStream; - import java.io.IOException; - import java.io.InputStream; + import java.io.*; class Test { void method() throws IOException { - InputStream in = new FileInputStream("file.txt"); - try (in) { + try (InputStream in = new FileInputStream("file.txt")) { int data = in.read(); } // Resource is referenced after try - can still use try(in) syntax From 05d1d829565cd1fef4a86de97fce3b809c70796b Mon Sep 17 00:00:00 2001 From: Knut Wannheden Date: Mon, 4 Aug 2025 12:05:53 +0200 Subject: [PATCH 13/16] Some more fixes --- .../staticanalysis/TryWithResources.java | 131 +++++++++++++++--- .../staticanalysis/TryWithResourcesTest.java | 3 + 2 files changed, 113 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 446a63373b..29234a1e5d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -84,6 +84,13 @@ public J.Try visitTry(J.Try tryable, ExecutionContext ctx) { } } + // Skip transformation if any resource is reassigned within the try block + for (String resourceName : resourcesThatAreClosed.keySet()) { + if (isReassignedInTryBlock(t, resourceName, resourcesThatAreClosed.get(resourceName))) { + return t; + } + } + // Find resource initializers from inside the try block Map resourceInitializers = findResourceInitializers(t, resourcesThatAreClosed.keySet()); @@ -144,6 +151,18 @@ private J.Block processBlock(J.Block body) { continue; } + // Skip transformation if any resource is reassigned within the try block + boolean skipDueToReassignment = false; + for (String resourceName : resourcesThatAreClosed.keySet()) { + if (isReassignedInTryBlock(tryBlock, resourceName, resourcesThatAreClosed.get(resourceName))) { + skipDueToReassignment = true; + break; + } + } + if (skipDueToReassignment) { + continue; + } + // Find resource initializers from both before the try block and inside it Map allResourceInitializers = findResourceInitializersBeforeTry(newBody, tryBlock, resourcesThatAreClosed.keySet()); allResourceInitializers.putAll(findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet())); @@ -324,6 +343,95 @@ private boolean isClosedInAnyCatchBlock(J.Try tryStatement, String varName) { return false; } + private boolean isReassignedInTryBlock(J.Try tryStatement, String varName, J.VariableDeclarations varDecl) { + // Check if the variable was initially assigned to null or has no initializer + boolean initiallyNullOrEmpty = false; + for (J.VariableDeclarations.NamedVariable namedVar : varDecl.getVariables()) { + if (namedVar.getSimpleName().equals(varName)) { + if (namedVar.getInitializer() == null) { + initiallyNullOrEmpty = true; + } else if (namedVar.getInitializer() instanceof J.Literal) { + J.Literal literal = (J.Literal) namedVar.getInitializer(); + initiallyNullOrEmpty = literal.getValue() == null; + } + break; + } + } + + int assignmentCount = 0; + boolean hasConditionalAssignment = false; + + for (Statement statement : tryStatement.getBody().getStatements()) { + if (statement instanceof J.Assignment) { + J.Assignment assignment = (J.Assignment) statement; + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + if (identifier.getSimpleName().equals(varName)) { + assignmentCount++; + } + } + } + // Only check control flow statements for nested assignments + else if (statement instanceof J.If || statement instanceof J.Switch || + statement instanceof J.ForLoop || statement instanceof J.ForEachLoop || + statement instanceof J.WhileLoop || statement instanceof J.DoWhileLoop || + statement instanceof J.Block) { + if (containsReassignment(statement, varName)) { + hasConditionalAssignment = true; + } + } + } + + // If the variable was initially null/empty: + if (initiallyNullOrEmpty) { + // Multiple assignments or conditional assignments are problematic + return assignmentCount > 1 || hasConditionalAssignment; + } + + // If the variable had an initial non-null value, any assignment in the try block is a reassignment + return assignmentCount > 0 || hasConditionalAssignment; + } + + private boolean containsReassignment(Statement statement, String varName) { + AtomicBoolean found = new AtomicBoolean(false); + + new JavaIsoVisitor() { + @Override + public J.Assignment visitAssignment(J.Assignment assignment, AtomicBoolean ctx) { + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier identifier = (J.Identifier) assignment.getVariable(); + if (identifier.getSimpleName().equals(varName)) { + found.set(true); + } + } + return super.visitAssignment(assignment, ctx); + } + + @Override + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, AtomicBoolean ctx) { + // Skip nested classes as they have separate scopes + return classDecl; + } + + @Override + public J.Lambda visitLambda(J.Lambda lambda, AtomicBoolean ctx) { + // Skip lambdas as they have separate scopes + return lambda; + } + + @Override + public J.NewClass visitNewClass(J.NewClass newClass, AtomicBoolean ctx) { + // Skip anonymous classes as they have separate scopes + if (newClass.getBody() != null) { + return newClass; + } + return super.visitNewClass(newClass, ctx); + } + }.visit(statement, found); + + return found.get(); + } + private J.Block replaceTryBlockAndRemoveDeclarations(J.Block block, J.Try oldTry, J.Try newTry, Collection declarations) { Set declarationsToRemove = new HashSet<>(declarations); return block.withStatements(ListUtils.map(block.getStatements(), statement -> { @@ -462,7 +570,7 @@ private J.Try transformToTryWithResources( // Find the named variable for (J.VariableDeclarations.NamedVariable namedVar : varDecl.getVariables()) { if (namedVar.getSimpleName().equals(varName)) { - resources.add(createResources(tryable, resourceInitializers, namedVar, varDecl, varName, i, entries)); + resources.add(createResources(resourceInitializers, namedVar, varDecl, varName, i, entries)); break; } } @@ -484,7 +592,7 @@ private J.Try transformToTryWithResources( return removeAssignments(resourcesThatAreClosed, tryWithResources); } - private J.Try.Resource createResources(J.Try tryable, Map resourceInitializers, J.VariableDeclarations.NamedVariable namedVar, J.VariableDeclarations varDecl, String varName, int i, List> entries) { + private J.Try.Resource createResources(Map resourceInitializers, J.VariableDeclarations.NamedVariable namedVar, J.VariableDeclarations varDecl, String varName, int i, List> entries) { // Create a new variable declaration with just this variable J.VariableDeclarations singleVarDecl = varDecl; if (varDecl.getVariables().size() > 1) { @@ -532,25 +640,6 @@ private J.Try removeAssignments(Map resourcesTha )); } - private boolean isReferencedInBlock(J.Block body, String varName) { - AtomicBoolean seen = new AtomicBoolean(false); - new JavaIsoVisitor() { - @Override - public J.Identifier visitIdentifier(J.Identifier id, ExecutionContext ctx) { - // only count it if it's not the variable in the LHS of an assignment - if (id.getSimpleName().equals(varName)) { - Cursor parent = getCursor().getParentOrThrow(); - if (!(parent.getValue() instanceof J.Assignment && - ((J.Assignment) parent.getValue()).getVariable() == id)) { - seen.set(true); - } - } - return super.visitIdentifier(id, ctx); - } - }.visit(body, null); - return seen.get(); - } - private boolean isAssignmentToResource(Statement statement, Set resourceNames) { if (statement instanceof J.Assignment) { J.Assignment assignment = (J.Assignment) statement; diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index 81589b04d3..f54604475b 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -15,6 +15,7 @@ */ package org.openrewrite.staticanalysis; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; import org.openrewrite.test.RecipeSpec; @@ -22,6 +23,7 @@ import static org.openrewrite.java.Assertions.java; +@SuppressWarnings("TryFinallyCanBeTryWithResources") class TryWithResourcesTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { @@ -324,6 +326,7 @@ void method() throws IOException { } @Test + @Disabled("This is rather tricky and quite uncommon") void multipleVariableDeclarations() { rewriteRun( java( From b8bddafd0531fdc971189d45088cddfc1843c251 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 4 Aug 2025 23:45:04 +0200 Subject: [PATCH 14/16] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../org/openrewrite/staticanalysis/TryWithResources.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 29234a1e5d..01f1da0dbc 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -15,7 +15,10 @@ */ package org.openrewrite.staticanalysis; -import org.openrewrite.*; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; import org.openrewrite.internal.ListUtils; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.tree.*; From c3e4409ca0fcce6236ed4452d382d3a94ba18615 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Mon, 4 Aug 2025 23:45:23 +0200 Subject: [PATCH 15/16] Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../java/org/openrewrite/staticanalysis/TryWithResources.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java index 01f1da0dbc..c3301d4001 100644 --- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java +++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java @@ -440,7 +440,8 @@ private J.Block replaceTryBlockAndRemoveDeclarations(J.Block block, J.Try oldTry return block.withStatements(ListUtils.map(block.getStatements(), statement -> { if (statement == oldTry) { return newTry; - } else if (declarationsToRemove.contains(statement)) { + } + if (declarationsToRemove.contains(statement)) { return null; } return statement; From 78d690073f43af6f2ddaf24e8f1a011404857b0b Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 5 Aug 2025 01:38:51 +0200 Subject: [PATCH 16/16] Apply suggestions from code review --- .../org/openrewrite/staticanalysis/TryWithResourcesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java index f54604475b..62ca3669b7 100644 --- a/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/TryWithResourcesTest.java @@ -325,8 +325,8 @@ void method() throws IOException { ); } - @Test @Disabled("This is rather tricky and quite uncommon") + @Test void multipleVariableDeclarations() { rewriteRun( java(