Skip to content

Commit e3743fc

Browse files
cushonError Prone Team
cushon
authored and
Error Prone Team
committed
Add a check for unnecessary usages of StringBuilder
PiperOrigin-RevId: 539967151
1 parent 3195ab0 commit e3743fc

File tree

3 files changed

+339
-0
lines changed

3 files changed

+339
-0
lines changed

‎core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java

+192
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
/*
2+
* Copyright 2023 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import static com.google.common.collect.Iterables.getOnlyElement;
20+
import static com.google.errorprone.matchers.Description.NO_MATCH;
21+
import static com.google.errorprone.matchers.method.MethodMatchers.constructor;
22+
import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
23+
import static com.google.errorprone.util.ASTHelpers.getSymbol;
24+
import static com.google.errorprone.util.ASTHelpers.getType;
25+
import static com.google.errorprone.util.ASTHelpers.isSubtype;
26+
import static com.google.errorprone.util.ASTHelpers.requiresParentheses;
27+
import static com.google.errorprone.util.ASTHelpers.targetType;
28+
import static java.util.stream.Collectors.joining;
29+
30+
import com.google.errorprone.BugPattern;
31+
import com.google.errorprone.VisitorState;
32+
import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
33+
import com.google.errorprone.fixes.SuggestedFix;
34+
import com.google.errorprone.matchers.Description;
35+
import com.google.errorprone.matchers.Matcher;
36+
import com.google.errorprone.suppliers.Supplier;
37+
import com.google.errorprone.util.ASTHelpers;
38+
import com.google.errorprone.util.ASTHelpers.TargetType;
39+
import com.sun.source.tree.ExpressionTree;
40+
import com.sun.source.tree.IdentifierTree;
41+
import com.sun.source.tree.MemberSelectTree;
42+
import com.sun.source.tree.MethodInvocationTree;
43+
import com.sun.source.tree.NewClassTree;
44+
import com.sun.source.tree.Tree;
45+
import com.sun.source.tree.VariableTree;
46+
import com.sun.source.util.TreePath;
47+
import com.sun.source.util.TreePathScanner;
48+
import com.sun.tools.javac.code.Symbol;
49+
import com.sun.tools.javac.code.Type;
50+
import java.util.ArrayList;
51+
import java.util.List;
52+
import javax.lang.model.element.ElementKind;
53+
54+
/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */
55+
@BugPattern(
56+
summary =
57+
"Prefer string concatenation over explicitly using `StringBuilder#append`, since `+` reads"
58+
+ " better and has equivalent or better performance.",
59+
severity = BugPattern.SeverityLevel.WARNING)
60+
public class UnnecessaryStringBuilder extends BugChecker implements NewClassTreeMatcher {
61+
private static final Matcher<ExpressionTree> MATCHER =
62+
constructor().forClass("java.lang.StringBuilder");
63+
64+
private static final Matcher<ExpressionTree> APPEND =
65+
instanceMethod().onExactClass("java.lang.StringBuilder").named("append");
66+
67+
private static final Matcher<ExpressionTree> TO_STRING =
68+
instanceMethod().onExactClass("java.lang.StringBuilder").named("toString");
69+
70+
@Override
71+
public Description matchNewClass(NewClassTree tree, VisitorState state) {
72+
if (!MATCHER.matches(tree, state)) {
73+
return NO_MATCH;
74+
}
75+
List<ExpressionTree> parts = new ArrayList<>();
76+
switch (tree.getArguments().size()) {
77+
case 0:
78+
break;
79+
case 1:
80+
ExpressionTree argument = getOnlyElement(tree.getArguments());
81+
if (isSubtype(getType(argument), JAVA_LANG_CHARSEQUENCE.get(state), state)) {
82+
parts.add(argument);
83+
}
84+
break;
85+
default:
86+
return NO_MATCH;
87+
}
88+
TreePath path = state.getPath();
89+
while (true) {
90+
TreePath parentPath = path.getParentPath();
91+
if (!(parentPath.getLeaf() instanceof MemberSelectTree)) {
92+
break;
93+
}
94+
TreePath grandParent = parentPath.getParentPath();
95+
if (!(grandParent.getLeaf() instanceof MethodInvocationTree)) {
96+
break;
97+
}
98+
MethodInvocationTree methodInvocationTree = (MethodInvocationTree) grandParent.getLeaf();
99+
if (!methodInvocationTree.getMethodSelect().equals(parentPath.getLeaf())) {
100+
break;
101+
}
102+
if (APPEND.matches(methodInvocationTree, state)) {
103+
if (methodInvocationTree.getArguments().size() != 1) {
104+
// an append method that doesn't transliterate to concat
105+
return NO_MATCH;
106+
}
107+
parts.add(getOnlyElement(methodInvocationTree.getArguments()));
108+
path = parentPath.getParentPath();
109+
} else if (TO_STRING.matches(methodInvocationTree, state)) {
110+
return describeMatch(
111+
methodInvocationTree,
112+
SuggestedFix.replace(methodInvocationTree, replacement(state, parts)));
113+
} else {
114+
// another instance method on StringBuilder
115+
return NO_MATCH;
116+
}
117+
}
118+
ASTHelpers.TargetType target = ASTHelpers.targetType(state.withPath(path));
119+
if (!isUsedAsStringBuilder(state, target)) {
120+
return describeMatch(
121+
path.getLeaf(), SuggestedFix.replace(path.getLeaf(), replacement(state, parts)));
122+
}
123+
Tree leaf = target.path().getLeaf();
124+
if (leaf instanceof VariableTree) {
125+
VariableTree variableTree = (VariableTree) leaf;
126+
if (isRewritableVariable(variableTree, state)) {
127+
return describeMatch(
128+
variableTree,
129+
SuggestedFix.builder()
130+
.replace(variableTree.getType(), "String")
131+
.replace(variableTree.getInitializer(), replacement(state, parts))
132+
.build());
133+
}
134+
}
135+
return NO_MATCH;
136+
}
137+
138+
/**
139+
* Returns true if the StringBuilder is assigned to a variable, and the type of the variable can
140+
* safely be refactored to be a String.
141+
*/
142+
boolean isRewritableVariable(VariableTree variableTree, VisitorState state) {
143+
Symbol sym = getSymbol(variableTree);
144+
if (!sym.getKind().equals(ElementKind.LOCAL_VARIABLE)) {
145+
return false;
146+
}
147+
boolean[] ok = {true};
148+
new TreePathScanner<Void, Void>() {
149+
@Override
150+
public Void visitIdentifier(IdentifierTree tree, Void unused) {
151+
if (sym.equals(getSymbol(tree))) {
152+
TargetType target = targetType(state.withPath(getCurrentPath()));
153+
if (isUsedAsStringBuilder(state, target)) {
154+
ok[0] = false;
155+
}
156+
}
157+
return super.visitIdentifier(tree, unused);
158+
}
159+
}.scan(state.getPath().getCompilationUnit(), null);
160+
return ok[0];
161+
}
162+
163+
private static boolean isUsedAsStringBuilder(VisitorState state, TargetType target) {
164+
if (target.path().getLeaf().getKind().equals(Tree.Kind.MEMBER_REFERENCE)) {
165+
// e.g. sb::append
166+
return true;
167+
}
168+
return ASTHelpers.isSubtype(target.type(), JAVA_LANG_APPENDABLE.get(state), state);
169+
}
170+
171+
private static String replacement(VisitorState state, List<ExpressionTree> parts) {
172+
if (parts.isEmpty()) {
173+
return "\"\"";
174+
}
175+
return parts.stream()
176+
.map(
177+
x -> {
178+
String source = state.getSourceForNode(x);
179+
if (requiresParentheses(x, state)) {
180+
source = String.format("(%s)", source);
181+
}
182+
return source;
183+
})
184+
.collect(joining(" + "));
185+
}
186+
187+
private static final Supplier<Type> JAVA_LANG_APPENDABLE =
188+
VisitorState.memoize(state -> state.getTypeFromString("java.lang.Appendable"));
189+
190+
private static final Supplier<Type> JAVA_LANG_CHARSEQUENCE =
191+
VisitorState.memoize(state -> state.getTypeFromString("java.lang.CharSequence"));
192+
}

‎core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java

+2
Original file line numberDiff line numberDiff line change
@@ -395,6 +395,7 @@
395395
import com.google.errorprone.bugpatterns.UnnecessaryParentheses;
396396
import com.google.errorprone.bugpatterns.UnnecessarySetDefault;
397397
import com.google.errorprone.bugpatterns.UnnecessaryStaticImport;
398+
import com.google.errorprone.bugpatterns.UnnecessaryStringBuilder;
398399
import com.google.errorprone.bugpatterns.UnnecessaryTestMethodPrefix;
399400
import com.google.errorprone.bugpatterns.UnnecessaryTypeArgument;
400401
import com.google.errorprone.bugpatterns.UnsafeFinalization;
@@ -1032,6 +1033,7 @@ public static ScannerSupplier errorChecks() {
10321033
UnnecessaryMethodInvocationMatcher.class,
10331034
UnnecessaryMethodReference.class,
10341035
UnnecessaryParentheses.class,
1036+
UnnecessaryStringBuilder.class,
10351037
UnrecognisedJavadocTag.class,
10361038
UnsafeFinalization.class,
10371039
UnsafeReflectiveConstructionCast.class,

‎core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java

+145
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
* Copyright 2023 The Error Prone Authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.google.errorprone.bugpatterns;
18+
19+
import com.google.errorprone.BugCheckerRefactoringTestHelper;
20+
import com.google.errorprone.CompilationTestHelper;
21+
import org.junit.Test;
22+
import org.junit.runner.RunWith;
23+
import org.junit.runners.JUnit4;
24+
25+
@RunWith(JUnit4.class)
26+
public class UnnecessaryStringBuilderTest {
27+
private final BugCheckerRefactoringTestHelper refactoringHelper =
28+
BugCheckerRefactoringTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass());
29+
private final CompilationTestHelper testHelper =
30+
CompilationTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass());
31+
32+
@Test
33+
public void positive() {
34+
refactoringHelper
35+
.addInputLines(
36+
"Test.java",
37+
"class Test {",
38+
" void f(String hello) {",
39+
" System.err.println(new StringBuilder().append(hello).append(\"world\"));",
40+
" System.err.println(new StringBuilder(hello).append(\"world\"));",
41+
" System.err.println(new StringBuilder(10).append(hello).append(\"world\"));",
42+
" System.err.println(new StringBuilder(hello).append(\"world\").toString());",
43+
" System.err.println(new StringBuilder().toString());",
44+
" }",
45+
"}")
46+
.addOutputLines(
47+
"Test.java",
48+
"class Test {",
49+
" void f(String hello) {",
50+
" System.err.println(hello + \"world\");",
51+
" System.err.println(hello + \"world\");",
52+
" System.err.println(hello + \"world\");",
53+
" System.err.println(hello + \"world\");",
54+
" System.err.println(\"\");",
55+
" }",
56+
"}")
57+
.doTest();
58+
}
59+
60+
@Test
61+
public void variable() {
62+
refactoringHelper
63+
.addInputLines(
64+
"Test.java",
65+
"class Test {",
66+
" void f(String hello) {",
67+
" String a = new StringBuilder().append(hello).append(\"world\").toString();",
68+
" StringBuilder b = new StringBuilder().append(hello).append(\"world\");",
69+
" StringBuilder c = new StringBuilder().append(hello).append(\"world\");",
70+
" System.err.println(b);",
71+
" System.err.println(b + \"\");",
72+
" System.err.println(c);",
73+
" c.append(\"goodbye\");",
74+
" }",
75+
"}")
76+
.addOutputLines(
77+
"Test.java",
78+
"class Test {",
79+
" void f(String hello) {",
80+
" String a = hello + \"world\";",
81+
" String b = hello + \"world\";",
82+
" StringBuilder c = new StringBuilder().append(hello).append(\"world\");",
83+
" System.err.println(b);",
84+
" System.err.println(b + \"\");",
85+
" System.err.println(c);",
86+
" c.append(\"goodbye\");",
87+
" }",
88+
"}")
89+
.doTest();
90+
}
91+
92+
@Test
93+
public void negative() {
94+
testHelper
95+
.addSourceLines(
96+
"Test.java",
97+
"class Test {",
98+
" void f(Iterable<String> xs) {",
99+
" StringBuilder sb = new StringBuilder();",
100+
" for (String s : xs) {",
101+
" sb.append(s);",
102+
" }",
103+
" System.err.println(sb);",
104+
" }",
105+
"}")
106+
.doTest();
107+
}
108+
109+
@Test
110+
public void negativeMethodReference() {
111+
testHelper
112+
.addSourceLines(
113+
"Test.java",
114+
"class Test {",
115+
" void f(Iterable<String> xs) {",
116+
" StringBuilder sb = new StringBuilder();",
117+
" xs.forEach(sb::append);",
118+
" System.err.println(sb);",
119+
" }",
120+
"}")
121+
.doTest();
122+
}
123+
124+
@Test
125+
public void needsParens() {
126+
refactoringHelper
127+
.addInputLines(
128+
"Test.java",
129+
"abstract class Test {",
130+
" abstract void g(String x);",
131+
" void f(boolean b, String hello) {",
132+
" g(new StringBuilder().append(b ? hello : \"\").append(\"world\").toString());",
133+
" }",
134+
"}")
135+
.addOutputLines(
136+
"Test.java",
137+
"abstract class Test {",
138+
" abstract void g(String x);",
139+
" void f(boolean b, String hello) {",
140+
" g((b ? hello : \"\") + \"world\");",
141+
" }",
142+
"}")
143+
.doTest();
144+
}
145+
}

0 commit comments

Comments
 (0)