From 58a9e8082b2344f8fb2c3814112b46104708bbab Mon Sep 17 00:00:00 2001 From: ghm Date: Wed, 3 Jan 2024 06:51:17 -0800 Subject: [PATCH] Update a few checks (and a class of tests, with AbstractToString) to handle #formatted. There are some ERROR-level checks under AbstractToString, but I think #formatted can't be used in the depot yet? PiperOrigin-RevId: 595389167 --- .../bugpatterns/AbstractToString.java | 4 +- .../bugpatterns/AnnotateFormatMethod.java | 31 ++++++++--- .../bugpatterns/StringFormatWithLiteral.java | 55 +++++++++++-------- .../bugpatterns/AnnotateFormatMethodTest.java | 18 ++++++ .../StringFormatWithLiteralTest.java | 45 +++++++++++++++ 5 files changed, 121 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java index 3be2fb60fe9..c87e48b91e6 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java @@ -96,7 +96,9 @@ protected abstract Optional toStringFix( symbolHasAnnotation(FormatMethod.class); private static final Matcher STRING_FORMAT = - staticMethod().onClass("java.lang.String").named("format"); + anyOf( + staticMethod().onClass("java.lang.String").named("format"), + instanceMethod().onExactClass("java.lang.String").named("formatted")); private static final Matcher VALUE_OF = staticMethod() diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/AnnotateFormatMethod.java b/core/src/main/java/com/google/errorprone/bugpatterns/AnnotateFormatMethod.java index 3b7a4854e65..16a8df71427 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/AnnotateFormatMethod.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/AnnotateFormatMethod.java @@ -20,7 +20,9 @@ import static com.google.common.collect.MoreCollectors.toOptional; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; @@ -47,9 +49,9 @@ */ @BugPattern( summary = - "This method passes a pair of parameters through to String.format, but the enclosing" - + " method wasn't annotated @FormatMethod. Doing so gives compile-time rather than" - + " run-time protection against malformed format strings.", + "This method uses a pair of parameters as a format string and its arguments, but the" + + " enclosing method wasn't annotated @FormatMethod. Doing so gives compile-time rather" + + " than run-time protection against malformed format strings.", tags = FRAGILE_CODE, severity = WARNING) public final class AnnotateFormatMethod extends BugChecker implements MethodInvocationTreeMatcher { @@ -60,17 +62,28 @@ public final class AnnotateFormatMethod extends BugChecker implements MethodInvo private static final Matcher STRING_FORMAT = staticMethod().onClass("java.lang.String").named("format"); + private static final Matcher FORMATTED = + instanceMethod().onExactClass("java.lang.String").named("formatted"); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!STRING_FORMAT.matches(tree, state)) { + VarSymbol formatString; + VarSymbol formatArgs; + if (STRING_FORMAT.matches(tree, state)) { + if (tree.getArguments().size() != 2) { + return Description.NO_MATCH; + } + formatString = asSymbol(tree.getArguments().get(0)); + formatArgs = asSymbol(tree.getArguments().get(1)); + } else if (FORMATTED.matches(tree, state)) { + if (tree.getArguments().size() != 1) { + return Description.NO_MATCH; + } + formatString = asSymbol(getReceiver(tree)); + formatArgs = asSymbol(tree.getArguments().get(0)); + } else { return Description.NO_MATCH; } - if (tree.getArguments().size() != 2) { - return Description.NO_MATCH; - } - VarSymbol formatString = asSymbol(tree.getArguments().get(0)); - VarSymbol formatArgs = asSymbol(tree.getArguments().get(1)); if (formatString == null || formatArgs == null) { return Description.NO_MATCH; } diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/StringFormatWithLiteral.java b/core/src/main/java/com/google/errorprone/bugpatterns/StringFormatWithLiteral.java index 503bfae2461..bf3f3950add 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/StringFormatWithLiteral.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/StringFormatWithLiteral.java @@ -16,9 +16,13 @@ package com.google.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.util.ASTHelpers.getReceiver; +import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; @@ -42,12 +46,30 @@ public final class StringFormatWithLiteral extends BugChecker private static final Matcher STRING_FORMAT_METHOD_MATCHER = staticMethod().onClass("java.lang.String").named("format"); + private static final Matcher FORMATTED = + instanceMethod().onExactClass("java.lang.String").named("formatted"); + private static final Pattern SPECIFIER_ALLOW_LIST_REGEX = Pattern.compile("%(d|s|S|c|b|B)"); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (STRING_FORMAT_METHOD_MATCHER.matches(tree, state) && shouldRefactorStringFormat(tree)) { - return describeMatch(tree, refactor(tree)); + if (STRING_FORMAT_METHOD_MATCHER.matches(tree, state)) { + ImmutableList arguments = + tree.getArguments().stream().skip(1).collect(toImmutableList()); + if (shouldRefactorStringFormat(tree.getArguments().get(0), arguments)) { + return describeMatch( + tree, + SuggestedFix.replace( + tree, getFormattedUnifiedString(tree.getArguments().get(0), arguments))); + } + } + if (FORMATTED.matches(tree, state)) { + if (shouldRefactorStringFormat(getReceiver(tree), tree.getArguments())) { + return describeMatch( + tree, + SuggestedFix.replace( + tree, getFormattedUnifiedString(getReceiver(tree), tree.getArguments()))); + } } return Description.NO_MATCH; } @@ -57,15 +79,15 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState * string included). Format strings (first argument) as variables or constants are excluded from * refactoring. The refactoring also has an allowlist of "non trivial" formatting specifiers. This * is done since there are some instances where the String.format() invocation is justified even - * with + * with a CONSTANT but non-literal format string. */ - private static boolean shouldRefactorStringFormat(MethodInvocationTree tree) { - if (!tree.getArguments().stream() - .allMatch(argumentTree -> argumentTree instanceof LiteralTree)) { + private static boolean shouldRefactorStringFormat( + ExpressionTree formatString, List arguments) { + if (!(formatString instanceof LiteralTree) + || !arguments.stream().allMatch(argumentTree -> argumentTree instanceof LiteralTree)) { return false; } - LiteralTree formatString = (LiteralTree) tree.getArguments().get(0); - return onlyContainsSpecifiersInAllowList((String) formatString.getValue()); + return onlyContainsSpecifiersInAllowList((String) ((LiteralTree) formatString).getValue()); } private static boolean onlyContainsSpecifiersInAllowList(String formatString) { @@ -74,29 +96,18 @@ private static boolean onlyContainsSpecifiersInAllowList(String formatString) { return !noSpecifierFormatBase.contains("%"); } - private static SuggestedFix refactor(MethodInvocationTree tree) { - return SuggestedFix.replace( - tree, getFormattedUnifiedString(getFormatString(tree), tree.getArguments())); - } - /** * Formats the string originally on the String.format to be a unified string with all the literal * parameters, when available. */ private static String getFormattedUnifiedString( - String formatString, List arguments) { + ExpressionTree formatString, List arguments) { String unescapedFormatString = String.format( - formatString, + (String) ((LiteralTree) formatString).getValue(), arguments.stream() - .skip(1) // skip the format string argument. - .map(literallTree -> ((LiteralTree) literallTree).getValue()) + .map(literalTree -> ((LiteralTree) literalTree).getValue()) .toArray(Object[]::new)); return '"' + SourceCodeEscapers.javaCharEscaper().escape(unescapedFormatString) + '"'; } - - private static String getFormatString(MethodInvocationTree tree) { - LiteralTree formatStringTree = (LiteralTree) tree.getArguments().get(0); - return formatStringTree.getValue().toString(); - } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/AnnotateFormatMethodTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/AnnotateFormatMethodTest.java index 8c58c80faae..47b8490f6bf 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/AnnotateFormatMethodTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/AnnotateFormatMethodTest.java @@ -16,7 +16,10 @@ package com.google.errorprone.bugpatterns; +import static org.junit.Assume.assumeTrue; + import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.util.RuntimeVersion; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -45,6 +48,21 @@ public void positiveCase() { .doTest(); } + @Test + public void formatted() { + assumeTrue(RuntimeVersion.isAtLeast15()); + compilationHelper + .addSourceLines( + "AnnotateFormatMethodPositiveCases.java", + "class AnnotateFormatMethodPositiveCases {", + " // BUG: Diagnostic contains: FormatMethod", + " String formatMe(String formatString, Object... args) {", + " return formatString.formatted(args);", + " }", + "}") + .doTest(); + } + @Test public void alreadyAnnotated() { compilationHelper diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/StringFormatWithLiteralTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/StringFormatWithLiteralTest.java index b08b042c4d2..dda227eef24 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/StringFormatWithLiteralTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/StringFormatWithLiteralTest.java @@ -16,8 +16,11 @@ package com.google.errorprone.bugpatterns; +import static org.junit.Assume.assumeTrue; + import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; +import com.google.errorprone.util.RuntimeVersion; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -195,6 +198,27 @@ public void refactoringStringFormatWithNoArguments() { .doTest(); } + @Test + public void refactoringFormattedWithNoArguments() { + assumeTrue(RuntimeVersion.isAtLeast15()); + refactoringHelper + .addInputLines( + "ExampleClass.java", + "public class ExampleClass {", + " String test() {", + " return \"Formatting nothing\".formatted();", + " }", + "}") + .addOutputLines( + "ExampleClass.java", + "public class ExampleClass {", + " String test() {", + " return \"Formatting nothing\";", + " }", + "}") + .doTest(); + } + @Test public void refactoringStringFormatWithIntegerLiteral() { refactoringHelper @@ -215,6 +239,27 @@ public void refactoringStringFormatWithIntegerLiteral() { .doTest(); } + @Test + public void refactoringFormattedWithIntegerLiteral() { + assumeTrue(RuntimeVersion.isAtLeast15()); + refactoringHelper + .addInputLines( + "ExampleClass.java", + "public class ExampleClass {", + " String test() {", + " return \"Formatting this integer: %d\".formatted(1);", + " }", + "}") + .addOutputLines( + "ExampleClass.java", + "public class ExampleClass {", + " String test() {", + " return \"Formatting this integer: 1\";", + " }", + "}") + .doTest(); + } + @Test public void refactoringStringFormatWithBooleanLiteral() { refactoringHelper