From 3fb1a08101747ee0be1d1ace2accebbdd096711a Mon Sep 17 00:00:00 2001 From: joanvr Date: Thu, 24 Aug 2023 15:42:02 +0200 Subject: [PATCH 1/5] do not remove try block if there are resources --- .../staticanalysis/EmptyBlockVisitor.java | 8 ++++--- .../staticanalysis/EmptyBlockTest.java | 23 +++++++++++++++++++ 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java index 5e68b7426..ede357c4f 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java @@ -82,7 +82,7 @@ public J.Block visitBlock(J.Block block, P p) { J.Block nestedBlock = (J.Block) s; if (isEmptyBlock(nestedBlock) && ((Boolean.TRUE.equals(emptyBlockStyle.getStaticInit()) && nestedBlock.isStatic()) || - (Boolean.TRUE.equals(emptyBlockStyle.getInstanceInit()) && !nestedBlock.isStatic()))) { + (Boolean.TRUE.equals(emptyBlockStyle.getInstanceInit()) && !nestedBlock.isStatic()))) { filtered.set(true); return null; } @@ -97,7 +97,9 @@ public J.Block visitBlock(J.Block block, P p) { public J.Try visitTry(J.Try tryable, P p) { J.Try t = super.visitTry(tryable, p); - if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralTry()) && isEmptyBlock(t.getBody())) { + if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralTry()) && + isEmptyBlock(t.getBody()) && + (t.getResources() == null || t.getResources().isEmpty())) { doAfterVisit(new DeleteStatement<>(tryable)); } else if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralFinally()) && t.getFinally() != null && !t.getCatches().isEmpty() && isEmptyBlock(t.getFinally())) { @@ -211,7 +213,7 @@ public J.Switch visitSwitch(J.Switch switch_, P p) { private boolean isEmptyBlock(Statement blockNode) { if (blockNode instanceof J.Block) { - J.Block block = (J.Block)blockNode; + J.Block block = (J.Block) blockNode; if (EmptyBlockStyle.BlockPolicy.STATEMENT.equals(emptyBlockStyle.getBlockPolicy())) { return block.getStatements().isEmpty(); } else if (EmptyBlockStyle.BlockPolicy.TEXT.equals(emptyBlockStyle.getBlockPolicy())) { diff --git a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java index 8e5b1f619..a7c1aa2cc 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java @@ -414,4 +414,27 @@ public class A { ) ); } + + @Test + void emptyTryWithResources() { + rewriteRun( + //language=java + java( + """ + import java.io.FileInputStream;import java.io.IOException;import java.io.UncheckedIOException;public class A { + private final InputStream stdin = new FileInputStream("test.in"); + private final InputStream stdout = new FileInputStream("test.out"); + + public void destroy() { + // close all streams in a try-with-resources + try (stdin; stdout) { + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + } + """ + ) + ); + } } From 764045ed588762a9478169302fb70263845e8081 Mon Sep 17 00:00:00 2001 From: joanvr Date: Thu, 24 Aug 2023 15:50:11 +0200 Subject: [PATCH 2/5] undo styling --- .../java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java index ede357c4f..4d59eca8d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java @@ -82,7 +82,7 @@ public J.Block visitBlock(J.Block block, P p) { J.Block nestedBlock = (J.Block) s; if (isEmptyBlock(nestedBlock) && ((Boolean.TRUE.equals(emptyBlockStyle.getStaticInit()) && nestedBlock.isStatic()) || - (Boolean.TRUE.equals(emptyBlockStyle.getInstanceInit()) && !nestedBlock.isStatic()))) { + (Boolean.TRUE.equals(emptyBlockStyle.getInstanceInit()) && !nestedBlock.isStatic()))) { filtered.set(true); return null; } From 05ebad240e6764bc30fa92134b44a468c19177ac Mon Sep 17 00:00:00 2001 From: joanvr Date: Thu, 24 Aug 2023 16:44:20 +0200 Subject: [PATCH 3/5] improved empty resources detection --- .../staticanalysis/EmptyBlockVisitor.java | 31 +++++++++-- .../staticanalysis/EmptyBlockTest.java | 51 ++++++++++++++++++- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java index 4d59eca8d..d904881be 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java @@ -18,16 +18,14 @@ import lombok.EqualsAndHashCode; import lombok.Value; import org.openrewrite.internal.ListUtils; +import org.openrewrite.internal.lang.Nullable; import org.openrewrite.java.DeleteStatement; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.format.ShiftFormat; import org.openrewrite.java.style.EmptyBlockStyle; -import org.openrewrite.java.tree.Expression; -import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.Space; -import org.openrewrite.java.tree.Statement; +import org.openrewrite.java.tree.*; import org.openrewrite.marker.Markers; import java.util.ArrayList; @@ -99,7 +97,7 @@ public J.Try visitTry(J.Try tryable, P p) { if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralTry()) && isEmptyBlock(t.getBody()) && - (t.getResources() == null || t.getResources().isEmpty())) { + isEmptyResources(t.getResources())) { doAfterVisit(new DeleteStatement<>(tryable)); } else if (Boolean.TRUE.equals(emptyBlockStyle.getLiteralFinally()) && t.getFinally() != null && !t.getCatches().isEmpty() && isEmptyBlock(t.getFinally())) { @@ -223,6 +221,29 @@ private boolean isEmptyBlock(Statement blockNode) { return false; } + private boolean isEmptyResources(@Nullable List resources) { + if (resources == null || resources.isEmpty()) { + return true; + } + // Searching for access to instances from outside the scope to detect potential side effects. + // If that's the case, we cannot remove this resources block. + for (J.Try.Resource resource : resources) { + // Any reference to an identifier used here comes from outside the scope. + if (resource.getVariableDeclarations() instanceof J.Identifier) { + return false; + } else if (resource.getVariableDeclarations() instanceof J.VariableDeclarations) { + J.VariableDeclarations variableDeclarations = (J.VariableDeclarations) resource.getVariableDeclarations(); + for (J.VariableDeclarations.NamedVariable variable : variableDeclarations.getVariables()) { + // If the variable is not initialized with a new instance, it means it can come from outside the scope. + if (!(variable.getInitializer() instanceof J.NewClass)) { + return false; + } + } + } + } + return true; + } + private static class ExtractSideEffectsOfIfCondition

extends JavaVisitor

{ private final J.Block enclosingBlock; private final J.If toExtract; diff --git a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java index a7c1aa2cc..630c04f71 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java @@ -416,7 +416,7 @@ public class A { } @Test - void emptyTryWithResources() { + void emptyTryWithResourcesWithExternalResources() { rewriteRun( //language=java java( @@ -437,4 +437,53 @@ public void destroy() { ) ); } + + @Test + void emptyTryWithResourcesWithVariableInitializationOfExternalResource() { + rewriteRun( + //language=java + java( + """ + import java.io.FileInputStream;import java.io.IOException;import java.io.InputStream;import java.io.UncheckedIOException;public class A { + private final InputStream stdin = new FileInputStream("test.in"); + + public void destroy() { + // close stream in a try-with-resources + try (InputStream s = stdin) { + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + } + """ + ) + ); + } + + @Test + void emptyTryWithResourcesWithVariableInitializationMethodInvocation() { + rewriteRun( + //language=java + java( + """ + import java.io.FileInputStream;import java.io.IOException;import java.io.InputStream;import java.io.UncheckedIOException;public class A { + private final InputStream stdin = new FileInputStream("test.in"); + + private InputStream getStdin() { + return stdin; + } + + public void destroy() { + // close stream in a try-with-resources + try (InputStream s = getStdin()) { + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + } + """ + ) + ); + } + } From abb01e0146d6f2cd234bb3f67168f71507eee872 Mon Sep 17 00:00:00 2001 From: joanvr Date: Thu, 24 Aug 2023 16:48:41 +0200 Subject: [PATCH 4/5] fixed imports --- .../org/openrewrite/staticanalysis/EmptyBlockVisitor.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java index d904881be..53d2d1275 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java @@ -25,7 +25,10 @@ import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.format.ShiftFormat; import org.openrewrite.java.style.EmptyBlockStyle; -import org.openrewrite.java.tree.*; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.Space; +import org.openrewrite.java.tree.Statement; import org.openrewrite.marker.Markers; import java.util.ArrayList; From 8d16fdd3da18d06ba072d17821c194b704dce8ef Mon Sep 17 00:00:00 2001 From: joanvr Date: Thu, 24 Aug 2023 17:33:17 +0200 Subject: [PATCH 5/5] simplified use case --- .../staticanalysis/EmptyBlockVisitor.java | 21 +----- .../staticanalysis/EmptyBlockTest.java | 65 ++----------------- 2 files changed, 8 insertions(+), 78 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java index 53d2d1275..c9218dc7d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java +++ b/src/main/java/org/openrewrite/staticanalysis/EmptyBlockVisitor.java @@ -225,26 +225,7 @@ private boolean isEmptyBlock(Statement blockNode) { } private boolean isEmptyResources(@Nullable List resources) { - if (resources == null || resources.isEmpty()) { - return true; - } - // Searching for access to instances from outside the scope to detect potential side effects. - // If that's the case, we cannot remove this resources block. - for (J.Try.Resource resource : resources) { - // Any reference to an identifier used here comes from outside the scope. - if (resource.getVariableDeclarations() instanceof J.Identifier) { - return false; - } else if (resource.getVariableDeclarations() instanceof J.VariableDeclarations) { - J.VariableDeclarations variableDeclarations = (J.VariableDeclarations) resource.getVariableDeclarations(); - for (J.VariableDeclarations.NamedVariable variable : variableDeclarations.getVariables()) { - // If the variable is not initialized with a new instance, it means it can come from outside the scope. - if (!(variable.getInitializer() instanceof J.NewClass)) { - return false; - } - } - } - } - return true; + return resources == null || resources.isEmpty(); } private static class ExtractSideEffectsOfIfCondition

extends JavaVisitor

{ diff --git a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java index 630c04f71..4d28214aa 100644 --- a/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/EmptyBlockTest.java @@ -116,7 +116,7 @@ void emptyTry() { public class A { { final String fileName = "fileName"; - try(FileInputStream fis = new FileInputStream(fileName)) { + try { } catch (IOException e) { } } @@ -416,68 +416,18 @@ public class A { } @Test - void emptyTryWithResourcesWithExternalResources() { + void emptyTryWithResources() { rewriteRun( //language=java java( """ - import java.io.FileInputStream;import java.io.IOException;import java.io.UncheckedIOException;public class A { - private final InputStream stdin = new FileInputStream("test.in"); - private final InputStream stdout = new FileInputStream("test.out"); - - public void destroy() { - // close all streams in a try-with-resources - try (stdin; stdout) { - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - } - """ - ) - ); - } - - @Test - void emptyTryWithResourcesWithVariableInitializationOfExternalResource() { - rewriteRun( - //language=java - java( - """ - import java.io.FileInputStream;import java.io.IOException;import java.io.InputStream;import java.io.UncheckedIOException;public class A { - private final InputStream stdin = new FileInputStream("test.in"); - - public void destroy() { - // close stream in a try-with-resources - try (InputStream s = stdin) { - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - } - """ - ) - ); - } + import java.io.*; - @Test - void emptyTryWithResourcesWithVariableInitializationMethodInvocation() { - rewriteRun( - //language=java - java( - """ - import java.io.FileInputStream;import java.io.IOException;import java.io.InputStream;import java.io.UncheckedIOException;public class A { - private final InputStream stdin = new FileInputStream("test.in"); - - private InputStream getStdin() { - return stdin; - } - - public void destroy() { - // close stream in a try-with-resources - try (InputStream s = getStdin()) { + public class A { + { + final String fileName = "fileName"; + try (FileInputStream fis = new FileInputStream(fileName)) { } catch (IOException e) { - throw new UncheckedIOException(e); } } } @@ -485,5 +435,4 @@ public void destroy() { ) ); } - }