SONARJAVA-6790: Implement S9350: equals() implementations should not compare mismatched members

[nathsou]

Summary

• Implement S9350 in SonarJava as a native check that flags equals() implementations comparing unmatched instance fields or getters of the enclosing type.
• Cover ==/!=, Objects.equals, Guava Objects.equal, Arrays.equals, instance equals, and 0-arg getters/record accessors, including the swapped-pair exception for order-independent equality.

Links

• Jira: https://sonarsource.atlassian.net/browse/SONARJAVA-6790
• RSPEC PR: https://github.com/SonarSource/rspec/pull/7901

AI disclosure

• LLM model used for implementation: cursor-grok-4.6-high

[hashicorp-vault-sonar-prod]

SONARJAVA-6790

[gitar-bot]

💡 Edge Case: Per-statement swap exception misses order-independent equality split across statements

The swapped-pair exception is now scoped per enclosing statement (pairsByStatement.get(comparison.statement)), so an order-independent equality whose forward and reversed comparisons live in different statements will no longer be recognized as symmetric and will be reported as a false positive. Example: boolean f = this.a == that.b; on one line and boolean b = this.b == that.a; on another produces two issues even though together they express symmetric equality. This is uncommon, but if you want to preserve suppression for such cases consider also matching reversed pairs found anywhere in the same method while keeping the per-statement grouping for the DistinctStatements case.

_{Was this helpful? React with 👍 / 👎}

[github-actions]

❌ Ruling needs updating. A fix PR has been created: #5948

Please review and merge it into your branch.

[romainbrenguier]

It needs a test without semantic, otherwise it looks ok

[romainbrenguier]

We should have a similar test using .withoutSemantic

[nathsou]

Added test_without_semantic() on the compiling sample. The check still reports the same issues from intra-file symbols when no bytecode is provided, so this uses .withoutSemantic().verifyIssues() like SillyEqualsCheckTest.

[gitar-bot]

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Implements rule S9350 to flag equals() implementations comparing mismatched members with comprehensive test coverage and defensive branch handling. Consider expanding the per-statement swap exception to catch order-independent equality splits across multiple statements.

💡 Edge Case: Per-statement swap exception misses order-independent equality split across statements

📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:94-101 📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:269-275

The swapped-pair exception is now scoped per enclosing statement (pairsByStatement.get(comparison.statement)), so an order-independent equality whose forward and reversed comparisons live in different statements will no longer be recognized as symmetric and will be reported as a false positive. Example: boolean f = this.a == that.b; on one line and boolean b = this.b == that.a; on another produces two issues even though together they express symmetric equality. This is uncommon, but if you want to preserve suppression for such cases consider also matching reversed pairs found anywhere in the same method while keeping the per-statement grouping for the DistinctStatements case.

✅ 3 resolved

✅ Edge Case: Mismatched-member check ignores which instance operands belong to

📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:145-159 📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:46-47
addIfDubious/member extract only the field or getter symbol and never verify that one operand is accessed on this while the other is accessed on the compared parameter. As a result, a comparison of two different members of the same instance is flagged as a violation. For example this.start == this.end or that.start == that.end inside equals (a legitimate normalization/guard check) is reported as "pairing start with end breaks the equality contract", a false positive. The secondary location labels (SECONDARY_THIS="Compared member on this", SECONDARY_OTHER="...on the other instance") also become inaccurate in these cases. Consider only pairing when the two operands reference different instances (one on this/implicit, the other on the pattern/parameter variable) before recording a ComparisonSite.

✅ Quality: MemberRef.symbol component is now unused after name-based switch

📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:185-199 📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:285
This commit removed the last reads of MemberRef.symbol (comparison in addIfDubious and pair() now use displayName), leaving the symbol record component dead — it is still populated in field()/getter() but never read. Drop the symbol component from the MemberRef record (and its constructor arguments) to remove the dead state and clarify that member identity is now name-based.

✅ Edge Case: Name-based equal fallback can match unrelated 2-arg methods

📄 java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:192-202
isTwoArgEqualityHelper falls back to matching any static-or-unknown method literally named equal taking 2 arguments. A user-defined static helper named equal(x, y) that is not an equality comparison would be treated as one, so equal(this.a, that.b) would be flagged even though the method has nothing to do with equality. Consider restricting the fallback to known equality helper types, or requiring a boolean return type, to reduce false positives.

🤖 Prompt for agents

Code Review: Implements rule S9350 to flag `equals()` implementations comparing mismatched members with comprehensive test coverage and defensive branch handling. Consider expanding the per-statement swap exception to catch order-independent equality splits across multiple statements.

1. 💡 Edge Case: Per-statement swap exception misses order-independent equality split across statements
   Files: java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:94-101, java-checks/src/main/java/org/sonar/java/checks/EqualsMismatchedMembersCheck.java:269-275

   The swapped-pair exception is now scoped per enclosing statement (`pairsByStatement.get(comparison.statement)`), so an order-independent equality whose forward and reversed comparisons live in different statements will no longer be recognized as symmetric and will be reported as a false positive. Example: `boolean f = this.a == that.b;` on one line and `boolean b = this.b == that.a;` on another produces two issues even though together they express symmetric equality. This is uncommon, but if you want to preserve suppression for such cases consider also matching reversed pairs found anywhere in the same method while keeping the per-statement grouping for the DistinctStatements case.

Implementation Status ✅ 1 / 1 issues implemented

✅ SONARJAVA-6790 — 1 / 1 objectives

The PR successfully implements rule S9350 with comprehensive unit tests and documentation to detect mismatched members in equals() implementations.

✅ 1 complete

• ✅ Implement rule S9350 to detect when equals() implementations compare mismatched members

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
91.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube