From 4fc4181fa400657094ad61da577f44523240f7a8 Mon Sep 17 00:00:00 2001 From: ghm Date: Fri, 7 Sep 2018 02:23:06 -0700 Subject: [PATCH] Add a bug pattern to discourage writing #equals methods by just calling 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() && ; RELNOTES: [EqualsUsingHashCode] add a bug pattern to discourage writing equals using hashCode. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=211944934 --- .../bugpatterns/EqualsUsingHashCode.java | 88 +++++++++++++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../bugpatterns/EqualsUsingHashCodeTest.java | 81 +++++++++++++++++ docs/bugpattern/EqualsUsingHashCode.md | 24 +++++ 4 files changed, 195 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java create mode 100644 docs/bugpattern/EqualsUsingHashCode.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java new file mode 100644 index 00000000000..a68db1d6836 --- /dev/null +++ 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 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() { + @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 index 4ba35a8854e..dd51a87008c 100644 --- 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 new file mode 100644 index 00000000000..0bc569d92ea --- /dev/null +++ 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 new file mode 100644 index 00000000000..f02cbebd47e --- /dev/null +++ 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.