Productionize ThrowsUncheckedException

MOE_MIGRATED_REVID=131366221

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

@@ -18,14 +18,16 @@
 
 import static com.google.errorprone.BugPattern.Category.JDK;
 import static com.google.errorprone.BugPattern.MaturityLevel.MATURE;
-import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.util.ASTHelpers.getType;
+import static com.google.errorprone.util.ASTHelpers.isSubtype;
 
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 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.util.ASTHelpers;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.tools.javac.code.Type;
@@ -41,40 +43,26 @@
   name = "ThrowsUncheckedException",
   summary = "Unchecked exceptions do not need to be declared in the method signature.",
   category = JDK,
-  severity = ERROR,
+  severity = SUGGESTION,
   maturity = MATURE
 )
 public class ThrowsUncheckedException extends BugChecker implements MethodTreeMatcher {
   @Override
   public Description matchMethod(MethodTree tree, VisitorState state) {
-    List<ExpressionTree> runtimeExceptions = new ArrayList<>();
-
+    if (tree.getThrows().isEmpty()) {
+      return NO_MATCH;
+    }
+    List<ExpressionTree> uncheckedExceptions = new ArrayList<>();
     for (ExpressionTree exception : tree.getThrows()) {
-      Type exceptionType = ASTHelpers.getType(exception);
-      if (ASTHelpers.isSubtype(exceptionType, state.getSymtab().runtimeExceptionType, state)
-          || ASTHelpers.isSubtype(exceptionType, state.getSymtab().errorType, state)) {
-        runtimeExceptions.add(exception);
+      Type exceptionType = getType(exception);
+      if (isSubtype(exceptionType, state.getSymtab().runtimeExceptionType, state)
+          || isSubtype(exceptionType, state.getSymtab().errorType, state)) {
+        uncheckedExceptions.add(exception);
       }
     }
-
-    // With all runtime exceptions in a list, if all the exceptions are Runtime suggest to
-    // remove all otherwise just gives description.
-    Description.Builder builder = buildDescription(tree);
-    if (runtimeExceptions.isEmpty()) {
-      return Description.NO_MATCH;
-    } else if (runtimeExceptions.size() == tree.getThrows().size()) {
-      int lengthOfExceptions = 0;
-      // loop is intended to find the end position for the replace() for the first exception,
-      // which is why the loop starts counting from the second item in the list
-      for (int i = 1; i < runtimeExceptions.size(); i++) {
-        // +2 to account for space and comma
-        lengthOfExceptions += runtimeExceptions.get(i).toString().length() + 2;
-      }
-      // the " throws " string is exactly 8 characters before the exception if formatted correctly
-      // and +1 for the " " after all the exceptions
-      builder.addFix(
-          SuggestedFix.replace(runtimeExceptions.get(0), " ", -8, lengthOfExceptions + 1));
+    if (uncheckedExceptions.isEmpty()) {
+      return NO_MATCH;
     }
-    return builder.build();
+    return describeMatch(tree, SuggestedFixes.deleteExceptions(tree, state, uncheckedExceptions));
   }
 }

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

@@ -18,8 +18,12 @@
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.Iterables.getLast;
 
+import com.google.common.base.Function;
 import com.google.common.base.Joiner;
+import com.google.common.base.Predicates;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
@@ -29,12 +33,15 @@
 import com.google.errorprone.util.ErrorProneToken;
 import com.sun.source.doctree.DocTree;
 import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.ModifiersTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.util.DocTreePath;
 import com.sun.tools.javac.api.JavacTrees;
 import com.sun.tools.javac.code.Symbol;
+import com.sun.tools.javac.parser.Tokens;
 import com.sun.tools.javac.tree.DCTree;
 import com.sun.tools.javac.tree.JCTree;
 import com.sun.tools.javac.tree.TreeMaker;
@@ -306,4 +313,38 @@ public void visitIdent(JCTree.JCIdent tree) {
             });
     return fix.build();
   }
+
+  /** Deletes the given exceptions from a method's throws clause. */
+  public static Fix deleteExceptions(
+      MethodTree tree, final VisitorState state, List<ExpressionTree> toDelete) {
+    List<? extends ExpressionTree> trees = tree.getThrows();
+    if (toDelete.size() == trees.size()) {
+      return SuggestedFix.replace(
+          getThrowsPosition(tree, state), state.getEndPosition(getLast(trees)), "");
+    }
+    String replacement =
+        FluentIterable.from(tree.getThrows())
+            .filter(Predicates.not(Predicates.in(toDelete)))
+            .transform(
+                new Function<ExpressionTree, String>() {
+                  @Nullable
+                  public String apply(ExpressionTree input) {
+                    return state.getSourceForNode(input);
+                  }
+                })
+            .join(Joiner.on(", "));
+    return SuggestedFix.replace(
+        ((JCTree) tree.getThrows().get(0)).getStartPosition(),
+        state.getEndPosition(getLast(tree.getThrows())),
+        replacement);
+  }
+
+  private static int getThrowsPosition(MethodTree tree, VisitorState state) {
+    for (ErrorProneToken token : state.getTokensForNode(tree)) {
+      if (token.kind() == Tokens.TokenKind.THROWS) {
+        return ((JCTree) tree).getStartPosition() + token.pos();
+      }
+    }
+    throw new AssertionError();
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/ThrowsUncheckedExceptionTest.java

@@ -15,12 +15,14 @@
  */
 package com.google.errorprone.bugpatterns;
 
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
 import com.google.errorprone.CompilationTestHelper;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
+
 /** @author yulissa@google.com (Yulissa Arroyo-Paredes) */
 @RunWith(JUnit4.class)
 public final class ThrowsUncheckedExceptionTest {
@@ -41,4 +43,80 @@ public void testPositiveCase() throws Exception {
   public void testNegativeCase() throws Exception {
     compilationHelper.addSourceFile("ThrowsUncheckedExceptionNegativeCases.java").doTest();
   }
+
+  @Test
+  public void deleteAll() throws Exception {
+    BugCheckerRefactoringTestHelper.newInstance(new ThrowsUncheckedException(), getClass())
+        .addInputLines(
+            "in/Test.java",
+            "import java.io.IOError;",
+            "interface Test {",
+            "  void f() throws IOError, RuntimeException;",
+            "}")
+        .addOutputLines(
+            "out/Test.java", //
+            "import java.io.IOError;",
+            "interface Test {",
+            "  void f();",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void deleteLeft() throws Exception {
+    BugCheckerRefactoringTestHelper.newInstance(new ThrowsUncheckedException(), getClass())
+        .addInputLines(
+            "in/Test.java",
+            "import java.io.IOError;",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws IOError, RuntimeException, IOException;",
+            "}")
+        .addOutputLines(
+            "out/Test.java",
+            "import java.io.IOError;",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws IOException;",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void deleteRight() throws Exception {
+    BugCheckerRefactoringTestHelper.newInstance(new ThrowsUncheckedException(), getClass())
+        .addInputLines(
+            "in/Test.java",
+            "import java.io.IOError;",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws IOException, IOError, RuntimeException;",
+            "}")
+        .addOutputLines(
+            "out/Test.java",
+            "import java.io.IOError;",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws IOException;",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void preserveOrder() throws Exception {
+    BugCheckerRefactoringTestHelper.newInstance(new ThrowsUncheckedException(), getClass())
+        .addInputLines(
+            "in/Test.java",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws ReflectiveOperationException, IOException, RuntimeException;",
+            "}")
+        .addOutputLines(
+            "out/Test.java",
+            "import java.io.IOException;",
+            "interface Test {",
+            "  void f() throws ReflectiveOperationException, IOException;",
+            "}")
+        .doTest();
+  }
 }

core/src/test/java/com/google/errorprone/bugpatterns/testdata/ThrowsUncheckedExceptionPositiveCases.java

@@ -19,12 +19,12 @@
 
 /** @author yulissa@google.com (Yulissa Arroyo-Paredes) */
 public class ThrowsUncheckedExceptionPositiveCases {
-  // BUG: Diagnostic contains: 'public void doSomething() {'
+  // BUG: Diagnostic contains: 'public void doSomething()  {'
   public void doSomething() throws IllegalArgumentException {
     throw new IllegalArgumentException("thrown");
   }
 
-  // BUG: Diagnostic contains: 'public void doSomethingElse() {'
+  // BUG: Diagnostic contains: 'public void doSomethingElse()  {'
   public void doSomethingElse() throws RuntimeException, NullPointerException {
     throw new NullPointerException("thrown");
   }
@@ -39,7 +39,7 @@ public void doEverything() throws RuntimeException, IOException, IndexOutOfBound
     throw new IllegalArgumentException("thrown");
   }
 
-  // BUG: Diagnostic contains: 'public void doBetter() {'
+  // BUG: Diagnostic contains: 'public void doBetter()  {'
   public void doBetter() throws RuntimeException, AssertionError {
     throw new RuntimeException("thrown");
   }

docs/bugpattern/ThrowsUncheckedException.md

@@ -0,0 +1,5 @@
+Effective Java Item 62 says:
+
+> Use the Javadoc @throws tag to document each unchecked exception that a
+> method can throw, but do not use the throws keyword to include unchecked
+> exceptions in the method declaration.