Skip to content

Commit f2f6bdd

Browse files
tomasz-tylenda-sonarsourcetomasz-tylenda-sonarsource
authored
SONARJAVA-5599 S3024 Arguments to append should not be concatenated (#5189)
1 parent 54ebc25 commit f2f6bdd

11 files changed

Lines changed: 416 additions & 0 deletions

File tree

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

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

‎its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S3024.json‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java": [
3+
507,
4+
508,
5+
509,
6+
510,
7+
511,
8+
512,
9+
513,
10+
514,
11+
515,
12+
516
13+
]
14+
}

‎its/ruling/src/test/resources/eclipse-jetty/java-S3024.json‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
{
2+
"org.eclipse.jetty:jetty-project:jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionData.java": [
3+
507,
4+
508,
5+
509,
6+
510,
7+
511,
8+
512,
9+
513,
10+
514,
11+
515,
12+
516
13+
]
14+
}

‎its/ruling/src/test/resources/guava/java-S3024.json‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"com.google.guava:guava:src/com/google/common/collect/Iterators.java": [
3+
313,
4+
315
5+
]
6+
}

‎its/ruling/src/test/resources/sonar-server/java-S3024.json‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
{
2+
"org.sonarsource.sonarqube:sonar-server:src/main/java/org/sonar/server/debt/DebtModelXMLExporter.java": [
3+
67,
4+
71,
5+
78,
6+
79,
7+
81,
8+
83,
9+
97,
10+
114,
11+
116,
12+
118,
13+
120,
14+
123,
15+
125,
16+
127
17+
]
18+
}

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

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package checks;
2+
3+
import java.util.Iterator;
4+
5+
public class StringBufferAndBuilderConcatenationCheckSample {
6+
public String appendSimple() {
7+
return new StringBuilder()
8+
.append("text1")
9+
.append("text2")
10+
.toString();
11+
}
12+
13+
public String concatThree() {
14+
return new StringBuilder()
15+
.append("text1" + "text2" + "text3") // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf2]]
16+
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^
17+
// fix@qf2 {{Call "append" multiple times.}}
18+
// edit@qf2 [[sc=14;ec=43]] {{("text1").append("text2").append("text3")}}
19+
.toString();
20+
}
21+
22+
public String concatIntegerNoncompliant() {
23+
return new StringBuilder()
24+
.append("text" + 1) // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf3]]
25+
// ^^^^^^^^^^
26+
// fix@qf3 {{Call "append" multiple times.}}
27+
// edit@qf3 [[sc=14;ec=26]] {{("text").append(1)}}
28+
.toString();
29+
}
30+
31+
public String concatIntegerCompliant() {
32+
return new StringBuilder()
33+
.append(1 + 1)
34+
.toString();
35+
}
36+
37+
public String complexArgumentList() {
38+
return new StringBuilder()
39+
.append("two " + (1 + 1)) // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf4]]
40+
// ^^^^^^^^^^^^^^^^
41+
// fix@qf4 {{Call "append" multiple times.}}
42+
// edit@qf4 [[sc=14;ec=32]] {{("two ").append(1 + 1)}}
43+
.toString();
44+
}
45+
46+
public StringBuilder notChained() {
47+
StringBuilder stringBuilder = new StringBuilder();
48+
stringBuilder.append("text1" + "text2"); // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf5]]
49+
// ^^^^^^^^^^^^^^^^^
50+
// fix@qf5 {{Call "append" multiple times.}}
51+
// edit@qf5 [[sc=25;ec=44]] {{("text1").append("text2")}}
52+
return stringBuilder;
53+
}
54+
55+
public void parameter(StringBuilder passed) {
56+
passed.append("hello " + "hello"); // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf6]]
57+
// ^^^^^^^^^^^^^^^^^^
58+
// fix@qf6 {{Call "append" multiple times.}}
59+
// edit@qf6 [[sc=18;ec=38]] {{("hello ").append("hello")}}
60+
}
61+
62+
public String call(Iterator<String> iter) {
63+
return new StringBuilder()
64+
.append("<" + iter.next() + "/>") // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf7]]
65+
// ^^^^^^^^^^^^^^^^^^^^^^^^
66+
// fix@qf7 {{Call "append" multiple times.}}
67+
// edit@qf7 [[sc=14;ec=40]] {{("<").append(iter.next()).append("/>")}}
68+
.toString();
69+
}
70+
71+
public String nestedCall(String s) {
72+
return new StringBuilder()
73+
.append(s.toLowerCase())
74+
.toString();
75+
}
76+
77+
public String stringBufferAppendSimple() {
78+
return new StringBuffer()
79+
.append("text1")
80+
.append("text2")
81+
.toString();
82+
}
83+
84+
public String appendWithStartEnd() {
85+
// Breaking this up would require changing the second and third argument.
86+
return new StringBuilder()
87+
.append("text1" + "text2" , 2, 3)
88+
.toString();
89+
}
90+
91+
public String stringBufferConcatSimple() {
92+
return new StringBuffer()
93+
.append("text1" + "text2") // Noncompliant {{Use multiple calls to "append" instead of string concatenation.}} [[quickfixes=qf101]]
94+
// ^^^^^^^^^^^^^^^^^
95+
// fix@qf101 {{Call "append" multiple times.}}
96+
// edit@qf101 [[sc=14;ec=33]] {{("text1").append("text2")}}
97+
.toString();
98+
}
99+
}

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

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
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.ArrayList;
20+
import java.util.List;
21+
import java.util.Optional;
22+
import java.util.stream.Collectors;
23+
import org.sonar.check.Rule;
24+
import org.sonar.java.checks.helpers.QuickFixHelper;
25+
import org.sonar.java.checks.methods.AbstractMethodDetection;
26+
import org.sonar.java.model.ExpressionUtils;
27+
import org.sonar.java.reporting.JavaQuickFix;
28+
import org.sonar.java.reporting.JavaTextEdit;
29+
import org.sonar.plugins.java.api.semantic.MethodMatchers;
30+
import org.sonar.plugins.java.api.tree.Arguments;
31+
import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
32+
import org.sonar.plugins.java.api.tree.ExpressionTree;
33+
import org.sonar.plugins.java.api.tree.MethodInvocationTree;
34+
import org.sonar.plugins.java.api.tree.Tree;
35+
36+
/**
37+
* Suggest that <code>stringBuilder.append("text1" + "text2")</code> is replaced with
38+
* <code>stringBuilder.append("text1").append("text2")</code>.
39+
*/
40+
@Rule(key = "S3024")
41+
public class StringBufferAndBuilderConcatenationCheck extends AbstractMethodDetection {
42+
43+
private static final String JAVA_LANG_STRING = "java.lang.String";
44+
45+
@Override
46+
protected MethodMatchers getMethodInvocationMatchers() {
47+
return MethodMatchers.create()
48+
.ofTypes("java.lang.StringBuffer", "java.lang.StringBuilder")
49+
.names("append")
50+
.addParametersMatcher(JAVA_LANG_STRING)
51+
.build();
52+
}
53+
54+
@Override
55+
protected void onMethodInvocationFound(MethodInvocationTree mit) {
56+
ExpressionTree arg = mit.arguments().get(0);
57+
58+
getConcatenationTree(arg).ifPresent(binaryExpressionTree ->
59+
QuickFixHelper.newIssue(context)
60+
.forRule(this)
61+
.onTree(arg)
62+
.withMessage("Use multiple calls to \"append\" instead of string concatenation.")
63+
.withQuickFix(() -> getQuickFix(mit.arguments()))
64+
.report()
65+
);
66+
}
67+
68+
/**
69+
* If the tree represents <code>arg1 + arg2</code>, then return <code>Optional.of(tree)</code>,
70+
* otherwise return <code>Optional.empty()</code>.
71+
* Using optional, rather than if-then, helps us avoid some problems in coverage by unit tests.
72+
*/
73+
private static Optional<BinaryExpressionTree> getConcatenationTree(ExpressionTree tree) {
74+
return Optional.of(tree)
75+
.filter(BinaryExpressionTree.class::isInstance)
76+
.map(BinaryExpressionTree.class::cast)
77+
.filter(bet -> bet.is(Tree.Kind.PLUS));
78+
}
79+
80+
private JavaQuickFix getQuickFix(Arguments arguments) {
81+
String replacement = splitExpressionOnPlus(arguments.get(0)).stream()
82+
.map(ExpressionUtils::skipParentheses)
83+
.map(tree -> "(" + QuickFixHelper.contentForTree(tree, context) + ")")
84+
.collect(Collectors.joining(".append"));
85+
86+
return JavaQuickFix.newQuickFix("Call \"append\" multiple times.")
87+
.addTextEdit(JavaTextEdit.replaceTree(arguments, replacement))
88+
.build();
89+
}
90+
91+
private static List<ExpressionTree> splitExpressionOnPlus(ExpressionTree tree) {
92+
List<ExpressionTree> accumulator = new ArrayList<>();
93+
splitExpressionOnPlus(accumulator, tree);
94+
return accumulator;
95+
}
96+
97+
private static void splitExpressionOnPlus(List<ExpressionTree> accumulator, ExpressionTree tree) {
98+
getConcatenationTree(tree).ifPresentOrElse(
99+
bet -> {
100+
ExpressionTree left = bet.leftOperand();
101+
ExpressionTree right = bet.rightOperand();
102+
splitExpressionOnPlus(accumulator, left);
103+
accumulator.add(right);
104+
},
105+
() -> accumulator.add(tree)
106+
);
107+
}
108+
}

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
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+
22+
import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
23+
24+
class StringBufferAndBuilderConcatenationCheckTest {
25+
@Test
26+
void test() {
27+
CheckVerifier.newVerifier()
28+
.onFile(mainCodeSourcesPath("checks/StringBufferAndBuilderConcatenationCheckSample.java"))
29+
.withCheck(new StringBufferAndBuilderConcatenationCheck())
30+
.verifyIssues();
31+
}
32+
33+
@Test
34+
void test_without_semantic() {
35+
CheckVerifier.newVerifier()
36+
.onFile(mainCodeSourcesPath("checks/StringBufferAndBuilderConcatenationCheckSample.java"))
37+
.withCheck(new StringBufferAndBuilderConcatenationCheck())
38+
.withoutSemantic()
39+
.verifyIssues();
40+
}
41+
}

0 commit comments

Comments
 (0)