SONARJAVA-6839: Implement S9362: hashCode() and equals() should use consistent fields - #6004
Conversation
This comment has been minimized.
This comment has been minimized.
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
- 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.
- 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).
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.
romainbrenguier
left a comment
romainbrenguier
left a comment
There was a problem hiding this comment.
It looks ok but I think we can clarify a bit the Collector class part.
Rename the collection operation and separate its result from the mutable AST visitor so callers consume an explicit read-and-assigned-fields value.
romainbrenguier
left a comment
romainbrenguier
left a comment
There was a problem hiding this comment.
Some non-blocking comments.
| private final Map<Symbol, Tree> readFields = new LinkedHashMap<>(); | ||
| private final Set<Symbol> assignedFields = new HashSet<>(); | ||
| private boolean failed; |
There was a problem hiding this comment.
These could have been replaced with a ReadAndAssignedFields field
There was a problem hiding this comment.
Addressed in 8b7428f. FieldReadCollector now owns one initialized ReadAndAssignedFields field instead of separately declaring its read fields, assigned fields, and failure state. When a scan fails, the collector replaces that record with an asFailed() copy while retaining the collected maps.
Validation: mvn -pl java-checks -am test -Dtest=HashCodeMismatchedFieldsCheckTest -Dsurefire.failIfNoSpecifiedTests=false passes.
Represent collected field data as a record and let the AST visitor own a single result value instead of duplicating its components.
Code Review ✅ Approved 3 resolved / 3 findingsImplements rule S9362 to flag hashCode() methods that read instance fields not used by equals(), ensuring contract consistency. Addressed secondary location placement and memoization detection feedback. ✅ 3 resolved✅ Quality: Secondary location for getter-delegated fields points inside getter
✅ Edge Case: Memoization detection misses
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|
2 participants
Summary
S9362(HashCodeMismatchedFieldsCheck): flag a locally declaredhashCode()that reads an instance field which the class'sequals(Object)override never reads, breaking theObject.hashCode()contract.hashCode()method name, with one secondary location per mismatched field."hash"), and bails out (no issue) on any unresolved symbol, unresolvable helper call, or anequals()that compares no state.HashCodeMismatchedFieldsCheckSample.javacovering both RSPEC examples plus the memoized-hash, unresolvable-helper, inherited-field, static-field, and asymmetric-direction exceptions, and a.withoutSemantic()CheckVerifiertest (the sample has no external dependency, so local symbol resolution still succeeds without semantic info; documented in the test).S9362.html/S9362.json/profiles/Sonar_way/S9362from the RSPEC branch withrule-api generate.Links
AI disclosure
cursor-grok-4.6-highrouting tier was not delegated to a separate subagent for this step)