From fb06f25465dabe2f41f44906290faf886e4f01de Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 19 Oct 2023 09:27:16 -0700 Subject: [PATCH] Extend `SuperEqualsIsObjectEquals` to cover `hashCode`, renaming it to "`SuperCallToObjectMethod`." ...as suggested by @Marcono1234 in https://github.com/google/error-prone/pull/4147#issuecomment-1768878594. Also, add docs. PiperOrigin-RevId: 574898389 --- ...uals.java => SuperCallToObjectMethod.java} | 37 +++++---- .../scanner/BuiltInCheckerSuppliers.java | 4 +- ....java => SuperCallToObjectMethodTest.java} | 34 ++++++-- docs/bugpattern/SuperCallToObjectMethod.md | 82 +++++++++++++++++++ 4 files changed, 136 insertions(+), 21 deletions(-) rename core/src/main/java/com/google/errorprone/bugpatterns/{SuperEqualsIsObjectEquals.java => SuperCallToObjectMethod.java} (76%) rename core/src/test/java/com/google/errorprone/bugpatterns/{SuperEqualsIsObjectEqualsTest.java => SuperCallToObjectMethodTest.java} (82%) create mode 100644 docs/bugpattern/SuperCallToObjectMethod.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java b/core/src/main/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethod.java similarity index 76% rename from core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java rename to core/src/main/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethod.java index 49f95a4ade1..eb5742e0f0b 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEquals.java +++ b/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; diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 672e0a77d6c..9879def0835 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/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, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethodTest.java similarity index 82% rename from core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java rename to core/src/test/java/com/google/errorprone/bugpatterns/SuperCallToObjectMethodTest.java index 13e6cb89139..a0c1594cb84 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/SuperEqualsIsObjectEqualsTest.java +++ b/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()); } } diff --git a/docs/bugpattern/SuperCallToObjectMethod.md b/docs/bugpattern/SuperCallToObjectMethod.md new file mode 100644 index 00000000000..f5ac8a1560d --- /dev/null +++ b/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); +} +```