Extend SuperEqualsIsObjectEquals to cover hashCode, renaming it to "SuperCallToObjectMethod."

core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java → core/src/main/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethod.java

@@ -19,6 +19,7 @@
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR;
+import static com.google.errorprone.fixes.SuggestedFix.replace;
 import static com.google.errorprone.matchers.Description.NO_MATCH;
 import static com.google.errorprone.util.ASTHelpers.enclosingClass;
 import static com.google.errorprone.util.ASTHelpers.getSymbol;
@@ -32,7 +33,6 @@
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
-import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.sun.source.tree.BlockTree;
 import com.sun.source.tree.IdentifierTree;
@@ -41,10 +41,13 @@
 
 /** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
 @BugPattern(
-    summary = "`super.equals(obj)` is equivalent to `this == obj` here",
+    summary =
+        "`super.equals(obj)` and `super.hashCode()` are often bugs when they call the methods"
+            + " defined in `java.lang.Object`",
     severity = WARNING,
-    tags = LIKELY_ERROR)
-public class SuperEqualsIsObjectEquals extends BugChecker implements MethodInvocationTreeMatcher {
+    tags = LIKELY_ERROR,
+    altNames = {"SuperEqualsIsObjectEquals"})
+public class SuperCallToObjectMethod extends BugChecker implements MethodInvocationTreeMatcher {
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
     var methodSelect = tree.getMethodSelect();
@@ -53,29 +56,35 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
     }
     var memberSelect = (MemberSelectTree) methodSelect;
     var expression = memberSelect.getExpression();
+    var methodName = memberSelect.getIdentifier();
+    var methodIsEquals = methodName.equals(state.getNames().equals);
+    var methodIsHashCode = methodName.equals(state.getNames().hashCode);
     if (expression.getKind() == IDENTIFIER
         && ((IdentifierTree) expression).getName().equals(state.getNames()._super)
         // We can't use a Matcher because onExactClass suffers from b/130658266.
         && enclosingClass(getSymbol(tree)) == state.getSymtab().objectType.tsym
-        && memberSelect.getIdentifier().equals(state.getNames().equals)
+        && (methodIsEquals || methodIsHashCode)
         /*
          * We ignore an override that is merely `return super.equals(obj)`. Such an override is less
          * likely to be a bug because it may exist for the purpose of adding Javadoc.
          *
          * TODO(cpovirk): Consider flagging even that if the method does *not* have Javadoc.
          */
-        && !methodBodyIsOnlyReturnSuperEquals(state)) {
+        && !methodBodyIsOnlyReturnSuper(state)) {
       /*
        * There will often be better fixes than this, some of which would change behavior. But let's
        * at least suggest the simple thing that's always equivalent.
        */
-      return describeMatch(
-          tree,
-          SuggestedFix.replace(
-              tree,
-              maybeParenthesize(
-                  "this == " + state.getSourceForNode(getOnlyElement(tree.getArguments())),
-                  state)));
+      var fix =
+          methodIsEquals
+              ? maybeParenthesize(
+                  "this == " + state.getSourceForNode(getOnlyElement(tree.getArguments())), state)
+              : "System.identityHashCode(this)";
+      var message =
+          methodIsEquals
+              ? "`super.equals(obj)` is equivalent to `this == obj` here"
+              : "`super.hashCode()` is equivalent to `System.identityHashCode(this)` here";
+      return buildDescription(tree).setMessage(message).addFix(replace(tree, fix)).build();
     }
     return NO_MATCH;
   }
@@ -90,7 +99,7 @@ private static String maybeParenthesize(String s, VisitorState state) {
         : s;
   }
 
-  private static boolean methodBodyIsOnlyReturnSuperEquals(VisitorState state) {
+  private static boolean methodBodyIsOnlyReturnSuper(VisitorState state) {
     var parentPath = state.getPath().getParentPath();
     if (parentPath.getLeaf().getKind() != RETURN) {
       return false;

core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

@@ -351,7 +351,7 @@
 import com.google.errorprone.bugpatterns.StringSplitter;
 import com.google.errorprone.bugpatterns.StronglyTypeByteString;
 import com.google.errorprone.bugpatterns.SubstringOfZero;
-import com.google.errorprone.bugpatterns.SuperEqualsIsObjectEquals;
+import com.google.errorprone.bugpatterns.SuperCallToObjectMethod;
 import com.google.errorprone.bugpatterns.SuppressWarningsDeprecated;
 import com.google.errorprone.bugpatterns.SuppressWarningsWithoutExplanation;
 import com.google.errorprone.bugpatterns.SwigMemoryLeak;
@@ -1040,7 +1040,7 @@ public static ScannerSupplier warningChecks() {
           StringCharset.class,
           StringSplitter.class,
           Suggester.class,
-          SuperEqualsIsObjectEquals.class,
+          SuperCallToObjectMethod.class,
           SwigMemoryLeak.class,
           SynchronizeOnNonFinalField.class,
           ThreadJoinLoop.class,

core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java → core/src/test/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethodTest.java

@@ -22,9 +22,9 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 
-/** {@link SuperEqualsIsObjectEquals}Test */
+/** {@link SuperCallToObjectMethod}Test */
 @RunWith(JUnit4.class)
-public class SuperEqualsIsObjectEqualsTest {
+public class SuperCallToObjectMethodTest {
   @Test
   public void positive() {
     helper()
@@ -37,7 +37,7 @@ public void positive() {
             "    if (obj instanceof Foo) {",
             "      return i == ((Foo) obj).i;",
             "    }",
-            "    // BUG: Diagnostic contains: ",
+            "    // BUG: Diagnostic contains: equals",
             "    return super.equals(obj);",
             "  }",
             "}")
@@ -154,11 +154,35 @@ public void refactoringNeedsParens() {
         .doTest();
   }
 
+  @Test
+  public void refactoringHashCode() {
+    refactoringHelper()
+        .addInputLines(
+            "Foo.java",
+            "class Foo {",
+            "  int i;",
+            "  @Override",
+            "  public int hashCode() {",
+            "    return super.hashCode() * 31 + i;",
+            "  }",
+            "}")
+        .addOutputLines(
+            "Foo.java",
+            "class Foo {",
+            "  int i;",
+            "  @Override",
+            "  public int hashCode() {",
+            "    return System.identityHashCode(this) * 31 + i;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
   private CompilationTestHelper helper() {
-    return CompilationTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass());
+    return CompilationTestHelper.newInstance(SuperCallToObjectMethod.class, getClass());
   }
 
   private BugCheckerRefactoringTestHelper refactoringHelper() {
-    return BugCheckerRefactoringTestHelper.newInstance(SuperEqualsIsObjectEquals.class, getClass());
+    return BugCheckerRefactoringTestHelper.newInstance(SuperCallToObjectMethod.class, getClass());
   }
 }

docs/bugpattern/SuperCallToObjectMethod.md

@@ -0,0 +1,82 @@
+Implementations of `equals` and `hashCode` should usually not delegate to
+`Object.equals` and `Object.hashCode`.
+
+Those two methods implement equality based on object identity. That
+implementation *is* sometimes what the author intended. (This check attempts to
+identify those cases and *not* report a warning for them. But in some cases, it
+still produces a warning when it shouldn't.)
+
+But when `super.equals` and `super.hashCode` call the methods defined in
+`Object`, the developer often did *not* intend to use object identity. Often,
+developers write something like:
+
+```java
+private final int id;
+
+@Override
+public boolean equals(Object obj) {
+  if (obj instanceof Foo) {
+    return super.equals(obj) && id == ((Foo) obj).id;
+  }
+  return obj;
+}
+
+@Override
+public int hashCode() {
+  return super.hashCode() ^ id;
+}
+```
+
+This appears to be an attempt to define equality in terms of the `id` field in
+this class and any fields in the superclass. However, when the superclass that
+defines `equals` or `hashCode` is `Object`, the code instead defines equality in
+terms of a mix of object identity and field values. The result is equivalent to
+defining it in terms of identity alone—which is equivalent to not overriding
+`equals` and `hashCode` at all!
+
+Typically, the code should be rewritten to remove the `super` calls entirely:
+
+```java
+private final int id;
+
+@Override
+public boolean equals(Object obj) {
+  if (obj instanceof Foo) {
+    return id == ((Foo) obj).id;
+  }
+  return obj;
+}
+
+@Override
+public int hashCode() {
+  return id;
+}
+```
+
+Note that the suggested edits for this check instead preserve behavior, which
+likely means preserving bugs! However, in cases in which object identity *is*
+intended, we recommend applying the suggested edit to make that behavior
+explicit in the code:
+
+```java
+// This class's definition of equality is unusual and perhaps not ideal.
+// But it is at least explicit.
+
+private final Integer id;
+
+@Override
+public boolean equals(Object obj) {
+  if (obj instanceof Foo) {
+    if (id == null) {
+      return this == obj;
+    }
+    return id.equals(((Foo) obj).id);
+  }
+  return obj;
+}
+
+@Override
+public int hashCode() {
+  return id != null ? id : System.identityHashCode(this);
+}
+```