Discourage closing System.{out,err} with try-with-resources

PiperOrigin-RevId: 556928543

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<ExpressionTree> MATCHER =
+      anyOf(staticField("java.lang.System", "err"), staticField("java.lang.System", "out"));
+
+  @Override
+  public Description matchTry(TryTree tree, VisitorState state) {
+    new SuppressibleTreePathScanner<Void, Void>(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;
+  }
+}

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,

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();
+  }
+}

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
+```