Skip to content

Commit a355cf3

Browse files
romainbrenguierromainbrenguier
authored
SONARJAVA-5669 Implement rule S7629: defaultFinisher in Gather factory call (#5233)
1 parent 8decf1a commit a355cf3

File tree

11 files changed

+256
-2
lines changed

11 files changed

+256
-2
lines changed

‎its/autoscan/src/test/java/org/sonar/java/it/AutoScanTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -199,7 +199,7 @@ public void javaCheckTestSources() throws Exception {
199199
softly.assertThat(newDiffs).containsExactlyInAnyOrderElementsOf(knownDiffs.values());
200200
softly.assertThat(newTotal).isEqualTo(knownTotal);
201201
softly.assertThat(rulesCausingFPs).hasSize(9);
202-
softly.assertThat(rulesNotReporting).hasSize(17);
202+
softly.assertThat(rulesNotReporting).hasSize(18);
203203

204204
/**
205205
* 4. Check total number of differences (FPs + FNs)

‎its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2866,5 +2866,11 @@
28662866
"hasTruePositives": false,
28672867
"falseNegatives": 0,
28682868
"falsePositives": 0
2869+
},
2870+
{
2871+
"ruleKey": "7629",
2872+
"hasTruePositives": false,
2873+
"falseNegatives": 0,
2874+
"falsePositives": 0
28692875
}
28702876
]

‎its/autoscan/src/test/resources/autoscan/diffs/diff_S7629.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"ruleKey": "S7629",
3+
"hasTruePositives": false,
4+
"falseNegatives": 0,
5+
"falsePositives": 0
6+
}

‎java-checks-test-sources/default/src/main/java/checks/DefaultFinisherInGathererFactoryCheckSample.java‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package checks;
2+
3+
import java.util.concurrent.atomic.AtomicInteger;
4+
import java.util.stream.Gatherer;
5+
6+
public class DefaultFinisherInGathererFactoryCheckSample {
7+
Gatherer<Integer, AtomicInteger, Integer> noncompliantGatherer = Gatherer.ofSequential(
8+
() -> new AtomicInteger(-1),
9+
(state, number, downstream) -> {
10+
if (state.get() < 0) {
11+
state.set(number);
12+
return true;
13+
}
14+
return downstream.push(number - state.get());
15+
}, Gatherer.defaultFinisher()); // noncompliant {{Remove the default finisher from this Gatherer factory.}} [[quickfixes=qf1]]
16+
// ^^^^^^^^^^^^^^^^^^^^^^^^^^
17+
// fix@qf1 {{Remove finisher argument.}}
18+
// edit@qf1 [[sc=6;ec=34]] {{}}
19+
20+
21+
Gatherer<Integer, AtomicInteger, Integer> noncompliantGatherWithLambdaFinisher = Gatherer.ofSequential(
22+
() -> new AtomicInteger(-1),
23+
(state, number, downstream) -> {
24+
if (state.get() < 0) {
25+
state.set(number);
26+
return true;
27+
}
28+
return downstream.push(number - state.get());
29+
}, (state, number) -> { }); // noncompliant {{Remove the default finisher from this Gatherer factory.}} [[quickfixes=qf2]]
30+
// ^^^^^^^^^^^^^^^^^^^^^^
31+
// fix@qf2 {{Remove finisher argument.}}
32+
// edit@qf2 [[sc=6;ec=30]] {{}}
33+
34+
Gatherer<Integer, AtomicInteger, Integer> compliantGatherer = Gatherer.ofSequential(
35+
() -> new AtomicInteger(-1),
36+
(state, number, downstream) -> {
37+
if (state.get() < 0) {
38+
state.set(number);
39+
return true;
40+
}
41+
return downstream.push(number - state.get());
42+
});
43+
}

‎java-checks/src/main/java/org/sonar/java/checks/DefaultFinisherInGathererFactoryCheck.java‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2025 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.java.checks;
18+
19+
import java.util.List;
20+
import org.sonar.check.Rule;
21+
import org.sonar.java.checks.helpers.QuickFixHelper;
22+
import org.sonar.java.matcher.TreeMatcher;
23+
import org.sonar.java.reporting.JavaQuickFix;
24+
import org.sonar.java.reporting.JavaTextEdit;
25+
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
26+
import org.sonar.plugins.java.api.semantic.MethodMatchers;
27+
import org.sonar.plugins.java.api.tree.Arguments;
28+
import org.sonar.plugins.java.api.tree.ExpressionTree;
29+
import org.sonar.plugins.java.api.tree.MethodInvocationTree;
30+
import org.sonar.plugins.java.api.tree.Tree;
31+
32+
import static org.sonar.java.matcher.TreeMatcher.hasSize;
33+
import static org.sonar.java.matcher.TreeMatcher.isLambdaExpression;
34+
import static org.sonar.java.matcher.TreeMatcher.withBody;
35+
36+
@Rule(key = "S7629")
37+
public class DefaultFinisherInGathererFactoryCheck extends IssuableSubscriptionVisitor {
38+
39+
private static final String ISSUE_MESSAGE = "Remove the default finisher from this Gatherer factory.";
40+
41+
private final MethodMatchers ofSequentialMatchers = MethodMatchers.create()
42+
.ofTypes("java.util.stream.Gatherer")
43+
.names("ofSequential")
44+
.addParametersMatcher("java.util.function.Supplier",
45+
"java.util.stream.Gatherer$Integrator", "java.util.function.BiConsumer")
46+
.addParametersMatcher(
47+
"java.util.stream.Gatherer$Integrator", "java.util.function.BiConsumer")
48+
.build();
49+
50+
private final MethodMatchers defaultFinisherMatchers = MethodMatchers.create()
51+
.ofTypes("java.util.stream.Gatherer")
52+
.names("defaultFinisher")
53+
.addParametersMatcher()
54+
.build();
55+
56+
@Override
57+
public List<Tree.Kind> nodesToVisit() {
58+
return List.of(Tree.Kind.METHOD_INVOCATION);
59+
}
60+
61+
@Override
62+
public void visitNode(Tree tree) {
63+
MethodInvocationTree mit = (MethodInvocationTree) tree;
64+
if (ofSequentialMatchers.matches(mit)) {
65+
ExpressionTree lastArgument = mit.arguments().get(mit.arguments().size() - 1);
66+
if (TreeMatcher.calls(defaultFinisherMatchers)
67+
.or(isLambdaExpression(withBody(hasSize(0)))).check(lastArgument)) {
68+
QuickFixHelper.newIssue(context)
69+
.forRule(this)
70+
.onTree(lastArgument)
71+
.withMessage(ISSUE_MESSAGE)
72+
.withQuickFix(() -> computeQuickFix(mit.arguments()))
73+
.report();
74+
}
75+
}
76+
}
77+
78+
private static JavaQuickFix computeQuickFix(Arguments arguments) {
79+
return JavaQuickFix
80+
.newQuickFix("Remove finisher argument.")
81+
.addTextEdit(JavaTextEdit.replaceBetweenTree(
82+
arguments.get(arguments.size() - 2), false, arguments.get(arguments.size() - 1), true, ""))
83+
.build();
84+
}
85+
86+
}

‎java-checks/src/test/java/org/sonar/java/checks/DefaultFinisherInGathererFactoryCheckTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2025 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the Sonar Source-Available License Version 1, as published by SonarSource SA.
8+
*
9+
* This program is distributed in the hope that it will be useful,
10+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
11+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
12+
* See the Sonar Source-Available License for more details.
13+
*
14+
* You should have received a copy of the Sonar Source-Available License
15+
* along with this program; if not, see https://sonarsource.com/license/ssal/
16+
*/
17+
package org.sonar.java.checks;
18+
19+
import org.junit.jupiter.api.Test;
20+
import org.sonar.java.checks.verifier.CheckVerifier;
21+
import org.sonar.java.checks.verifier.TestUtils;
22+
23+
class DefaultFinisherInGathererFactoryCheckTest {
24+
25+
@Test
26+
void test() {
27+
CheckVerifier.newVerifier()
28+
.onFile(TestUtils.mainCodeSourcesPath("checks/DefaultFinisherInGathererFactoryCheckSample.java"))
29+
.withCheck(new DefaultFinisherInGathererFactoryCheck())
30+
.verifyIssues();
31+
}
32+
33+
@Test
34+
void testWithoutSemantic() {
35+
CheckVerifier.newVerifier()
36+
.onFile(TestUtils.mainCodeSourcesPath("checks/DefaultFinisherInGathererFactoryCheckSample.java"))
37+
.withCheck(new DefaultFinisherInGathererFactoryCheck())
38+
.withoutSemantic()
39+
.verifyIssues();
40+
}
41+
42+
}

‎java-frontend/src/main/java/org/sonar/java/matcher/TreeMatcher.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ public static TreeMatcher<ExpressionTree> calls(
8080
&& methodMatchers.matches(mit) && methodInvocationMatcher.check(mit)));
8181
}
8282

83+
public static TreeMatcher<ExpressionTree> calls(MethodMatchers methodMatchers) {
84+
return calls(methodMatchers, any());
85+
}
86+
8387
public static TreeMatcher<ExpressionTree> isCall(TreeMatcher<MethodInvocationTree> methodInvocationMatcher) {
8488
return new TreeMatcher<>(
8589
expressionTree -> (expressionTree instanceof MethodInvocationTree mit
@@ -93,6 +97,10 @@ public static TreeMatcher<StatementTree> isInvocationOf(
9397
&& calls(methodMatchers, methodInvocationMatcher).check(exprStatement.expression())));
9498
}
9599

100+
public static TreeMatcher<StatementTree> isInvocationOf(MethodMatchers methodMatchers) {
101+
return isInvocationOf(methodMatchers, any());
102+
}
103+
96104
public static TreeMatcher<StatementTree> isInvocation(TreeMatcher<MethodInvocationTree> methodInvocationMatcher) {
97105
return new TreeMatcher<>(
98106
statementTree -> (statementTree instanceof ExpressionStatementTree exprStatement

‎java-frontend/src/test/java/org/sonar/java/matcher/TreeMatcherTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@ class C {
221221
MethodMatchers matchNone = mock(MethodMatchers.class);
222222
when(matchNone.matches(ArgumentMatchers.any(MethodInvocationTree.class))).thenReturn(false);
223223

224+
assertTrue(TreeMatcher.calls(matchAll).check(callOnX));
224225
assertTrue(TreeMatcher.calls(matchAll, invokedOn(isIdentifier("x"))).check(callOnX));
225226
assertFalse(TreeMatcher.calls(matchAll, invokedOn(isIdentifier("y"))).check(callOnX));
226227
assertFalse(TreeMatcher.calls(matchNone, invokedOn(isIdentifier("x"))).check(callOnX));
@@ -244,6 +245,7 @@ class C {
244245
when(matchNone.matches(ArgumentMatchers.any(MethodInvocationTree.class))).thenReturn(false);
245246

246247
assertTrue(TreeMatcher.isInvocationOf(matchAll, invokedOn(isIdentifier("x"))).check(firstStatement));
248+
assertTrue(TreeMatcher.isInvocationOf(matchAll).check(firstStatement));
247249
assertFalse(TreeMatcher.isInvocationOf(matchAll, invokedOn(isIdentifier("y"))).check(firstStatement));
248250
assertFalse(TreeMatcher.isInvocationOf(matchNone, invokedOn(isIdentifier("x"))).check(firstStatement));
249251
}

‎sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S7629.html‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>Passing an empty finisher to <code>Gatherer.of</code> or to <code>Gatherer.ofSequential</code> provides no additional value and removing the
3+
finisher clearly communicates that no finisher is applied.</p>
4+
<h2>How to fix it</h2>
5+
<p>Call the overload of <code>Gatherer.of</code> or <code>Gatherer.ofSequential</code> that does not take a finisher.</p>
6+
<h3>Code examples</h3>
7+
<h4>Noncompliant code example</h4>
8+
<pre data-diff-id="1" data-diff-type="noncompliant">
9+
Gatherer&lt;Integer, AtomicInteger, Integer&gt; gatherer = Gatherer.ofSequential(
10+
() -&gt; new AtomicInteger(-1),
11+
(state, number, downstream) -&gt; {
12+
if (state.get() &lt; 0) {
13+
state.set(number);
14+
return true;
15+
}
16+
return downstream.push(number - state.get());
17+
},
18+
Gatherer.defaultFinisher()); // Noncompliant: useless finisher
19+
</pre>
20+
<h4>Compliant solution</h4>
21+
<pre data-diff-id="1" data-diff-type="compliant">
22+
Gatherer&lt;Integer, AtomicInteger, Integer&gt; gatherer = Gatherer.ofSequential(
23+
() -&gt; new AtomicInteger(-1),
24+
(state, number, downstream) -&gt; {
25+
if (state.get() &lt; 0) {
26+
state.set(number);
27+
return true;
28+
}
29+
return downstream.push(number - state.get());
30+
}); // Compliant
31+
</pre>
32+
<h2>Resources</h2>
33+
<ul>
34+
<li> Oracle Documentation - <a href="https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/util/stream/Gatherer.html">Gatherer API</a>
35+
</li>
36+
</ul>
37+

‎sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S7629.json‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{
2+
"title": "When a defaultFinisher is passed to a Gatherer factory, use the overload that does not take a finisher",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"java24"
11+
],
12+
"defaultSeverity": "Minor",
13+
"ruleSpecification": "RSPEC-7629",
14+
"sqKey": "S7629",
15+
"scope": "All",
16+
"quickfix": "unknown",
17+
"code": {
18+
"impacts": {
19+
"MAINTAINABILITY": "LOW"
20+
},
21+
"attribute": "CONVENTIONAL"
22+
}
23+
}

0 commit comments

Comments
 (0)