Make the type check in OverrideThrowableToString more robust.

Also simplify the fix generation, and don't emit a suggestion if the class overrides *both* (that doesn't seem erroneous to me). RELNOTES: Fixes in OverrideThrowableToString. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=216839302
google · Nov 2, 2018 · a14728d · a14728d
1 parent 1c04d2d
commit a14728d
Show file tree

Hide file tree

Showing 2 changed files with 33 additions and 31 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OverrideThrowableToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/OverrideThrowableToString.java
@@ -16,22 +16,21 @@
 
 package com.google.errorprone.bugpatterns;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static com.google.errorprone.BugPattern.Category.JDK;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 
+import com.google.common.collect.ImmutableList;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.BugPattern.ProvidesFix;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.fixes.SuggestedFixes;
 import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matchers;
 import com.google.errorprone.util.ASTHelpers;
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.MethodTree;
-import com.sun.source.tree.Tree;
-import com.sun.tools.javac.code.Symbol;
-import java.util.Objects;
-import java.util.Optional;
 
 /**
  * Warns against overriding toString() in a Throwable class and suggests getMessage()
@@ -42,37 +41,30 @@
     name = "OverrideThrowableToString",
     summary =
         "To return a custom message with a Throwable class, one should "
-            + "override getMessage() instead of toString() for Throwable.",
+            + "override getMessage() instead of toString().",
     category = JDK,
     severity = WARNING,
     providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
-public class OverrideThrowableToString extends BugChecker implements ClassTreeMatcher {
+public final class OverrideThrowableToString extends BugChecker implements ClassTreeMatcher {
 
   @Override
-  public Description matchClass(ClassTree classTree, VisitorState visitorState) {
+  public Description matchClass(ClassTree classTree, VisitorState state) {
-    Symbol throwableClass = visitorState.getSymbolFromString("java.lang.Throwable");
+    if (!ASTHelpers.isSubtype(
-    if (Objects.equals(ASTHelpers.getSymbol(classTree.getExtendsClause()), throwableClass)) {
+        ASTHelpers.getType(classTree), state.getSymtab().throwableType, state)) {
-      Optional<? extends Tree> methodTree =
+      return Description.NO_MATCH;
-          classTree.getMembers().stream()
-              .filter(
-                  m ->
-                      m instanceof MethodTree
-                          && ((MethodTree) m).getName().contentEquals("toString"))
-              .findFirst();
-      if (methodTree.isPresent()) {
-        SuggestedFix.Builder builder = SuggestedFix.builder();
-        MethodTree tree = (MethodTree) methodTree.get();
-        if (!tree.getParameters().isEmpty()) {
-          return Description.NO_MATCH;
-        }
-        String newTree =
-            tree.getModifiers().toString().replaceAll("@Override[(][)]", "@Override")
-                + "String getMessage()\n"
-                + visitorState.getSourceForNode(tree.getBody());
-        builder.replace(tree, newTree);
-        return describeMatch(classTree, builder.build());
-      }
     }
-    return Description.NO_MATCH;
+    ImmutableList<MethodTree> methods =
+        classTree.getMembers().stream()
+            .filter(m -> m instanceof MethodTree)
+            .map(m -> (MethodTree) m)
+            .collect(toImmutableList());
+    if (methods.stream().anyMatch(m -> m.getName().contentEquals("getMessage"))) {
+      return Description.NO_MATCH;
+    }
+    return methods.stream()
+        .filter(m -> Matchers.toStringMethodDeclaration().matches(m, state))
+        .findFirst()
+        .map(m -> describeMatch(classTree, SuggestedFixes.renameMethod(m, "getMessage", state)))
+        .orElse(Description.NO_MATCH);
   }
 }
diff --git a/...va/com/google/errorprone/bugpatterns/testdata/OverrideThrowableToStringNegativeCases.java b/...va/com/google/errorprone/bugpatterns/testdata/OverrideThrowableToStringNegativeCases.java
@@ -37,4 +37,14 @@ public String getMessage() {
       return "";
     }
   }
+
+  class OverridesBoth extends Throwable {
+    public String toString() {
+      return "";
+    }
+
+    public String getMessage() {
+      return "";
+    }
+  }
 }