SONARJAVA-6952 Implement rule S9391: Detect for-loops that can be replaced with streams - #6127
Conversation
Add ForLoopStreamSuggestionCheck that identifies for-each loops performing filter, map, or collect operations that could be simplified using the Java Stream API. The check detects patterns including: - Loops with if conditions followed by collection.add() (filter/map) - Loops with direct collection.add() (collect) - Loops with break statements (findFirst pattern) Supports both regular for-each loops and loops with if statements as the body. Uses semantic analysis when available, falls back to name-based heuristics for syntax-only analysis.
hashicorp-vault-sonar-prod
Bot
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
|
❌ Ruling needs updating. A fix PR has been created: #6128 Please review and merge it into your branch. |
- Add JavaVersionAwareVisitor gate (Java 8+ only) - Subscribe to FOR_EACH_STATEMENT instead of BLOCK to avoid quadratic traversal - Restrict to single-statement loop bodies (reject multi-statement, break, continue, return) - Bail out when if-statement has else branch - Skip unknown types (no false positives without semantic for non-JDK types) - Remove FOR_STATEMENT path (unreliable iterable extraction) - Remove name-based heuristic (caused false positives on non-collection types) - Remove dead code (FIND_FIRST, hasSourceModification, unused visitor methods) - Exclude loop-body-declared variables from accumulator set - Only collect variables declared before the loop in the same block - Move test samples to java-checks-test-sources per convention - Add comprehensive compliant/noncompliant test cases - Remove design doc from repo root Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🤖 Generated with GitHub Actions
|
❌ Ruling needs updating. A fix PR has been created: #6128 Please review and merge it into your branch. |
- Remove addFirst/offerFirst from ADD_METHODS (reverses order, not stream-equivalent) - Handle unbraced for-each bodies (was silently ignored) - Remove dead code: collectLoopBodyVariables and toBlock methods - Make collectCollectionSymbols static (S2325) - Remove always-false null checks (S2589) - Add test samples for addLast, offerLast, unbraced forms, and new compliant cases Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ruling Diff SummaryDetected changes in 6 rule files: 0 issues removed, 47 issues added. S9391 (
|
|
❌ Ruling needs updating. A fix PR has been created: #6137 Please review and merge it into your branch. |
nathsou
left a comment
nathsou
left a comment
There was a problem hiding this comment.
The main loop patterns look good. I left four non-blocking semantic-equivalence edge cases.
| return false; | ||
| } | ||
| MethodInvocationTree mit = (MethodInvocationTree) expr; | ||
| if (mit.arguments().size() != 1 || !isAddMethod(mit)) { |
There was a problem hiding this comment.
Checking only the name and arity also accepts unrelated overloads. A Collection subtype can declare void add(int metric), and a loop calling that method would be reported even though it is not Collection.add(E) and has no collector equivalent. Could we match the resolved supported collection/queue/deque methods and signatures with MethodMatchers?
There was a problem hiding this comment.
Addressed: switched from name+arity matching to MethodMatchers.ofSubTypes("java.util.Collection").names("add", "addLast", "offer", "offerLast").addParametersMatcher(ANY), combined with isInheritedCollectionMethod which verifies the resolved method symbol belongs to a Collection API type (Collection, List, Set, Queue, Deque) or overrides one. This filters out unrelated overloads like Metrics.add(int metric) — see test case nonCollectionAddOverload(). Also removed redundant BlockingQueue/BlockingDeque from COLLECTION_API_TYPES since they extend Queue/Deque.
| if (initializer == null) { | ||
| return; | ||
| } | ||
| Type type = initializer.symbolType(); |
There was a problem hiding this comment.
A collection-typed initializer does not establish that this is a fresh empty result. For List<String> result = existing;, replacing the loop with an assigned collected stream discards existing elements and breaks the alias; new ArrayList<>(existing) similarly starts pre-populated. Could we restrict candidates to recognized fresh empty collection constructions/factories?
There was a problem hiding this comment.
Addressed: addIfCollection now requires the initializer to pass isFreshEmptyCollection, which only accepts new XxxCollection() expressions where the type is a subtype of java.util.Collection and no constructor argument is itself a Collection (to reject copy constructors like new ArrayList<>(existing)). Aliased initializers like List<String> result = existing are also rejected since they're not NEW_CLASS nodes. See test cases collectFromExistingCollection() and collectFromCopiedCollection().
| return; | ||
| } | ||
|
|
||
| if (singleStmt.is(Tree.Kind.EXPRESSION_STATEMENT)) { |
There was a problem hiding this comment.
Could we also exclude the accumulation target when it is the enhanced-for source? for (String item : items) { items.add(transform(item)); } is not a collect-into-result pattern, and a stream rewrite would read and mutate the same collection (often causing ConcurrentModificationException). Comparing the source and target symbols should avoid this case.
There was a problem hiding this comment.
Addressed: added excludeSourceSymbol which removes the enhanced-for source collection from candidate symbols before checking the loop body. When for (String item : items) { items.add(...); }, the items symbol is excluded from collectionSymbols, so no issue is raised. See test case selfModifyingLoop().
| } | ||
| } else if (singleStmt.is(Tree.Kind.IF_STATEMENT)) { | ||
| IfStatementTree ifStmt = (IfStatementTree) singleStmt; | ||
| if (isSimpleFilterCollect(collectionSymbols, ifStmt)) { |
There was a problem hiding this comment.
A direct stream rewrite may not compile when the mapped expression or filter condition throws a checked exception. For example, a loop can call parse(path) from a method declaring throws IOException, but paths.stream().map(path -> parse(path)) cannot use Function without handling that exception. Could we skip patterns containing checked-throwing invocations?
There was a problem hiding this comment.
Good point. Detecting checked exceptions thrown from within lambda-incompatible expressions would add significant complexity. Since this is a non-blocking suggestion rule (the user still decides whether to apply the refactoring), I think we can defer this to a follow-up if we observe FPs in practice.
- Use MethodMatchers to verify add/offer calls resolve to Collection methods, preventing false positives on unrelated overloads - Restrict collection candidates to fresh empty constructions (new XxxCollection()), excluding assignments from existing collections and copy constructors - Exclude loops where the iteration source is the same as the accumulation target, avoiding false positives on self-modifying loops Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
❌ Ruling needs updating. A fix PR has been created: #6151 Please review and merge it into your branch. |
🤖 Generated with GitHub Actions
- Add argType.isUnknown() check in isFreshEmptyCollection to prevent false positives when constructor argument types cannot be resolved - Add pre-loop mutation guard: exclude collections that receive method calls between declaration and the loop - Extract "java.util.Collection" string constant - Improve without-semantic test coverage with simpleCollect and filterCollect patterns - Add preLoopMutation compliant test case Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add isInheritedCollectionMethod check to verify that the matched method is actually inherited from java.util.Collection, not a custom overload declared in a subclass (e.g. CustomList.add(int metric)). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🤖 Generated with GitHub Actions
|
❌ Ruling needs updating. A fix PR has been created: #6154 Please review and merge it into your branch. |
…erage Expand isInheritedCollectionMethod to recognize methods declared on all standard collection interfaces (Queue, Deque, List, Set, BlockingQueue, BlockingDeque), not just java.util.Collection. This fixes false negatives when the receiver is declared as a Queue/Deque interface type. Add test cases for Queue.offer, Deque.addLast, Deque.offerLast with interface-typed receivers, and a compliant case for a Collection subtype with a custom non-override add(int) overload. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-rule-s9391-sonarjava-6952' into romain/new-rule-s9391-sonarjava-6952
|
❌ Ruling needs updating. A fix PR has been created: #6154 Please review and merge it into your branch. |
- Remove redundant BlockingQueue/BlockingDeque from COLLECTION_API_TYPES (already covered by Queue/Deque via overriddenSymbols) - Add test cases for initial-capacity constructor, assignment before loop, chained method target, static method call before loop, and field target Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review ✅ Approved 26 resolved / 26 findingsImplements rule S9391 to detect for-each loops performing filter, map, or collect operations that could be simplified using the Stream API. The implementation went through extensive review and refinement addressing semantic precision, false positive elimination across unresolved identifiers, loop body edge cases (break/continue/return, else branches, unbraced bodies), collection detection heuristics, and alignment between semantic and syntax-only analysis paths. All findings have been resolved. ✅ 26 resolved✅ Bug: All unresolved identifiers collapse to one symbol → mass FPs
✅ Bug: break/continue/return in loop body do not prevent the issue
✅ Bug: Statements before the add are not counted as extra statements
✅ Bug: else branch of the loop body is never analyzed
✅ Bug: Rule suggests Stream API without checking the Java version
...and 21 more resolved from earlier reviews Review coverageFunctional validation No results 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
Summary
This PR implements rule S9391 which detects for-each loops performing filter, map, or collect operations that could be simplified using the Java Stream API.
Changes
Patterns Detected
Agent workflow
Iterated on the PR with
uv run ci_loop.pyfor 3 iterations.✔️ : The PR is now ready for review.