Conversation
Issues: - #591
| private J.Block processBlock(J.Block body) { | ||
| // Find all try blocks in the method body | ||
| List<J.Try> tryBlocks = new ArrayList<>(); | ||
| findTryBlocks(body, tryBlocks); |
There was a problem hiding this comment.
Avoid mutating input parameters.
https://techannotation.wordpress.com/2021/02/03/be-a-good-citizens-by-not-mutating-input-parameter/
There was a problem hiding this comment.
| findTryBlocks(body, tryBlocks); | |
| List<J.Try> tryBlocks = collectTryBlocks(body); |
|
|
||
| for (Statement statement : finallyBlock.getStatements()) { | ||
| boolean shouldKeep = true; | ||
|
|
There was a problem hiding this comment.
if (res != null) { } block also removes any other side-effects inside it such as logging or metrics.
@Pankraz76 clarify here!
| List<J.VariableDeclarations> variableDeclarations = new ArrayList<>(); | ||
| for (Statement statement : t.getBody().getStatements()) { | ||
| if (statement instanceof J.VariableDeclarations) { | ||
| variableDeclarations.add((J.VariableDeclarations) statement); | ||
| } | ||
| } |
There was a problem hiding this comment.
| List<J.VariableDeclarations> variableDeclarations = new ArrayList<>(); | |
| for (Statement statement : t.getBody().getStatements()) { | |
| if (statement instanceof J.VariableDeclarations) { | |
| variableDeclarations.add((J.VariableDeclarations) statement); | |
| } | |
| } | |
| List<J.VariableDeclarations> variableDeclarations = collectVariableDeclarations(t.getBody()); |
| J.Try t = super.visitTry(tryable, ctx); | ||
|
|
||
| // Only process try blocks with a finally block | ||
| if (t.getFinally() == null) { |
There was a problem hiding this comment.
| if (t.getFinally() == null) { | |
| if (Objects.isNull(t.getFinally())) { |
| Map<String, Expression> resourceInitializers = findResourceInitializers(t, resourcesThatAreClosed.keySet()); | ||
|
|
||
| // Transform the try block to use try-with-resources | ||
| return transformToTryWithResources(t, resourcesThatAreClosed, resourceInitializers); |
There was a problem hiding this comment.
| return transformToTryWithResources(t, resourcesThatAreClosed, resourceInitializers); | |
| return transformToTryWithResources(t, resourcesThatAreClosed, findResourceInitializers(t, resourcesThatAreClosed.keySet())); |
just a suggestion to keep it inline
| J.Block newBody = body; | ||
| for (J.Try tryBlock : tryBlocks) { | ||
| // Only process try blocks with a finally block | ||
| if (tryBlock.getFinally() == null) { |
There was a problem hiding this comment.
| if (tryBlock.getFinally() == null) { | |
| if (Objects.isNull(t.getFinally())) { |
| Map<String, Expression> resourceInitializers = findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet()); | ||
|
|
||
| // Transform the try block to use try-with-resources | ||
| J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, resourceInitializers); |
There was a problem hiding this comment.
| J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, resourceInitializers); | |
| J.Try newTryBlock = transformToTryWithResources(tryBlock, resourcesThatAreClosed, findResourceInitializers(tryBlock, resourcesThatAreClosed.keySet())); |
@Pankraz76 clarify here! Is inline better ?
| 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.
This one method looks big can we break it down ?
@Pankraz76
| ); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
| @Test | |
| @Test | |
| void renameIgnoredIfUnused() { | |
| rewriteRun( | |
| java( | |
| """ | |
| import java.io.*; | |
| public void testConnection() throws MessagingException { | |
| Transport transport = null; | |
| try { | |
| transport = connectTransport(); | |
| } | |
| finally { | |
| if (transport != null) { | |
| transport.close(); | |
| } | |
| } | |
| } | |
| """, | |
| """ | |
| import java.io.*; | |
| class Test { | |
| public void testConnection() throws MessagingException { | |
| try (Transport ignored = connectTransport()) { | |
| } | |
| } | |
| } | |
| } | |
| """ | |
| ) | |
| ); | |
| } | |
| @Test |
imports missing but this is kind of an edge case missing:
|
@knutwannheden @Pankraz76 |
|
I am not planning on making any changes to this PR nor merging it as-is. Feel free to use it as a starting point for a PR. If you spot problems, ai suggest you create a test that show cases the bug and then you can fix it. |
3 participants
Issues:
try-with-resourcesstatement #591