From 80654fafcc407b76b4facde346cba9726a5617f2 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Fri, 11 Aug 2023 16:13:42 -0700 Subject: [PATCH] Discourage closing `System.{out,err}` with try-with-resources PiperOrigin-RevId: 556128207 --- .../ClosingStandardOutputStreams.java | 63 ++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../ClosingStandardOutputStreamsTest.java | 65 +++++++++++++++++++ .../ClosingStandardOutputStreams.md | 52 +++++++++++++++ 4 files changed, 182 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreams.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreamsTest.java create mode 100644 docs/bugpattern/ClosingStandardOutputStreams.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreams.java b/core/src/main/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreams.java new file mode 100644 index 00000000000..20a4c0275fd --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreams.java @@ -0,0 +1,63 @@ +/* + * Copyright 2023 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 static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.FieldMatchers.staticField; +import static com.google.errorprone.matchers.Matchers.anyOf; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.TryTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.TryTree; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Don't use try-with-resources to manage standard output streams, closing the stream will" + + " cause subsequent output to standard output or standard error to be lost", + severity = WARNING) +public class ClosingStandardOutputStreams extends BugChecker implements TryTreeMatcher { + private static final Matcher MATCHER = + anyOf(staticField("java.lang.System", "err"), staticField("java.lang.System", "out")); + + @Override + public Description matchTry(TryTree tree, VisitorState state) { + new SuppressibleTreePathScanner(state) { + + @Override + public Void visitTry(TryTree tree, Void unused) { + tree.getResources().forEach(r -> r.accept(this, null)); + return null; + } + + @Override + public Void visitMemberSelect(MemberSelectTree tree, Void unused) { + if (MATCHER.matches(tree, state)) { + state.reportMatch(describeMatch(tree)); + } + return null; + } + }.scan(state.getPath(), null); + return NO_MATCH; + } +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 5efa078e05b..2b6c69e1f45 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -81,6 +81,7 @@ import com.google.errorprone.bugpatterns.ClassName; import com.google.errorprone.bugpatterns.ClassNamedLikeTypeParameter; import com.google.errorprone.bugpatterns.ClassNewInstance; +import com.google.errorprone.bugpatterns.ClosingStandardOutputStreams; import com.google.errorprone.bugpatterns.CollectionToArraySafeParameter; import com.google.errorprone.bugpatterns.CollectorShouldNotUseState; import com.google.errorprone.bugpatterns.ComparableAndComparator; @@ -851,6 +852,7 @@ public static ScannerSupplier warningChecks() { ClassCanBeStatic.class, ClassNewInstance.class, CloseableProvides.class, + ClosingStandardOutputStreams.class, CollectionUndefinedEquality.class, CollectorShouldNotUseState.class, ComparableAndComparator.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreamsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreamsTest.java new file mode 100644 index 00000000000..94f3636fe7b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/ClosingStandardOutputStreamsTest.java @@ -0,0 +1,65 @@ +/* + * Copyright 2023 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.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class ClosingStandardOutputStreamsTest { + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(ClosingStandardOutputStreams.class, getClass()); + + @Test + public void positive() { + testHelper + .addSourceLines( + "Test.java", + "import java.io.OutputStreamWriter;", + "import java.io.PrintWriter;", + "class Test {", + " void f() {", + " // BUG: Diagnostic contains:", + " try (PrintWriter pw = new PrintWriter(new OutputStreamWriter(System.err), true))" + + " {", + " pw.println(\"hello\");", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "import java.io.OutputStreamWriter;", + "import java.io.PrintWriter;", + "class Test {", + " void f(OutputStreamWriter os) {", + " System.err.println(42);", + " try (PrintWriter pw = new PrintWriter(os, true)) {", + " pw.println(\"hello\");", + " }", + " }", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/ClosingStandardOutputStreams.md b/docs/bugpattern/ClosingStandardOutputStreams.md new file mode 100644 index 00000000000..46200e2d61a --- /dev/null +++ b/docs/bugpattern/ClosingStandardOutputStreams.md @@ -0,0 +1,52 @@ +Closing the standard output streams `System.out` or `System.err` will cause all +subsequent standard output to be dropped, including stack traces from exceptions +that propagate to the top level. + +Avoid using try-with-resources to manage `PrintWriter`s or `OutputStream`s that +wrap `System.out` or `System.err`, since the try-with-resource statemnet will +close the underlying streams. + +That is, prefer this: + +``` {.good} +PrintWriter pw = new PrintWriter(new OutputStreamWriter(System.err)); +pw.println("hello"); +pw.flush(); +``` + +Instead of this: + +``` {.bad} +try (PrintWriter pw = new PrintWriter(new OutputStreamWriter(System.err))) { + pw.println("hello"); +} +``` + +Consider the following example: + +``` +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import static java.nio.charset.StandardCharsets.UTF_8; + +public class X { + public static void main(String[] args) { + System.err.println("one"); + try (PrintWriter err = new PrintWriter(new OutputStreamWriter(System.err, UTF_8))) { + err.print("two"); + } + // System.err has been closed, no more output will be printed! + System.err.println("three"); + throw new AssertionError(); + } +} +``` + +The program will print the following, and return with exit code 1. Note that the +last `println` doesn't produce any output, and the exception's stack trace is +not printed: + +``` +one +two +```