From 2678ba3dc2b35e4c80693f90cb553f10e81ed79c Mon Sep 17 00:00:00 2001 From: Burak KALAYCI Date: Tue, 11 Aug 2026 15:06:06 +0300 Subject: [PATCH] Detect EqualsUsingHashCode when hashCode results are stored in locals EqualsUsingHashCode previously only recognized direct hashCode() comparisons in the return expression. Equivalent equals implementations that extract both hash codes into locals before comparing them were missed. Track locals initialized solely from hashCode() and flag when the terminal return condition compares only those values. Fixes #5830 --- .../bugpatterns/EqualsUsingHashCode.java | 162 +++++++++++++++++- .../bugpatterns/EqualsUsingHashCodeTest.java | 61 +++++++ 2 files changed, 218 insertions(+), 5 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 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(); + } }