UnnecessaryCloseInTryWithResources: only remove a trailing close of a Closeable

src/main/java/org/openrewrite/staticanalysis/UnnecessaryCloseInTryWithResources.java

@@ -23,13 +23,18 @@
 import org.openrewrite.internal.ListUtils;
 import org.openrewrite.java.JavaIsoVisitor;
 import org.openrewrite.java.MethodMatcher;
+import org.openrewrite.java.search.SemanticallyEqual;
 import org.openrewrite.java.tree.J;
+import org.openrewrite.java.tree.Space;
+import org.openrewrite.java.tree.Statement;
 import org.openrewrite.staticanalysis.groovy.GroovyFileChecker;
 import org.openrewrite.staticanalysis.java.JavaFileChecker;
 import org.openrewrite.staticanalysis.kotlin.KotlinFileChecker;
 
 import java.time.Duration;
+import java.util.List;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import static java.util.Collections.singleton;
 
@@ -38,10 +43,10 @@ public class UnnecessaryCloseInTryWithResources extends Recipe {
     final String displayName = "Unnecessary close in try-with-resources";
 
     @Getter
-    final String description = "Remove unnecessary `AutoCloseable#close()` statements in " +
-            "try-with-resources. Try-with-resources already guarantees that each " +
-            "declared resource is closed when the block exits, so an explicit " +
-            "`close()` call is redundant and can be confusing.";
+    final String description = "Remove `close()` calls at the end of a try-with-resources block that close the last " +
+            "declared resource, when that resource is a `java.io.Closeable`, whose `close()` has no effect once the " +
+            "resource is already closed. A `close()` in any other position, or on any other resource, is left in " +
+            "place, because removing it would change when, or in what order, resources are closed.";
 
     @Getter
     final Duration estimatedEffortPerOccurrence = Duration.ofMinutes(2);
@@ -62,40 +67,60 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
     }
 
     private static class UnnecessaryAutoCloseableVisitor extends JavaIsoVisitor<ExecutionContext> {
-        private static final MethodMatcher AUTO_CLOSEABLE_METHOD_MATCHER = new MethodMatcher("java.lang.AutoCloseable close()", true);
+        // Closeable requires close() to be idempotent; AutoCloseable does not make that guarantee.
+        private static final MethodMatcher CLOSEABLE_CLOSE_METHOD_MATCHER = new MethodMatcher("java.io.Closeable close()", true);
 
         @Override
         public J.Try visitTry(J.Try aTry, ExecutionContext ctx) {
             J.Try tr = super.visitTry(aTry, ctx);
-            if (tr.getResources() != null) {
-                String[] resourceNames = new String[tr.getResources().size()];
-                for (int i = 0; i < tr.getResources().size(); i++) {
-                    J.Try.Resource tryResource = tr.getResources().get(i);
-                    if (tryResource.getVariableDeclarations() instanceof J.VariableDeclarations) {
-                        J.VariableDeclarations varDecls = (J.VariableDeclarations) tryResource.getVariableDeclarations();
-                        resourceNames[i] = varDecls.getVariables().get(0).getSimpleName();
-                    } else if (tryResource.getVariableDeclarations() instanceof J.Identifier) {
-                        J.Identifier identifier = (J.Identifier) tryResource.getVariableDeclarations();
-                        resourceNames[i] = identifier.getSimpleName();
-                    }
+            if (tr.getResources() == null || tr.getResources().isEmpty()) {
+                return tr;
+            }
+
+            // Resources close in reverse order of declaration, so dropping any but the last would reorder closes
+            J lastResource = tr.getResources().get(tr.getResources().size() - 1).getVariableDeclarations();
+            J.Identifier lastResourceName;
+            if (lastResource instanceof J.VariableDeclarations) {
+                lastResourceName = ((J.VariableDeclarations) lastResource).getVariables().get(0).getName();
+            } else if (lastResource instanceof J.Identifier) {
+                lastResourceName = (J.Identifier) lastResource;
+            } else {
+                return tr;
+            }
+
+            J.Block body = tr.getBody();
+            if (!body.getEnd().getComments().isEmpty()) {
+                return tr;
+            }
+
+            // Anything after an explicit close can observe the closed resource, so only trailing closes are redundant
+            List<Statement> statements = body.getStatements();
+            int keep = statements.size();
+            while (keep > 0 && statements.get(keep - 1) instanceof J.MethodInvocation) {
+                J.MethodInvocation mi = (J.MethodInvocation) statements.get(keep - 1);
+                if (!CLOSEABLE_CLOSE_METHOD_MATCHER.matches(mi) ||
+                        !(mi.getSelect() instanceof J.Identifier) ||
+                        !SemanticallyEqual.areEqual(lastResourceName, mi.getSelect()) ||
+                        containsComment(mi)) {
+                    break;
                 }
+                keep--;
+            }
+
+            int firstRemoved = keep;
+            return tr.withBody(body.withStatements(ListUtils.map(statements, (i, statement) -> i < firstRemoved ? statement : null)));
+        }
 
-                tr = tr.withBody(tr.getBody().withStatements(ListUtils.map(tr.getBody().getStatements(), statement -> {
-                    if (statement instanceof J.MethodInvocation) {
-                        J.MethodInvocation mi = (J.MethodInvocation) statement;
-                        if (AUTO_CLOSEABLE_METHOD_MATCHER.matches(mi) && mi.getSelect() instanceof J.Identifier) {
-                            String selectName = ((J.Identifier) mi.getSelect()).getSimpleName();
-                            for (String resourceName : resourceNames) {
-                                if (resourceName.equals(selectName)) {
-                                    return null;
-                                }
-                            }
-                        }
+        private static boolean containsComment(J tree) {
+            return new JavaIsoVisitor<AtomicBoolean>() {
+                @Override
+                public Space visitSpace(Space space, Space.Location loc, AtomicBoolean found) {
+                    if (!space.getComments().isEmpty()) {
+                        found.set(true);
                     }
-                    return statement;
-                })));
-            }
-            return tr;
+                    return space;
+                }
+            }.reduce(tree, new AtomicBoolean()).get();
         }
     }
 }

src/main/resources/META-INF/rewrite/recipes.csv

@@ -188,7 +188,7 @@ maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanaly
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.URLEqualsHashCodeRecipes$URLHashCodeRecipe,URL Hash Code,"Uses of `hashCode()` cause `java.net.URL` to make blocking internet connections. Instead, use `java.net.URI`.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.URLEqualsHashCodeRecipes,URL Equals and Hash Code,"Uses of `equals()` and `hashCode()` cause `java.net.URL` to make blocking internet connections. Instead, use `java.net.URI`.",3,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryCatch,Remove catch for a checked exception if the try block does not throw that exception,A refactoring operation may result in a checked exception that is no longer thrown from a `try` block. This recipe will find and remove unnecessary catch blocks.,1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,"[{""name"":""includeJavaLangException"",""type"":""boolean"",""displayName"":""Include `java.lang.Exception`"",""description"":""Whether to include `java.lang.Exception` in the list of checked exceptions to remove. Unlike other checked exceptions, `java.lang.Exception` is also the superclass of unchecked exceptions. So removing `catch(Exception e)` may result in changed runtime behavior in the presence of unchecked exceptions. Default `false`"",""value"":false},{""name"":""includeJavaLangThrowable"",""type"":""boolean"",""displayName"":""Include `java.lang.Throwable`"",""description"":""Whether to include `java.lang.Throwable` in the list of exceptions to remove. Unlike other checked exceptions, `java.lang.Throwable` is also the superclass of unchecked exceptions. So removing `catch(Throwable e)` may result in changed runtime behavior in the presence of unchecked exceptions. Default `false`"",""value"":false}]",
-maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryCloseInTryWithResources,Unnecessary close in try-with-resources,"Remove unnecessary `AutoCloseable#close()` statements in try-with-resources. Try-with-resources already guarantees that each declared resource is closed when the block exits, so an explicit `close()` call is redundant and can be confusing.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
+maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryCloseInTryWithResources,Unnecessary close in try-with-resources,"Remove `close()` calls at the end of a try-with-resources block that close the last declared resource, when that resource is a `java.io.Closeable`, whose `close()` has no effect once the resource is already closed. A `close()` in any other position, or on any other resource, is left in place, because removing it would change when, or in what order, resources are closed.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryExplicitTypeArguments,Unnecessary explicit type arguments,"When explicit type arguments are inferable by the compiler, they may be removed.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryParentheses,Remove unnecessary parentheses,"Removes unnecessary parentheses from code where extra parentheses pairs are redundant. Redundant parentheses add visual noise and can obscure the actual structure of an expression, making code harder to read at a glance.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,
 maven,org.openrewrite.recipe:rewrite-static-analysis,org.openrewrite.staticanalysis.UnnecessaryPrimitiveAnnotations,Remove `@Nullable` and `@CheckForNull` annotations from primitives,"Primitives can't be null anyway, so these annotations are not useful in this context. Leaving them in place gives the false impression that a null value is possible, which can confuse readers and static analysis tools alike.",1,,Static analysis and remediation,,Remediations for issues identified by SAST tools.,,

src/test/java/org/openrewrite/staticanalysis/UnnecessaryCloseInTryWithResourcesTest.java

@@ -84,6 +84,71 @@ public void doSomething() {
         );
     }
 
+    @Test
+    void removeCloseOfReferencedResource() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething(Scanner scanner) {
+                      try (scanner) {
+                          boolean hasNext = scanner.hasNext();
+                          scanner.close();
+                      }
+                  }
+              }
+              """,
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething(Scanner scanner) {
+                      try (scanner) {
+                          boolean hasNext = scanner.hasNext();
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void removeRepeatedTrailingClose() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (Scanner scanner = new Scanner("abc")) {
+                          boolean hasNext = scanner.hasNext();
+                          scanner.close();
+                          scanner.close();
+                      }
+                  }
+              }
+              """,
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (Scanner scanner = new Scanner("abc")) {
+                          boolean hasNext = scanner.hasNext();
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
     @Test
     void onlyRemoveAutoCloseableClose() {
         rewriteRun(
@@ -119,4 +184,158 @@ public void doSomething() {
           )
         );
     }
+
+    @Test
+    void doNotRemoveCloseWhenResourceStateIsObservedAfterwards() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.io.Closeable;
+
+              class Test {
+                  static class Resource implements Closeable {
+                      boolean closed;
+
+                      @Override
+                      public void close() {
+                          closed = true;
+                      }
+                  }
+
+                  void declaration() {
+                      try (Resource resource = new Resource()) {
+                          resource.close();
+                          if (!resource.closed) {
+                              throw new AssertionError();
+                          }
+                      }
+                  }
+
+                  void reference(Resource resource) {
+                      try (resource) {
+                          resource.close();
+                          if (!resource.closed) {
+                              throw new AssertionError();
+                          }
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void doNotRemoveCloseFollowedByOtherStatements() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (Scanner scanner = new Scanner("abc")) {
+                          scanner.close();
+                          System.out.println("scanner released early");
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void doNotRemoveCloseOfResourceThatIsNotClosedFirst() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.io.FileWriter;
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (FileWriter fileWriter = new FileWriter("test"); Scanner scanner = new Scanner("abc")) {
+                          fileWriter.write(scanner.next());
+                          fileWriter.close();
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void doNotRemoveCloseOfResourceThatIsNotCloseable() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              class A {
+                  static class Latch implements AutoCloseable {
+                      int closeCount;
+
+                      @Override
+                      public void close() {
+                          closeCount++;
+                      }
+                  }
+
+                  public void doSomething() {
+                      try (Latch latch = new Latch()) {
+                          latch.close();
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void doNotRemoveCloseWithComment() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (Scanner scanner = new Scanner("abc")) {
+                          boolean hasNext = scanner.hasNext();
+                          // release the scanner before the block exits
+                          scanner.close();
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
+
+    @Test
+    void doNotRemoveCloseWithCommentInsideTheCall() {
+        rewriteRun(
+          //language=java
+          java(
+            """
+              import java.util.Scanner;
+
+              class A {
+                  public void doSomething() {
+                      try (Scanner scanner = new Scanner("abc")) {
+                          boolean hasNext = scanner.hasNext();
+                          scanner./* release early */close(/* no arguments */);
+                      }
+                  }
+              }
+              """
+          )
+        );
+    }
 }