Generalize FilesLinesLeak to check other methods in Files

RELNOTES: FilesLinesLeak was renamed to StreamResourceLeak, and now checks other methods in Files

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=179277039

check_api/src/main/java/com/google/errorprone/fixes/SuggestedFixes.java

@@ -26,6 +26,7 @@
 import static com.sun.source.tree.Tree.Kind.ASSIGNMENT;
 import static com.sun.source.tree.Tree.Kind.NEW_ARRAY;
 import static com.sun.tools.javac.code.TypeTag.CLASS;
+import static java.util.stream.Collectors.joining;
 
 import com.google.common.base.Function;
 import com.google.common.base.Joiner;
@@ -61,9 +62,11 @@
 import com.sun.tools.javac.api.JavacTaskImpl;
 import com.sun.tools.javac.api.JavacTool;
 import com.sun.tools.javac.api.JavacTrees;
+import com.sun.tools.javac.code.BoundKind;
 import com.sun.tools.javac.code.Kinds.KindSelector;
 import com.sun.tools.javac.code.Symbol;
 import com.sun.tools.javac.code.Type;
+import com.sun.tools.javac.code.Types.DefaultTypeVisitor;
 import com.sun.tools.javac.main.Arguments;
 import com.sun.tools.javac.parser.Tokens;
 import com.sun.tools.javac.parser.Tokens.TokenKind;
@@ -671,4 +674,60 @@ private static long countErrors(DiagnosticCollector<JavaFileObject> diagnosticCo
         .filter(d -> d.getKind() == Diagnostic.Kind.ERROR)
         .count();
   }
+
+  /**
+   * Pretty-prints a Type for use in fixes, qualifying any enclosed type names using {@link
+   * #qualifyType}}.
+   */
+  public static String prettyType(
+      @Nullable VisitorState state, @Nullable SuggestedFix.Builder fix, Type type) {
+    return type.accept(
+        new DefaultTypeVisitor<String, Void>() {
+          @Override
+          public String visitWildcardType(Type.WildcardType t, Void unused) {
+            StringBuilder sb = new StringBuilder();
+            sb.append(t.kind);
+            if (t.kind != BoundKind.UNBOUND) {
+              sb.append(t.type.accept(this, null));
+            }
+            return sb.toString();
+          }
+
+          @Override
+          public String visitClassType(Type.ClassType t, Void unused) {
+            StringBuilder sb = new StringBuilder();
+            if (state == null || fix == null) {
+              sb.append(t.tsym.getSimpleName());
+            } else {
+              sb.append(qualifyType(state, fix, t.tsym));
+            }
+            if (t.getTypeArguments().nonEmpty()) {
+              sb.append('<');
+              sb.append(
+                  t.getTypeArguments()
+                      .stream()
+                      .map(a -> a.accept(this, null))
+                      .collect(joining(", ")));
+              sb.append(">");
+            }
+            return sb.toString();
+          }
+
+          @Override
+          public String visitCapturedType(Type.CapturedType t, Void unused) {
+            return t.wildcard.accept(this, null);
+          }
+
+          @Override
+          public String visitArrayType(Type.ArrayType t, Void unused) {
+            return t.elemtype.accept(this, null) + "[]";
+          }
+
+          @Override
+          public String visitType(Type t, Void unused) {
+            return t.toString();
+          }
+        },
+        null);
+  }
 }

core/src/main/java/com/google/errorprone/bugpatterns/AbstractMustBeClosedChecker.java

@@ -46,8 +46,8 @@
 import javax.lang.model.element.ElementKind;
 
 /**
- * An abstract check for resources that must be closed; used by {@link FilesLinesLeak} and {@link
- * MustBeClosedChecker}.
+ * An abstract check for resources that must be closed; used by {@link StreamResourceLeak} and
+ * {@link MustBeClosedChecker}.
  */
 public abstract class AbstractMustBeClosedChecker extends BugChecker {
 
@@ -65,7 +65,7 @@ public abstract class AbstractMustBeClosedChecker extends BugChecker {
    * Check that constructors and methods annotated with {@link MustBeClosed} occur within the
    * resource variable initializer of a try-with-resources statement.
    */
-  protected Description matchNewClassOrMethodInvocation(Tree tree, VisitorState state) {
+  protected Description matchNewClassOrMethodInvocation(ExpressionTree tree, VisitorState state) {
     Description description = checkClosed(tree, state);
     if (description == NO_MATCH) {
       return NO_MATCH;
@@ -77,7 +77,7 @@ protected Description matchNewClassOrMethodInvocation(Tree tree, VisitorState st
     return description;
   }
 
-  private Description checkClosed(Tree tree, VisitorState state) {
+  private Description checkClosed(ExpressionTree tree, VisitorState state) {
     MethodTree callerMethodTree = enclosingMethod(state);
     if (state.getPath().getParentPath().getLeaf().getKind() == Tree.Kind.RETURN
         && callerMethodTree != null) {
@@ -135,7 +135,6 @@ private static MethodTree enclosingMethod(VisitorState state) {
    * Returns whether an invocation occurs within the resource variable initializer of a
    * try-with-resources statement.
    */
-  // TODO(cushon): This method has been copied from FilesLinesLeak. Move it to a shared class.
   private boolean inTWR(VisitorState state) {
     TreePath path = state.getPath().getParentPath();
     while (path.getLeaf().getKind() == Tree.Kind.CONDITIONAL_EXPRESSION) {
@@ -188,5 +187,5 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void unused) {
     return closed[0];
   }
 
-  protected void addFix(Description.Builder description, Tree tree, VisitorState state) {}
+  protected void addFix(Description.Builder description, ExpressionTree tree, VisitorState state) {}
 }

core/src/main/java/com/google/errorprone/bugpatterns/StreamResourceLeak.java

@@ -0,0 +1,168 @@
+/*
+ * Copyright 2016 Google 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.google.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.Category.JDK;
+import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.ProvidesFix;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+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.method.MethodMatchers;
+import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.BlockTree;
+import com.sun.source.tree.EnhancedForLoopTree;
+import com.sun.source.tree.ExpressionStatementTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.IdentifierTree;
+import com.sun.source.tree.MemberSelectTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.StatementTree;
+import com.sun.source.tree.Tree;
+import com.sun.source.tree.VariableTree;
+import com.sun.source.util.TreePath;
+import com.sun.source.util.TreeScanner;
+import com.sun.tools.javac.tree.JCTree;
+import java.util.List;
+import java.util.Objects;
+import java.util.regex.Pattern;
+
+/** @author cushon@google.com (Liam Miller-Cushon) */
+@BugPattern(
+  name = "StreamResourceLeak",
+  category = JDK,
+  summary =
+      "Streams that encapsulate a closeable resource should be closed using"
+          + " try-with-resources",
+  severity = ERROR,
+  providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION
+)
+public class StreamResourceLeak extends AbstractMustBeClosedChecker
+    implements MethodInvocationTreeMatcher {
+
+  public static final Matcher<ExpressionTree> MATCHER =
+      MethodMatchers.staticMethod()
+          .onClass("java.nio.file.Files")
+          .withNameMatching(Pattern.compile("lines|newDirectoryStream|list|walk|find"));
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!MATCHER.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    return matchNewClassOrMethodInvocation(tree, state);
+  }
+
+  @Override
+  protected void addFix(Description.Builder description, ExpressionTree tree, VisitorState state) {
+    TreePath parentPath = state.getPath().getParentPath();
+    Tree parent = parentPath.getLeaf();
+    SuggestedFix.Builder fix = SuggestedFix.builder();
+    String streamType = SuggestedFixes.prettyType(state, fix, ASTHelpers.getReturnType(tree));
+    if (parent instanceof MemberSelectTree) {
+      StatementTree statement = state.findEnclosing(StatementTree.class);
+      if (statement instanceof VariableTree) {
+        // Variables need to be declared outside the try-with-resources:
+        // e.g. `int count = Files.lines(p).count();`
+        // -> `int count; try (Stream<String> stream = Files.lines(p)) { count = stream.count(); }`
+        VariableTree var = (VariableTree) statement;
+        int pos = ((JCTree) var).getStartPosition();
+        int initPos = ((JCTree) var.getInitializer()).getStartPosition();
+        int eqPos = pos + state.getSourceForNode(var).substring(0, initPos - pos).lastIndexOf('=');
+        fix.replace(
+            eqPos,
+            initPos,
+            String.format(
+                ";\ntry (%s stream = %s) {\n%s =",
+                streamType, state.getSourceForNode(tree), var.getName().toString()));
+      } else {
+        // the non-variable case, e.g. `return Files.lines(p).count()`
+        // -> try (Stream<Stream> stream = Files.lines(p)) { return stream.count(); }`
+        fix.prefixWith(
+            statement,
+            String.format("try (%s stream = %s) {\n", streamType, state.getSourceForNode(tree)));
+      }
+      fix.replace(tree, "stream");
+      fix.postfixWith(statement, "}");
+      description.addFix(fix.build());
+    } else if (parent instanceof VariableTree) {
+      // If the stream is assigned to a variable, wrap the variable in a try-with-resources
+      // that includes all statements in the same block that reference the variable.
+      Tree grandParent = parentPath.getParentPath().getLeaf();
+      if (!(grandParent instanceof BlockTree)) {
+        return;
+      }
+      List<? extends StatementTree> statements = ((BlockTree) grandParent).getStatements();
+      int idx = statements.indexOf(parent);
+      int lastUse = idx;
+      for (int i = idx + 1; i < statements.size(); i++) {
+        boolean[] found = {false};
+        statements
+            .get(i)
+            .accept(
+                new TreeScanner<Void, Void>() {
+                  @Override
+                  public Void visitIdentifier(IdentifierTree tree, Void unused) {
+                    if (Objects.equals(ASTHelpers.getSymbol(tree), ASTHelpers.getSymbol(parent))) {
+                      found[0] = true;
+                    }
+                    return null;
+                  }
+                },
+                null);
+        if (found[0]) {
+          lastUse = i;
+        }
+      }
+      fix.prefixWith(parent, "try (");
+      fix.replace(
+          state.getEndPosition(((VariableTree) parent).getInitializer()),
+          state.getEndPosition(parent),
+          ") {");
+      fix.postfixWith(statements.get(lastUse), "}");
+      description.addFix(fix.build());
+    } else if (parent instanceof EnhancedForLoopTree) {
+      // If the stream is used in a loop (e.g. directory streams), wrap the loop in
+      // try-with-resources.
+      fix.prefixWith(
+          parent,
+          String.format("try (%s stream = %s) {\n", streamType, state.getSourceForNode(tree)));
+      fix.replace(tree, "stream");
+      fix.postfixWith(parent, "}");
+      description.addFix(fix.build());
+    } else if (parent instanceof MethodInvocationTree) {
+      // If the stream is used in a method that is called in an expression statement, wrap it in
+      // try-with-resources.
+      Tree grandParent = parentPath.getParentPath().getLeaf();
+      if (!(grandParent instanceof ExpressionStatementTree)) {
+        return;
+      }
+      fix.prefixWith(
+          parent,
+          String.format("try (%s stream = %s) {\n", streamType, state.getSourceForNode(tree)));
+      fix.replace(tree, "stream");
+      fix.postfixWith(grandParent, "}");
+      description.addFix(fix.build());
+    }
+  }
+}