diff --git a/README.md b/README.md index ea4a5d2df..a6ac7d8f8 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `LambdaMethodReference`: Lambda should use a method reference. - `SafeLoggingExceptionMessageFormat`: SafeLoggable exceptions do not interpolate parameters. - `StrictUnusedVariable`: Functions shouldn't have unused parameters. +- `StringBuilderConstantParameters`: StringBuilder with a constant number of parameters should be replaced by simple concatenation. ## com.palantir.baseline-checkstyle Checkstyle rules can be suppressed on a per-line or per-block basis. (It is good practice to first consider formatting diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java new file mode 100644 index 000000000..b0dd8bda1 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/StringBuilderConstantParameters.java @@ -0,0 +1,164 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.common.base.Preconditions; +import com.google.common.collect.Streams; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.BinaryTree; +import com.sun.source.tree.ConditionalExpressionTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.util.SimpleTreeVisitor; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +@AutoService(BugChecker.class) +@BugPattern( + name = "StringBuilderConstantParameters", + severity = SeverityLevel.WARNING, + summary = "StringBuilder with a constant number of parameters should be replaced by simple concatenation") +public final class StringBuilderConstantParameters + extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + private static final String MESSAGE = + "StringBuilder with a constant number of parameters should be replaced by simple concatenation.\nThe Java " + + "compiler (jdk8) replaces concatenation of a constant number of arguments with a StringBuilder, " + + "while jdk 9+ take advantage of JEP 280 (https://openjdk.java.net/jeps/280) to efficiently " + + "pre-size the result for better performance than a StringBuilder."; + + private static final long serialVersionUID = 1L; + + private static final Matcher STRING_BUILDER_TYPE_MATCHER = Matchers.isSameType(StringBuilder.class); + private static final Matcher STRING_BUILDER_TO_STRING = + MethodMatchers.instanceMethod() + .onExactClass(StringBuilder.class.getName()) + .named("toString") + .withParameters(); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!STRING_BUILDER_TO_STRING.matches(tree, state)) { + return Description.NO_MATCH; + } + Optional> result = tree.getMethodSelect().accept(StringBuilderVisitor.INSTANCE, state); + if (!result.isPresent()) { + return Description.NO_MATCH; + } + List arguments = result.get(); + Stream prefixStream = arguments.stream().findFirst() + .map(ASTHelpers::getType) + .filter(type -> + ASTHelpers.isSameType(type, state.getTypeFromString("java.lang.String"), state)) + .map(ignored -> Stream.of()) + .orElseGet(() -> Stream.of("\"\"")); + + return buildDescription(tree) + .setMessage(MESSAGE) + .addFix(SuggestedFix.builder() + .replace(tree, Streams.concat(prefixStream, arguments.stream() + .map(node -> getArgumentSourceString(state, node))) + .collect(Collectors.joining(" + "))) + .build()) + .build(); + } + + private static String getArgumentSourceString(VisitorState state, ExpressionTree tree) { + String originalSource = state.getSourceForNode(tree); + // Ternary expressions must be parenthesized to avoid leaking into preceding or following expressions. + if (tree instanceof ConditionalExpressionTree || tree instanceof BinaryTree) { + return '(' + originalSource + ')'; + } + return originalSource; + } + + /** + * {@link StringBuilderVisitor} checks if a {@link StringBuilder#toString()} invocation can be followed up + * a fluent invocation chain, therefore must have a constant number of arguments. + * If so, the visitor results in a present {@link Optional} of {@link ExpressionTree arguments} in the order + * they are {@link StringBuilder#append(Object) appended}, otherwise an {@link Optional#empty() empty optional} + * is returned. + * This allows us to maintain a single implementation for validation and building a {@link SuggestedFix} without + * sacrificing build time allocating objects for {@link StringBuilder builders} which don't fit our pattern. + */ + private static final class StringBuilderVisitor + extends SimpleTreeVisitor>, VisitorState> { + private static final StringBuilderVisitor INSTANCE = new StringBuilderVisitor(); + + private StringBuilderVisitor() { + super(Optional.empty()); + } + + @Override + public Optional> visitNewClass(NewClassTree node, VisitorState state) { + if (!STRING_BUILDER_TYPE_MATCHER.matches(node.getIdentifier(), state)) { + return defaultAction(node, state); + } + if (node.getArguments().isEmpty()) { + return Optional.of(new ArrayList<>()); + } + if (node.getArguments().size() == 1 + // We shouldn't replace pre-sized builders until we target java 11 across most libraries. + && (ASTHelpers.isSameType( + ASTHelpers.getType(node.getArguments().get(0)), + state.getTypeFromString("java.lang.String"), state) + || ASTHelpers.isSameType( + ASTHelpers.getType(node.getArguments().get(0)), + state.getTypeFromString("java.lang.CharSequence"), state))) { + List resultList = new ArrayList<>(); + resultList.add(node.getArguments().get(0)); + return Optional.of(resultList); + } + return Optional.empty(); + } + + @Override + public Optional> visitMemberSelect(MemberSelectTree node, VisitorState state) { + if (node.getIdentifier().contentEquals("append") + || node.getIdentifier().contentEquals("toString")) { + return node.getExpression().accept(this, state); + } + return defaultAction(node, state); + } + + @Override + public Optional> visitMethodInvocation( + MethodInvocationTree node, + VisitorState state) { + Optional> result = node.getMethodSelect().accept(this, state); + if (result.isPresent()) { + Preconditions.checkState(node.getArguments().size() == 1, "Expected a single argument to 'append'"); + result.get().add(node.getArguments().get(0)); + } + return result; + } + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StringBuilderConstantParametersTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StringBuilderConstantParametersTests.java new file mode 100644 index 000000000..ba44e4988 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/StringBuilderConstantParametersTests.java @@ -0,0 +1,329 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * 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.palantir.baseline.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Before; +import org.junit.Test; + +public class StringBuilderConstantParametersTests { + private CompilationTestHelper compilationHelper; + + @Before + public void before() { + compilationHelper = CompilationTestHelper.newInstance(StringBuilderConstantParameters.class, getClass()); + } + + @Test + public void shouldWarnOnConstantNumberOfParams() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f() {", + " // BUG: Diagnostic contains: StringBuilder with a constant number of parameters", + " return new StringBuilder().append(\"foo\").append(1).toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void shouldWarnOnConstantNumberOfParams_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f() {", + " return new StringBuilder().append(\"foo\").append(1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f() {", + " return \"foo\" + 1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + + @Test + public void shouldWarnOnConstantNumberOfParams_stringCtor() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f() {", + " // BUG: Diagnostic contains: StringBuilder with a constant number of parameters", + " return new StringBuilder(\"ctor\").append(\"foo\").append(1).toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void shouldWarnOnConstantNumberOfParams_stringCtor_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f() {", + " return new StringBuilder(\"ctor\").append(\"foo\").append(1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f() {", + " return \"ctor\" + \"foo\" + 1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void shouldWarnOnConstantNumberOfParams_charSequenceCtor() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f(CharSequence charSequence) {", + " // BUG: Diagnostic contains: StringBuilder with a constant number of parameters", + " return new StringBuilder(charSequence).append(\"foo\").append(1).toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void shouldWarnOnConstantNumberOfParams_charSequenceCtor_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(CharSequence charSequence) {", + " return new StringBuilder(charSequence).append(\"foo\").append(1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(CharSequence charSequence) {", + " return \"\" + charSequence + \"foo\" + 1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void shouldWarnOnConstantNumberOfNonConstantParams() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f(long param0, double param1) {", + " // BUG: Diagnostic contains: StringBuilder with a constant number of parameters", + " return new StringBuilder().append(param0).append(param1).toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void shouldWarnOnConstantNumberOfNonConstantParams_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(long param0, double param1) {", + " return new StringBuilder().append(param0).append(param1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(long param0, double param1) {", + " return \"\" + param0 + param1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void shouldWarnOnConstantNumberOfNonConstantParams_firstString_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(String param0, double param1) {", + " return new StringBuilder().append(param0).append(param1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(String param0, double param1) {", + " return param0 + param1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void shouldWarnOnNoParams() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f() {", + " // BUG: Diagnostic contains: StringBuilder with a constant number of parameters", + " return new StringBuilder().toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void shouldWarnOnNoParams_fix() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f() {", + " return new StringBuilder().toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f() {", + " return \"\";", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestedFixRetainsCast() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(Object obj) {", + " return new StringBuilder().append((String) obj).append(1).toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(Object obj) {", + " return (String) obj + 1;", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestedFixHandlesTernary() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(Object obj) {", + " return new StringBuilder()", + " .append(\"a\")", + " .append(obj == null ? \"nil\" : obj)", + " .append(\"b\")", + " .toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(Object obj) {", + " return \"a\" + (obj == null ? \"nil\" : obj) + \"b\";", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void suggestedFixHandlesAddition() { + BugCheckerRefactoringTestHelper.newInstance(new StringBuilderConstantParameters(), getClass()) + .addInputLines( + "Test.java", + "class Test {", + " String f(int param0, int param1) {", + " return new StringBuilder()", + " .append(\"a\")", + " .append(param0 + param1)", + " .append(\"b\")", + " .toString();", + " }", + "}") + .addOutputLines( + "Test.java", + "class Test {", + " String f(int param0, int param1) {", + " return \"a\" + (param0 + param1) + \"b\";", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void negativeDynamicStringBuilder() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f(int count) {", + " StringBuilder sb = new StringBuilder();", + " for (int i = 0; i < count; i++) {", + " sb.append(i);", + " }", + " return sb.toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void negativeDynamicStringBuilderWithConstantAppends() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f(int count) {", + " StringBuilder sb = new StringBuilder();", + " for (int i = 0; i < count; i++) {", + " sb.append(i);", + " }", + " return sb.append(count).append(\"foo\").toString();", + " }", + "}" + ).doTest(); + } + + @Test + public void negativePreSizedBuilder() { + compilationHelper.addSourceLines( + "Test.java", + "class Test {", + " String f(int count) {", + " return new StringBuilder(3).append(count).toString();", + " }", + "}" + ).doTest(); + } +} diff --git a/changelog/@unreleased/pr-832.v2.yml b/changelog/@unreleased/pr-832.v2.yml new file mode 100644 index 000000000..47e0de88e --- /dev/null +++ b/changelog/@unreleased/pr-832.v2.yml @@ -0,0 +1,9 @@ +type: improvement +improvement: + description: Error Prone StringBuilderConstantParameters. StringBuilder with a constant + number of parameters should be replaced by simple concatenation. The Java compiler + (jdk8) replaces concatenation of a constant number of arguments with a StringBuilder, + while jdk 9+ take advantage of JEP 280 (https://openjdk.java.net/jeps/280) to + efficiently pre-size the result for better performance than a StringBuilder. + links: + - https://github.com/palantir/gradle-baseline/pull/832 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index f59484c1d..4fd3de459 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -31,6 +31,7 @@ public class BaselineErrorProneExtension { "PreferListsPartition", "PreferSafeLoggableExceptions", "PreferSafeLoggingPreconditions", + "StringBuilderConstantParameters", // Built-in checks "ArrayEquals",