Add a bug pattern to discourage writing #equals methods by just calli…

…ng hashCode. I'm kinda surprised this is a thing. We only match where the hashCode is the last thing compared within a `return, as I think this is a fairly sane pattern, especially with memoized hashCodes: return hashCode() == o.hashCode() && <expensive comparison>; RELNOTES: [EqualsUsingHashCode] add a bug pattern to discourage writing equals using hashCode. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=211944934
google · Sep 12, 2018 · 4fc4181 · 4fc4181
1 parent e5eb00a
commit 4fc4181
Show file tree

Hide file tree

Showing 4 changed files with 195 additions and 0 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2018 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
+import static com.google.errorprone.matchers.Description.NO_MATCH;
+import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.enclosingMethod;
+import static com.google.errorprone.matchers.Matchers.equalsMethodDeclaration;
+import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
+
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.BugPattern.ProvidesFix;
+import com.google.errorprone.BugPattern.StandardTags;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.sun.source.tree.BinaryTree;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.ReturnTree;
+import com.sun.source.util.TreeScanner;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * Discourages implementing {@code equals} using {@code hashCode}.
+ *
+ * @author ghm@google.com (Graeme Morgan)
+ */
+@BugPattern(
+    name = "EqualsUsingHashCode",
+    summary =
+        "Implementing #equals by just comparing hashCodes is fragile. Hashes collide "
+            + "frequently, and this will lead to false positives in #equals.",
+    severity = WARNING,
+    tags = StandardTags.FRAGILE_CODE,
+    providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION)
+public final class EqualsUsingHashCode extends BugChecker implements MethodInvocationTreeMatcher {
+
+  private static final Matcher<ExpressionTree> MATCHER =
+      allOf(
+          instanceMethod().anyClass().named("hashCode"),
+          enclosingMethod(equalsMethodDeclaration()));
+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!MATCHER.matches(tree, state)) {
+      return NO_MATCH;
+    }
+    ReturnTree returnTree = state.findEnclosing(ReturnTree.class);
+    if (returnTree == null) {
+      return NO_MATCH;
+    }
+    AtomicBoolean isTerminalCondition = new AtomicBoolean(false);
+    returnTree.accept(
+        new TreeScanner<Void, Void>() {
+          @Override
+          public Void visitMethodInvocation(MethodInvocationTree methodTree, Void unused) {
+            if (methodTree.equals(tree)) {
+              isTerminalCondition.set(true);
+            }
+            return super.visitMethodInvocation(methodTree, null);
+          }
+
+          @Override
+          public Void visitBinary(BinaryTree binaryTree, Void unused) {
+            return scan(binaryTree.getRightOperand(), null);
+          }
+        },
+        null);
+    return isTerminalCondition.get() ? describeMatch(tree) : NO_MATCH;
+  }
+}
diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java
@@ -85,6 +85,7 @@
 import com.google.errorprone.bugpatterns.EqualsNaN;
 import com.google.errorprone.bugpatterns.EqualsReference;
 import com.google.errorprone.bugpatterns.EqualsUnsafeCast;
+import com.google.errorprone.bugpatterns.EqualsUsingHashCode;
 import com.google.errorprone.bugpatterns.EqualsWrongThing;
 import com.google.errorprone.bugpatterns.ExpectedExceptionChecker;
 import com.google.errorprone.bugpatterns.ExpectedExceptionRefactoring;
@@ -540,6 +541,7 @@ public static ScannerSupplier errorChecks() {
           EqualsHashCode.class,
           EqualsIncompatibleType.class,
           EqualsUnsafeCast.class,
+          EqualsUsingHashCode.class,
           ExtendingJUnitAssert.class,
           FallThrough.class,
           Finally.class,

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java
@@ -0,0 +1,81 @@
+/*
+ * Copyright 2018 The Error Prone Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.google.errorprone.bugpatterns;
+
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/**
+ * Tests for {@link EqualsUsingHashCode} bugpattern.
+ *
+ * @author ghm@google.com (Graeme Morgan)
+ */
+@RunWith(JUnit4.class)
+public final class EqualsUsingHashCodeTest {
+  private final CompilationTestHelper helper =
+      CompilationTestHelper.newInstance(EqualsUsingHashCode.class, getClass());
+
+  @Test
+  public void negative() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.base.Objects;",
+            "class Test {",
+            "  private int a;",
+            "  @Override public boolean equals(Object o) {",
+            "    Test that = (Test) o;",
+            "    return o.hashCode() == hashCode() && a == that.a;",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void positive() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  private int a;",
+            "  private int b;",
+            "  @Override public boolean equals(Object o) {",
+            "    // BUG: Diagnostic contains:",
+            "    return o.hashCode() == hashCode();",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void positiveBinary() {
+    helper
+        .addSourceLines(
+            "Test.java",
+            "class Test {",
+            "  private int a;",
+            "  private int b;",
+            "  @Override public boolean equals(Object o) {",
+            "    // BUG: Diagnostic contains:",
+            "    return o instanceof Test && o.hashCode() == hashCode();",
+            "  }",
+            "}")
+        .doTest();
+  }
+}
diff --git a/docs/bugpattern/EqualsUsingHashCode.md b/docs/bugpattern/EqualsUsingHashCode.md
@@ -0,0 +1,24 @@
+Don't implement `#equals` using just a `hashCode` comparison:
+
+```java {.bad}
+class MyClass {
+  private final int a;
+  private final int b;
+  private final String c;
+
+  ...
+
+  @Override
+  public boolean equals(@Nullable Object o) {
+    return o.hashCode() == hashCode();
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hashCode(a, b, c);
+  }
+```
+
+The number of `Object`s with randomly distributed `hashCode` required to give a
+50% chance of collision (and therefore, with this pattern, erroneously correct
+equality) is only ~77k.