Skip to content

SONARJAVA-6759: Avoid overlap between S1244 and S9147 for NaN comparisons - #5937

Open
romainbrenguier wants to merge 5 commits into
masterfrom
romain/sonarjava-6759
Open

SONARJAVA-6759: Avoid overlap between S1244 and S9147 for NaN comparisons#5937
romainbrenguier wants to merge 5 commits into
masterfrom
romain/sonarjava-6759

Conversation

@romainbrenguier

@romainbrenguier romainbrenguier commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • S1244 (FloatEqualityCheck) no longer raises issues when a float/double is compared to Double.NaN or Float.NaN, since this case is already covered by S9147 (NanEqualityCheck)
  • Added S1244 as a related rule in the S9147 Rspec HTML documentation
    See rspec PR: https://github.com/SonarSource/rspec/pull/7904
  • Added test cases in FloatEquality.java to verify NaN comparisons are compliant for S1244

Test plan

  • FloatEqualityCheckTest passes (both with and without semantic analysis)
  • NanEqualityCheckTest passes (unchanged)
  • CI integration tests pass

🤖 Generated with Claude Code

…sons

S1244 no longer raises issues when a float/double is compared to
Double.NaN or Float.NaN, since this case is already covered by S9147.
Also adds S1244 as a related rule in the S9147 Rspec.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@hashicorp-vault-sonar-prod

hashicorp-vault-sonar-prod Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

SONARJAVA-6759

Comment thread java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java Outdated
romainbrenguier and others added 4 commits August 18, 2026 11:34
…coverage

Move duplicated NaN detection logic from FloatEqualityCheck and
NanEqualityCheck into ExpressionUtils.getNanOwnerTypeName(), and add
additional test cases for Float.NaN parenthesized and self-comparison
patterns to improve coverage on new code.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e gate

Add comprehensive tests for ExpressionUtils.getNanOwnerTypeName() covering
all branches: member select (Double.NaN, Float.NaN), static imports,
parenthesized expressions, unknown symbols, and non-NaN expressions.
Also extend FloatEquality test sample with edge cases for non-NaN member
selects and cross-type NaN comparisons.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rTypeName edge cases

Cover lines 417 (identifier not named NaN), 431 (null owner/type), and 437
(owner type not Double/Float) in ExpressionUtils to fix coverage gate.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…uality Gate

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@romainbrenguier
romainbrenguier marked this pull request as ready for review August 18, 2026 11:53
@gitar-bot

gitar-bot Bot commented Aug 18, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 1 resolved / 1 findings

Extracts shared NaN detection logic to prevent rule overlap between S1244 and S9147, addressing the NaN detection logic duplication finding. No issues found.

✅ 1 resolved
Quality: NaN detection logic duplicated across S1244 and S9147

📄 java-checks/src/main/java/org/sonar/java/checks/FloatEqualityCheck.java:100-114
isNanExpression/isFloatingPointNan in FloatEqualityCheck are near-identical copies of getNanTypeName/resolveNanOwnerType in NanEqualityCheck. Since the two must stay in sync for the overlap-avoidance to remain correct, any future change to NaN detection in one rule must be manually mirrored in the other, risking divergence. Consider extracting a shared helper (e.g. in ExpressionUtils or a common util) so both rules reference one source of truth.

Implementation Status ✅ 1 / 1 issues implemented
SONARJAVA-6759 — 2 / 2 objectives

The PR successfully updates FloatEqualityCheck (S1244) to avoid raising on NaN comparisons and marks S9147 as related to S1244 in the Rspec.

✅ 2 complete
  • ✅ Ensure S1244 does not raise when a float is compared to NaN
  • ✅ Mark S9147 as related to S1244 in the Rspec
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

Copy link
Copy Markdown
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant