Refinement of EfficientStringsChecker.

It's okay if callers try mixing "static final" values into strings,
since the compiler will inline these to avoid the StringBuilder.

We also expand to catch any arguments that might be dynamically
calculated, such as method invocations.

Identify additional inefficient code patterns:

-- Passing dynamic strings into a StringBuilder, which acquires a
transparent StringBuilder for each append.
-- Using "str += val;" style concatenation, which acquires a
transparent StringBuilder for each append.
-- Using StringBuffer which has synchronization overhead.

Bug: 170978902
Test: atest error_prone_android_framework_test
Change-Id: Ia3758dd55a0e6753b0cc5bc83ae8fe45b6bfde1f

2 files changed, 198 insertions, 12 deletions

diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java
index 3fbd51deec65..d2cb030faef6 100644
--- a/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java
+++ b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java
@@ -18,7 +18,11 @@ package com.google.errorprone.bugpatterns.android;
 
 import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
 import static com.google.errorprone.matchers.Matchers.allOf;
+import static com.google.errorprone.matchers.Matchers.anyOf;
 import static com.google.errorprone.matchers.Matchers.contains;
+import static com.google.errorprone.matchers.Matchers.hasModifier;
+import static com.google.errorprone.matchers.Matchers.instanceMethod;
+import static com.google.errorprone.matchers.Matchers.isSubtypeOf;
 import static com.google.errorprone.matchers.Matchers.kindIs;
 import static com.google.errorprone.matchers.Matchers.methodInvocation;
 import static com.google.errorprone.matchers.Matchers.not;
@@ -28,19 +32,28 @@ import com.google.auto.service.AutoService;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.CompoundAssignmentTreeMatcher;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
+import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.matchers.Matcher;
 import com.google.errorprone.predicates.TypePredicate;
 import com.google.errorprone.util.ASTHelpers;
+import com.sun.source.tree.BinaryTree;
+import com.sun.source.tree.CompoundAssignmentTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.LiteralTree;
 import com.sun.source.tree.MethodInvocationTree;
+import com.sun.source.tree.NewClassTree;
+import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
 import com.sun.tools.javac.code.Symbol.VarSymbol;
 import com.sun.tools.javac.code.Type;
 
 import java.util.List;
+import java.util.Objects;
+
+import javax.lang.model.element.Modifier;
 
 /**
  * Android offers several efficient alternatives to some upstream {@link String}
@@ -52,7 +65,7 @@ import java.util.List;
     summary = "Verifies efficient Strings best-practices",
     severity = WARNING)
 public final class EfficientStringsChecker extends BugChecker
-        implements MethodInvocationTreeMatcher {
+        implements MethodInvocationTreeMatcher, NewClassTreeMatcher, CompoundAssignmentTreeMatcher {
 
     private static final Matcher<ExpressionTree> FORMAT_CALL = methodInvocation(
             staticMethod().onClass("java.lang.String").named("format"));
@@ -60,17 +73,37 @@ public final class EfficientStringsChecker extends BugChecker
             staticMethod().onClass(withSimpleName("Preconditions")).withAnyName());
     private static final Matcher<ExpressionTree> OBJECTS_CALL = methodInvocation(
             staticMethod().onClass("java.util.Objects").named("requireNonNull"));
+    private static final Matcher<ExpressionTree> APPEND_CALL = methodInvocation(
+            instanceMethod().onExactClass("java.lang.StringBuilder")
+                    .withSignature("append(java.lang.String)"));
+
+    /**
+     * Identify any dynamic values that will likely cause us to allocate a
+     * transparent StringBuilder.
+     */
+    private static final Matcher<ExpressionTree> DYNAMIC_VALUE = anyOf(
+            allOf(kindIs(Kind.MEMBER_SELECT),
+                    not(allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL)))),
+            allOf(kindIs(Kind.IDENTIFIER),
+                    not(allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL)))),
+            kindIs(Kind.METHOD_INVOCATION));
+
+    /**
+     * Identify an expression that is either a direct "+" binary operator, or
+     * that contains a "+" binary operator nested deep inside.
+     */
+    private static final Matcher<Tree> PLUS = anyOf(kindIs(Kind.PLUS),
+            contains(BinaryTree.class, kindIs(Kind.PLUS)));
 
     /**
-     * Match an expression which combines both string literals any other dynamic
-     * values, since these allocate a transparent StringBuilder.
-     * <p>
-     * This won't match a single isolated string literal, or a chain consisting
-     * of only string literals, since those don't require dynamic construction.
+     * Identify an expression that is using a "+" binary operator to combine
+     * dynamic values, which will likely end up allocating a transparent
+     * {@link StringBuilder}.
      */
-    private static final Matcher<ExpressionTree> CONTAINS_DYNAMIC_STRING = allOf(
-            contains(ExpressionTree.class, kindIs(Kind.STRING_LITERAL)),
-            contains(ExpressionTree.class, not(kindIs(Kind.STRING_LITERAL))));
+    private static final Matcher<Tree> PLUS_DYNAMIC_VALUE = allOf(
+            PLUS, contains(ExpressionTree.class, DYNAMIC_VALUE));
+
+    private static final Matcher<Tree> IS_STRING_BUFFER = isSubtypeOf("java.lang.StringBuffer");
 
     @Override
     public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
@@ -98,9 +131,9 @@ public final class EfficientStringsChecker extends BugChecker
         } else if (PRECONDITIONS_CALL.matches(tree, state)
                 || OBJECTS_CALL.matches(tree, state)) {
             final List<? extends ExpressionTree> args = tree.getArguments();
-            for (int i = 1 ; i < args.size(); i++) {
-                final ExpressionTree arg = args.get(i);
-                if (CONTAINS_DYNAMIC_STRING.matches(arg, state)) {
+            if (args.size() > 1) {
+                final ExpressionTree arg = args.get(1);
+                if (PLUS_DYNAMIC_VALUE.matches(arg, state)) {
                     return buildDescription(arg)
                             .setMessage("Building dynamic messages is discouraged, since they "
                                     + "always allocate a transparent StringBuilder, even in "
@@ -108,6 +141,37 @@ public final class EfficientStringsChecker extends BugChecker
                             .build();
                 }
             }
+        } else if (APPEND_CALL.matches(tree, state)) {
+            final ExpressionTree arg = tree.getArguments().get(0);
+            if (PLUS_DYNAMIC_VALUE.matches(arg, state)) {
+                return buildDescription(arg)
+                        .setMessage("Call append() directly for each argument instead of "
+                                + "allocating a transparent StringBuilder")
+                        .build();
+            }
+        }
+        return Description.NO_MATCH;
+    }
+
+    @Override
+    public Description matchNewClass(NewClassTree tree, VisitorState state) {
+        if (IS_STRING_BUFFER.matches(tree, state)) {
+            return buildDescription(tree)
+                    .setMessage("Strongly encouraged to replace with StringBuilder "
+                            + "which avoids synchronization overhead")
+                    .build();
+        }
+        return Description.NO_MATCH;
+    }
+
+    @Override
+    public Description matchCompoundAssignment(CompoundAssignmentTree tree, VisitorState state) {
+        if (tree.getKind() == Kind.PLUS_ASSIGNMENT && "java.lang.String"
+                .equals(String.valueOf(ASTHelpers.getType(tree.getVariable())))) {
+            return buildDescription(tree)
+                    .setMessage("Strongly encouraged to replace with StringBuilder "
+                            + "which avoids transparent StringBuilder allocations")
+                    .build();
         }
         return Description.NO_MATCH;
     }
diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java
index a755564d52dd..ae9c316b8ca7 100644
--- a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java
+++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java
@@ -116,4 +116,126 @@ public class EfficientStringsCheckerTest {
                         "}")
                 .doTest();
     }
+
+    @Test
+    public void testPreconditions_Complex() {
+        compilationHelper
+                .addSourceFile("/android/util/Preconditions.java")
+                .addSourceLines("Example.java",
+                        "import android.util.Preconditions;",
+                        "public class Example {",
+                        "  String[] classArray = new String[] { null };",
+                        "  String classVar;",
+                        "  static final String CONST_VAR = \"baz\";",
+                        "  public String classMethod() { return \"baz\"; }",
+                        "  public static final String CONST_METHOD() { return \"baz\"; }",
+                        "  public void checkNotNull(Example example, Object val) {",
+                        "    String methodVar = \"baz\";",
+                        "    Preconditions.checkNotNull(val, \"foo\");",
+                        "    Preconditions.checkNotNull(val, (\"foo\"));",
+                        "    Preconditions.checkNotNull(val, classArray[0]);",
+                        "    Preconditions.checkNotNull(val, classVar);",
+                        "    Preconditions.checkNotNull(val, CONST_VAR);",
+                        "    Preconditions.checkNotNull(val, example.classVar);",
+                        "    Preconditions.checkNotNull(val, Example.CONST_VAR);",
+                        "    Preconditions.checkNotNull(val, methodVar);",
+                        "    Preconditions.checkNotNull(val, classMethod());",
+                        "    Preconditions.checkNotNull(val, CONST_METHOD());",
+                        "    Preconditions.checkNotNull(val, \"foo\" + \"bar\");",
+                        "    Preconditions.checkNotNull(val, (\"foo\" + \"bar\"));",
+                        "    // BUG: Diagnostic contains:",
+                        "    Preconditions.checkNotNull(val, \"foo\" + classArray[0]);",
+                        "    // BUG: Diagnostic contains:",
+                        "    Preconditions.checkNotNull(val, \"foo\" + classVar);",
+                        "    Preconditions.checkNotNull(val, \"foo\" + CONST_VAR);",
+                        "    // BUG: Diagnostic contains:",
+                        "    Preconditions.checkNotNull(val, \"foo\" + methodVar);",
+                        "    // BUG: Diagnostic contains:",
+                        "    Preconditions.checkNotNull(val, \"foo\" + classMethod());",
+                        "    // BUG: Diagnostic contains:",
+                        "    Preconditions.checkNotNull(val, \"foo\" + CONST_METHOD());",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testStringBuffer() {
+        compilationHelper
+                .addSourceLines("Example.java",
+                        "public class Example {",
+                        "  public void example() {",
+                        "    // BUG: Diagnostic contains:",
+                        "    StringBuffer sb = new StringBuffer();",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testStringBuilder() {
+        compilationHelper
+                .addSourceLines("Example.java",
+                        "public class Example {",
+                        "  StringBuilder sb = new StringBuilder();",
+                        "  String[] classArray = new String[] { null };",
+                        "  String classVar;",
+                        "  static final String CONST_VAR = \"baz\";",
+                        "  public String classMethod() { return \"baz\"; }",
+                        "  public static final String CONST_METHOD() { return \"baz\"; }",
+                        "  public void generic(Example example) {",
+                        "    sb.append(\"foo\");",
+                        "    sb.append(\"foo\" + \"bar\");",
+                        "    sb.append(classArray[0]);",
+                        "    sb.append(example.classArray[0]);",
+                        "    sb.append(classVar);",
+                        "    sb.append(CONST_VAR);",
+                        "    sb.append(example.classVar);",
+                        "    sb.append(Example.CONST_VAR);",
+                        "    sb.append(classMethod());",
+                        "    sb.append(CONST_METHOD());",
+                        "  }",
+                        "  public void string(String val) {",
+                        "    sb.append(\"foo\").append(val);",
+                        "    sb.append(\"foo\").append(val != null ? \"bar\" : \"baz\");",
+                        "    // BUG: Diagnostic contains:",
+                        "    sb.append(\"foo\" + val);",
+                        "  }",
+                        "  public void number(int val) {",
+                        "    sb.append(\"foo\").append(val);",
+                        "    sb.append(\"foo\").append(val + val);",
+                        "    sb.append(\"foo\").append(val > 0 ? \"bar\" : \"baz\");",
+                        "    // BUG: Diagnostic contains:",
+                        "    sb.append(\"foo\" + val);",
+                        "    // BUG: Diagnostic contains:",
+                        "    sb.append(\"foo\" + String.valueOf(val));",
+                        "    // BUG: Diagnostic contains:",
+                        "    sb.append(\"foo\" + Integer.toString(val));",
+                        "  }",
+                        "}")
+                .doTest();
+    }
+
+    @Test
+    public void testPlusAssignment() {
+        compilationHelper
+                .addSourceLines("Example.java",
+                        "public class Example {",
+                        "  public void string(String val) {",
+                        "    String s = \"foo\";",
+                        "    // BUG: Diagnostic contains:",
+                        "    s += \"bar\";",
+                        "    // BUG: Diagnostic contains:",
+                        "    s += val;",
+                        "    // BUG: Diagnostic contains:",
+                        "    s += (\"bar\" + \"baz\");",
+                        "  }",
+                        "  public void number(int val) {",
+                        "    int other = 42;",
+                        "    other += 24;",
+                        "    other += val;",
+                        "  }",
+                        "}")
+                .doTest();
+    }
 }