diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java index 6fc7f6aea7f..e9644387d4a 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/EqualsUsingHashCode.java @@ -22,6 +22,11 @@ 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 static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.sun.source.tree.Tree.Kind.CONDITIONAL_AND; +import static com.sun.source.tree.Tree.Kind.EQUAL_TO; +import static com.sun.source.tree.Tree.Kind.IDENTIFIER; +import static com.sun.source.tree.Tree.Kind.METHOD_INVOCATION; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.StandardTags; @@ -29,12 +34,24 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.AssignmentTree; import com.sun.source.tree.BinaryTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; /** * Discourages implementing {@code equals} using {@code hashCode}. @@ -49,10 +66,11 @@ tags = StandardTags.FRAGILE_CODE) public final class EqualsUsingHashCode extends BugChecker implements MethodInvocationTreeMatcher { + private static final Matcher HASH_CODE = + instanceMethod().anyClass().named("hashCode"); + private static final Matcher MATCHER = - allOf( - instanceMethod().anyClass().named("hashCode"), - enclosingMethod(equalsMethodDeclaration())); + allOf(HASH_CODE, enclosingMethod(equalsMethodDeclaration())); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -60,9 +78,14 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return NO_MATCH; } ReturnTree returnTree = state.findEnclosing(ReturnTree.class); - if (returnTree == null) { - return NO_MATCH; + if (returnTree != null) { + return matchDirectHashCodeInReturn(tree, returnTree); } + return matchHashCodeExtractedToLocal(tree, state); + } + + private Description matchDirectHashCodeInReturn( + MethodInvocationTree tree, ReturnTree returnTree) { AtomicBoolean isTerminalCondition = new AtomicBoolean(false); returnTree.accept( new TreeScanner() { @@ -82,4 +105,133 @@ public Void visitBinary(BinaryTree binaryTree, Void unused) { null); return isTerminalCondition.get() ? describeMatch(tree) : NO_MATCH; } + + /** + * Flags {@code equals} methods that store both {@code hashCode()} results in locals and then + * return a comparison of those locals as the terminal condition (optionally guarded by {@code + * &&}). + */ + private Description matchHashCodeExtractedToLocal(MethodInvocationTree tree, VisitorState state) { + Tree parent = state.getPath().getParentPath().getLeaf(); + if (!(parent instanceof VariableTree variableTree) + || !tree.equals(variableTree.getInitializer())) { + return NO_MATCH; + } + MethodTree methodTree = state.findEnclosing(MethodTree.class); + if (methodTree == null || methodTree.getBody() == null) { + return NO_MATCH; + } + + Map hashCodeLocals = new HashMap<>(); + Set reassigned = new HashSet<>(); + methodTree + .getBody() + .accept( + new TreeScanner() { + @Override + public Void visitVariable(VariableTree node, Void unused) { + ExpressionTree initializer = node.getInitializer(); + if (initializer != null && HASH_CODE.matches(initializer, state)) { + Symbol sym = getSymbol(node); + if (sym instanceof VarSymbol varSymbol) { + hashCodeLocals.put(varSymbol, (MethodInvocationTree) initializer); + } + } + return super.visitVariable(node, null); + } + + @Override + public Void visitAssignment(AssignmentTree node, Void unused) { + Symbol sym = getSymbol(node.getVariable()); + if (sym instanceof VarSymbol varSymbol) { + reassigned.add(varSymbol); + } + return super.visitAssignment(node, null); + } + }, + null); + reassigned.forEach(hashCodeLocals::remove); + if (hashCodeLocals.size() < 2) { + return NO_MATCH; + } + + AtomicReference soleReturn = new AtomicReference<>(); + AtomicBoolean multipleReturns = new AtomicBoolean(false); + methodTree + .getBody() + .accept( + new TreeScanner() { + @Override + public Void visitReturn(ReturnTree node, Void unused) { + if (soleReturn.get() == null) { + soleReturn.set(node); + } else { + multipleReturns.set(true); + } + return null; + } + }, + null); + if (multipleReturns.get() + || soleReturn.get() == null + || soleReturn.get().getExpression() == null) { + return NO_MATCH; + } + + ExpressionTree terminal = terminalCondition(soleReturn.get().getExpression()); + if (!(terminal instanceof BinaryTree binaryTree) || binaryTree.getKind() != EQUAL_TO) { + return NO_MATCH; + } + if (!isHashCodeValue(binaryTree.getLeftOperand(), hashCodeLocals, state) + || !isHashCodeValue(binaryTree.getRightOperand(), hashCodeLocals, state)) { + return NO_MATCH; + } + + // Report once, on the hashCode() call that initializes the left-hand local when possible. + if (terminalLeftInitializedBy(tree, binaryTree.getLeftOperand(), hashCodeLocals)) { + return describeMatch(tree); + } + // Fall back: if the left side is a direct hashCode() call and this is it, report. + if (binaryTree.getLeftOperand().equals(tree)) { + return describeMatch(tree); + } + return NO_MATCH; + } + + /** Walks right through {@code &&} chains to match the existing "terminal condition" behavior. */ + private static ExpressionTree terminalCondition(ExpressionTree expression) { + ExpressionTree current = expression; + while (current instanceof BinaryTree binaryTree && binaryTree.getKind() == CONDITIONAL_AND) { + current = binaryTree.getRightOperand(); + } + return current; + } + + private static boolean isHashCodeValue( + ExpressionTree expression, + Map hashCodeLocals, + VisitorState state) { + if (expression.getKind() == METHOD_INVOCATION && HASH_CODE.matches(expression, state)) { + return true; + } + if (expression.getKind() == IDENTIFIER) { + Symbol sym = getSymbol((IdentifierTree) expression); + return sym instanceof VarSymbol varSymbol && hashCodeLocals.containsKey(varSymbol); + } + return false; + } + + private static boolean terminalLeftInitializedBy( + MethodInvocationTree tree, + ExpressionTree leftOperand, + Map hashCodeLocals) { + if (leftOperand.getKind() != IDENTIFIER) { + return false; + } + Symbol sym = getSymbol((IdentifierTree) leftOperand); + if (!(sym instanceof VarSymbol varSymbol)) { + return false; + } + return tree.equals(hashCodeLocals.get(varSymbol)); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java index 5f4a130431b..6052db7c125 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/EqualsUsingHashCodeTest.java @@ -89,4 +89,65 @@ public boolean equals(Object o) { """) .doTest(); } + + @Test + public void positiveExtractedLocals() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + private int a; + + @Override + public boolean equals(Object o) { + // BUG: Diagnostic contains: + int left = hashCode(); + int right = o.hashCode(); + return left == right; + } + } + """) + .doTest(); + } + + @Test + public void positiveExtractedLocalsWithInstanceofGuard() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + @Override + public boolean equals(Object o) { + // BUG: Diagnostic contains: + int left = hashCode(); + int right = o.hashCode(); + return o instanceof Test && left == right; + } + } + """) + .doTest(); + } + + @Test + public void negativeExtractedLocalsWithFieldCheck() { + helper + .addSourceLines( + "Test.java", + """ + class Test { + private int a; + + @Override + public boolean equals(Object o) { + Test that = (Test) o; + int left = hashCode(); + int right = o.hashCode(); + return left == right && a == that.a; + } + } + """) + .doTest(); + } }