From 857ed0eb28b830e6da68aca6384e6952c683aa2f Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 5 Mar 2021 16:28:42 -0500 Subject: [PATCH 1/4] Add AutoCloseableMustBeClosed suggested bug checker If a constructor or method returns an AutoCloseable, it should be annotated @MustBeClosed to ensure callers appropriately close resources in concert with the MustBeClosedChecker. See https://errorprone.info/bugpattern/MustBeClosedChecker --- .../errorprone/AutoCloseableMustBeClosed.java | 77 +++++++ .../AutoCloseableMustBeClosedTest.java | 200 ++++++++++++++++++ 2 files changed, 277 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java new file mode 100644 index 000000000..396a07a98 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java @@ -0,0 +1,77 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.sun.source.tree.MethodTree; + +@AutoService(BugChecker.class) +@BugPattern( + name = "AutoCloseableMustBeClosed", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = SeverityLevel.SUGGESTION, + summary = AutoCloseableMustBeClosed.SUMMARY) +public final class AutoCloseableMustBeClosed extends BugChecker implements MethodTreeMatcher { + + static final String SUMMARY = "If a constructor or method returns an AutoCloseable, " + + "it should be annotated @MustBeClosed to ensure callers appropriately close resources"; + private static final String MUST_BE_CLOSED_TYPE = "com.google.errorprone.annotations.MustBeClosed"; + private static final String CAN_IGNORE_RETURN_VALUE_TYPE = "com.google.errorprone.annotations.CanIgnoreReturnValue"; + + private static final Matcher methodReturnsAutoCloseable = Matchers.allOf( + Matchers.not(Matchers.methodIsConstructor()), + Matchers.methodReturns(Matchers.isSubtypeOf(AutoCloseable.class))); + + private static final Matcher constructsAutoCloseable = Matchers.allOf( + Matchers.methodIsConstructor(), Matchers.enclosingClass(Matchers.isSubtypeOf(AutoCloseable.class))); + + private static final Matcher methodNotAnnotatedMustBeClosed = + Matchers.not(Matchers.hasAnnotation(MUST_BE_CLOSED_TYPE)); + + private static final Matcher methodNotAnnotatedIgnoreReturnValue = + Matchers.not(Matchers.hasAnnotation(CAN_IGNORE_RETURN_VALUE_TYPE)); + + private static final Matcher methodShouldBeAnnotatedMustBeClosed = Matchers.allOf( + Matchers.anyOf(methodReturnsAutoCloseable, constructsAutoCloseable), + methodNotAnnotatedMustBeClosed, + methodNotAnnotatedIgnoreReturnValue); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (methodShouldBeAnnotatedMustBeClosed.matches(tree, state)) { + SuggestedFix.Builder builder = SuggestedFix.builder().addImport(MUST_BE_CLOSED_TYPE); + String annotation = SuggestedFixes.qualifyType(state, builder, MUST_BE_CLOSED_TYPE); + return buildDescription(tree) + .setMessage(SUMMARY) + .addFix(builder.prefixWith(tree, "@" + annotation + " ").build()) + .build(); + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java new file mode 100644 index 000000000..e8443c566 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java @@ -0,0 +1,200 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public final class AutoCloseableMustBeClosedTest { + + private CompilationTestHelper compilationHelper; + private RefactoringValidator refactoringTestHelper; + + @BeforeEach + public void before() { + compilationHelper = CompilationTestHelper.newInstance(AutoCloseableMustBeClosed.class, getClass()); + refactoringTestHelper = RefactoringValidator.of(new AutoCloseableMustBeClosed(), getClass()); + } + + @Test + public void testAutoCloseableReturn() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.io.*;", + "class Test {", + " // BUG: Diagnostic contains: should be annotated @MustBeClosed", + " private AutoCloseable autoCloseable() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "}") + .doTest(); + } + + @Test + public void testInputStream() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.io.*;", + "class Test {", + " // BUG: Diagnostic contains: should be annotated @MustBeClosed", + " private InputStream inputStream() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "}") + .doTest(); + } + + @Test + public void testOutputStream() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.io.*;", + "class Test {", + " // BUG: Diagnostic contains: should be annotated @MustBeClosed", + " private OutputStream outputStream() {", + " return new ByteArrayOutputStream();", + " }", + "}") + .doTest(); + } + + @Test + public void testConstructor() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.io.*;", + "class Test extends FilterOutputStream {", + " // BUG: Diagnostic contains: should be annotated @MustBeClosed", + " public Test() {", + " super(new ByteArrayOutputStream());", + " }", + "}") + .doTest(); + } + + @Test + public void testAlreadyAnnotated() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.*;", + "import java.io.*;", + "class Test {", + " @MustBeClosed", + " private Writer alreadyAnnotated() {", + " return new StringWriter();", + " }", + "}") + .doTest(); + } + + @Test + public void testCanIgnoreReturnValue() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.errorprone.annotations.*;", + "import java.io.*;", + "class Test {", + " @CanIgnoreReturnValue", + " private Closeable ignoreReturn() {", + " return new StringReader(\"\");", + " }", + "}") + .doTest(); + } + + @Test + public void testSuggestedFixAddMustBeClosed() { + refactoringTestHelper + .addInputLines( + "Test.java", + "import com.google.errorprone.annotations.CanIgnoreReturnValue;", + "import java.io.*;", + "class Test extends FilterOutputStream {", + "", + " public Test() {", + " super(new ByteArrayOutputStream());", + " }", + "", + " public static Test create() {", + " return new Test();", + " }", + "", + " private AutoCloseable autoCloseable() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "", + " private InputStream inputStream() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "", + " private OutputStream outputStream() {", + " return new ByteArrayOutputStream();", + " }", + "", + " @CanIgnoreReturnValue", + " private Closeable ignoreReturn() {", + " return new StringReader(\"\");", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.google.errorprone.annotations.CanIgnoreReturnValue;", + "import com.google.errorprone.annotations.MustBeClosed;", + "import java.io.*;", + "class Test extends FilterOutputStream {", + "", + " @MustBeClosed", + " public Test() {", + " super(new ByteArrayOutputStream());", + " }", + "", + " @MustBeClosed", + " public static Test create() {", + " return new Test();", + " }", + "", + " @MustBeClosed", + " private AutoCloseable autoCloseable() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "", + " @MustBeClosed", + " private InputStream inputStream() {", + " return new ByteArrayInputStream(new byte[0]);", + " }", + "", + " @MustBeClosed", + " private OutputStream outputStream() {", + " return new ByteArrayOutputStream();", + " }", + "", + " @CanIgnoreReturnValue", + " private Closeable ignoreReturn() {", + " return new StringReader(\"\");", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } +} From 80ced69fbc7c3b1f2a8e70a59e8eb0bf12732b0b Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 5 Mar 2021 16:57:39 -0500 Subject: [PATCH 2/4] AutoCloseableMustBeClosed ignores Streams --- .../errorprone/AutoCloseableMustBeClosed.java | 10 ++++++++-- .../AutoCloseableMustBeClosedTest.java | 18 ++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java index 396a07a98..8fb3c9daa 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java @@ -28,6 +28,7 @@ import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; import com.sun.source.tree.MethodTree; +import java.util.stream.BaseStream; @AutoService(BugChecker.class) @BugPattern( @@ -46,10 +47,15 @@ public final class AutoCloseableMustBeClosed extends BugChecker implements Metho private static final Matcher methodReturnsAutoCloseable = Matchers.allOf( Matchers.not(Matchers.methodIsConstructor()), - Matchers.methodReturns(Matchers.isSubtypeOf(AutoCloseable.class))); + Matchers.methodReturns(Matchers.isSubtypeOf(AutoCloseable.class)), + // ignore Stream for now, see https://errorprone.info/bugpattern/StreamResourceLeak + Matchers.not(Matchers.methodReturns(Matchers.isSubtypeOf(BaseStream.class)))); private static final Matcher constructsAutoCloseable = Matchers.allOf( - Matchers.methodIsConstructor(), Matchers.enclosingClass(Matchers.isSubtypeOf(AutoCloseable.class))); + Matchers.methodIsConstructor(), + Matchers.enclosingClass(Matchers.isSubtypeOf(AutoCloseable.class)), + // ignore Stream for now, see https://errorprone.info/bugpattern/StreamResourceLeak + Matchers.not(Matchers.enclosingClass(Matchers.isSubtypeOf(BaseStream.class)))); private static final Matcher methodNotAnnotatedMustBeClosed = Matchers.not(Matchers.hasAnnotation(MUST_BE_CLOSED_TYPE)); diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java index e8443c566..fef3ecae9 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosedTest.java @@ -92,6 +92,24 @@ public void testConstructor() { .doTest(); } + @Test + public void testIgnoreStream() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.stream.*;", + "class Test {", + " private Stream a() {", + " return Stream.of(1);", + " }", + "", + " private IntStream b() {", + " return IntStream.of(1);", + " }", + "}") + .doTest(); + } + @Test public void testAlreadyAnnotated() { compilationHelper From 814633bf3cfa415cd78e7e7178d4c673d905cba2 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 5 Mar 2021 17:47:05 -0500 Subject: [PATCH 3/4] address review comments --- .../baseline/errorprone/AutoCloseableMustBeClosed.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java index 8fb3c9daa..b4d161ac5 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/AutoCloseableMustBeClosed.java @@ -37,11 +37,10 @@ linkType = BugPattern.LinkType.CUSTOM, providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, severity = SeverityLevel.SUGGESTION, - summary = AutoCloseableMustBeClosed.SUMMARY) + summary = "If a constructor or method returns an AutoCloseable, it should be annotated " + + "@MustBeClosed to ensure callers appropriately close resources") public final class AutoCloseableMustBeClosed extends BugChecker implements MethodTreeMatcher { - static final String SUMMARY = "If a constructor or method returns an AutoCloseable, " - + "it should be annotated @MustBeClosed to ensure callers appropriately close resources"; private static final String MUST_BE_CLOSED_TYPE = "com.google.errorprone.annotations.MustBeClosed"; private static final String CAN_IGNORE_RETURN_VALUE_TYPE = "com.google.errorprone.annotations.CanIgnoreReturnValue"; @@ -71,10 +70,9 @@ public final class AutoCloseableMustBeClosed extends BugChecker implements Metho @Override public Description matchMethod(MethodTree tree, VisitorState state) { if (methodShouldBeAnnotatedMustBeClosed.matches(tree, state)) { - SuggestedFix.Builder builder = SuggestedFix.builder().addImport(MUST_BE_CLOSED_TYPE); + SuggestedFix.Builder builder = SuggestedFix.builder(); String annotation = SuggestedFixes.qualifyType(state, builder, MUST_BE_CLOSED_TYPE); return buildDescription(tree) - .setMessage(SUMMARY) .addFix(builder.prefixWith(tree, "@" + annotation + " ").build()) .build(); } From 27ecd0042cd2491fa906d331be327223be9651f7 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Fri, 5 Mar 2021 22:47:05 +0000 Subject: [PATCH 4/4] Add generated changelog entries --- changelog/@unreleased/pr-1673.v2.yml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/@unreleased/pr-1673.v2.yml diff --git a/changelog/@unreleased/pr-1673.v2.yml b/changelog/@unreleased/pr-1673.v2.yml new file mode 100644 index 000000000..df9287bea --- /dev/null +++ b/changelog/@unreleased/pr-1673.v2.yml @@ -0,0 +1,8 @@ +type: improvement +improvement: + description: |- + A new suggested error-prone rule `AutoCloseableMustBeClosed` annotates methods and constructors that return an `AutoCloseable` type as `@MustBeClosed` to allow for `MustBeClosedChecker` to perform analysis that resources are appropriately closed. + + See https://errorprone.info/bugpattern/MustBeClosedChecker + links: + - https://github.com/palantir/gradle-baseline/pull/1673