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

Commit a52266b3 authored by Jeff Sharkey's avatar Jeff Sharkey
Browse files

Recommend efficient String operations.

Android offers several efficient alternatives to some upstream
String operations, such as the newly added TextUtils.formatSimple().

This checker also detects and discourages transparent StringBuilder
operations related to Preconditions, where we always pay the cost of
building the failure message string, even in the successful case.

Bug: 170978902
Test: atest error_prone_android_framework_test
Change-Id: I8cef4c50d8b0da3f1e66727dfa724ad44b88963b
parent af7c5f95
Loading
Loading
Loading
Loading
+146 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2020 The Android Open Source Project
 *
 * 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.android;

import static com.google.errorprone.BugPattern.SeverityLevel.WARNING;
import static com.google.errorprone.matchers.Matchers.allOf;
import static com.google.errorprone.matchers.Matchers.contains;
import static com.google.errorprone.matchers.Matchers.kindIs;
import static com.google.errorprone.matchers.Matchers.methodInvocation;
import static com.google.errorprone.matchers.Matchers.not;
import static com.google.errorprone.matchers.Matchers.staticMethod;

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.MethodInvocationTreeMatcher;
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.ExpressionTree;
import com.sun.source.tree.LiteralTree;
import com.sun.source.tree.MethodInvocationTree;
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;

/**
 * Android offers several efficient alternatives to some upstream {@link String}
 * operations.
 */
@AutoService(BugChecker.class)
@BugPattern(
    name = "AndroidFrameworkEfficientStrings",
    summary = "Verifies efficient Strings best-practices",
    severity = WARNING)
public final class EfficientStringsChecker extends BugChecker
        implements MethodInvocationTreeMatcher {

    private static final Matcher<ExpressionTree> FORMAT_CALL = methodInvocation(
            staticMethod().onClass("java.lang.String").named("format"));
    private static final Matcher<ExpressionTree> PRECONDITIONS_CALL = methodInvocation(
            staticMethod().onClass(withSimpleName("Preconditions")).withAnyName());
    private static final Matcher<ExpressionTree> OBJECTS_CALL = methodInvocation(
            staticMethod().onClass("java.util.Objects").named("requireNonNull"));

    /**
     * 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.
     */
    private static final Matcher<ExpressionTree> CONTAINS_DYNAMIC_STRING = allOf(
            contains(ExpressionTree.class, kindIs(Kind.STRING_LITERAL)),
            contains(ExpressionTree.class, not(kindIs(Kind.STRING_LITERAL))));

    @Override
    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
        if (FORMAT_CALL.matches(tree, state)) {
            // Skip over possible locale to find format string
            final List<? extends ExpressionTree> args = tree.getArguments();
            final ExpressionTree formatArg;
            final List<VarSymbol> vars = ASTHelpers.getSymbol(tree).params();
            if (vars.get(0).type.toString().equals("java.util.Locale")) {
                formatArg = args.get(1);
            } else {
                formatArg = args.get(0);
            }

            // Determine if format string is "simple" enough to replace
            if (formatArg.getKind() == Kind.STRING_LITERAL) {
                final String format = String.valueOf(((LiteralTree) formatArg).getValue());
                if (isSimple(format)) {
                    return buildDescription(formatArg)
                            .setMessage("Simple format strings can be replaced with "
                                    + "TextUtils.formatSimple() for a 6x performance improvement")
                            .build();
                }
            }
        } 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)) {
                    return buildDescription(arg)
                            .setMessage("Building dynamic messages is discouraged, since they "
                                    + "always allocate a transparent StringBuilder, even in "
                                    + "the successful case")
                            .build();
                }
            }
        }
        return Description.NO_MATCH;
    }

    static boolean isSimple(String format) {
        for (int i = 0; i < format.length(); i++) {
            char c = format.charAt(i);
            if (c == '%') {
                i++;
                c = format.charAt(i);
                switch (c) {
                    case 'b':
                    case 'c':
                    case 'd':
                    case 'f':
                    case 's':
                    case 'x':
                    case '%':
                        break;
                    default:
                        return false;
                }
            }
        }
        return true;
    }

    static TypePredicate withSimpleName(final String filter) {
        return new TypePredicate() {
            @Override
            public boolean apply(Type type, VisitorState state) {
                return type.tsym.getSimpleName().toString().equals(filter);
            }
        };
    }
}
+119 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2020 The Android Open Source Project
 *
 * 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.android;

import static junit.framework.Assert.assertFalse;
import static junit.framework.Assert.assertTrue;

import com.google.errorprone.CompilationTestHelper;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class EfficientStringsCheckerTest {
    private CompilationTestHelper compilationHelper;

    @Before
    public void setUp() {
        compilationHelper = CompilationTestHelper.newInstance(
                EfficientStringsChecker.class, getClass());
    }

    @Test
    public void testSimple() {
        assertTrue(EfficientStringsChecker.isSimple(""));
        assertTrue(EfficientStringsChecker.isSimple("%s"));
        assertTrue(EfficientStringsChecker.isSimple("String %s%s and %%%% number %d%d together"));

        assertFalse(EfficientStringsChecker.isSimple("%04d"));
        assertFalse(EfficientStringsChecker.isSimple("%02x:%02x:%02x"));
    }

    @Test
    public void testFormat() {
        compilationHelper
                .addSourceLines("Example.java",
                        "import java.util.Locale;",
                        "public class Example {",
                        "  public void example(String str) {",
                        "    String.format(str, str);",
                        "    // BUG: Diagnostic contains:",
                        "    String.format(\"foo %s bar\", str);",
                        "    // BUG: Diagnostic contains:",
                        "    String.format(\"foo %d bar\", 42);",
                        "    String.format(\"foo %04d bar\", 42);",
                        "  }",
                        "  public void exampleLocale(String str) {",
                        "    String.format(Locale.US, str, str);",
                        "    // BUG: Diagnostic contains:",
                        "    String.format(Locale.US, \"foo %s bar\", str);",
                        "    // BUG: Diagnostic contains:",
                        "    String.format(Locale.US, \"foo %d bar\", 42);",
                        "    String.format(Locale.US, \"foo %04d bar\", 42);",
                        "  }",
                        "}")
                .doTest();
    }

    @Test
    public void testPreconditions() {
        compilationHelper
                .addSourceFile("/android/util/Preconditions.java")
                .addSourceLines("Example.java",
                        "import android.util.Preconditions;",
                        "import java.util.Objects;",
                        "public class Example {",
                        "  String str;",
                        "  public void checkState(boolean val) {",
                        "    Preconditions.checkState(val);",
                        "    Preconditions.checkState(val, str);",
                        "    Preconditions.checkState(val, \"foo\");",
                        "    Preconditions.checkState(val, \"foo\" + \"bar\");",
                        "    // BUG: Diagnostic contains:",
                        "    Preconditions.checkState(val, \"foo \" + val);",
                        "  }",
                        "  public void checkArgument(boolean val) {",
                        "    Preconditions.checkArgument(val);",
                        "    Preconditions.checkArgument(val, str);",
                        "    Preconditions.checkArgument(val, \"foo\");",
                        "    Preconditions.checkArgument(val, \"foo\" + \"bar\");",
                        "    // BUG: Diagnostic contains:",
                        "    Preconditions.checkArgument(val, \"foo \" + val);",
                        "  }",
                        "  public void checkNotNull(Object val) {",
                        "    Preconditions.checkNotNull(val);",
                        "    Preconditions.checkNotNull(val, str);",
                        "    Preconditions.checkNotNull(val, \"foo\");",
                        "    Preconditions.checkNotNull(val, \"foo\" + \"bar\");",
                        "    // BUG: Diagnostic contains:",
                        "    Preconditions.checkNotNull(val, \"foo \" + val);",
                        "  }",
                        "  public void requireNonNull(Object val) {",
                        "    Objects.requireNonNull(val);",
                        "    Objects.requireNonNull(val, str);",
                        "    Objects.requireNonNull(val, \"foo\");",
                        "    Objects.requireNonNull(val, \"foo\" + \"bar\");",
                        "    // BUG: Diagnostic contains:",
                        "    Objects.requireNonNull(val, \"foo \" + val);",
                        "  }",
                        "}")
                .doTest();
    }
}
+43 −0
Original line number Diff line number Diff line
/*
 * Copyright (C) 2020 The Android Open Source Project
 *
 * 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 android.util;

public class Preconditions {
    public static void checkArgument(boolean expression) {
        throw new UnsupportedOperationException();
    }

    public static void checkArgument(boolean expression, Object errorMessage) {
        throw new UnsupportedOperationException();
    }

    public static <T> T checkNotNull(T reference) {
        throw new UnsupportedOperationException();
    }

    public static <T> T checkNotNull(T reference, Object errorMessage) {
        throw new UnsupportedOperationException();
    }

    public static void checkState(boolean expression) {
        throw new UnsupportedOperationException();
    }

    public static void checkState(boolean expression, String message) {
        throw new UnsupportedOperationException();
    }
}