From a241d951798380942bc0dbd1ae61a18a232a77cb Mon Sep 17 00:00:00 2001 From: Chaoren Lin Date: Fri, 4 Oct 2024 19:42:17 -0700 Subject: [PATCH] Update `MustBeClosed` to handle IO decorators. Tested: TAP for global presubmit queue [] PiperOrigin-RevId: 682534861 --- .../AbstractMustBeClosedChecker.java | 38 ++ .../bugpatterns/CloseableDecoratorTypes.java | 53 +++ .../bugpatterns/MustBeClosedCheckerTest.java | 353 ++++++------------ 3 files changed, 212 insertions(+), 232 deletions(-) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/CloseableDecoratorTypes.java diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java index d6ed6678c41..fc3c4585ee0 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java @@ -16,6 +16,7 @@ package com.google.errorprone.bugpatterns; +import static com.google.errorprone.bugpatterns.CloseableDecoratorTypes.CLOSEABLE_DECORATOR_TYPES; import static com.google.errorprone.fixes.SuggestedFixes.prettyType; import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; import static com.google.errorprone.matchers.Description.NO_MATCH; @@ -62,6 +63,7 @@ import com.sun.source.util.TreePath; import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; import com.sun.tools.javac.code.Type; import com.sun.tools.javac.util.Position; @@ -299,6 +301,17 @@ && isSameType(type.getReturnType(), streamType, state)) { } } break; + case NEW_CLASS: + NewClassTree newClassTree = (NewClassTree) path.getLeaf(); + if (isClosingDecorator(newClassTree, prev.getLeaf(), state)) { + if (HAS_MUST_BE_CLOSED_ANNOTATION.matches(newClassTree, state)) { + // if the decorator is also annotated then it would already be enforced + return Optional.empty(); + } + // otherwise, enforce that the decorator must be closed + continue OUTER; + } + break; case VARIABLE: Symbol sym = getSymbol(path.getLeaf()); if (sym instanceof VarSymbol) { @@ -520,4 +533,29 @@ private static Optional wrapTryFinallyAroundVariableScope( .closeBraceAfter(enclosingBlock) .wrapped(); } + + /** + * Returns true if {@code decorator} is the instantiation of an {@link AutoCloseable} decorator + * type that decorates the given {@code resource} and always closes the decorated {@code resource} + * when closed. + */ + private static boolean isClosingDecorator( + NewClassTree decorator, Tree resource, VisitorState state) { + if (decorator.getArguments().isEmpty() || !decorator.getArguments().get(0).equals(resource)) { + // we assume the decorated resource is always the first argument to the decorator constructor + return false; + } + MethodSymbol constructor = getSymbol(decorator); + if (!constructor.getThrownTypes().isEmpty()) { + // resource would not be closed if the decorator constructor throws + return false; + } + Type resourceType = constructor.params().get(0).type; + Type decoratorType = constructor.owner.type; + return CLOSEABLE_DECORATOR_TYPES.keySet().stream() + .filter(key -> isSameType(resourceType, state.getTypeFromString(key), state)) + .limit(1) + .flatMap(key -> CLOSEABLE_DECORATOR_TYPES.get(key).stream()) + .anyMatch(value -> isSubtype(decoratorType, state.getTypeFromString(value), state)); + } } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/CloseableDecoratorTypes.java b/core/src/main/java/com/google/errorprone/bugpatterns/CloseableDecoratorTypes.java new file mode 100644 index 00000000000..c88811ec5ce --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/CloseableDecoratorTypes.java @@ -0,0 +1,53 @@ +/* + * Copyright 2024 The Error Prone Authors. + * + * 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.google.errorprone.bugpatterns; + +import com.google.common.collect.ImmutableSetMultimap; +import java.io.FilterInputStream; +import java.io.FilterOutputStream; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Reader; +import java.io.Writer; + +/** + * Common {@link AutoCloseable} decorator types that wrap around {@link AutoCloseable} resources, + * which are always closed when the enclosing decorators are closed. + * + *

Each value in the map is the common supertype of some {@link AutoCloseable} decorator types, + * with the key being the common {@link AutoCloseable} type that they decorate. The decorated type + * is always assumed to be the first argument to the constructor of the decorator type. + */ +public final class CloseableDecoratorTypes { + public static final ImmutableSetMultimap CLOSEABLE_DECORATOR_TYPES = + ImmutableSetMultimap.builder() + .putAll( + InputStream.class.getName(), + FilterInputStream.class.getName(), // e.g., BufferedInputStream + InputStreamReader.class.getName()) + .putAll( + OutputStream.class.getName(), + FilterOutputStream.class.getName(), // e.g., BufferedOutputStream + OutputStreamWriter.class.getName()) + .put(Reader.class.getName(), Reader.class.getName()) // e.g., BufferedReader + .put(Writer.class.getName(), Writer.class.getName()) // e.g., BufferedWriter + .build(); + + private CloseableDecoratorTypes() {} +} diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/MustBeClosedCheckerTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/MustBeClosedCheckerTest.java index 741941b5e15..b1ccb228a28 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/MustBeClosedCheckerTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/MustBeClosedCheckerTest.java @@ -33,17 +33,20 @@ public class MustBeClosedCheckerTest { private final CompilationTestHelper compilationHelper = CompilationTestHelper.newInstance(MustBeClosedChecker.class, getClass()); - @Test - public void positiveCases() { - compilationHelper - .addSourceLines( - "MustBeClosedCheckerPositiveCases.java", - """ + private static final String POSITIVE_CASES = + """ package com.google.errorprone.bugpatterns.testdata; +import static java.io.OutputStream.nullOutputStream; + import com.google.errorprone.annotations.MustBeClosed; +import java.io.ByteArrayOutputStream; +import java.io.FilterOutputStream; +import java.io.IOException; +import java.io.OutputStream; import java.util.function.Supplier; import java.util.stream.Stream; +import java.util.zip.GZIPOutputStream; @SuppressWarnings({"UnusedNestedClass", "UnusedVariable"}) class MustBeClosedCheckerPositiveCases { @@ -260,24 +263,72 @@ abstract class ChildExplicitConstructor extends Parent { } } } -}""") + + @MustBeClosed + OutputStream mustBeClosedOutputStream() { + return nullOutputStream(); + } + + void decoratorConstructorThrows() throws IOException { + // BUG: Diagnostic contains: + try (var s = new GZIPOutputStream(mustBeClosedOutputStream())) {} + } + + void notClosedByDecorator() throws IOException { + class NotFilterOutputStream extends ByteArrayOutputStream { + NotFilterOutputStream(OutputStream out) {} + } + // BUG: Diagnostic contains: + try (var s = new NotFilterOutputStream(mustBeClosedOutputStream())) {} + } + + OutputStream decoratorMustBeClosed() { + class MustBeClosedFilter extends FilterOutputStream { + @MustBeClosed + MustBeClosedFilter(OutputStream out) { + super(out); + } + } + // BUG: Diagnostic contains: + return new MustBeClosedFilter( + // handled above + mustBeClosedOutputStream()); + } +} +"""; + + @Test + public void positiveCases() { + compilationHelper + .addSourceLines("MustBeClosedCheckerPositiveCases.java", POSITIVE_CASES) .doTest(); } @Test - public void negativeCase() { + public void negativeCases() { compilationHelper .addSourceLines( "MustBeClosedCheckerNegativeCases.java", """ package com.google.errorprone.bugpatterns.testdata; +import static java.io.InputStream.nullInputStream; +import static java.io.OutputStream.nullOutputStream; +import static java.io.Reader.nullReader; +import static java.io.Writer.nullWriter; import static org.junit.Assert.fail; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.when; import com.google.common.collect.ImmutableList; import com.google.errorprone.annotations.MustBeClosed; +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStreamWriter; @SuppressWarnings({"UnnecessaryCast", "LambdaToMemberReference"}) public class MustBeClosedCheckerNegativeCases { @@ -437,246 +488,49 @@ void inferredFunctionalExpressionReturningCloseable(ResourceFactory factory) { MustBeClosedAnnotatedConstructor::new) .forEach(this::consumeCloseable); } -}""") - .doTest(); - } - - @Test - public void refactoring() { - refactoringHelper - .addInputLines( - "MustBeClosedCheckerPositiveCases.java", - """ -package com.google.errorprone.bugpatterns.testdata; - -import com.google.errorprone.annotations.MustBeClosed; -import java.util.function.Supplier; -import java.util.stream.Stream; - -@SuppressWarnings({"UnusedNestedClass", "UnusedVariable"}) -class MustBeClosedCheckerPositiveCases { - - class DoesNotImplementAutoCloseable { - @MustBeClosed - // BUG: Diagnostic contains: MustBeClosed should only annotate constructors of AutoCloseables. - DoesNotImplementAutoCloseable() {} - - @MustBeClosed - // BUG: Diagnostic contains: MustBeClosed should only annotate methods that return an - // AutoCloseable. - void doesNotReturnAutoCloseable() {} - } - - class Closeable implements AutoCloseable { - - @Override - public void close() {} - - public int method() { - return 1; - } - } - - class Foo { - - @MustBeClosed - Closeable mustBeClosedAnnotatedMethod() { - return new Closeable(); - } - - void sameClass() { - // BUG: Diagnostic contains: - mustBeClosedAnnotatedMethod(); - } - } - - class MustBeClosedAnnotatedConstructor extends Closeable { - - @MustBeClosed - MustBeClosedAnnotatedConstructor() {} - - void sameClass() { - // BUG: Diagnostic contains: - new MustBeClosedAnnotatedConstructor(); - } - } - - void positiveCase1() { - // BUG: Diagnostic contains: - new Foo().mustBeClosedAnnotatedMethod(); - } - - void positiveCase2() { - // BUG: Diagnostic contains: - Closeable closeable = new Foo().mustBeClosedAnnotatedMethod(); - } - - void positiveCase3() { - try { - // BUG: Diagnostic contains: - new Foo().mustBeClosedAnnotatedMethod(); - } finally { - } - } - - void positiveCase4() { - try (Closeable c = new Foo().mustBeClosedAnnotatedMethod()) { - // BUG: Diagnostic contains: - new Foo().mustBeClosedAnnotatedMethod(); - } - } - - void positiveCase5() { - // BUG: Diagnostic contains: - new MustBeClosedAnnotatedConstructor(); - } - - Closeable positiveCase6() { - // BUG: Diagnostic contains: - return new MustBeClosedAnnotatedConstructor(); - } - - Closeable positiveCase7() { - // BUG: Diagnostic contains: - return new Foo().mustBeClosedAnnotatedMethod(); - } - - int existingDeclarationUsesVar() { - // BUG: Diagnostic contains: - var result = new Foo().mustBeClosedAnnotatedMethod(); - return 0; - } - - boolean twoCloseablesInOneExpression() { - // BUG: Diagnostic contains: - return new Foo().mustBeClosedAnnotatedMethod() == new Foo().mustBeClosedAnnotatedMethod(); - } - - void voidLambda() { - // Lambda has a fixless finding because no reasonable fix can be suggested. - // BUG: Diagnostic contains: - Runnable runnable = () -> new Foo().mustBeClosedAnnotatedMethod(); - } - - void expressionLambda() { - Supplier supplier = - () -> - // BUG: Diagnostic contains: - new Foo().mustBeClosedAnnotatedMethod(); - } - - void statementLambda() { - Supplier supplier = - () -> { - // BUG: Diagnostic contains: - return new Foo().mustBeClosedAnnotatedMethod(); - }; - } - - void methodReference() { - Supplier supplier = - // TODO(b/218377318): BUG: Diagnostic contains: - new Foo()::mustBeClosedAnnotatedMethod; - } - - void anonymousClass() { - new Foo() { - @Override - public Closeable mustBeClosedAnnotatedMethod() { - // BUG: Diagnostic contains: - return new MustBeClosedAnnotatedConstructor(); - } - }; - } - - void subexpression() { - // BUG: Diagnostic contains: - new Foo().mustBeClosedAnnotatedMethod().method(); - } - - void ternary(boolean condition) { - // BUG: Diagnostic contains: - int result = condition ? new Foo().mustBeClosedAnnotatedMethod().method() : 0; - } - - int variableDeclaration() { - // BUG: Diagnostic contains: - int result = new Foo().mustBeClosedAnnotatedMethod().method(); - return result; - } - void tryWithResources_nonFinal() { - Foo foo = new Foo(); - // BUG: Diagnostic contains: - Closeable closeable = foo.mustBeClosedAnnotatedMethod(); - try { - closeable = null; - } finally { - closeable.close(); - } - } - - void tryWithResources_noClose() { - Foo foo = new Foo(); - // BUG: Diagnostic contains: - Closeable closeable = foo.mustBeClosedAnnotatedMethod(); - try { - } finally { - } + @MustBeClosed + C mustBeClosed(C c) { + return c; } - class CloseableFoo implements AutoCloseable { + void closedByDecorator() throws IOException { + try (var in = new BufferedInputStream(mustBeClosed(nullInputStream()))) {} + try (var out = new BufferedOutputStream(mustBeClosed(nullOutputStream()))) {} - @MustBeClosed - CloseableFoo() {} + try (var in = new BufferedInputStream(mustBeClosed(nullInputStream()), 1024)) {} + try (var out = new BufferedOutputStream(mustBeClosed(nullOutputStream()), 1024)) {} - // Doesn't autoclose Foo on Stream close. - Stream stream() { - return null; - } + try (var r = new InputStreamReader(mustBeClosed(nullInputStream()))) {} + try (var w = new OutputStreamWriter(mustBeClosed(nullOutputStream()))) {} - @Override - public void close() {} + try (var r = new BufferedReader(mustBeClosed(nullReader()))) {} + try (var w = new BufferedWriter(mustBeClosed(nullWriter()))) {} } - - void twrStream() { - // BUG: Diagnostic contains: - try (Stream stream = new CloseableFoo().stream()) {} +} +""") + .doTest(); } - void constructorsTransitivelyRequiredAnnotation() { - abstract class Parent implements AutoCloseable { - @MustBeClosed - Parent() {} - - // BUG: Diagnostic contains: Invoked constructor is marked @MustBeClosed - Parent(int i) { - this(); - } - } - - // BUG: Diagnostic contains: Implicitly invoked constructor is marked @MustBeClosed - abstract class ChildDefaultConstructor extends Parent {} - - abstract class ChildExplicitConstructor extends Parent { - // BUG: Diagnostic contains: Invoked constructor is marked @MustBeClosed - ChildExplicitConstructor() {} - - // BUG: Diagnostic contains: Invoked constructor is marked @MustBeClosed - ChildExplicitConstructor(int a) { - super(); - } - } - } -}""") + @Test + public void refactoring() { + refactoringHelper + .addInputLines("MustBeClosedCheckerPositiveCases.java", POSITIVE_CASES) .addOutputLines( "MustBeClosedCheckerPositiveCases_expected.java", """ package com.google.errorprone.bugpatterns.testdata; +import static java.io.OutputStream.nullOutputStream; + import com.google.errorprone.annotations.MustBeClosed; +import java.io.ByteArrayOutputStream; +import java.io.FilterOutputStream; +import java.io.IOException; +import java.io.OutputStream; import java.util.function.Supplier; import java.util.stream.Stream; +import java.util.zip.GZIPOutputStream; @SuppressWarnings({"UnusedNestedClass", "UnusedVariable"}) class MustBeClosedCheckerPositiveCases { @@ -915,7 +769,42 @@ abstract class ChildExplicitConstructor extends Parent { } } } -}""") + + @MustBeClosed + OutputStream mustBeClosedOutputStream() { + return nullOutputStream(); + } + + void decoratorConstructorThrows() throws IOException { + // BUG: Diagnostic contains: + try (OutputStream outputStream = mustBeClosedOutputStream(); + var s = new GZIPOutputStream(outputStream)) {} + } + + void notClosedByDecorator() throws IOException { + class NotFilterOutputStream extends ByteArrayOutputStream { + NotFilterOutputStream(OutputStream out) {} + } + // BUG: Diagnostic contains: + try (OutputStream outputStream = mustBeClosedOutputStream(); + var s = new NotFilterOutputStream(outputStream)) {} + } + + @MustBeClosed + OutputStream decoratorMustBeClosed() { + class MustBeClosedFilter extends FilterOutputStream { + @MustBeClosed + MustBeClosedFilter(OutputStream out) { + super(out); + } + } + // BUG: Diagnostic contains: + return new MustBeClosedFilter( + // handled above + mustBeClosedOutputStream()); + } +} +""") .allowBreakingChanges() // The fix is best-effort, and some variable names may clash .doTest(); }