diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/HashCodeMismatchedFieldsCheckSampleNonCompiling.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/HashCodeMismatchedFieldsCheckSampleNonCompiling.java new file mode 100644 index 00000000000..7c815ff0f9b --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/HashCodeMismatchedFieldsCheckSampleNonCompiling.java @@ -0,0 +1,46 @@ +import java.util.Objects; + +class HashCodeMismatchedFieldsCheckSampleNonCompiling { + + // "undefinedField" cannot be resolved: the field-read collector must bail out on the unknown + // identifier symbol instead of assuming it is unrelated state, and must not report an issue here. + static class UnresolvedIdentifier { + private final long id; + + UnresolvedIdentifier(long id) { + this.id = id; + } + + @Override + public boolean equals(Object other) { + return other instanceof UnresolvedIdentifier that && id == that.id; + } + + @Override + public int hashCode() { // No issue - "undefinedField" cannot be resolved, so the scan bails out + return Objects.hash(id, undefinedField); + } + } + + // A call to an unresolvable method must also make the scan bail out rather than treat it as a + // harmless side-effect-free helper. + static class UnresolvedHelperCall { + private final long id; + private final int version; + + UnresolvedHelperCall(long id, int version) { + this.id = id; + this.version = version; + } + + @Override + public boolean equals(Object other) { + return other instanceof UnresolvedHelperCall that && id == that.id && unresolvedHelper(); + } + + @Override + public int hashCode() { // No issue - equals() calls an unresolved method, so the scan bails out + return Objects.hash(id, version); + } + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java new file mode 100644 index 00000000000..528ffe7f239 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java @@ -0,0 +1,466 @@ +package checks; + +import java.util.Map; +import java.util.Objects; + +class HashCodeMismatchedFieldsCheckSample { + + static class OrderKey { + private final long id; + private final int version; + + OrderKey(long id, int version) { + this.id = id; + this.version = version; + } + + @Override + public boolean equals(Object other) { + return other instanceof OrderKey key && id == key.id; + } + + @Override + public int hashCode() { // Noncompliant {{This hashCode() implementation is inconsistent with equals(): it reads "version", which equals() never reads, so equal objects may hash differently.}} +// ^^^^^^^^ + return Objects.hash(id, version); +// ^^^^^^^< {{Not compared in equals()}} + } + } + + static class CompliantOrderKey { + private final long id; + private final int version; + + CompliantOrderKey(long id, int version) { + this.id = id; + this.version = version; + } + + @Override + public boolean equals(Object other) { + return other instanceof CompliantOrderKey key && id == key.id && version == key.version; + } + + @Override + public int hashCode() { + return Objects.hash(id, version); + } + } + + static class Person { + private final String firstName; + private final String lastName; + + Person(String firstName, String lastName) { + this.firstName = firstName; + this.lastName = lastName; + } + + String getFirstName() { + return firstName; + } + + String getLastName() { + return lastName; + } + + @Override + public boolean equals(Object other) { + return other instanceof Person person && getFirstName().equals(person.getFirstName()); + } + + @Override + public int hashCode() { // Noncompliant {{This hashCode() implementation is inconsistent with equals(): it reads "lastName", which equals() never reads, so equal objects may hash differently.}} +// ^^^^^^^^ + return Objects.hash(getFirstName(), getLastName()); +// ^^^^^^^^^^^^^< {{Not compared in equals()}} + } + } + + static class CompliantPerson { + private final String firstName; + private final String lastName; + + CompliantPerson(String firstName, String lastName) { + this.firstName = firstName; + this.lastName = lastName; + } + + String getFirstName() { + return firstName; + } + + String getLastName() { + return lastName; + } + + @Override + public boolean equals(Object other) { + return other instanceof CompliantPerson person + && getFirstName().equals(person.getFirstName()) + && getLastName().equals(person.getLastName()); + } + + @Override + public int hashCode() { + return Objects.hash(getFirstName(), getLastName()); + } + } + + static class MultipleMismatches { + private final long id; + private final int version; + private final String tag; + + MultipleMismatches(long id, int version, String tag) { + this.id = id; + this.version = version; + this.tag = tag; + } + + @Override + public boolean equals(Object other) { + return other instanceof MultipleMismatches that && id == that.id; + } + + @Override + public int hashCode() { // Noncompliant {{This hashCode() implementation is inconsistent with equals(): it reads "tag", "version", which equals() never reads, so equal objects may hash differently.}} +// ^^^^^^^^ + int result = Objects.hashCode(id); + result = 31 * result + Objects.hashCode(version); +// ^^^^^^^< {{Not compared in equals()}} + result = 31 * result + Objects.hashCode(tag); +// ^^^< {{Not compared in equals()}} + return result; + } + } + + static class MemoizedHash { + private final int x; + private final int y; + private int hc; + + MemoizedHash(int x, int y) { + this.x = x; + this.y = y; + } + + @Override + public boolean equals(Object other) { + return other instanceof MemoizedHash point && x == point.x && y == point.y; + } + + @Override + public int hashCode() { + if (this.hc == 0) { + this.hc = Objects.hash(x, y); + } + return this.hc; + } + } + + static class EagerCachedHash { + private final int x; + private final int cachedHash; + + EagerCachedHash(int x) { + this.x = x; + this.cachedHash = Objects.hash(x); + } + + @Override + public boolean equals(Object other) { + return other instanceof EagerCachedHash that && x == that.x; + } + + @Override + public int hashCode() { + return cachedHash; + } + } + + static class IdentityFieldContainingHash { + private final long id; + private final String contentHash; + + IdentityFieldContainingHash(long id, String contentHash) { + this.id = id; + this.contentHash = contentHash; + } + + @Override + public boolean equals(Object other) { + return other instanceof IdentityFieldContainingHash that && id == that.id; + } + + @Override + public int hashCode() { // Noncompliant {{This hashCode() implementation is inconsistent with equals(): it reads "contentHash", which equals() never reads, so equal objects may hash differently.}} +// ^^^^^^^^ + return Objects.hash(id, contentHash); +// ^^^^^^^^^^^< {{Not compared in equals()}} + } + } + + static class ComplexHashCodeHelper { + private final Map values; + + ComplexHashCodeHelper(Map values) { + this.values = values; + } + + @Override + public boolean equals(Object other) { + return other instanceof ComplexHashCodeHelper that && values.equals(that.values); + } + + @Override + public int hashCode() { + return computeHash(); + } + + private int computeHash() { + return values.entrySet().stream().mapToInt(Object::hashCode).sum(); + } + } + + static class ComplexEqualsHelper { + private final Map values; + + ComplexEqualsHelper(Map values) { + this.values = values; + } + + @Override + public boolean equals(Object other) { + return other instanceof ComplexEqualsHelper that && sameValues(that); + } + + private boolean sameValues(ComplexEqualsHelper that) { + return values.entrySet().stream().allMatch(e -> Objects.equals(e.getValue(), that.values.get(e.getKey()))); + } + + @Override + public int hashCode() { + return values.hashCode(); + } + } + + static class OnlyEquals { + private final long id; + + OnlyEquals(long id) { + this.id = id; + } + + @Override + public boolean equals(Object other) { + return other instanceof OnlyEquals that && id == that.id; + } + } + + static class OnlyHashCode { + private final long id; + + OnlyHashCode(long id) { + this.id = id; + } + + @Override + public int hashCode() { + return Objects.hash(id); + } + } + + static class ExtraInEquals { + private final long id; + private final int version; + + ExtraInEquals(long id, int version) { + this.id = id; + this.version = version; + } + + @Override + public boolean equals(Object other) { + return other instanceof ExtraInEquals that && id == that.id && version == that.version; + } + + @Override + public int hashCode() { + return Objects.hash(id); + } + } + + static class ReferenceEquality { + private final long id; + + ReferenceEquality(long id) { + this.id = id; + } + + @Override + public boolean equals(Object other) { + return this == other; + } + + @Override + public int hashCode() { + return Objects.hash(id); + } + } + + static class StaticFieldInHashCode { + private static int instanceCount; + private final long id; + + StaticFieldInHashCode(long id) { + this.id = id; + } + + @Override + public boolean equals(Object other) { + return other instanceof StaticFieldInHashCode that && id == that.id; + } + + @Override + public int hashCode() { + return Objects.hash(id, instanceCount); + } + } + + static class BaseWithField { + protected final long baseId; + + BaseWithField(long baseId) { + this.baseId = baseId; + } + } + + static class InheritedFieldUser extends BaseWithField { + private final int localId; + + InheritedFieldUser(long baseId, int localId) { + super(baseId); + this.localId = localId; + } + + @Override + public boolean equals(Object other) { + return other instanceof InheritedFieldUser that && localId == that.localId; + } + + @Override + public int hashCode() { + return Objects.hash(localId, baseId); + } + } + + abstract static class AbstractPair { + public abstract boolean equals(Object other); + + public abstract int hashCode(); + } + + interface HasIdentity { + boolean equals(Object other); + + int hashCode(); + } + + static class StaticEqualsDelegate { + private final long id; + private final int b; + + StaticEqualsDelegate(long id, int b) { + this.id = id; + this.b = b; + } + + @Override + public boolean equals(Object other) { + // "b" is actually compared, but only inside the static helper: the check cannot verify that without + // scanning a two-argument static method, so it must not assume "b" is unused and report a false positive. + return other instanceof StaticEqualsDelegate that && id == that.id && sameB(this, that); + } + + private static boolean sameB(StaticEqualsDelegate a, StaticEqualsDelegate b) { + return a.b == b.b; + } + + @Override + public int hashCode() { + return Objects.hash(id, b); + } + } + + static class EqualsOverloadDelegate { + private final long id; + private final int b; + + EqualsOverloadDelegate(long id, int b) { + this.id = id; + this.b = b; + } + + @Override + public boolean equals(Object other) { + return other instanceof EqualsOverloadDelegate that && equals(that); + } + + // Overload, not an override of Object.equals(Object): "b" is compared here, but the check must not + // blindly trust every one-argument "equals" call on the enclosing class as if it were Object.equals(). + private boolean equals(EqualsOverloadDelegate that) { + return id == that.id && b == that.b; + } + + @Override + public int hashCode() { + return Objects.hash(id, b); + } + } + + static class GetClassAndLocalClass { + private final long id; + private final int version; + + GetClassAndLocalClass(long id, int version) { + this.id = id; + this.version = version; + } + + @Override + public boolean equals(Object other) { + if (other == null || getClass() != other.getClass()) { + return false; + } + Runnable ignored = new Runnable() { + @Override + public void run() { + // Field reads inside a nested class must not be attributed to the enclosing equals()/hashCode(). + } + }; + GetClassAndLocalClass that = (GetClassAndLocalClass) other; + return id == that.id && version == that.version; + } + + @Override + public int hashCode() { + return Objects.hash(id, version); + } + } + + record PointRecord(int x, int y, int z) { + + @Override + public boolean equals(Object other) { + return other instanceof PointRecord that && x == that.x && y == that.y; + } + + @Override + public int hashCode() { // Noncompliant {{This hashCode() implementation is inconsistent with equals(): it reads "z", which equals() never reads, so equal objects may hash differently.}} +// ^^^^^^^^ + return Objects.hash(x, y, z); +// ^< {{Not compared in equals()}} + } + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java new file mode 100644 index 00000000000..e2ca285604e --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -0,0 +1,286 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.MethodTreeUtils; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.JavaFileScannerContext; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; + +/** + * Flags a {@code hashCode()} implementation that reads an instance field which the class's + * {@code equals(Object)} implementation never reads, breaking the {@code Object.hashCode()} contract. + */ +@Rule(key = "S9362") +public class HashCodeMismatchedFieldsCheck extends IssuableSubscriptionVisitor { + + private static final String ISSUE_MESSAGE = + "This hashCode() implementation is inconsistent with equals(): it reads \"%s\", which equals() never reads, so equal objects may hash differently."; + private static final String SECONDARY_MESSAGE = "Not compared in equals()"; + private static final Set MEMOIZED_HASH_FIELD_NAMES = Set.of( + "hash", + "hashcode", + "cachedhash", + "cachedhashcode", + "memoizedhash", + "memoizedhashcode", + "hashcache"); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS, Tree.Kind.RECORD); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + Symbol owner = classTree.symbol(); + + EqualsAndHashCode methods = EqualsAndHashCode.find(classTree); + if (methods == null) { + return; + } + + Map> fieldsByHelper = collectHelperFields(owner, methods.otherMethods); + + ReadAndAssignedFields equalsFields = collectReadFields(methods.equalsMethod, owner, fieldsByHelper, Role.EQUALS); + ReadAndAssignedFields hashCodeFields = collectReadFields(methods.hashCodeMethod, owner, fieldsByHelper, Role.HASH_CODE); + if (equalsFields.failed() || hashCodeFields.failed() || equalsFields.readFields().isEmpty()) { + // Bail out on unresolved members, or when equals() compares no state (likely reference equality). + return; + } + + Map extraFields = computeExtraFields(equalsFields, hashCodeFields); + if (!extraFields.isEmpty()) { + reportMismatch(methods.hashCodeMethod, extraFields); + } + } + + private static Map computeExtraFields(ReadAndAssignedFields equalsFields, ReadAndAssignedFields hashCodeFields) { + Map extraFields = new LinkedHashMap<>(hashCodeFields.readFields()); + extraFields.keySet().removeAll(equalsFields.readFields().keySet()); + // A field caching a previously computed hash value does not add new identity state: recognize it either by + // name, or because hashCode() itself assigns to it (the memoization pattern), regardless of its name. + extraFields.keySet().removeIf(field -> + MEMOIZED_HASH_FIELD_NAMES.contains(field.name().toLowerCase(Locale.ROOT)) || hashCodeFields.assignedFields().contains(field)); + return extraFields; + } + + /** + * The locally declared {@code equals(Object)} and {@code hashCode()} methods of a class, plus every other + * concrete method declared in that class as helper candidates. + */ + private static final class EqualsAndHashCode { + + private final MethodTree equalsMethod; + private final MethodTree hashCodeMethod; + private final List otherMethods; + + private EqualsAndHashCode(MethodTree equalsMethod, MethodTree hashCodeMethod, List otherMethods) { + this.equalsMethod = equalsMethod; + this.hashCodeMethod = hashCodeMethod; + this.otherMethods = otherMethods; + } + + private static EqualsAndHashCode find(ClassTree classTree) { + MethodTree equalsMethod = null; + MethodTree hashCodeMethod = null; + List otherMethods = new ArrayList<>(); + for (Tree member : classTree.members()) { + if (!(member instanceof MethodTree methodTree) || methodTree.block() == null) { + continue; + } + if (MethodTreeUtils.isEqualsMethod(methodTree)) { + equalsMethod = methodTree; + } else if (MethodTreeUtils.isHashCodeMethod(methodTree)) { + hashCodeMethod = methodTree; + } else { + otherMethods.add(methodTree); + } + } + if (equalsMethod == null || hashCodeMethod == null) { + return null; + } + return new EqualsAndHashCode(equalsMethod, hashCodeMethod, otherMethods); + } + } + + private static Map> collectHelperFields(Symbol owner, List otherMethods) { + Map> fieldsByHelper = new HashMap<>(); + for (MethodTree helper : otherMethods) { + Symbol.MethodSymbol helperSymbol = helper.symbol(); + if (helperSymbol.isUnknown() || !helper.parameters().isEmpty()) { + continue; + } + ReadAndAssignedFields helperFields = collectReadFields(helper, owner, Map.of(), Role.HELPER); + if (!helperFields.failed()) { + fieldsByHelper.put(helperSymbol, helperFields.readFields()); + } + } + return fieldsByHelper; + } + + private static ReadAndAssignedFields collectReadFields( + MethodTree method, + Symbol owner, + Map> fieldsByHelper, + Role role) { + FieldReadCollector collector = new FieldReadCollector(owner, fieldsByHelper, role); + method.block().accept(collector); + return collector.fields; + } + + private void reportMismatch(MethodTree hashCodeMethod, Map extraFields) { + List names = extraFields.keySet().stream() + .map(Symbol::name) + .sorted() + .toList(); + List secondaryLocations = extraFields.values().stream() + .map(location -> new JavaFileScannerContext.Location(SECONDARY_MESSAGE, location)) + .toList(); + reportIssue(hashCodeMethod.simpleName(), String.format(ISSUE_MESSAGE, String.join("\", \"", names)), secondaryLocations, null); + } + + private enum Role { + /** Pre-scanning a candidate getter/helper method: no instance calls are trusted, and helpers are not chained. */ + HELPER, + EQUALS, + HASH_CODE + } + + private record ReadAndAssignedFields(Map readFields, Set assignedFields, boolean failed) { + + private ReadAndAssignedFields() { + this(new LinkedHashMap<>(), new HashSet<>(), false); + } + + private ReadAndAssignedFields asFailed() { + return new ReadAndAssignedFields(readFields, assignedFields, true); + } + } + + /** + * Collects same-owner, non-static field reads inside a method body. Any unresolved symbol, or any + * instance-method call that is not on the small allow-list for the method's role, marks the scan as + * failed: callers must then skip reporting entirely instead of guessing from a partial field set. + */ + private static final class FieldReadCollector extends BaseTreeVisitor { + + private final Symbol enclosingClass; + private final Map> fieldsByHelper; + private final Role role; + private ReadAndAssignedFields fields = new ReadAndAssignedFields(); + + private FieldReadCollector(Symbol enclosingClass, Map> fieldsByHelper, Role role) { + this.enclosingClass = enclosingClass; + this.fieldsByHelper = fieldsByHelper; + this.role = role; + } + + @Override + public void visitClass(ClassTree tree) { + // Do not attribute field reads from a nested or anonymous class to the enclosing equals()/hashCode(). + } + + @Override + public void visitIdentifier(IdentifierTree tree) { + if (!fields.failed()) { + String name = tree.name(); + if (!"this".equals(name) && !"super".equals(name)) { + Symbol symbol = tree.symbol(); + if (symbol.isUnknown()) { + fields = fields.asFailed(); + } else if (symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { + fields.readFields().putIfAbsent(symbol, tree); + } + } + } + super.visitIdentifier(tree); + } + + @Override + public void visitAssignmentExpression(AssignmentExpressionTree tree) { + ExpressionUtils.extractIdentifierSymbol(tree.variable()) + .filter(symbol -> !symbol.isUnknown() && symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) + .ifPresent(fields.assignedFields()::add); + super.visitAssignmentExpression(tree); + } + + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + if (!fields.failed()) { + Symbol.MethodSymbol symbol = tree.methodSymbol(); + if (symbol.isUnknown()) { + fields = fields.asFailed(); + } else { + Map helperFields = fieldsByHelper.get(symbol); + if (helperFields != null) { + helperFields.keySet().forEach(field -> fields.readFields().putIfAbsent(field, tree)); + } else if (symbol.isStatic()) { + // A same-class static helper we could not pre-scan (e.g. it takes parameters) may hide field + // reads: bail out rather than silently ignoring it. An external static utility (e.g. Objects.hash) + // is assumed side-effect free and is not owned by the enclosing class. + if (ownedByEnclosing(symbol)) { + fields = fields.asFailed(); + } + } else if (ownedByEnclosing(symbol) || !isAllowedInstanceCall(symbol)) { + // A same-class instance method other than the trusted getClass()/equals()/hashCode() allow-list + // (e.g. a differently-parameterized equals(SpecificType) overload) may hide field reads. + fields = fields.asFailed(); + } + } + } + super.visitMethodInvocation(tree); + } + + private boolean isAllowedInstanceCall(Symbol.MethodSymbol symbol) { + String name = symbol.name(); + if ("getClass".equals(name) && symbol.parameterTypes().isEmpty()) { + return true; + } + return switch (role) { + case EQUALS -> "equals".equals(name) && symbol.parameterTypes().size() == 1; + case HASH_CODE -> "hashCode".equals(name) && symbol.parameterTypes().isEmpty(); + case HELPER -> false; + }; + } + + private boolean ownedByEnclosing(Symbol symbol) { + Symbol symbolOwner = symbol.owner(); + // Compare erasures so a field of Holder still belongs to Holder. + return symbolOwner != null && !symbolOwner.isUnknown() && symbolOwner.isTypeSymbol() + && enclosingClass.type().erasure().equals(symbolOwner.type().erasure()); + } + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java new file mode 100644 index 00000000000..89d67930a7d --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java @@ -0,0 +1,53 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. + * See the Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; + +class HashCodeMismatchedFieldsCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/HashCodeMismatchedFieldsCheckSample.java")) + .withCheck(new HashCodeMismatchedFieldsCheck()) + .verifyIssues(); + } + + @Test + void test_without_semantic() { + // The sample has no external dependency, so local symbol resolution still succeeds without semantic info. + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/HashCodeMismatchedFieldsCheckSample.java")) + .withCheck(new HashCodeMismatchedFieldsCheck()) + .withoutSemantic() + .verifyIssues(); + } + + @Test + void test_non_compiling() { + // Covers the isUnknown() bail-out paths for unresolved identifiers and unresolved method calls. + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/HashCodeMismatchedFieldsCheckSampleNonCompiling.java")) + .withCheck(new HashCodeMismatchedFieldsCheck()) + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.html new file mode 100644 index 00000000000..32b5beb2b3f --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.html @@ -0,0 +1,173 @@ +

This rule raises an issue when hashCode() reads an instance field that equals() does not read. Two objects that compare +equal can then produce different hash codes.

+

Why is this an issue?

+

The Object.hashCode() contract requires that objects considered equal by equals() produce the same hash code. When +hashCode() factors in a field that equals() ignores, two instances that equals() treats as identical can still +return different hash codes, because that extra field is free to differ between them.

+

That defect is easy to introduce and easy to miss in review: adding a field to a class and remembering to include it in hashCode()’s +aggregation while forgetting to add the matching comparison in `equals() still compiles and looks complete.

+

Breaking the contract has consequences that only surface at runtime, and often only for specific data. A HashMap or +HashSet computes an object’s bucket from its hash code before ever calling equals(). If an equal object later has a +different hash code, it lands in a different bucket, so lookups, deduplication, and cache retrieval silently fail to find it.

+

The reverse situation, a field compared in equals() but not read in hashCode(), does not break this contract: +hashCode() is still free to return the same value for those objects. It can reduce how well hash codes spread across buckets, but it is +not a correctness defect, so this rule does not flag it.

+

How to fix it

+

Either add the missing field to equals(), or remove it from hashCode().

+

Adding the field to equals() is usually the safer choice: it keeps the field as part of the object’s identity and restores the +contract without discarding information that hashCode() was already using. Removing the field from hashCode() is appropriate +only when that field is not actually part of what makes two instances equal.

+

Code examples

+

Noncompliant code example

+
+final class OrderKey {
+  private final long id;
+  private final int version;
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof OrderKey key && id == key.id;
+  }
+
+  @Override
+  public int hashCode() { // Noncompliant: reads "version", which equals() never reads
+    return Objects.hash(id, version);
+  }
+}
+
+

Compliant solution

+
+final class OrderKey {
+  private final long id;
+  private final int version;
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof OrderKey key && id == key.id && version == key.version;
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(id, version);
+  }
+}
+
+

Simple getters that only return a field are resolved the same way as direct field reads.

+

Noncompliant code example

+
+final class Person {
+  private final String firstName;
+  private final String lastName;
+
+  String getFirstName() {
+    return firstName;
+  }
+
+  String getLastName() {
+    return lastName;
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof Person person && getFirstName().equals(person.getFirstName());
+  }
+
+  @Override
+  public int hashCode() { // Noncompliant: reads "lastName", which equals() never reads
+    return Objects.hash(getFirstName(), getLastName());
+  }
+}
+
+

Compliant solution

+
+final class Person {
+  private final String firstName;
+  private final String lastName;
+
+  String getFirstName() {
+    return firstName;
+  }
+
+  String getLastName() {
+    return lastName;
+  }
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof Person person
+      && getFirstName().equals(person.getFirstName())
+      && getLastName().equals(person.getLastName());
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(getFirstName(), getLastName());
+  }
+}
+
+

Exceptions

+

This rule only compares equals(Object) and hashCode() when both are declared directly in the same class. It does not +analyze inherited implementations, generated ones such as a record’s or a Lombok-annotated class’s, or static fields.

+

A field that only caches a previously computed hash value is not flagged, because that field does not add new identity state beyond what +equals() already compares:

+
+final class Point {
+  private final int x;
+  private final int y;
+  private int cachedHash;
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof Point point && x == point.x && y == point.y;
+  }
+
+  @Override
+  public int hashCode() {
+    if (cachedHash == 0) {
+      cachedHash = Objects.hash(x, y);
+    }
+    return cachedHash;
+  }
+}
+
+

The rule does not raise an issue when hashCode() delegates to a method whose field reads it cannot resolve, such as one that calls +another type’s methods or applies further logic beyond returning a field:

+
+final class Config {
+  private final Map<String, String> values;
+
+  @Override
+  public boolean equals(Object other) {
+    return other instanceof Config config && values.equals(config.values);
+  }
+
+  @Override
+  public int hashCode() {
+    return computeHash();
+  }
+
+  private int computeHash() {
+    return values.entrySet().stream().mapToInt(Object::hashCode).sum();
+  }
+}
+
+

Resources

+

Documentation

+ +

Related rules

+
    +
  • {rule:java:S1206} - "equals(Object obj)" and "hashCode()" should be overridden in pairs
  • +
  • {rule:java:S9350} - equals() implementations should not compare mismatched members
  • +
+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.json new file mode 100644 index 00000000000..2daf6cab2ff --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.json @@ -0,0 +1,23 @@ +{ + "title": "hashCode() and equals() should use consistent fields", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "suspicious" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9362", + "sqKey": "S9362", + "scope": "Main", + "quickfix": "unknown", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9362 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9362 new file mode 100644 index 00000000000..e69de29bb2d