diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java new file mode 100644 index 0000000000..6387290331 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java @@ -0,0 +1,292 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.PERFORMANCE; +import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static java.util.stream.Collectors.joining; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.auto.value.AutoValue; +import com.google.common.base.Preconditions; +import com.google.common.base.Verify; +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.annotations.Var; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.Collections; +import java.util.Formattable; +import java.util.List; +import java.util.Locale; +import java.util.Objects; +import java.util.Optional; +import java.util.stream.Stream; +import tech.picnic.errorprone.utils.SourceCode; + +/** + * A {@link BugChecker} that flags {@link String#format(String, Object...)}, {@link + * String#format(Locale, String, Object...)} and {@link String#formatted(Object...)} invocations + * that can be omitted by delegating to another format method. + */ +// XXX: The special-casing of Throwable applies only to SLF4J 1.6.0+; see +// https://www.slf4j.org/faq.html#paramException. That should be documented. +// XXX: Some of the `Matcher`s defined here are also declared by the `Slf4jLogStatement` and +// `RedundantStringConversion` checks. Look into deduplicating them. +// XXX: Should we also simplify e.g. `LOG.error(String.join("sep", arg1, arg2), throwable)`? Perhaps +// that's too obscure. +@AutoService(BugChecker.class) +@BugPattern( + summary = "String formatting can be deferred", + link = BUG_PATTERNS_BASE_URL + "EagerStringFormatting", + linkType = CUSTOM, + severity = WARNING, + tags = {PERFORMANCE, SIMPLIFICATION}) +public final class EagerStringFormatting extends BugChecker implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher FORMATTABLE = isSubtypeOf(Formattable.class); + private static final Matcher LOCALE = isSubtypeOf(Locale.class); + private static final Matcher SLF4J_MARKER = isSubtypeOf("org.slf4j.Marker"); + private static final Matcher THROWABLE = isSubtypeOf(Throwable.class); + private static final Matcher REQUIRE_NOT_NULL_INVOCATION = + staticMethod().onClass(Objects.class.getCanonicalName()).named("requireNonNull"); + private static final Matcher GUAVA_GUARD_INVOCATION = + anyOf( + staticMethod() + .onClass(Preconditions.class.getCanonicalName()) + .namedAnyOf("checkArgument", "checkNotNull", "checkState"), + staticMethod() + .onClass(Verify.class.getCanonicalName()) + .namedAnyOf("verify", "verifyNotNull")); + private static final Matcher SLF4J_LOGGER_INVOCATION = + instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .namedAnyOf("trace", "debug", "info", "warn", "error"); + private static final Matcher STATIC_FORMAT_STRING = + staticMethod().onClass(String.class.getCanonicalName()).named("format"); + private static final Matcher INSTANCE_FORMAT_STRING = + instanceMethod().onDescendantOf(String.class.getCanonicalName()).named("formatted"); + + /** Instantiates a new {@link EagerStringFormatting} instance. */ + public EagerStringFormatting() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + Tree parent = state.getPath().getParentPath().getLeaf(); + if (!(parent instanceof MethodInvocationTree methodInvocation)) { + /* + * Fast path: this isn't a method invocation whose result is an argument to another method + * invocation. (We assume that this expression isn't redundantly wrapped in parentheses.) + */ + return Description.NO_MATCH; + } + + return StringFormatExpression.tryCreate(tree, state) + .map(expr -> analyzeFormatStringContext(expr, methodInvocation, state)) + .orElse(Description.NO_MATCH); + } + + private Description analyzeFormatStringContext( + StringFormatExpression stringFormat, MethodInvocationTree context, VisitorState state) { + List arguments = context.getArguments(); + + if (REQUIRE_NOT_NULL_INVOCATION.matches(context, state)) { + if (arguments.size() != 2) { + /* Vacuous validation that string formatting doesn't yield `null`. */ + // XXX: Maybe flag this, while we're here? + return Description.NO_MATCH; + } + + if (stringFormat.arguments().stream() + .anyMatch(EagerStringFormatting::isNonFinalLocalVariable)) { + /* + * The format operation depends on a variable that isn't final or effectively final; moving + * it into a lambda expression would cause a compilation error. + */ + // XXX: Define a custom message for this case? + return describeMatch(context); + } + + /* + * Suggest that the string formatting is deferred. Note that moving the format operation into + * a lambda expression will cause a compilation in case any of its arguments aren't + * effectively final. + */ + return describeMatch(context, SuggestedFix.prefixWith(stringFormat.expression(), "() -> ")); + } + + if (GUAVA_GUARD_INVOCATION.matches(context, state)) { + if (arguments.size() < 2) { + /* Vacuous validation that string formatting doesn't yield `null`. */ + // XXX: Maybe flag this, while we're here? + return Description.NO_MATCH; + } + + if (stringFormat.simplifiableFormatString().isEmpty()) { + /* We can't simplify this case; only flag it. */ + // XXX: Use a custom message. + return describeMatch(context); + } + + // XXX: Collapse into previous statement? (Depends on the extent to which we'll customize + // messages.) + if (arguments.size() > 2) { + /* + * The format string produces a format string itself, or its result is the input to another + * format operation. This is a complex case that we'll only flag. + */ + // XXX: Use a custom message. + return describeMatch(context); + } + + return describeMatch(context, stringFormat.suggestFlattening("%s", state)); + } + + if (SLF4J_LOGGER_INVOCATION.matches(context, state)) { + if (stringFormat.simplifiableFormatString().isEmpty()) { + /* We can't simplify this case; only flag it. */ + // XXX: Use a custom message. + return describeMatch(context); + } + + int leftOffset = SLF4J_MARKER.matches(arguments.get(0), state) ? 1 : 0; + int rightOffset = THROWABLE.matches(arguments.get(arguments.size() - 1), state) ? 1 : 0; + if (arguments.size() != leftOffset + 1 + rightOffset) { + /* + * The format string produces a format string itself, or its result is the input to another + * format operation. This is a complex case that we'll only flag. + */ + // XXX: Use a custom message. + return describeMatch(context); + } + + return describeMatch(context, stringFormat.suggestFlattening("{}", state)); + } + + /* + * The string formatting operation does not appear to happen in a context that admits of + * simplification or optimization. + */ + return Description.NO_MATCH; + } + + private static boolean isNonFinalLocalVariable(Tree tree) { + Symbol symbol = ASTHelpers.getSymbol(tree); + return symbol instanceof VarSymbol + && symbol.owner instanceof MethodSymbol + && !ASTHelpers.isConsideredFinal(symbol); + } + + /** Description of a string format expression. */ + @AutoValue + abstract static class StringFormatExpression { + /** The full string format expression. */ + abstract Tree expression(); + + /** The string format arguments to be plugged into its format string. */ + abstract ImmutableList arguments(); + + /** + * The constant format string, if it contains only {@code %s} placeholders, and the number of + * said placeholders matches the number of format arguments. + */ + abstract Optional simplifiableFormatString(); + + private SuggestedFix suggestFlattening(String newPlaceholder, VisitorState state) { + return SuggestedFix.replace( + expression(), + Stream.concat( + Stream.of(deriveFormatStringExpression(newPlaceholder)), + arguments().stream().map(arg -> SourceCode.treeToString(arg, state))) + .collect(joining(", "))); + } + + final String deriveFormatStringExpression(String newPlaceholder) { + return SourceCode.toStringConstantExpression( + String.format( + simplifiableFormatString() + .orElseThrow( + () -> new IllegalStateException("Format string cannot be simplified")), + Collections.nCopies(arguments().size(), newPlaceholder).toArray())); + } + + private static Optional tryCreate( + MethodInvocationTree tree, VisitorState state) { + if (INSTANCE_FORMAT_STRING.matches(tree, state)) { + return Optional.of( + create( + tree, + ASTHelpers.getReceiver(tree), + ImmutableList.copyOf(tree.getArguments()), + state)); + } + + if (STATIC_FORMAT_STRING.matches(tree, state)) { + List arguments = tree.getArguments(); + int argOffset = LOCALE.matches(arguments.get(0), state) ? 1 : 0; + return Optional.of( + create( + tree, + arguments.get(argOffset), + ImmutableList.copyOf(arguments.subList(argOffset + 1, arguments.size())), + state)); + } + + return Optional.empty(); + } + + private static StringFormatExpression create( + Tree expression, + Tree formatString, + ImmutableList arguments, + VisitorState state) { + return new AutoValue_EagerStringFormatting_StringFormatExpression( + expression, + arguments, + Optional.ofNullable(ASTHelpers.constValue(formatString, String.class)) + .filter(template -> isSimplifiable(template, arguments, state))); + } + + private static boolean isSimplifiable( + String formatString, ImmutableList arguments, VisitorState state) { + if (arguments.stream().anyMatch(arg -> FORMATTABLE.matches(arg, state))) { + /* `Formattable` arguments can have arbitrary format semantics. */ + return false; + } + + @Var int placeholderCount = 0; + for (int p = formatString.indexOf('%'); p != -1; p = formatString.indexOf('%', p + 2)) { + if (p == formatString.length() - 1) { + /* Malformed format string with trailing `%`. */ + return false; + } + + char modifier = formatString.charAt(p + 1); + if (modifier == 's') { + placeholderCount++; + } else if (modifier != '%') { + /* Only `%s` and `%%` (a literal `%`) are supported. */ + return false; + } + } + + return placeholderCount == arguments.size(); + } + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java index 7836961f9e..382d55923d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogStatement.java @@ -28,8 +28,6 @@ /** A {@link BugChecker} that flags SLF4J usages that are likely to be in error. */ // XXX: The special-casing of Throwable applies only to SLF4J 1.6.0+; see // https://www.slf4j.org/faq.html#paramException. That should be documented. -// XXX: Also simplify `LOG.error(String.format("Something %s", arg), throwable)`. -// XXX: Also simplify `LOG.error(String.join("sep", arg1, arg2), throwable)`? Perhaps too obscure. // XXX: Write a similar checker for Spring RestTemplates, String.format and friends, Guava // preconditions, ... @AutoService(BugChecker.class) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java new file mode 100644 index 0000000000..5f5f1d3812 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java @@ -0,0 +1,227 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +// XXX: Lots of test coverage still missing, which other tests are possibly redundant. Check with +// PIT. +final class EagerStringFormattingTest { + @Test + void identification() { + CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass()) + .addSourceLines( + "A.java", + "import static com.google.common.base.Preconditions.checkArgument;", + "import static com.google.common.base.Preconditions.checkNotNull;", + "import static com.google.common.base.Preconditions.checkState;", + "import static com.google.common.base.Verify.verify;", + "import static com.google.common.base.Verify.verifyNotNull;", + "import static java.util.Objects.requireNonNull;", + "", + "import java.util.Locale;", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "", + "class A {", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private int nonFinalField = 0;", + "", + " void m() {", + " int effectivelyFinalLocal = 0;", + " /* A local variable that is also not effectively final. */", + " int nonFinalLocal = 0;", + " nonFinalLocal = 1;", + "", + " String.format(\"%s\", \"foo\");", + " String.format(Locale.US, \"%s\", \"foo\");", + " \"%s\".formatted(\"foo\");", + " String.format(\"%s\", \"foo\", \"bar\");", + " String.format(\"%s %s\", \"foo\", \"bar\");", + " String.format(\"%s %s %%\", \"foo\", \"bar\");", + "", + " System.out.println(String.format(\"%s\", nonFinalLocal));", + "", + " requireNonNull(\"never-null\");", + " requireNonNull(String.format(\"Never-null format string: %s\", nonFinalLocal));", + " requireNonNull(\"never-null\", () -> String.format(\"Format string: %s\", nonFinalField));", + " // BUG: Diagnostic contains:", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", effectivelyFinalLocal));", + " // BUG: Diagnostic contains:", + " requireNonNull(", + " \"never-null\", String.format(\"Custom format string: %s, %d\", getClass(), nonFinalField));", + " // BUG: Diagnostic contains:", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + "", + " checkArgument(true);", + " checkNotNull(\"never-null\", \"Without format string\");", + " checkState(true, \"With format string: %s\", nonFinalLocal);", + " // BUG: Diagnostic contains:", + " verify(true, String.format(\"Vacuous format string\"));", + " // BUG: Diagnostic contains:", + " verifyNotNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic contains:", + " checkArgument(true, String.format(\"Custom format string: %d\", nonFinalLocal));", + " // BUG: Diagnostic contains:", + " checkNotNull(\"never-null\", String.format(\"Generated format string: %%s\"), nonFinalLocal);", + " // BUG: Diagnostic contains:", + " checkState(", + " true,", + " \"Format string with format string argument: %s\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + "", + " LOG.trace(\"Without format string\");", + " LOG.debug(\"With format string: {}\", nonFinalLocal);", + " LOG.info((Marker) null, \"With marker\");", + " LOG.warn((Marker) null, \"With marker and format string: {}\", nonFinalLocal);", + " LOG.error(\"With throwable\", new RuntimeException());", + " LOG.trace(\"With throwable and format string: {}\", nonFinalLocal, new RuntimeException());", + " LOG.debug((Marker) null, \"With marker and throwable\", new RuntimeException());", + " LOG.debug(", + " (Marker) null,", + " \"With marker, throwable and format string: {}\",", + " nonFinalLocal,", + " new RuntimeException());", + " // BUG: Diagnostic contains:", + " LOG.trace(String.format(\"Vacuous format string\"));", + " // BUG: Diagnostic contains:", + " LOG.trace(String.format(\"With format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic contains:", + " LOG.trace((Marker) null, String.format(\"With marker and format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic contains:", + " LOG.trace(", + " String.format(\"With throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " // BUG: Diagnostic contains:", + " LOG.trace(", + " (Marker) null,", + " String.format(\"With marker, throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " // BUG: Diagnostic contains:", + " LOG.trace(String.format(\"Generated format string: {}\"), nonFinalLocal);", + " // BUG: Diagnostic contains:", + " LOG.trace(", + " \"Format string with format string argument: {}\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(EagerStringFormatting.class, getClass()) + .addInputLines( + "A.java", + "import static com.google.common.base.Preconditions.checkArgument;", + "import static com.google.common.base.Preconditions.checkNotNull;", + "import static com.google.common.base.Preconditions.checkState;", + "import static com.google.common.base.Verify.verify;", + "import static com.google.common.base.Verify.verifyNotNull;", + "import static java.util.Objects.requireNonNull;", + "", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "", + "class A {", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private int nonFinalField = 0;", + "", + " void m() {", + " int effectivelyFinalLocal = 0;", + " /* A local variable that is also not effectively final. */", + " int nonFinalLocal = 0;", + " nonFinalLocal = 1;", + "", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", effectivelyFinalLocal));", + " requireNonNull(", + " \"never-null\", String.format(\"Custom format string: %s, %d\", getClass(), nonFinalField));", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + "", + " verify(true, String.format(\"Vacuous format string: '\\\"%%\"));", + " verifyNotNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + " checkArgument(true, String.format(\"Custom format string: %d\", nonFinalLocal));", + " checkNotNull(\"never-null\", String.format(\"Generated format string: %%s\"), nonFinalLocal);", + " checkState(", + " true,", + " \"Format string with format string argument: %s\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + "", + " LOG.trace(String.format(\"Vacuous format string\"));", + " LOG.trace(String.format(\"With format string: %s\", nonFinalLocal));", + " LOG.trace((Marker) null, String.format(\"With marker and format string: %s\", nonFinalLocal));", + " LOG.trace(", + " String.format(\"With throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " LOG.trace(", + " (Marker) null,", + " String.format(\"With marker, throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " LOG.trace(String.format(\"Generated format string: {}\"), nonFinalLocal);", + " LOG.trace(", + " \"Format string with format string argument: {}\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + " }", + "}") + .addOutputLines( + "A.java", + "import static com.google.common.base.Preconditions.checkArgument;", + "import static com.google.common.base.Preconditions.checkNotNull;", + "import static com.google.common.base.Preconditions.checkState;", + "import static com.google.common.base.Verify.verify;", + "import static com.google.common.base.Verify.verifyNotNull;", + "import static java.util.Objects.requireNonNull;", + "", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "", + "class A {", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private int nonFinalField = 0;", + "", + " void m() {", + " int effectivelyFinalLocal = 0;", + " /* A local variable that is also not effectively final. */", + " int nonFinalLocal = 0;", + " nonFinalLocal = 1;", + "", + " requireNonNull(\"never-null\", () -> String.format(\"Format string: %s\", effectivelyFinalLocal));", + " requireNonNull(", + " \"never-null\",", + " () -> String.format(\"Custom format string: %s, %d\", getClass(), nonFinalField));", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + "", + " verify(true, \"Vacuous format string: '\\\"%\");", + " verifyNotNull(\"never-null\", \"Format string: %s\", nonFinalLocal);", + " checkArgument(true, String.format(\"Custom format string: %d\", nonFinalLocal));", + " checkNotNull(\"never-null\", String.format(\"Generated format string: %%s\"), nonFinalLocal);", + " checkState(", + " true,", + " \"Format string with format string argument: %s\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + "", + " LOG.trace(\"Vacuous format string\");", + " LOG.trace(\"With format string: {}\", nonFinalLocal);", + " LOG.trace((Marker) null, \"With marker and format string: {}\", nonFinalLocal);", + " LOG.trace(\"With throwable and format string: {}\", nonFinalLocal, new RuntimeException());", + " LOG.trace(", + " (Marker) null,", + " \"With marker, throwable and format string: {}\",", + " nonFinalLocal,", + " new RuntimeException());", + " LOG.trace(String.format(\"Generated format string: {}\"), nonFinalLocal);", + " LOG.trace(", + " \"Format string with format string argument: {}\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}