AutoCloseableMustBeClosed doesn't match method overrides (#1685)

AutoCloseableMustBeClosed doesn't match method overrides
palantir · Mar 9, 2021 · f898554 · f898554
1 parent 53c9967
commit f898554
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 3 deletions.
diff --git a/...error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java b/...error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java
@@ -27,7 +27,9 @@
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.matchers.Matchers;
+import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.MethodTree;
+import com.sun.tools.javac.code.Symbol.MethodSymbol;
 import java.util.stream.BaseStream;
 
 @AutoService(BugChecker.class)
@@ -48,7 +50,15 @@ public final class AutoCloseableMustBeClosed extends BugChecker implements Metho
             Matchers.not(Matchers.methodIsConstructor()),
             Matchers.methodReturns(Matchers.isSubtypeOf(AutoCloseable.class)),
             // ignore Stream for now, see https://errorprone.info/bugpattern/StreamResourceLeak
-            Matchers.not(Matchers.methodReturns(Matchers.isSubtypeOf(BaseStream.class))));
+            Matchers.not(Matchers.methodReturns(Matchers.isSubtypeOf(BaseStream.class))),
+            // Avoid noise when methods override base interfaces that aren't annotated. In most cases
+            // the supertype reference is used, so annotations are less helpful on overrides.
+            // The override must be annotated MustBeClosed if the supertype is annotated.
+            Matchers.anyOf(
+                    // It might be worthwhile to make an exception for generic types, for example
+                    // Supplier<InputStream> subtypes should definitely be annotated.
+                    Matchers.not(OverrideMethod.INSTANCE),
+                    Matchers.hasAnnotationOnAnyOverriddenMethod(MUST_BE_CLOSED_TYPE)));
 
     private static final Matcher<MethodTree> constructsAutoCloseable = Matchers.allOf(
             Matchers.methodIsConstructor(),
@@ -63,9 +73,9 @@ public final class AutoCloseableMustBeClosed extends BugChecker implements Metho
             Matchers.not(Matchers.hasAnnotation(CAN_IGNORE_RETURN_VALUE_TYPE));
 
     private static final Matcher<MethodTree> methodShouldBeAnnotatedMustBeClosed = Matchers.allOf(
-            Matchers.anyOf(methodReturnsAutoCloseable, constructsAutoCloseable),
             methodNotAnnotatedMustBeClosed,
-            methodNotAnnotatedIgnoreReturnValue);
+            methodNotAnnotatedIgnoreReturnValue,
+            Matchers.anyOf(methodReturnsAutoCloseable, constructsAutoCloseable));
 
     @Override
     public Description matchMethod(MethodTree tree, VisitorState state) {
@@ -78,4 +88,17 @@ public Description matchMethod(MethodTree tree, VisitorState state) {
         }
         return Description.NO_MATCH;
     }
+
+    private enum OverrideMethod implements Matcher<MethodTree> {
+        INSTANCE;
+
+        @Override
+        public boolean matches(MethodTree tree, VisitorState state) {
+            MethodSymbol methodSym = ASTHelpers.getSymbol(tree);
+            if (methodSym == null) {
+                return false;
+            }
+            return !ASTHelpers.findSuperMethods(methodSym, state.getTypes()).isEmpty();
+        }
+    }
 }
diff --git a/...r-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java b/...r-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java
@@ -126,6 +126,22 @@ public void testAlreadyAnnotated() {
                 .doTest();
     }
 
+    @Test
+    public void testIgnoreOverride() {
+        compilationHelper
+                .addSourceLines(
+                        "Test.java",
+                        "import java.io.IOException;",
+                        "import java.net.Socket;",
+                        "import javax.net.ssl.SSLSocketFactory;",
+                        "abstract class Test extends SSLSocketFactory {",
+                        "    public Socket createSocket() throws IOException {",
+                        "        return new Socket();",
+                        "    }",
+                        "}")
+                .doTest();
+    }
+
     @Test
     public void testCanIgnoreReturnValue() {
         compilationHelper
@@ -215,4 +231,43 @@ public void testSuggestedFixAddMustBeClosed() {
                         "}")
                 .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
     }
+
+    @Test
+    public void testFixOverridesAnnotatedMethod() {
+        refactoringTestHelper
+                .addInputLines(
+                        "Test.java",
+                        "import com.google.errorprone.annotations.MustBeClosed;",
+                        "import java.io.*;",
+                        "class Test {",
+                        "  interface Iface {",
+                        "    @MustBeClosed",
+                        "    InputStream stream() throws IOException;",
+                        "  }",
+                        "  static class Impl implements Iface {",
+                        "    @Override",
+                        "    public InputStream stream() throws IOException {",
+                        "      return new FileInputStream(new File(\"test\"));",
+                        "    }",
+                        "  }",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.google.errorprone.annotations.MustBeClosed;",
+                        "import java.io.*;",
+                        "class Test {",
+                        "  interface Iface {",
+                        "    @MustBeClosed",
+                        "    InputStream stream() throws IOException;",
+                        "  }",
+                        "  static class Impl implements Iface {",
+                        "    @MustBeClosed",
+                        "    @Override",
+                        "    public InputStream stream() throws IOException {",
+                        "      return new FileInputStream(new File(\"test\"));",
+                        "    }",
+                        "  }",
+                        "}")
+                .doTest();
+    }
 }
diff --git a/changelog/@unreleased/pr-1685.v2.yml b/changelog/@unreleased/pr-1685.v2.yml
@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: AutoCloseableMustBeClosed doesn't match method overrides
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1685