Try with resources #599
Draft
Try with resources #599
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pankraz76
approved these changes
Jun 20, 2025
There was a problem hiding this comment.
well done. thank you for dedication.
could give more separation on impl. but this random detail. Its working fine, due to good testing. @knutwannheden thanks for the starter.
| return false; | ||
| } | ||
|
|
||
| private J.Try transformToTryWithResources(J.Try tryable, Map<String, J.VariableDeclarations> resourcesThatAreClosed, Map<String, Expression> resourceInitializers) { |
There was a problem hiding this comment.
does not even fit my screen. SOC.
| // Create resources for the try-with-resources statement | ||
| List<J.Try.Resource> resources = new ArrayList<>(); | ||
|
|
||
| List<Map.Entry<String, J.VariableDeclarations>> entries = new ArrayList<>(resourcesThatAreClosed.entrySet()); |
There was a problem hiding this comment.
this seems to be core logic making some iteration / transformation.
Comment on lines
424
to
432
| // Remove assignments to resources in the try block | ||
| List<Statement> 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)); |
There was a problem hiding this comment.
Suggested change
| // Remove assignments to resources in the try block | |
| List<Statement> 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)); | |
| RemoveAssignments(); |
SOC.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
|
Agreed could be refined more. Also, what if we could move some of the functionality to some other class like ResourceUtils if could be used by other recipe ? |
| """ | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
I have a few more test cases to try to "break" the recipe:
@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);
}
}
}
"""
)
);
}There was a problem hiding this comment.
In case you find it useful (although I must warn you that this is mostly AI generated), this should fix some more tests:
Subject: [PATCH] `TryWithResources` recipe
Issues:
- #591
---
Index: src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
--- a/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java (revision f17b2245b77e4388629ed6fc8f0187409043a25e)
+++ b/src/main/java/org/openrewrite/staticanalysis/TryWithResources.java (date 1750453794223)
@@ -75,14 +75,16 @@
}
// Find resources that are closed in the finally block
- Map<String, J.VariableDeclarations> resourcesThatAreClosed = findResourcesThatAreClosedInFinally(variableDeclarations, t.getFinally());
+ Map<String, J.VariableDeclarations> resourcesThatAreClosed = findResourcesThatAreClosedInFinallyWithSafetyChecks(variableDeclarations, t.getFinally(), t);
if (resourcesThatAreClosed.isEmpty()) {
return t;
}
- // Transform the try block to use try-with-resources
- return transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet()));
+ // For visitTry we don't have access to the containing block, so we can't detect
+ // if resources are used after the try block. This is handled by processBlock instead.
+ // Return the original try block unchanged.
+ return t;
}
private List<J.VariableDeclarations> collectVariableDeclarations(J.Try t) {
@@ -119,18 +121,33 @@
}
// Find resources that are closed in the finally block
- Map<String, J.VariableDeclarations> resourcesThatAreClosed = findResourcesThatAreClosedInFinally(variableDeclarations, tryBlock.getFinally());
+ Map<String, J.VariableDeclarations> resourcesThatAreClosed = findResourcesThatAreClosedInFinallyWithSafetyChecks(variableDeclarations, tryBlock.getFinally(), tryBlock);
if (resourcesThatAreClosed.isEmpty()) {
continue;
}
+ // Determine which resources are used after the try block
+ Set<String> resourcesUsedAfterTry = new HashSet<>();
+ for (String varName : resourcesThatAreClosed.keySet()) {
+ if (isVariableUsedAfterStatement(varName, newBody, tryBlock)) {
+ resourcesUsedAfterTry.add(varName);
+ }
+ }
+
// Transform the try block to use try-with-resources
J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed,
- findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet()));
+ findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet()), resourcesUsedAfterTry, newBody);
// Replace the old try block with the new one and remove the variable declarations
- newBody = replaceTryBlockAndRemoveDeclarations(newBody, tryBlock, newTryBlock, resourcesThatAreClosed.values());
+ // Don't remove declarations for resources that use Java 9+ syntax
+ Collection<J.VariableDeclarations> declarationsToRemove = new ArrayList<>();
+ for (Map.Entry<String, J.VariableDeclarations> entry : resourcesThatAreClosed.entrySet()) {
+ if (!resourcesUsedAfterTry.contains(entry.getKey())) {
+ declarationsToRemove.add(entry.getValue());
+ }
+ }
+ newBody = replaceTryBlockAndRemoveDeclarations(newBody, tryBlock, newTryBlock, declarationsToRemove);
}
return newBody;
@@ -211,6 +228,43 @@
}
}
}
+
+ return resourcesThatAreClosed;
+ }
+
+ private Map<String, J.VariableDeclarations> findResourcesThatAreClosedInFinallyWithSafetyChecks(List<J.VariableDeclarations> variableDeclarations, J.Block finallyBlock, J.Try tryBlock) {
+ Map<String, J.VariableDeclarations> 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)) {
+ // Additional safety checks
+ boolean isSafeToTransform = true;
+
+ // Check if resource is closed in any catch blocks (unsafe)
+ if (!tryBlock.getCatches().isEmpty() && isResourceClosedInCatchBlocks(varName, tryBlock)) {
+ isSafeToTransform = false;
+ }
+
+ // Check if resource is reassigned in try block (unsafe)
+ if (isSafeToTransform && isResourceReassignedInTry(varName, tryBlock, namedVar)) {
+ isSafeToTransform = false;
+ }
+
+ if (isSafeToTransform) {
+ resourcesThatAreClosed.put(varName, varDecl);
+ }
+ }
+ }
+ }
+ }
return resourcesThatAreClosed;
}
@@ -224,6 +278,70 @@
return false;
}
+ private boolean isResourceClosedInCatchBlocks(String varName, J.Try tryBlock) {
+ for (J.Try.Catch catchBlock : tryBlock.getCatches()) {
+ for (Statement statement : catchBlock.getBody().getStatements()) {
+ if (isCloseStatement(statement, varName)) {
+ return true;
+ }
+ }
+ }
+ return false;
+ }
+
+ private boolean isResourceReassignedInTry(String varName, J.Try tryBlock, J.VariableDeclarations.NamedVariable namedVar) {
+ // If variable is initialized to null, assignment in try block is safe
+ if (namedVar.getInitializer() instanceof J.Literal) {
+ J.Literal literal = (J.Literal) namedVar.getInitializer();
+ if (literal.getValue() == null) {
+ return false; // null -> value assignment is safe
+ }
+ }
+
+ // If variable has non-null initializer, any assignment in try block is unsafe
+ AtomicBoolean foundAssignment = new AtomicBoolean(false);
+
+ new JavaIsoVisitor<ExecutionContext>() {
+ @Override
+ public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) {
+ if (assignment.getVariable() instanceof J.Identifier) {
+ J.Identifier identifier = (J.Identifier) assignment.getVariable();
+ if (identifier.getSimpleName().equals(varName)) {
+ foundAssignment.set(true);
+ }
+ }
+ return super.visitAssignment(assignment, ctx);
+ }
+ }.visit(tryBlock.getBody(), null);
+ return foundAssignment.get();
+ }
+
+ private boolean isVariableUsedAfterStatement(String varName, J.Block containingBlock, Statement statement) {
+ List<Statement> statements = containingBlock.getStatements();
+ int statementIndex = statements.indexOf(statement);
+ if (statementIndex == -1 || statementIndex == statements.size() - 1) {
+ return false;
+ }
+
+ AtomicBoolean found = new AtomicBoolean(false);
+ for (int i = statementIndex + 1; i < statements.size(); i++) {
+ new JavaIsoVisitor<ExecutionContext>() {
+ @Override
+ public J.Identifier visitIdentifier(J.Identifier identifier, ExecutionContext ctx) {
+ if (identifier.getSimpleName().equals(varName)) {
+ found.set(true);
+ }
+ return super.visitIdentifier(identifier, ctx);
+ }
+ }.visit(statements.get(i), null);
+
+ if (found.get()) {
+ break;
+ }
+ }
+ return found.get();
+ }
+
private J.Block replaceTryBlockAndRemoveDeclarations(J.Block block, J.Try oldTry, J.Try newTry, Collection<J.VariableDeclarations> declarations) {
Set<Statement> declarationsToRemove = new HashSet<>(declarations);
return block.withStatements(ListUtils.map(block.getStatements(), statement -> {
@@ -349,7 +467,9 @@
private J.Try transformToTryWithResources(
J.Try tryable, Map<String,
J.VariableDeclarations> resourcesThatAreClosed,
- Map<String, Expression> resourceInitializers) {
+ Map<String, Expression> resourceInitializers,
+ Set<String> resourcesUsedAfterTry,
+ J.Block containingBlock) {
// Create resources for the try-with-resources statement
List<J.Try.Resource> resources = new ArrayList<>();
@@ -359,11 +479,19 @@
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)) {
- resources.add(createResources(tryable, resourceInitializers, namedVar, varDecl, varName, i, entries));
- break;
+ // Check if we should use Java 9+ syntax for this resource
+ boolean useJava9Syntax = resourcesUsedAfterTry.contains(varName);
+
+ if (useJava9Syntax) {
+ // Create a simple identifier reference for Java 9+ syntax
+ resources.add(createJava9Resource(varName, i, entries));
+ } else {
+ // Find the named variable for traditional syntax
+ for (J.VariableDeclarations.NamedVariable namedVar : varDecl.getVariables()) {
+ if (namedVar.getSimpleName().equals(varName)) {
+ resources.add(createResources(tryable, resourceInitializers, namedVar, varDecl, varName, i, entries));
+ break;
+ }
}
}
}
@@ -384,6 +512,36 @@
return removeAssignments(resourcesThatAreClosed, tryWithResources);
}
+ private J.Try.Resource createJava9Resource(String varName, int i, List<Map.Entry<String, J.VariableDeclarations>> entries) {
+ // Create a resource with just an identifier for Java 9+ syntax
+ Space prefix;
+ if (i == 0) {
+ prefix = Space.EMPTY;
+ } else {
+ prefix = Space.SINGLE_SPACE;
+ }
+
+ // Create an identifier for the variable
+ J.Identifier identifier = new J.Identifier(
+ Tree.randomId(),
+ Space.EMPTY,
+ Markers.EMPTY,
+ Collections.emptyList(),
+ varName,
+ null,
+ null
+ );
+
+ // Create the resource
+ return new J.Try.Resource(
+ Tree.randomId(),
+ prefix,
+ Markers.EMPTY,
+ identifier,
+ i < entries.size() - 1
+ );
+ }
+
private J.Try.Resource createResources(J.Try tryable, Map<String, Expression> resourceInitializers, J.VariableDeclarations.NamedVariable namedVar, J.VariableDeclarations varDecl, String varName, int i, List<Map.Entry<String, J.VariableDeclarations>> entries) {
// Create a new variable declaration with just this variable
J.VariableDeclarations singleVarDecl = varDecl;
@@ -415,8 +573,7 @@
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(" ");
+ prefix = Space.SINGLE_SPACE;
}
// Create the resource - only the last one should not have a semicolon
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
knutwannheden
added a commit
to openrewrite/rewrite
that referenced
this pull request
Jun 24, 2025
* Java: Fix formatting of try-with-resources Issues: - openrewrite/rewrite-static-analysis#599 * Update rewrite-java/src/main/java/org/openrewrite/java/format/ColumnPositionCalculator.java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: Tim te Beek <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
timtebeek
requested changes
Jun 24, 2025
There was a problem hiding this comment.
Great to see already; I've applied some small fixes as well as Knut's autoformat suggestion. Would you be ok to apply those further improvements before a next round of review?
I've also not yet tried Knut's other suggested tests or fixes; those could be helpful still as well.
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
Comment on lines
417
to
425
| // Remove assignments to resources in the try block | ||
| List<Statement> 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)); |
There was a problem hiding this comment.
I see this pattern repeated quite often: you create a new list, then loop over a collection, and selectively add/modify elements to place into the new list. Would you mind converting those over to use ListUtils as seen in 59b6970 ?
ListUtils is a way to make that type of operation not create a new list unless there is a underlying change to the collection. That has benefits both for performance, and for immutability in our LST model. I've pointed this out here, but it also applies above and below.
github-project-automation
bot
moved this from Ready to Review
to In Progress
in OpenRewrite
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
src/main/java/org/openrewrite/staticanalysis/TryWithResources.java
Outdated
Show resolved
Hide resolved
…java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…java Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Handling resources in finally blocks is verbose and prone to errors. Adopting try-with-resources guarantees that all AutoCloseable resources are closed automatically, resulting in cleaner, safer, and more maintainable code.
Acknowledgments
Huge thanks to @knutwannheden for crafting the original draft of the recipe. #593
Thank you, @Pankraz76 for raising the original issue that sparked this recipe. #591
/cc @knutwannheden for the original groundwork and inspiration!