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..036bdb7056 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EagerStringFormatting.java @@ -0,0 +1,325 @@ +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.Objects.requireNonNull; +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.base.VerifyException; +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} and {@link String#formatted} 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. +// XXX: This check currently only flags string format expressions that are a direct argument to +// another format-capable method invocation. Indirect cases, such as where the result is assigned to +// a variable, are currently not covered. +@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_NON_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"); + private static final String MESSAGE_NEVER_NULL_ARGUMENT = + "String formatting never yields `null` expression"; + + /** 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. + */ + // XXX: This logic assumes that the string format operation isn't redundantly wrapped in + // parentheses. Similar assumptions likely exist throughout the code base. Investigate how to + // structurally cover such cases. + 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) { + if (REQUIRE_NON_NULL_INVOCATION.matches(context, state)) { + return analyzeRequireNonNullStringFormatContext(stringFormat, context); + } + + if (GUAVA_GUARD_INVOCATION.matches(context, state)) { + return analyzeGuavaGuardStringFormatContext(stringFormat, context, state); + } + + if (SLF4J_LOGGER_INVOCATION.matches(context, state)) { + return analyzeSlf4jLoggerStringFormatContext(stringFormat, context, state); + } + + /* + * The string formatting operation does not appear to happen in a context that admits of + * simplification or optimization. + */ + return Description.NO_MATCH; + } + + private Description analyzeRequireNonNullStringFormatContext( + StringFormatExpression stringFormat, MethodInvocationTree context) { + List arguments = context.getArguments(); + if (arguments.size() != 2 || arguments.get(0).equals(stringFormat.expression())) { + /* Vacuous validation that string formatting doesn't yield `null`. */ + return buildDescription(context).setMessage(MESSAGE_NEVER_NULL_ARGUMENT).build(); + } + + 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. + */ + return buildDescription(context) + .setMessage(message() + " (but this requires introducing an effectively final variable)") + .build(); + } + + /* Suggest that the string formatting is deferred. */ + return describeMatch(context, SuggestedFix.prefixWith(stringFormat.expression(), "() -> ")); + } + + private Description analyzeGuavaGuardStringFormatContext( + StringFormatExpression stringFormat, MethodInvocationTree context, VisitorState state) { + List arguments = context.getArguments(); + if (arguments.get(0).equals(stringFormat.expression())) { + /* + * Vacuous `checkNotNull` or `verifyNotNull` validation that string formatting doesn't yield + * `null`. + */ + return buildDescription(context).setMessage(MESSAGE_NEVER_NULL_ARGUMENT).build(); + } + + if (stringFormat.simplifiableFormatString().isEmpty() || arguments.size() > 2) { + /* + * The format string cannot be simplified, or the format string produces a format string + * itself, or its result is the input to another format operation. These are complex cases + * that we'll only flag. + */ + return createSimplificationSuggestion(context, "Guava"); + } + + return describeMatch(context, stringFormat.suggestFlattening("%s", state)); + } + + private Description analyzeSlf4jLoggerStringFormatContext( + StringFormatExpression stringFormat, MethodInvocationTree context, VisitorState state) { + if (stringFormat.simplifiableFormatString().isEmpty()) { + /* We can't simplify this case; only flag it. */ + return createSimplificationSuggestion(context, "SLF4J"); + } + + List arguments = context.getArguments(); + 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. + */ + return createSimplificationSuggestion(context, "SLF4J"); + } + + return describeMatch(context, stringFormat.suggestFlattening("{}", state)); + } + + private static boolean isNonFinalLocalVariable(Tree tree) { + Symbol symbol = ASTHelpers.getSymbol(tree); + return symbol instanceof VarSymbol + && symbol.owner instanceof MethodSymbol + && !ASTHelpers.isConsideredFinal(symbol); + } + + private Description createSimplificationSuggestion(MethodInvocationTree context, String library) { + return buildDescription(context) + .setMessage( + "%s (assuming that %s's simplified formatting support suffices)" + .formatted(message(), library)) + .build(); + } + + /** Description of a string format expression. */ + @AutoValue + abstract static class StringFormatExpression { + /** The full string format expression. */ + abstract MethodInvocationTree expression(); + + /** The format string expression. */ + abstract Tree formatString(); + + /** 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, state)), + arguments().stream().map(arg -> SourceCode.treeToString(arg, state))) + .collect(joining(", "))); + } + + private String deriveFormatStringExpression(String newPlaceholder, VisitorState state) { + String formatString = + String.format( + simplifiableFormatString() + .orElseThrow(() -> new VerifyException("Format string cannot be simplified")), + Collections.nCopies(arguments().size(), newPlaceholder).toArray()); + + /* + * If the suggested replacement format string is the same as the original, then use the + * expression's existing source code representation. This way string constant references are + * not unnecessarily replaced. + */ + return formatString.equals(ASTHelpers.constValue(formatString(), String.class)) + ? SourceCode.treeToString(formatString(), state) + : SourceCode.toStringConstantExpression(formatString, state); + } + + private static Optional tryCreate( + MethodInvocationTree tree, VisitorState state) { + if (INSTANCE_FORMAT_STRING.matches(tree, state)) { + return Optional.of( + create( + tree, + requireNonNull(ASTHelpers.getReceiver(tree), "Receiver unexpectedly absent"), + 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( + MethodInvocationTree expression, + Tree formatString, + ImmutableList arguments, + VisitorState state) { + return new AutoValue_EagerStringFormatting_StringFormatExpression( + expression, + formatString, + 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..97af5abdf3 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/EagerStringFormattingTest.java @@ -0,0 +1,286 @@ +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; + +final class EagerStringFormattingTest { + @Test + void identification() { + CompilationTestHelper.newInstance(EagerStringFormatting.class, getClass()) + .expectErrorMessage("DEFER", m -> m.contains("String formatting can be deferred\n")) + .expectErrorMessage( + "DEFER_EXTRA_VARIABLE", + m -> + m.contains( + "String formatting can be deferred (but this requires introducing an effectively final variable)")) + .expectErrorMessage( + "DEFER_SIMPLIFIED_GUAVA", + m -> + m.contains( + "String formatting can be deferred (assuming that Guava's simplified formatting support suffices)")) + .expectErrorMessage( + "DEFER_SIMPLIFIED_SLF4J", + m -> + m.contains( + "String formatting can be deferred (assuming that SLF4J's simplified formatting support suffices)")) + .expectErrorMessage( + "VACUOUS", m -> m.contains("String formatting never yields `null` expression")) + .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.Formattable;", + "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() {", + " Formattable formattable = (formatter, flags, width, precision) -> {};", + " 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(\"never-null\", () -> String.format(\"Format string: %s\", nonFinalField));", + " // BUG: Diagnostic matches: VACUOUS", + " requireNonNull(String.format(\"Never-null format string: %s\", nonFinalField));", + " // BUG: Diagnostic matches: VACUOUS", + " requireNonNull(\"Never-null format string: %s\".formatted(nonFinalField), \"message\");", + " // BUG: Diagnostic matches: VACUOUS", + " requireNonNull(", + " String.format(\"Never-null format string\"), String.format(\"Malformed format string: %\"));", + " // BUG: Diagnostic matches: DEFER_EXTRA_VARIABLE", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", effectivelyFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " requireNonNull(", + " \"never-null\",", + " String.format(", + " \"Custom format string: %s, %d, %s\", getClass(), nonFinalField, \"string-constant\"));", + "", + " checkArgument(true);", + " checkNotNull(\"never-null\");", + " checkState(false);", + " verify(true);", + " verifyNotNull(\"never-null\");", + " checkArgument(false, \"Without format string\");", + " checkNotNull(\"never-null\", \"Without format string\");", + " checkState(true, \"Without format string\");", + " verify(false, \"Without format string\");", + " verifyNotNull(\"never-null\", \"Without format string\");", + " checkArgument(true, \"With format string: %s\", nonFinalLocal);", + " checkNotNull(\"never-null\", \"With format string: %s\", nonFinalLocal);", + " checkState(false, \"With format string: %s\", nonFinalLocal);", + " verify(true, \"With format string: %s\", nonFinalLocal);", + " verifyNotNull(\"never-null\", \"With format string: %s\", nonFinalLocal);", + " // BUG: Diagnostic matches: VACUOUS", + " checkNotNull(String.format(\"Never-null format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic matches: VACUOUS", + " verifyNotNull(\"Never-null format string: %s\".formatted(nonFinalLocal), \"message\");", + " // BUG: Diagnostic matches: VACUOUS", + " checkNotNull(", + " String.format(\"Never-null format string\"), String.format(\"Malformed format string: %\"));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkArgument(true, String.format(toString()));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkNotNull(\"never-null\", toString().formatted());", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkState(true, String.format(\"Custom format string: %d\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " verify(true, \"Mismatched format string:\".formatted(nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " verifyNotNull(\"never-null\", \"Mismatched format string: %d\".formatted());", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkArgument(true, String.format(\"Malformed format string: %\"));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkNotNull(\"never-null\", \"Format string with `Formattable`: %s\".formatted(formattable));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " checkState(true, String.format(\"Generated format string: %%s\"), nonFinalLocal);", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_GUAVA", + " verify(", + " true,", + " \"Format string with format string argument: %s\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " verifyNotNull(", + " \"never-null\", String.format(\"Format string: %s, %s\", nonFinalLocal, nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " checkArgument(true, \"Format string: %s%%\".formatted(nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " checkNotNull(", + " \"never-null\", String.format(Locale.US, \"Format string with locale: %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.info(", + " (Marker) null,", + " \"With marker, throwable and format string: {}\",", + " nonFinalLocal,", + " new RuntimeException());", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.warn(String.format(toString()));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.error(toString().formatted());", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.trace(String.format(\"Custom format string: %d\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.debug(\"Mismatched format string:\".formatted(nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.info(\"Mismatched format string %d:\".formatted());", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.warn(String.format(\"Malformed format string: %\"));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.error(\"Format string with `Formattable`: %s\".formatted(formattable));", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.trace(String.format(\"Generated format string: {}\"), nonFinalLocal);", + " // BUG: Diagnostic matches: DEFER_SIMPLIFIED_SLF4J", + " LOG.debug(", + " \"Format string with format string argument: {}\",", + " String.format(\"Format string argument: %s\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " LOG.info(String.format(\"Vacuous format string %%\"));", + " // BUG: Diagnostic matches: DEFER", + " LOG.warn(String.format(\"With format string: %s, %s\", nonFinalLocal, nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " LOG.error(String.format(Locale.ROOT, \"With vacuous localized format string %%\"));", + " // BUG: Diagnostic matches: DEFER", + " LOG.trace((Marker) null, String.format(\"With marker and format string: %s\", nonFinalLocal));", + " // BUG: Diagnostic matches: DEFER", + " LOG.debug(", + " String.format(\"With throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " // BUG: Diagnostic matches: DEFER", + " LOG.info(", + " (Marker) null,", + " String.format(\"With marker, throwable and format string: %s\", nonFinalLocal),", + " new RuntimeException());", + " }", + "}") + .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 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 static final String GUAVA_COMPATIBLE_PATTERN = \"with-only-%s-placeholder\";", + " private static final String GUAVA_INCOMPATIBLE_PATTERN = \"with-%%-marker\";", + "", + " void m() {", + " requireNonNull(\"never-null\", String.format(\"Format string: %s\", 0));", + "", + " checkArgument(true, String.format(\"Vacuous format string %%\"));", + " checkNotNull(\"never-null\", \"Format string: %s %s%%\".formatted(1, 2));", + " checkState(false, String.format(Locale.US, \"Format string with locale: %s\", 3));", + " verify(true, GUAVA_COMPATIBLE_PATTERN.formatted(4));", + " verifyNotNull(\"never-null\", String.format(Locale.ENGLISH, GUAVA_COMPATIBLE_PATTERN, 5));", + " checkArgument(false, GUAVA_INCOMPATIBLE_PATTERN.formatted());", + " checkNotNull(\"never-null\", String.format(GUAVA_INCOMPATIBLE_PATTERN));", + "", + " LOG.trace(\"Vacuous format string %%\".formatted());", + " LOG.debug(String.format(\"With format string: %s, %s%%\", 6, 7));", + " LOG.info(String.format(Locale.ROOT, \"With vacuous localized format string %%\"));", + " LOG.warn((Marker) null, \"With marker and format string: %s\".formatted(8));", + " LOG.error(", + " String.format(Locale.US, \"With throwable and format string: %s, %s\", 9, 10),", + " new RuntimeException());", + " LOG.trace(", + " (Marker) null,", + " \"With marker, throwable and format string: %s\".formatted(11),", + " new RuntimeException());", + " LOG.debug(GUAVA_COMPATIBLE_PATTERN.formatted(12));", + " LOG.info(String.format(Locale.ENGLISH, GUAVA_COMPATIBLE_PATTERN, 13));", + " LOG.warn(GUAVA_INCOMPATIBLE_PATTERN.formatted());", + " LOG.error(String.format(GUAVA_INCOMPATIBLE_PATTERN));", + " }", + "}") + .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 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 static final String GUAVA_COMPATIBLE_PATTERN = \"with-only-%s-placeholder\";", + " private static final String GUAVA_INCOMPATIBLE_PATTERN = \"with-%%-marker\";", + "", + " void m() {", + " requireNonNull(\"never-null\", () -> String.format(\"Format string: %s\", 0));", + "", + " checkArgument(true, \"Vacuous format string %\");", + " checkNotNull(\"never-null\", \"Format string: %s %s%\", 1, 2);", + " checkState(false, \"Format string with locale: %s\", 3);", + " verify(true, GUAVA_COMPATIBLE_PATTERN, 4);", + " verifyNotNull(\"never-null\", GUAVA_COMPATIBLE_PATTERN, 5);", + " checkArgument(false, \"with-%-marker\");", + " checkNotNull(\"never-null\", \"with-%-marker\");", + "", + " LOG.trace(\"Vacuous format string %\");", + " LOG.debug(\"With format string: {}, {}%\", 6, 7);", + " LOG.info(\"With vacuous localized format string %\");", + " LOG.warn((Marker) null, \"With marker and format string: {}\", 8);", + " LOG.error(\"With throwable and format string: {}, {}\", 9, 10, new RuntimeException());", + " LOG.trace(", + " (Marker) null, \"With marker, throwable and format string: {}\", 11, new RuntimeException());", + " LOG.debug(\"with-only-{}-placeholder\", 12);", + " LOG.info(\"with-only-{}-placeholder\", 13);", + " LOG.warn(\"with-%-marker\");", + " LOG.error(\"with-%-marker\");", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java index 741a50a644..5266385a2f 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java @@ -63,9 +63,11 @@ public static String treeToString(Tree tree, VisitorState state) { * found. * @return A non-{@code null} string. * @apiNote This method differs from {@link com.sun.tools.javac.util.Constants#format(Object)} in - * that it does not superfluously escape single quote characters. It is different from {@link + * that it does not superfluously escape single quote characters (the latter only does the + * "clean thing" starting from JDK 23). It is different from {@link * VisitorState#getConstantExpression(Object)} in that it is more performant and accepts any * {@link CharSequence} instance. + * @see JDK-8325078 */ // XXX: Drop this method if https://github.com/google/error-prone/pull/4586 is merged and released // with the proposed `CharSequence` compatibility change. diff --git a/integration-tests/checkstyle-expected-changes.patch b/integration-tests/checkstyle-expected-changes.patch index a7b24179e6..d3519b02de 100644 --- a/integration-tests/checkstyle-expected-changes.patch +++ b/integration-tests/checkstyle-expected-changes.patch @@ -15377,7 +15377,7 @@ - if (name == null) { - throw new IllegalArgumentException(String.format(Locale.ROOT, TOKEN_ID_EXCEPTION_FORMAT, id)); - } -+ checkArgument(name != null, String.format(Locale.ROOT, TOKEN_ID_EXCEPTION_FORMAT, id)); ++ checkArgument(name != null, TOKEN_ID_EXCEPTION_FORMAT, id); return name; } @@ -15389,11 +15389,11 @@ - throw new IllegalArgumentException( - String.format(Locale.ROOT, TOKEN_NAME_EXCEPTION_FORMAT, name)); - } -+ checkArgument(id != null, String.format(Locale.ROOT, TOKEN_NAME_EXCEPTION_FORMAT, name)); ++ checkArgument(id != null, TOKEN_NAME_EXCEPTION_FORMAT, name); return id; } -@@ -165,10 +163,9 @@ public final class TokenUtil { +@@ -165,10 +163,7 @@ public final class TokenUtil { * @throws IllegalArgumentException when name is unknown */ public static String getShortDescription(String name) { @@ -15401,13 +15401,11 @@ - throw new IllegalArgumentException( - String.format(Locale.ROOT, TOKEN_NAME_EXCEPTION_FORMAT, name)); - } -+ checkArgument( -+ TOKEN_NAME_TO_VALUE.containsKey(name), -+ String.format(Locale.ROOT, TOKEN_NAME_EXCEPTION_FORMAT, name)); ++ checkArgument(TOKEN_NAME_TO_VALUE.containsKey(name), TOKEN_NAME_EXCEPTION_FORMAT, name); final String tokenTypes = "com.puppycrawl.tools.checkstyle.api.tokentypes"; final ResourceBundle bundle = ResourceBundle.getBundle(tokenTypes, Locale.ROOT); -@@ -344,7 +341,7 @@ public final class TokenUtil { +@@ -344,7 +339,7 @@ public final class TokenUtil { public static BitSet asBitSet(String... tokens) { return Arrays.stream(tokens) .map(String::trim) diff --git a/integration-tests/checkstyle-expected-warnings.txt b/integration-tests/checkstyle-expected-warnings.txt index 1b8b8bd8d6..e78365a8af 100644 --- a/integration-tests/checkstyle-expected-warnings.txt +++ b/integration-tests/checkstyle-expected-warnings.txt @@ -1,3 +1,5 @@ +src/it/java/com/google/checkstyle/test/base/AbstractIndentationTestSupport.java:[100,21] [EagerStringFormatting] String formatting can be deferred (assuming that Guava's simplified formatting support suffices) +src/it/java/com/google/checkstyle/test/base/AbstractIndentationTestSupport.java:[85,21] [EagerStringFormatting] String formatting can be deferred (assuming that Guava's simplified formatting support suffices) src/it/java/com/google/checkstyle/test/chapter7javadoc/rule734nonrequiredjavadoc/NonRequiredJavadocTest.java:[33,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) src/it/java/com/sun/checkstyle/test/chapter5comments/rule52documentationcomments/InvalidJavadocPositionTest.java:[35,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionAbbreviationAsWordInNameTest.java:[116,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `enum` is not a valid identifier) @@ -105,6 +107,8 @@ src/test/java/com/puppycrawl/tools/checkstyle/checks/design/InterfaceIsTypeCheck src/test/java/com/puppycrawl/tools/checkstyle/checks/design/MutableExceptionCheckTest.java:[54,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportControlCheckTest.java:[100,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `null` is not a valid identifier) src/test/java/com/puppycrawl/tools/checkstyle/checks/imports/ImportOrderCheckTest.java:[81,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) +src/test/java/com/puppycrawl/tools/checkstyle/checks/indentation/IndentationCheckTest.java:[69,21] [EagerStringFormatting] String formatting can be deferred (assuming that Guava's simplified formatting support suffices) +src/test/java/com/puppycrawl/tools/checkstyle/checks/indentation/IndentationCheckTest.java:[80,21] [EagerStringFormatting] String formatting can be deferred (assuming that Guava's simplified formatting support suffices) src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/InvalidJavadocPositionCheckTest.java:[59,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocContentLocationCheckTest.java:[57,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `default` is not a valid identifier) src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocContentLocationCheckTest.java:[75,8] [JUnitMethodDeclaration] This method's name should not redundantly start with `test` (but note that `package` is not a valid identifier) diff --git a/integration-tests/prometheus-java-client-expected-changes.patch b/integration-tests/prometheus-java-client-expected-changes.patch index 8c83b749c3..131730b1b4 100644 --- a/integration-tests/prometheus-java-client-expected-changes.patch +++ b/integration-tests/prometheus-java-client-expected-changes.patch @@ -5921,7 +5921,7 @@ import static io.prometheus.metrics.instrumentation.dropwizard5.labels.MapperConfig.METRIC_GLOB_REGEX; import java.util.HashMap; -@@ -32,10 +33,9 @@ class GraphiteNamePattern { +@@ -32,10 +33,11 @@ class GraphiteNamePattern { * @param pattern The glob style pattern to be used. */ GraphiteNamePattern(final String pattern) throws IllegalArgumentException { @@ -5931,11 +5931,13 @@ - } + checkArgument( + VALIDATION_PATTERN.matcher(pattern).matches(), -+ String.format("Provided pattern [%s] does not matches [%s]", pattern, METRIC_GLOB_REGEX)); ++ "Provided pattern [%s] does not matches [%s]", ++ pattern, ++ METRIC_GLOB_REGEX); initializePattern(pattern); } -@@ -84,7 +84,7 @@ class GraphiteNamePattern { +@@ -84,7 +86,7 @@ class GraphiteNamePattern { escapedPattern.append("([^.]*)").append(quoted); } @@ -5954,7 +5956,7 @@ import java.util.HashMap; import java.util.Map; import java.util.regex.Pattern; -@@ -106,21 +108,19 @@ public final class MapperConfig { +@@ -106,21 +108,21 @@ public final class MapperConfig { } private void validateMatch(final String match) { @@ -5966,9 +5968,9 @@ - } + checkArgument( + MATCH_EXPRESSION_PATTERN.matcher(match).matches(), -+ String.format( -+ "Match expression [%s] does not match required pattern %s", -+ match, MATCH_EXPRESSION_PATTERN)); ++ "Match expression [%s] does not match required pattern %s", ++ match, ++ MATCH_EXPRESSION_PATTERN); } private void validateLabels(final Map labels) { @@ -5980,11 +5982,13 @@ - } + checkArgument( + LABEL_PATTERN.matcher(key).matches(), -+ String.format("Label [%s] does not match required pattern %s", match, LABEL_PATTERN)); ++ "Label [%s] does not match required pattern %s", ++ match, ++ LABEL_PATTERN); } } } -@@ -149,7 +149,6 @@ public final class MapperConfig { +@@ -149,7 +151,6 @@ public final class MapperConfig { public int hashCode() { int result = match != null ? match.hashCode() : 0; result = 31 * result + (name != null ? name.hashCode() : 0);