diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java new file mode 100644 index 00000000000..a23b4829f22 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilder.java @@ -0,0 +1,192 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.common.collect.Iterables.getOnlyElement; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.method.MethodMatchers.constructor; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.util.ASTHelpers.getSymbol; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.ASTHelpers.isSubtype; +import static com.google.errorprone.util.ASTHelpers.requiresParentheses; +import static com.google.errorprone.util.ASTHelpers.targetType; +import static java.util.stream.Collectors.joining; + +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.util.ASTHelpers.TargetType; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreePath; +import com.sun.source.util.TreePathScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import java.util.ArrayList; +import java.util.List; +import javax.lang.model.element.ElementKind; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "Prefer string concatenation over explicitly using `StringBuilder#append`, since `+` reads" + + " better and has equivalent or better performance.", + severity = BugPattern.SeverityLevel.WARNING) +public class UnnecessaryStringBuilder extends BugChecker implements NewClassTreeMatcher { + private static final Matcher MATCHER = + constructor().forClass("java.lang.StringBuilder"); + + private static final Matcher APPEND = + instanceMethod().onExactClass("java.lang.StringBuilder").named("append"); + + private static final Matcher TO_STRING = + instanceMethod().onExactClass("java.lang.StringBuilder").named("toString"); + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + if (!MATCHER.matches(tree, state)) { + return NO_MATCH; + } + List parts = new ArrayList<>(); + switch (tree.getArguments().size()) { + case 0: + break; + case 1: + ExpressionTree argument = getOnlyElement(tree.getArguments()); + if (isSubtype(getType(argument), JAVA_LANG_CHARSEQUENCE.get(state), state)) { + parts.add(argument); + } + break; + default: + return NO_MATCH; + } + TreePath path = state.getPath(); + while (true) { + TreePath parentPath = path.getParentPath(); + if (!(parentPath.getLeaf() instanceof MemberSelectTree)) { + break; + } + TreePath grandParent = parentPath.getParentPath(); + if (!(grandParent.getLeaf() instanceof MethodInvocationTree)) { + break; + } + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) grandParent.getLeaf(); + if (!methodInvocationTree.getMethodSelect().equals(parentPath.getLeaf())) { + break; + } + if (APPEND.matches(methodInvocationTree, state)) { + if (methodInvocationTree.getArguments().size() != 1) { + // an append method that doesn't transliterate to concat + return NO_MATCH; + } + parts.add(getOnlyElement(methodInvocationTree.getArguments())); + path = parentPath.getParentPath(); + } else if (TO_STRING.matches(methodInvocationTree, state)) { + return describeMatch( + methodInvocationTree, + SuggestedFix.replace(methodInvocationTree, replacement(state, parts))); + } else { + // another instance method on StringBuilder + return NO_MATCH; + } + } + ASTHelpers.TargetType target = ASTHelpers.targetType(state.withPath(path)); + if (!isUsedAsStringBuilder(state, target)) { + return describeMatch( + path.getLeaf(), SuggestedFix.replace(path.getLeaf(), replacement(state, parts))); + } + Tree leaf = target.path().getLeaf(); + if (leaf instanceof VariableTree) { + VariableTree variableTree = (VariableTree) leaf; + if (isRewritableVariable(variableTree, state)) { + return describeMatch( + variableTree, + SuggestedFix.builder() + .replace(variableTree.getType(), "String") + .replace(variableTree.getInitializer(), replacement(state, parts)) + .build()); + } + } + return NO_MATCH; + } + + /** + * Returns true if the StringBuilder is assigned to a variable, and the type of the variable can + * safely be refactored to be a String. + */ + boolean isRewritableVariable(VariableTree variableTree, VisitorState state) { + Symbol sym = getSymbol(variableTree); + if (!sym.getKind().equals(ElementKind.LOCAL_VARIABLE)) { + return false; + } + boolean[] ok = {true}; + new TreePathScanner() { + @Override + public Void visitIdentifier(IdentifierTree tree, Void unused) { + if (sym.equals(getSymbol(tree))) { + TargetType target = targetType(state.withPath(getCurrentPath())); + if (isUsedAsStringBuilder(state, target)) { + ok[0] = false; + } + } + return super.visitIdentifier(tree, unused); + } + }.scan(state.getPath().getCompilationUnit(), null); + return ok[0]; + } + + private static boolean isUsedAsStringBuilder(VisitorState state, TargetType target) { + if (target.path().getLeaf().getKind().equals(Tree.Kind.MEMBER_REFERENCE)) { + // e.g. sb::append + return true; + } + return ASTHelpers.isSubtype(target.type(), JAVA_LANG_APPENDABLE.get(state), state); + } + + private static String replacement(VisitorState state, List parts) { + if (parts.isEmpty()) { + return "\"\""; + } + return parts.stream() + .map( + x -> { + String source = state.getSourceForNode(x); + if (requiresParentheses(x, state)) { + source = String.format("(%s)", source); + } + return source; + }) + .collect(joining(" + ")); + } + + private static final Supplier JAVA_LANG_APPENDABLE = + VisitorState.memoize(state -> state.getTypeFromString("java.lang.Appendable")); + + private static final Supplier JAVA_LANG_CHARSEQUENCE = + VisitorState.memoize(state -> state.getTypeFromString("java.lang.CharSequence")); +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 404340f81b3..dac68a26247 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -395,6 +395,7 @@ import com.google.errorprone.bugpatterns.UnnecessaryParentheses; import com.google.errorprone.bugpatterns.UnnecessarySetDefault; import com.google.errorprone.bugpatterns.UnnecessaryStaticImport; +import com.google.errorprone.bugpatterns.UnnecessaryStringBuilder; import com.google.errorprone.bugpatterns.UnnecessaryTestMethodPrefix; import com.google.errorprone.bugpatterns.UnnecessaryTypeArgument; import com.google.errorprone.bugpatterns.UnsafeFinalization; @@ -1032,6 +1033,7 @@ public static ScannerSupplier errorChecks() { UnnecessaryMethodInvocationMatcher.class, UnnecessaryMethodReference.class, UnnecessaryParentheses.class, + UnnecessaryStringBuilder.class, UnrecognisedJavadocTag.class, UnsafeFinalization.class, UnsafeReflectiveConstructionCast.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java new file mode 100644 index 00000000000..ac9692a832b --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/UnnecessaryStringBuilderTest.java @@ -0,0 +1,145 @@ +/* + * Copyright 2023 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public class UnnecessaryStringBuilderTest { + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass()); + private final CompilationTestHelper testHelper = + CompilationTestHelper.newInstance(UnnecessaryStringBuilder.class, getClass()); + + @Test + public void positive() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " System.err.println(new StringBuilder().append(hello).append(\"world\"));", + " System.err.println(new StringBuilder(hello).append(\"world\"));", + " System.err.println(new StringBuilder(10).append(hello).append(\"world\"));", + " System.err.println(new StringBuilder(hello).append(\"world\").toString());", + " System.err.println(new StringBuilder().toString());", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(hello + \"world\");", + " System.err.println(\"\");", + " }", + "}") + .doTest(); + } + + @Test + public void variable() { + refactoringHelper + .addInputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " String a = new StringBuilder().append(hello).append(\"world\").toString();", + " StringBuilder b = new StringBuilder().append(hello).append(\"world\");", + " StringBuilder c = new StringBuilder().append(hello).append(\"world\");", + " System.err.println(b);", + " System.err.println(b + \"\");", + " System.err.println(c);", + " c.append(\"goodbye\");", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " void f(String hello) {", + " String a = hello + \"world\";", + " String b = hello + \"world\";", + " StringBuilder c = new StringBuilder().append(hello).append(\"world\");", + " System.err.println(b);", + " System.err.println(b + \"\");", + " System.err.println(c);", + " c.append(\"goodbye\");", + " }", + "}") + .doTest(); + } + + @Test + public void negative() { + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " void f(Iterable xs) {", + " StringBuilder sb = new StringBuilder();", + " for (String s : xs) {", + " sb.append(s);", + " }", + " System.err.println(sb);", + " }", + "}") + .doTest(); + } + + @Test + public void negativeMethodReference() { + testHelper + .addSourceLines( + "Test.java", + "class Test {", + " void f(Iterable xs) {", + " StringBuilder sb = new StringBuilder();", + " xs.forEach(sb::append);", + " System.err.println(sb);", + " }", + "}") + .doTest(); + } + + @Test + public void needsParens() { + refactoringHelper + .addInputLines( + "Test.java", + "abstract class Test {", + " abstract void g(String x);", + " void f(boolean b, String hello) {", + " g(new StringBuilder().append(b ? hello : \"\").append(\"world\").toString());", + " }", + "}") + .addOutputLines( + "Test.java", + "abstract class Test {", + " abstract void g(String x);", + " void f(boolean b, String hello) {", + " g((b ? hello : \"\") + \"world\");", + " }", + "}") + .doTest(); + } +}