SONARJAVA-6706 Implement new rule S2330 - #5867
Conversation
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
|
❌ Ruling needs updating. A fix PR has been created: #5868 Please review and merge it into your branch. |
|
❌ Ruling needs updating. A fix PR has been created: #5868 Please review and merge it into your branch. |
Ruling Diff SummaryDetected changes in 10 rule files: 0 issues removed, 155 issues added. S2330 (
|
Detect array covariance where an array of a derived type is assigned to a variable declared as an array of its base type, which can lead to ArrayStoreException at runtime.
Cover varargs method invocations with covariant array arguments, including multiple vararg arguments, mixed varargs with regular parameters, and edge cases like no vararg arguments passed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
594a768 to
2648f82
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Ruling needs updating. A fix PR has been created: #5875 Please review and merge it into your branch. |
|
❌ Ruling needs updating. A fix PR has been created: #5875 Please review and merge it into your branch. |
|
❌ Ruling needs updating. A fix PR has been created: #5880 Please review and merge it into your branch. |
…n S2330 Handle LAMBDA_EXPRESSION nodes directly to detect covariant array returns in expression-bodied lambdas (e.g. `() -> new Apple[1]`), which were previously missed since they have no RETURN_STATEMENT node. For varargs, check the first argument against the whole array type first, falling back to element-type check only if no issue was reported. This prevents double-reporting when an array-of-arrays argument matches both the vararg type and its element type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Ruling needs updating. A fix PR has been created: #5880 Please review and merge it into your branch. |
NoemieBenard
left a comment
NoemieBenard
left a comment
There was a problem hiding this comment.
Implementation looks good and the examples are very thorough, but I think the rule will be quite controversial. I already see some false positives worth addressing in the ruling diffs. Disabling the rule by default like in C# is also an option.
There was a problem hiding this comment.
The fact that the rule is in sonar way seems contradictory with the RSPEC and the choice of disabling it by default in C# , is it intentional?
| "tags": [ | ||
| "pitfall" | ||
| ], | ||
| "defaultSeverity": "Critical", |
There was a problem hiding this comment.
Default severity should be "Major" to match "RELIABILITY": "MEDIUM".
| "type": "CODE_SMELL", | ||
| "code": { | ||
| "impacts": { | ||
| "RELIABILITY": "MEDIUM" |
There was a problem hiding this comment.
Is it intentional to modify the metadata from the C# RSPEC? To me it makes sense to keep the "MAINTAINABILITY" impact together with the RELIABILITY impact (maybe with lower maintainability than reliability)
| 494 | ||
| ], | ||
| "commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/MethodUtils.java": [ | ||
| 1352, |
There was a problem hiding this comment.
Here the rule raises false positives on both parameters of Array.equals because the method takes parameters of type Object[] and these are arrays with a different sub-type. We should probably exclude standard library methods from the check (though there will probably be a high FP rate for the rule anyway). Based on the ruling diffs there is the same issue for Array.fill
…m S2330 These standard library methods are safe with covariant arrays because they either only read from the array or write type-safe values. Flagging them produces false positives (e.g. Arrays.fill, Arrays.equals, Arrays.sort). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
❌ Ruling needs updating. A fix PR has been created: #5887 Please review and merge it into your branch. |
CI failed: Ruling QA integration tests failed due to expected issue differences (63 differences) introduced by implementing the new rule S2330.OverviewBoth Linux and Windows CI pipelines encountered test failures in the ruling QA test suite ( FailuresRuling QA Integration Test Differences (confidence: high)
Summary
Code Review ✅ Approved 2 resolved / 2 findingsImplements rule S2330 to detect array covariance issues, addressing the expression-bodied lambdas and double-checked varargs findings. No issues found. ✅ 2 resolved✅ Edge Case: Expression-bodied lambdas escape covariance detection
✅ Edge Case: First vararg argument is checked twice in visitInvocation
Tip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
2 participants
Detect array covariance where an array of a derived type is assigned to a variable declared as an array of its base type, which can lead to ArrayStoreException at runtime.