Add Fix for SelfEquals.

MOE_MIGRATED_REVID=123141445

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

@@ -83,8 +83,6 @@ public Description matchMethodInvocation(
   }
 
   private Description describe(MethodInvocationTree methodInvocationTree, VisitorState state) {
-    // If we don't find a good field to use, then just replace with "true"
-    Fix fix = SuggestedFix.replace(methodInvocationTree, "true");
     /**
      * Cases:
      * <ol>
@@ -94,19 +92,9 @@ private Description describe(MethodInvocationTree methodInvocationTree, VisitorS
      * <li>Objects.equal(this.foo, this.foo) ==> Objects.equal(this.foo, other.foo)</li>
      * </ol>
      */
-    // Assumption: Both arguments are either identifiers or field accesses.
-    List<? extends ExpressionTree> args = methodInvocationTree.getArguments();
-    for (ExpressionTree arg : args) {
-      switch (arg.getKind()) {
-        case IDENTIFIER:
-        case MEMBER_SELECT:
-          break;
-        default:
-          throw new IllegalStateException(
-              "Expected arg " + arg + " to be a field access or identifier");
-      }
-    }
 
+    verifyArgsType(methodInvocationTree);
+    List<? extends ExpressionTree> args = methodInvocationTree.getArguments();  
     // Choose argument to replace.
     ExpressionTree toReplace;
     if (args.get(1).getKind() == Kind.IDENTIFIER) {
@@ -117,6 +105,29 @@ private Description describe(MethodInvocationTree methodInvocationTree, VisitorS
       // If we don't have a good reason to replace one or the other, replace the second.
       toReplace = args.get(1);
     }
+
+    Fix fix = generateFix(methodInvocationTree, state, toReplace);
+    return describeMatch(methodInvocationTree, fix);
+  }
+
+  /** Verifies arguments to be either identifiers or field accesses. */
+  protected static void verifyArgsType(MethodInvocationTree methodInvocationTree) {
+    // Assumption: Both arguments are either identifiers or field accesses.
+    for (ExpressionTree arg : methodInvocationTree.getArguments()) {
+      switch (arg.getKind()) {
+        case IDENTIFIER:
+        case MEMBER_SELECT:
+          break;
+        default:
+          throw new IllegalStateException(
+              "Expected arg " + arg + " to be a field access or identifier");
+      }
+    }
+  }
+
+  /** Finds a replacement for toReplace expression tree is possible. */
+  protected static Fix generateFix(
+      MethodInvocationTree methodInvocationTree, VisitorState state, ExpressionTree toReplace) {
     // Find containing block
     TreePath path = state.getPath();
     while (path.getLeaf().getKind() != Kind.BLOCK) {
@@ -130,16 +141,16 @@ private Description describe(MethodInvocationTree methodInvocationTree, VisitorS
 
         if (ASTHelpers.getSymbol(toReplace).isMemberOf(variableTypeSymbol, state.getTypes())) {
           if (toReplace.getKind() == Kind.IDENTIFIER) {
-            fix = SuggestedFix.prefixWith(toReplace, declaration.getName() + ".");
+            return SuggestedFix.prefixWith(toReplace, declaration.getName() + ".");
           } else {
-            fix =
+            return
                 SuggestedFix.replace(
                     ((JCFieldAccess) toReplace).getExpression(), declaration.getName().toString());
           }
         }
       }
     }
-
-    return describeMatch(methodInvocationTree, fix);
+    // If we don't find a good field to use, then just replace with "true"
+    return SuggestedFix.replace(methodInvocationTree, "true");
   }
 }

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

@@ -25,9 +25,12 @@
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.MethodInvocationTree;
+import java.util.List;
 
 /**
  * Points out if an object is tested for equality to itself.
@@ -64,6 +67,24 @@ public Description matchMethodInvocation(
       return Description.NO_MATCH;
     }
 
-    return describeMatch(methodInvocationTree);
+    return describe(methodInvocationTree, state);
+  }
+
+  private Description describe(MethodInvocationTree methodInvocationTree, VisitorState state) {
+    /**
+     * Cases:
+     * <ol>
+     * <li>foo.equals(foo) ==> foo.equals(other.foo)</li>
+     * <li>foo.equals(this.foo) ==> foo.equals(other.foo)</li>
+     * <li>this.foo.equals(foo) ==> this.foo.equals(other.foo)</li>
+     * <li>this.foo.equals(this.foo) ==> this.foo.equals(other.foo)</li>
+     * </ol>
+     */
+    GuavaSelfEquals.verifyArgsType(methodInvocationTree);
+    List<? extends ExpressionTree> args = methodInvocationTree.getArguments();
+    // Choose argument to replace.
+    ExpressionTree toReplace = args.get(0);
+    Fix fix = GuavaSelfEquals.generateFix(methodInvocationTree, state, toReplace);
+    return describeMatch(methodInvocationTree, fix);
   }
 }

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

@@ -20,31 +20,59 @@
  * @author eaftan@google.com (Eddie Aftandilian)
  */
 public class SelfEqualsPositiveCase {
+  private String simpleField;
 
-  public boolean test1() {
-    Object obj = new Object();
-    // BUG: Diagnostic contains: An object is tested for equality to itself
-    return obj.equals(obj);
+  public boolean test1(Object obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
+    // BUG: Diagnostic contains: simpleField.equals(other.simpleField);
+    return simpleField.equals(simpleField);
   }
 
-  private Object obj = new Object();
-  public boolean test2() {
-    // BUG: Diagnostic contains: An object is tested for equality to itself
-    return obj.equals(this.obj);
+  public boolean test2(SelfEqualsPositiveCase obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
+    // BUG: Diagnostic contains: simpleField.equals(other.simpleField);
+    return simpleField.equals(this.simpleField);
   }
 
-  public boolean test3() {
-    // BUG: Diagnostic contains: An object is tested for equality to itself
-    return this.obj.equals(obj);
+  public boolean test3(SelfEqualsPositiveCase obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
+    // BUG: Diagnostic contains: this.simpleField.equals(other.simpleField);
+    return this.simpleField.equals(simpleField);
   }
 
-  public boolean test4() {
-    // BUG: Diagnostic contains: An object is tested for equality to itself
-    return this.obj.equals(this.obj);
+  public boolean test4(SelfEqualsPositiveCase obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
+    // BUG: Diagnostic contains: this.simpleField.equals(other.simpleField);
+    return this.simpleField.equals(this.simpleField);
   }
 
-  public boolean test5() {
+  public boolean test5(SelfEqualsPositiveCase obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
     // BUG: Diagnostic contains: An object is tested for equality to itself
     return equals(this);
   }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (obj == null || getClass() != obj.getClass()) {
+      return false;
+    }
+    SelfEqualsPositiveCase other = (SelfEqualsPositiveCase) obj;
+    return simpleField.equals(((SelfEqualsPositiveCase)other).simpleField);
+  }
 }