SONARJAVA-4426 S5778 allow calling factory methods to create empty collections#5633
Conversation
rombirli
left a comment
There was a problem hiding this comment.
It seems quite difficult to maintain an exhaustive list of all commonly used functions that never throw an exception. It might make more sense to rethink the underlying implementation of this rule in the future. That said, I'm approving this PR anyway, as it should still eliminate a solid number of false positives.
| "ruleKey": "S5778", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 32, | ||
| "falseNegatives": 41, |
There was a problem hiding this comment.
I'm not sure to understand correctly what this value is, but is it expected that we have more FN than before? Even if TPs are missclassified as FNs we shouldn't have more issues raised than before by just expanding the whitelist
There was a problem hiding this comment.
For some reason, this check does not work without semantics, so nothing is reported in autoscan. The new false negatives are the additional test cases that I added.
There was a problem hiding this comment.
Ah I understand, thanks for this explanation.
| COLLECTIONS_EMPTY, | ||
| COLLECTION_OF, | ||
| COLLECTION_CTOR, | ||
| EMPTY |
There was a problem hiding this comment.
Looks good, I've seen your comment under the jira ticket, but there are many otheres non-raising functions, here are a few commonly used that could make sense to add.
- Arrays.asList(...)
Collections.singleton*: Collections.singleton(o) / Collections.singletonList(o) / Collections.singletonMap(k, v)...*.of:Stream.of(t)/Optional.ofNullable(v)...- boxing functions :
Byte.valueOf(b),Boolean.valueOf(b),Integer.valueOf(i) - ...
I'm not requesting any change, just proposing very commonly used functions you can add if you think it makes sense
There was a problem hiding this comment.
I added Collections.singleton*. Arrays.asList can throw NPE. While the other examples can lead to FPs, I don't think we should be adding a large collection of cases unless we have some evidence that they happen in practice.
I think we can validate this PR on Peachee and follow up if we see more cases worth excluding.
|
Code Review ✅ ApprovedExpands the list of ignored factory methods in OneExpectedRuntimeExceptionCheck to include Collections.singleton variants. No issues found. OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |




Expand the list of methods that we can safely ignore.
Summary by Gitar
OneExpectedRuntimeExceptionCheckto ignoreCollections.singleton,Collections.singletonList, andCollections.singletonMapmethod calls.OneExpectedRuntimeExceptionCheckSampleto verify the exclusion ofCollections.singleton*methods.its/autoscan/src/test/resources/autoscan/diffs/diff_S5961.jsonto reflect new false negative count for ruleS5961.This will update automatically on new commits.