Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5c46da2a authored by Jeff Sharkey's avatar Jeff Sharkey
Browse files

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
parent c0e3a096

errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java

+76 −12
Original line number Diff line number Diff line
@@ -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;

    }

errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java

+122 −0
Original line number Diff line number Diff line
@@ -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();

    }

}