From cc86f6f2f76be05cf65b5f61c14b60ad08169c15 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 12:14:39 +0200 Subject: [PATCH 1/6] SONARJAVA-6839: Implement S9362: hashCode() and equals() should use consistent fields --- .../HashCodeMismatchedFieldsCheckSample.java | 317 ++++++++++++++++++ .../checks/HashCodeMismatchedFieldsCheck.java | 216 ++++++++++++ .../HashCodeMismatchedFieldsCheckTest.java | 43 +++ .../org/sonar/l10n/java/rules/java/S9362.html | 173 ++++++++++ .../org/sonar/l10n/java/rules/java/S9362.json | 23 ++ .../main/resources/profiles/Sonar_way/S9362 | 0 6 files changed, 772 insertions(+) create mode 100644 java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.html create mode 100644 sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9362.json create mode 100644 sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9362 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..5b1a9ad9472 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java @@ -0,0 +1,317 @@ +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); // secondary=-1 + } + } + + 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()); // secondary=-1 + } + } + + 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.}} [[secondary=-1,-1]] + return Objects.hash(id, version, tag); + } + } + + static class MemoizedHash { + private final int x; + private final int y; + private int cachedHash; + + 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 (cachedHash == 0) { + cachedHash = Objects.hash(x, y); + } + return cachedHash; + } + } + + 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 { + abstract boolean equals(Object other); + + abstract int hashCode(); + } + + interface HasIdentity { + boolean equals(Object other); + + int hashCode(); + } +} 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..4f10e809a55 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -0,0 +1,216 @@ +/* + * 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.LinkedHashMap; +import java.util.List; +import java.util.Locale; +import java.util.Map; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.MethodTreeUtils; +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.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()"; + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + Symbol owner = classTree.symbol(); + if (owner.isUnknown() || owner.type().isUnknown()) { + return; + } + + 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; + } + + Map> fieldsByHelper = collectHelperFields(owner, otherMethods); + + FieldReadCollector equalsFields = scan(equalsMethod, owner, fieldsByHelper, Role.EQUALS); + FieldReadCollector hashCodeFields = scan(hashCodeMethod, owner, fieldsByHelper, Role.HASH_CODE); + if (equalsFields.failed || hashCodeFields.failed || equalsFields.fields.isEmpty()) { + // Bail out on unresolved members, or when equals() compares no state (likely reference equality). + return; + } + + Map extraFields = new LinkedHashMap<>(hashCodeFields.fields); + extraFields.keySet().removeAll(equalsFields.fields.keySet()); + // A field caching a previously computed hash value does not add new identity state. + extraFields.keySet().removeIf(field -> field.name().toLowerCase(Locale.ROOT).contains("hash")); + if (extraFields.isEmpty()) { + return; + } + + reportMismatch(hashCodeMethod, extraFields); + } + + private static Map> collectHelperFields(Symbol owner, List otherMethods) { + Map> fieldsByHelper = new HashMap<>(); + for (MethodTree helper : otherMethods) { + Symbol.MethodSymbol helperSymbol = helper.symbol(); + if (helperSymbol.isUnknown() || helperSymbol.isStatic() || !helper.parameters().isEmpty()) { + continue; + } + FieldReadCollector collector = scan(helper, owner, Map.of(), Role.HELPER); + if (!collector.failed) { + fieldsByHelper.put(helperSymbol, collector.fields); + } + } + return fieldsByHelper; + } + + private static FieldReadCollector scan(MethodTree method, Symbol owner, Map> fieldsByHelper, Role role) { + FieldReadCollector collector = new FieldReadCollector(owner, fieldsByHelper, role); + method.block().accept(collector); + return collector; + } + + 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 + } + + /** + * 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 final Map fields = new LinkedHashMap<>(); + private boolean failed; + + 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 (!failed) { + String name = tree.name(); + if (!"this".equals(name) && !"super".equals(name)) { + Symbol symbol = tree.symbol(); + if (symbol.isUnknown()) { + failed = true; + } else if (symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { + fields.putIfAbsent(symbol, tree); + } + } + } + super.visitIdentifier(tree); + } + + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + if (!failed) { + Symbol.MethodSymbol symbol = tree.methodSymbol(); + if (symbol.isUnknown()) { + failed = true; + } else { + Map helperFields = fieldsByHelper.get(symbol); + if (helperFields != null) { + helperFields.forEach(fields::putIfAbsent); + } else if (!symbol.isStatic() && !isAllowedInstanceCall(symbol)) { + failed = true; + } + } + } + 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..997c7ad9b38 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java @@ -0,0 +1,43 @@ +/* + * 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; + +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(); + } +} 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 From a000bcc83d9060f02425a9cf5eb0929520bdc13d Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 12:48:30 +0200 Subject: [PATCH 2/6] SONARJAVA-6839: Address review findings for S9362 - Fix a compile error in the test sample (package-private abstract equals/hashCode illegally narrowing Object's public methods). - Fail the scan instead of silently ignoring same-class static helper calls and non-standard same-class equals(SpecificType) overloads, which could otherwise hide field reads and cause false positives. - Replace non-functional [[secondary=N]] test assertions (not understood by the modern CheckVerifier) with real underline-caret secondary location assertions, verified to actually fail on corruption. - Recognize memoized hash fields by assignment inside hashCode(), not just by name. - Visit Tree.Kind.RECORD so hand-written record equals/hashCode are covered. - Add non-compiling regression tests for the isUnknown() bail-out paths. --- ...smatchedFieldsCheckSampleNonCompiling.java | 46 ++++++++++ .../HashCodeMismatchedFieldsCheckSample.java | 89 +++++++++++++++++-- .../checks/HashCodeMismatchedFieldsCheck.java | 33 +++++-- .../HashCodeMismatchedFieldsCheckTest.java | 10 +++ 4 files changed, 167 insertions(+), 11 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/files/non-compiling/checks/HashCodeMismatchedFieldsCheckSampleNonCompiling.java 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 index 5b1a9ad9472..f6aff2ec1b7 100644 --- a/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java @@ -21,7 +21,9 @@ public boolean equals(Object other) { @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); // secondary=-1 +// ^^^^^^^^ + return Objects.hash(id, version); +// ^^^^^^^< {{Not compared in equals()}} } } @@ -60,6 +62,7 @@ String getFirstName() { String getLastName() { return lastName; +// ^^^^^^^^> {{Not compared in equals()}} } @Override @@ -69,7 +72,8 @@ public boolean equals(Object other) { @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()); // secondary=-1 +// ^^^^^^^^ + return Objects.hash(getFirstName(), getLastName()); } } @@ -120,8 +124,14 @@ public boolean equals(Object other) { } @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.}} [[secondary=-1,-1]] - return Objects.hash(id, version, tag); + 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; } } @@ -304,9 +314,9 @@ public int hashCode() { } abstract static class AbstractPair { - abstract boolean equals(Object other); + public abstract boolean equals(Object other); - abstract int hashCode(); + public abstract int hashCode(); } interface HasIdentity { @@ -314,4 +324,71 @@ interface HasIdentity { 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); + } + } + + 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 index 4f10e809a55..db4ec17af6a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -18,15 +18,18 @@ 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.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; @@ -47,7 +50,7 @@ public class HashCodeMismatchedFieldsCheck extends IssuableSubscriptionVisitor { @Override public List nodesToVisit() { - return List.of(Tree.Kind.CLASS); + return List.of(Tree.Kind.CLASS, Tree.Kind.RECORD); } @Override @@ -88,8 +91,9 @@ public void visitNode(Tree tree) { Map extraFields = new LinkedHashMap<>(hashCodeFields.fields); extraFields.keySet().removeAll(equalsFields.fields.keySet()); - // A field caching a previously computed hash value does not add new identity state. - extraFields.keySet().removeIf(field -> field.name().toLowerCase(Locale.ROOT).contains("hash")); + // 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 -> field.name().toLowerCase(Locale.ROOT).contains("hash") || hashCodeFields.assignedFields.contains(field)); if (extraFields.isEmpty()) { return; } @@ -101,7 +105,7 @@ private static Map> collectHelperFields(S Map> fieldsByHelper = new HashMap<>(); for (MethodTree helper : otherMethods) { Symbol.MethodSymbol helperSymbol = helper.symbol(); - if (helperSymbol.isUnknown() || helperSymbol.isStatic() || !helper.parameters().isEmpty()) { + if (helperSymbol.isUnknown() || !helper.parameters().isEmpty()) { continue; } FieldReadCollector collector = scan(helper, owner, Map.of(), Role.HELPER); @@ -147,6 +151,7 @@ private static final class FieldReadCollector extends BaseTreeVisitor { private final Map> fieldsByHelper; private final Role role; private final Map fields = new LinkedHashMap<>(); + private final Set assignedFields = new HashSet<>(); private boolean failed; private FieldReadCollector(Symbol enclosingClass, Map> fieldsByHelper, Role role) { @@ -176,6 +181,17 @@ public void visitIdentifier(IdentifierTree tree) { super.visitIdentifier(tree); } + @Override + public void visitAssignmentExpression(AssignmentExpressionTree tree) { + if (tree.variable() instanceof IdentifierTree identifier) { + Symbol symbol = identifier.symbol(); + if (!symbol.isUnknown() && symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { + assignedFields.add(symbol); + } + } + super.visitAssignmentExpression(tree); + } + @Override public void visitMethodInvocation(MethodInvocationTree tree) { if (!failed) { @@ -186,7 +202,14 @@ public void visitMethodInvocation(MethodInvocationTree tree) { Map helperFields = fieldsByHelper.get(symbol); if (helperFields != null) { helperFields.forEach(fields::putIfAbsent); - } else if (!symbol.isStatic() && !isAllowedInstanceCall(symbol)) { + } 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. + failed = ownedByEnclosing(symbol); + } 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. failed = true; } } 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 index 997c7ad9b38..89d67930a7d 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheckTest.java @@ -20,6 +20,7 @@ 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 { @@ -40,4 +41,13 @@ void test_without_semantic() { .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(); + } } From 6c06befd0f4f4c918f476ecc99b2336d3152e3e6 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 13:02:57 +0200 Subject: [PATCH 3/6] SONARJAVA-6839: Fix S9362 CI (quality gate) failures - Extract the equals()/hashCode() member classification and the extra- fields computation out of visitNode() into helper methods to bring its Cognitive Complexity back under the threshold. - Remove a defensive but effectively unreachable early-return (a class's own symbol/type is always known while visiting its own declaration), which was dragging down new-code coverage. - Add regression tests for the getClass() allow-list and for pruning field reads inside a nested/anonymous class declared inside equals(), closing the remaining coverage gaps flagged by the SonarQube quality gate (89.6% -> fully covered new lines). --- .../HashCodeMismatchedFieldsCheckSample.java | 30 ++++++++ .../checks/HashCodeMismatchedFieldsCheck.java | 75 ++++++++++++------- 2 files changed, 80 insertions(+), 25 deletions(-) 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 index f6aff2ec1b7..c0093f2c9ad 100644 --- a/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java @@ -377,6 +377,36 @@ public int hashCode() { } } + 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 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 index db4ec17af6a..404ec735da0 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -57,48 +57,73 @@ public List nodesToVisit() { public void visitNode(Tree tree) { ClassTree classTree = (ClassTree) tree; Symbol owner = classTree.symbol(); - if (owner.isUnknown() || owner.type().isUnknown()) { - return; - } - 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) { + EqualsAndHashCode methods = EqualsAndHashCode.find(classTree); + if (methods == null) { return; } - Map> fieldsByHelper = collectHelperFields(owner, otherMethods); + Map> fieldsByHelper = collectHelperFields(owner, methods.otherMethods); - FieldReadCollector equalsFields = scan(equalsMethod, owner, fieldsByHelper, Role.EQUALS); - FieldReadCollector hashCodeFields = scan(hashCodeMethod, owner, fieldsByHelper, Role.HASH_CODE); + FieldReadCollector equalsFields = scan(methods.equalsMethod, owner, fieldsByHelper, Role.EQUALS); + FieldReadCollector hashCodeFields = scan(methods.hashCodeMethod, owner, fieldsByHelper, Role.HASH_CODE); if (equalsFields.failed || hashCodeFields.failed || equalsFields.fields.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(FieldReadCollector equalsFields, FieldReadCollector hashCodeFields) { Map extraFields = new LinkedHashMap<>(hashCodeFields.fields); extraFields.keySet().removeAll(equalsFields.fields.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 -> field.name().toLowerCase(Locale.ROOT).contains("hash") || hashCodeFields.assignedFields.contains(field)); - if (extraFields.isEmpty()) { - return; + 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; } - reportMismatch(hashCodeMethod, extraFields); + 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) { From c715ba7d275739b199a9fbb653a759cf21769629 Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 16:15:04 +0200 Subject: [PATCH 4/6] SONARJAVA-6839: Address PR review feedback for S9362 Tighten cached-hash detection, report delegated field reads at their hashCode call sites, and recognize qualified memoization assignments to prevent false results and improve issue locations. --- .../HashCodeMismatchedFieldsCheckSample.java | 50 +++++++++++++++++-- .../checks/HashCodeMismatchedFieldsCheck.java | 23 ++++++--- 2 files changed, 61 insertions(+), 12 deletions(-) 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 index c0093f2c9ad..528ffe7f239 100644 --- a/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/HashCodeMismatchedFieldsCheckSample.java @@ -62,7 +62,6 @@ String getFirstName() { String getLastName() { return lastName; -// ^^^^^^^^> {{Not compared in equals()}} } @Override @@ -74,6 +73,7 @@ public boolean equals(Object other) { 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()}} } } @@ -138,7 +138,7 @@ public int hashCode() { // Noncompliant {{This hashCode() implementation is inco static class MemoizedHash { private final int x; private final int y; - private int cachedHash; + private int hc; MemoizedHash(int x, int y) { this.x = x; @@ -152,13 +152,55 @@ public boolean equals(Object other) { @Override public int hashCode() { - if (cachedHash == 0) { - cachedHash = Objects.hash(x, y); + 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; 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 index 404ec735da0..1a75ec5e7be 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -26,6 +26,7 @@ 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; @@ -47,6 +48,14 @@ 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() { @@ -83,7 +92,8 @@ private static Map computeExtraFields(FieldReadCollector equalsFie extraFields.keySet().removeAll(equalsFields.fields.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 -> field.name().toLowerCase(Locale.ROOT).contains("hash") || hashCodeFields.assignedFields.contains(field)); + extraFields.keySet().removeIf(field -> + MEMOIZED_HASH_FIELD_NAMES.contains(field.name().toLowerCase(Locale.ROOT)) || hashCodeFields.assignedFields.contains(field)); return extraFields; } @@ -208,12 +218,9 @@ public void visitIdentifier(IdentifierTree tree) { @Override public void visitAssignmentExpression(AssignmentExpressionTree tree) { - if (tree.variable() instanceof IdentifierTree identifier) { - Symbol symbol = identifier.symbol(); - if (!symbol.isUnknown() && symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { - assignedFields.add(symbol); - } - } + ExpressionUtils.extractIdentifierSymbol(tree.variable()) + .filter(symbol -> !symbol.isUnknown() && symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) + .ifPresent(assignedFields::add); super.visitAssignmentExpression(tree); } @@ -226,7 +233,7 @@ public void visitMethodInvocation(MethodInvocationTree tree) { } else { Map helperFields = fieldsByHelper.get(symbol); if (helperFields != null) { - helperFields.forEach(fields::putIfAbsent); + helperFields.keySet().forEach(field -> fields.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) From f4b248806a9b0b163f4e527d2b018899e4ed249f Mon Sep 17 00:00:00 2001 From: nathsou Date: Mon, 24 Aug 2026 16:50:49 +0200 Subject: [PATCH 5/6] SONARJAVA-6839: Clarify S9362 field collection Rename the collection operation and separate its result from the mutable AST visitor so callers consume an explicit read-and-assigned-fields value. --- .../checks/HashCodeMismatchedFieldsCheck.java | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) 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 index 1a75ec5e7be..7c182127277 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -74,9 +74,9 @@ public void visitNode(Tree tree) { Map> fieldsByHelper = collectHelperFields(owner, methods.otherMethods); - FieldReadCollector equalsFields = scan(methods.equalsMethod, owner, fieldsByHelper, Role.EQUALS); - FieldReadCollector hashCodeFields = scan(methods.hashCodeMethod, owner, fieldsByHelper, Role.HASH_CODE); - if (equalsFields.failed || hashCodeFields.failed || equalsFields.fields.isEmpty()) { + 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; } @@ -87,9 +87,9 @@ public void visitNode(Tree tree) { } } - private static Map computeExtraFields(FieldReadCollector equalsFields, FieldReadCollector hashCodeFields) { - Map extraFields = new LinkedHashMap<>(hashCodeFields.fields); - extraFields.keySet().removeAll(equalsFields.fields.keySet()); + 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 -> @@ -143,18 +143,22 @@ private static Map> collectHelperFields(S if (helperSymbol.isUnknown() || !helper.parameters().isEmpty()) { continue; } - FieldReadCollector collector = scan(helper, owner, Map.of(), Role.HELPER); - if (!collector.failed) { - fieldsByHelper.put(helperSymbol, collector.fields); + ReadAndAssignedFields fields = collectReadFields(helper, owner, Map.of(), Role.HELPER); + if (!fields.failed) { + fieldsByHelper.put(helperSymbol, fields.readFields); } } return fieldsByHelper; } - private static FieldReadCollector scan(MethodTree method, Symbol owner, Map> fieldsByHelper, Role role) { + 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; + return new ReadAndAssignedFields(collector.readFields, collector.assignedFields, collector.failed); } private void reportMismatch(MethodTree hashCodeMethod, Map extraFields) { @@ -175,6 +179,19 @@ private enum Role { HASH_CODE } + private static final class ReadAndAssignedFields { + + private final Map readFields; + private final Set assignedFields; + private final boolean failed; + + private ReadAndAssignedFields(Map readFields, Set assignedFields, boolean failed) { + this.readFields = readFields; + this.assignedFields = assignedFields; + this.failed = failed; + } + } + /** * 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 @@ -185,7 +202,7 @@ private static final class FieldReadCollector extends BaseTreeVisitor { private final Symbol enclosingClass; private final Map> fieldsByHelper; private final Role role; - private final Map fields = new LinkedHashMap<>(); + private final Map readFields = new LinkedHashMap<>(); private final Set assignedFields = new HashSet<>(); private boolean failed; @@ -209,7 +226,7 @@ public void visitIdentifier(IdentifierTree tree) { if (symbol.isUnknown()) { failed = true; } else if (symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { - fields.putIfAbsent(symbol, tree); + readFields.putIfAbsent(symbol, tree); } } } @@ -233,7 +250,7 @@ public void visitMethodInvocation(MethodInvocationTree tree) { } else { Map helperFields = fieldsByHelper.get(symbol); if (helperFields != null) { - helperFields.keySet().forEach(field -> fields.putIfAbsent(field, tree)); + helperFields.keySet().forEach(field -> 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) From 8b7428faa93ee1834788dbe4529ffbed7c6d6229 Mon Sep 17 00:00:00 2001 From: nathsou Date: Tue, 25 Aug 2026 10:04:04 +0200 Subject: [PATCH 6/6] SONARJAVA-6839: Consolidate S9362 collection state Represent collected field data as a record and let the AST visitor own a single result value instead of duplicating its components. --- .../checks/HashCodeMismatchedFieldsCheck.java | 54 +++++++++---------- 1 file changed, 26 insertions(+), 28 deletions(-) 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 index 7c182127277..e2ca285604e 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/HashCodeMismatchedFieldsCheck.java @@ -76,7 +76,7 @@ public void visitNode(Tree tree) { 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()) { + if (equalsFields.failed() || hashCodeFields.failed() || equalsFields.readFields().isEmpty()) { // Bail out on unresolved members, or when equals() compares no state (likely reference equality). return; } @@ -88,12 +88,12 @@ public void visitNode(Tree tree) { } private static Map computeExtraFields(ReadAndAssignedFields equalsFields, ReadAndAssignedFields hashCodeFields) { - Map extraFields = new LinkedHashMap<>(hashCodeFields.readFields); - extraFields.keySet().removeAll(equalsFields.readFields.keySet()); + 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)); + MEMOIZED_HASH_FIELD_NAMES.contains(field.name().toLowerCase(Locale.ROOT)) || hashCodeFields.assignedFields().contains(field)); return extraFields; } @@ -143,9 +143,9 @@ private static Map> collectHelperFields(S if (helperSymbol.isUnknown() || !helper.parameters().isEmpty()) { continue; } - ReadAndAssignedFields fields = collectReadFields(helper, owner, Map.of(), Role.HELPER); - if (!fields.failed) { - fieldsByHelper.put(helperSymbol, fields.readFields); + ReadAndAssignedFields helperFields = collectReadFields(helper, owner, Map.of(), Role.HELPER); + if (!helperFields.failed()) { + fieldsByHelper.put(helperSymbol, helperFields.readFields()); } } return fieldsByHelper; @@ -158,7 +158,7 @@ private static ReadAndAssignedFields collectReadFields( Role role) { FieldReadCollector collector = new FieldReadCollector(owner, fieldsByHelper, role); method.block().accept(collector); - return new ReadAndAssignedFields(collector.readFields, collector.assignedFields, collector.failed); + return collector.fields; } private void reportMismatch(MethodTree hashCodeMethod, Map extraFields) { @@ -179,16 +179,14 @@ private enum Role { HASH_CODE } - private static final class ReadAndAssignedFields { + private record ReadAndAssignedFields(Map readFields, Set assignedFields, boolean failed) { - private final Map readFields; - private final Set assignedFields; - private final boolean failed; + private ReadAndAssignedFields() { + this(new LinkedHashMap<>(), new HashSet<>(), false); + } - private ReadAndAssignedFields(Map readFields, Set assignedFields, boolean failed) { - this.readFields = readFields; - this.assignedFields = assignedFields; - this.failed = failed; + private ReadAndAssignedFields asFailed() { + return new ReadAndAssignedFields(readFields, assignedFields, true); } } @@ -202,9 +200,7 @@ private static final class FieldReadCollector extends BaseTreeVisitor { private final Symbol enclosingClass; private final Map> fieldsByHelper; private final Role role; - private final Map readFields = new LinkedHashMap<>(); - private final Set assignedFields = new HashSet<>(); - private boolean failed; + private ReadAndAssignedFields fields = new ReadAndAssignedFields(); private FieldReadCollector(Symbol enclosingClass, Map> fieldsByHelper, Role role) { this.enclosingClass = enclosingClass; @@ -219,14 +215,14 @@ public void visitClass(ClassTree tree) { @Override public void visitIdentifier(IdentifierTree tree) { - if (!failed) { + if (!fields.failed()) { String name = tree.name(); if (!"this".equals(name) && !"super".equals(name)) { Symbol symbol = tree.symbol(); if (symbol.isUnknown()) { - failed = true; + fields = fields.asFailed(); } else if (symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) { - readFields.putIfAbsent(symbol, tree); + fields.readFields().putIfAbsent(symbol, tree); } } } @@ -237,29 +233,31 @@ public void visitIdentifier(IdentifierTree tree) { public void visitAssignmentExpression(AssignmentExpressionTree tree) { ExpressionUtils.extractIdentifierSymbol(tree.variable()) .filter(symbol -> !symbol.isUnknown() && symbol.isVariableSymbol() && !symbol.isStatic() && ownedByEnclosing(symbol)) - .ifPresent(assignedFields::add); + .ifPresent(fields.assignedFields()::add); super.visitAssignmentExpression(tree); } @Override public void visitMethodInvocation(MethodInvocationTree tree) { - if (!failed) { + if (!fields.failed()) { Symbol.MethodSymbol symbol = tree.methodSymbol(); if (symbol.isUnknown()) { - failed = true; + fields = fields.asFailed(); } else { Map helperFields = fieldsByHelper.get(symbol); if (helperFields != null) { - helperFields.keySet().forEach(field -> readFields.putIfAbsent(field, tree)); + 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. - failed = ownedByEnclosing(symbol); + 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. - failed = true; + fields = fields.asFailed(); } } }