diff options
3 files changed, 308 insertions, 0 deletions
diff --git a/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java new file mode 100644 index 000000000000..3fbd51deec65 --- /dev/null +++ b/errorprone/java/com/google/errorprone/bugpatterns/android/EfficientStringsChecker.java @@ -0,0 +1,146 @@ +/* + * 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); + } + }; + } +} diff --git a/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java new file mode 100644 index 000000000000..a755564d52dd --- /dev/null +++ b/errorprone/tests/java/com/google/errorprone/bugpatterns/android/EfficientStringsCheckerTest.java @@ -0,0 +1,119 @@ +/* + * 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(); + } +} diff --git a/errorprone/tests/res/android/util/Preconditions.java b/errorprone/tests/res/android/util/Preconditions.java new file mode 100644 index 000000000000..558cdaf483a4 --- /dev/null +++ b/errorprone/tests/res/android/util/Preconditions.java @@ -0,0 +1,43 @@ +/* + * 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(); + } +} |