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 index 0d61d36674..29341f124a 100644 --- 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 @@ -8,6 +8,7 @@ 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; @@ -65,7 +66,7 @@ public final class EagerStringFormatting extends BugChecker implements MethodInv 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 = + private static final Matcher REQUIRE_NON_NULL_INVOCATION = staticMethod().onClass(Objects.class.getCanonicalName()).named("requireNonNull"); private static final Matcher GUAVA_GUARD_INVOCATION = anyOf( @@ -110,68 +111,16 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState 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 || 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(), "() -> ")); + if (REQUIRE_NON_NULL_INVOCATION.matches(context, state)) { + return analyzeRequireNonNullStringFormatContext(stringFormat, context); } if (GUAVA_GUARD_INVOCATION.matches(context, state)) { - 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)); + return analyzeGuavaGuardStringFormatContext(stringFormat, context, state); } if (SLF4J_LOGGER_INVOCATION.matches(context, state)) { - if (stringFormat.simplifiableFormatString().isEmpty()) { - /* We can't simplify this case; only flag it. */ - return createSimplificationSuggestion(context, "SLF4J"); - } - - 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)); + return analyzeSlf4jLoggerStringFormatContext(stringFormat, context, state); } /* @@ -181,6 +130,73 @@ private Description analyzeFormatStringContext( 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 @@ -235,7 +251,7 @@ private static Optional tryCreate( return Optional.of( create( tree, - ASTHelpers.getReceiver(tree), + requireNonNull(ASTHelpers.getReceiver(tree), "Receiver unexpectedly absent"), ImmutableList.copyOf(tree.getArguments()), state)); }