Skip to content

Commit

Permalink
Update a few checks (and a class of tests, with AbstractToString) to …
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@graememorgan
graememorgan authored and Error Prone Team committed Jan 3, 2024
1 parent fd21bc9 commit 58a9e80
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 32 deletions.
4 changes: 3 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/AbstractToString.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ protected abstract Optional<Fix> toStringFix(
symbolHasAnnotation(FormatMethod.class);

private static final Matcher<ExpressionTree> 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<ExpressionTree> VALUE_OF =
staticMethod()
Expand Down
31 changes: 22 additions & 9 deletions core/src/main/java/com/google/errorprone/bugpatterns/AnnotateFormatMethod.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -60,17 +62,28 @@ public final class AnnotateFormatMethod extends BugChecker implements MethodInvo

private static final Matcher<ExpressionTree> STRING_FORMAT =
staticMethod().onClass("java.lang.String").named("format");
private static final Matcher<ExpressionTree> 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;
}
Expand Down
55 changes: 33 additions & 22 deletions core/src/main/java/com/google/errorprone/bugpatterns/StringFormatWithLiteral.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,12 +46,30 @@ public final class StringFormatWithLiteral extends BugChecker
private static final Matcher<ExpressionTree> STRING_FORMAT_METHOD_MATCHER =
staticMethod().onClass("java.lang.String").named("format");

private static final Matcher<ExpressionTree> 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<ExpressionTree> 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;
}
Expand All @@ -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<? extends ExpressionTree> 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) {
Expand All @@ -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<? extends ExpressionTree> arguments) {
ExpressionTree formatString, List<? extends ExpressionTree> 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();
}
}
18 changes: 18 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/AnnotateFormatMethodTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
45 changes: 45 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/StringFormatWithLiteralTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 58a9e80

Please sign in to comment.