diff --git a/error-prone-contrib/src/main/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheck.java b/error-prone-contrib/src/main/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheck.java new file mode 100644 index 00000000..9a93fdc5 --- /dev/null +++ b/error-prone-contrib/src/main/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheck.java @@ -0,0 +1,135 @@ +package com.picnicinternational.errorprone.bugpatterns; + +import static com.google.common.base.Verify.verify; +import static com.google.errorprone.matchers.Matchers.isSameType; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; + +import com.google.auto.service.AutoService; +import com.google.common.base.Splitter; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.LinkType; +import com.google.errorprone.BugPattern.ProvidesFix; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.BugPattern.StandardTags; +import com.google.errorprone.VisitorState; +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.Kind; +import java.util.List; +import java.util.Optional; +import java.util.regex.Pattern; + +// XXX: Also simplify `LOG.error(String.format("Something %s", arg), throwable)`. +// XXX: Write a similar checker for Spring RestTemplates, String.format and friends, Guava +// preconditions, ... +@AutoService(BugChecker.class) +@BugPattern( + name = "Slf4jLogStatement", + summary = "Make sure SLF4J log statements contain proper placeholders with matching arguments", + linkType = LinkType.NONE, + severity = SeverityLevel.WARNING, + tags = StandardTags.LIKELY_ERROR, + providesFix = ProvidesFix.REQUIRES_HUMAN_ATTENTION +) +public final class Slf4jLogStatementCheck extends BugChecker + implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher MARKER = isSubtypeOf("org.slf4j.Marker"); + private static final Matcher STRING = isSameType(String.class); + private static final Matcher THROWABLE = isSubtypeOf(Throwable.class); + private static final Matcher SLF4J_LOGGER_INVOCATION = + instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .withNameMatching(Pattern.compile("trace|debug|info|warn|error")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!SLF4J_LOGGER_INVOCATION.matches(tree, state)) { + return Description.NO_MATCH; + } + + List args = getTrimmedArguments(tree, state); + Optional formatString = getFormatString(args, state); + if (!formatString.isPresent()) { + return Description.NO_MATCH; + } + + Description.Builder description = buildDescription(tree); + return validateFormatString(formatString.get(), args.get(0), state, description) + && validateArguments(formatString.get(), args.subList(1, args.size()), description) + ? Description.NO_MATCH + : description.build(); + } + + private static List getTrimmedArguments( + MethodInvocationTree tree, VisitorState state) { + List args = tree.getArguments(); + verify(!args.isEmpty(), "Unexpected invocation of nullary SLF4J log method"); + /* + * SLF4J log statements may accept a "marker" as a first argument, before the format string. + * We ignore such markers. + */ + int lTrim = MARKER.matches(args.get(0), state) ? 1 : 0; + /* + * SLF4J treats the final argument to a log statement specially if it is a `Throwabe`: it + * will always choose to render the associated stacktrace, even if the argument has a + * matching `{}` placeholder. (In this case the `{}` will simply be logged verbatim.) So for + * the purpose of matching arguments against format string placeholders a trailing + * `Throwable` effectively doesn't exist. + */ + int rTrim = THROWABLE.matches(args.get(args.size() - 1), state) ? 1 : 0; + return args.subList(lTrim, args.size() - rTrim); + } + + private static Optional getFormatString( + List args, VisitorState state) { + verify(!args.isEmpty(), "Failed to identify SLF4J log method format string"); + return Optional.ofNullable(ASTHelpers.constValue(args.get(0), String.class)); + } + + private static boolean validateFormatString( + String formatString, + ExpressionTree tree, + VisitorState state, + Description.Builder description) { + String fixed = formatString.replace("%s", "{}"); + if (fixed.equals(formatString)) { + return true; + } + + description.setMessage("SLF4J log statement placeholders are of the form `{}`, not `%s`"); + if (tree.getKind() == Kind.STRING_LITERAL) { + /* + * We only suggest string replacement if the format string is a literal argument, cause + * if the format string is a string constant defined elsewhere then it is not clear + * whether the constant's definition must be updated or whether the constant should be + * replaced at this usage site. + */ + description.addFix( + SuggestedFix.replace(tree, Util.treeToString(tree, state).replace("%s", "{}"))); + } + + return false; + } + + private static boolean validateArguments( + String formatString, List args, Description.Builder description) { + int placeholders = Splitter.on("{}").splitToList(formatString).size() - 1; + if (placeholders == args.size()) { + return true; + } + + description.setMessage( + String.format( + "Log statement contains %s placeholders, but specifies %s matching argument(s)", + placeholders, args.size())); + return false; + } +} diff --git a/error-prone-contrib/src/test/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheckTest.java b/error-prone-contrib/src/test/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheckTest.java new file mode 100644 index 00000000..5edae2f4 --- /dev/null +++ b/error-prone-contrib/src/test/java/com/picnicinternational/errorprone/bugpatterns/Slf4jLogStatementCheckTest.java @@ -0,0 +1,149 @@ +package com.picnicinternational.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import java.io.IOException; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class Slf4jLogStatementCheckTest { + private final CompilationTestHelper compilationTestHelper = + CompilationTestHelper.newInstance(Slf4jLogStatementCheck.class, getClass()); + private final BugCheckerRefactoringTestHelper refactoringTestHelper = + BugCheckerRefactoringTestHelper.newInstance(new Slf4jLogStatementCheck(), getClass()); + + @Test + public void testIdentification() { + compilationTestHelper + .addSourceLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "import org.slf4j.MarkerFactory;", + "", + "class A {", + " private static final String FMT0 = \"format-string-without-placeholders\";", + " private static final String FMT1 = \"format-string-with-{}-placeholder\";", + " private static final String FMT2 = \"format-string-with-{}-{}-placeholders\";", + " private static final String FMT_ERR = \"format-string-with-%s-placeholder\";", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private final Marker marker = MarkerFactory.getMarker(A.class.getName());", + " private final Object o = new Object();", + " private final String s = o.toString();", + " private final Throwable t = new Throwable();", + "", + " void m() {", + " LOG.trace(s);", + " LOG.debug(s, o);", + " LOG.info(s, t);", + " LOG.warn(s, o, t);", + " LOG.error(marker, s);", + " LOG.trace(marker, s, o);", + " LOG.debug(marker, s, t);", + " LOG.info(marker, s, o, t);", + "", + " LOG.warn(FMT0);", + " // BUG: Diagnostic contains: Log statement contains 0 placeholders, but specifies 1 matching argument(s)", + " LOG.error(FMT0, o);", + " LOG.trace(FMT0, t);", + " // BUG: Diagnostic contains:", + " LOG.debug(FMT0, o, t);", + " LOG.info(marker, FMT0);", + " // BUG: Diagnostic contains:", + " LOG.warn(marker, FMT0, o);", + " LOG.error(marker, FMT0, t);", + " // BUG: Diagnostic contains:", + " LOG.trace(marker, FMT0, o, t);", + "", + " // BUG: Diagnostic contains: Log statement contains 1 placeholders, but specifies 0 matching argument(s)", + " LOG.debug(FMT1);", + " LOG.info(FMT1, o);", + " // BUG: Diagnostic contains:", + " LOG.warn(FMT1, t);", + " LOG.error(FMT1, o, t);", + " // BUG: Diagnostic contains: Log statement contains 1 placeholders, but specifies 2 matching argument(s)", + " LOG.trace(FMT1, o, o);", + " // BUG: Diagnostic contains:", + " LOG.debug(FMT1, o, o, t);", + " // BUG: Diagnostic contains:", + " LOG.info(marker, FMT1);", + " LOG.warn(marker, FMT1, o);", + " // BUG: Diagnostic contains:", + " LOG.error(marker, FMT1, t);", + " LOG.trace(marker, FMT1, o, t);", + " // BUG: Diagnostic contains:", + " LOG.debug(marker, FMT1, o, o);", + " // BUG: Diagnostic contains:", + " LOG.info(marker, FMT1, o, o, t);", + "", + " // BUG: Diagnostic contains: SLF4J log statement placeholders are of the form `{}`, not `%s`", + " LOG.warn(FMT_ERR);", + " // BUG: Diagnostic contains:", + " LOG.error(FMT_ERR, t);", + " // BUG: Diagnostic contains:", + " LOG.trace(FMT_ERR, o);", + " // BUG: Diagnostic contains:", + " LOG.debug(FMT_ERR, o, t);", + " }", + "}") + .doTest(); + } + + // XXX: Drop what's unused. + @Test + public void testReplacement() throws IOException { + refactoringTestHelper + .addInputLines( + "in/A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "import org.slf4j.MarkerFactory;", + "", + "class A {", + " private static final String FMT_ERR = \"format-string-with-%s-placeholder\";", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private final Marker marker = MarkerFactory.getMarker(A.class.getName());", + " private final Object o = new Object();", + " private final String s = o.toString();", + " private final Throwable t = new Throwable();", + "", + " void m() {", + " LOG.error(FMT_ERR, o);", + " LOG.error(\"format-string-with-'%s'-placeholder\", o);", + " LOG.error(\"format-string-with-\\\"%s\\\"-placeholder\", o);", + " LOG.error(\"format-string-with-%s\" + \"-placeholder\", o);", + " }", + "}") + .addOutputLines( + "out/A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "import org.slf4j.Marker;", + "import org.slf4j.MarkerFactory;", + "", + "class A {", + " private static final String FMT_ERR = \"format-string-with-%s-placeholder\";", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private final Marker marker = MarkerFactory.getMarker(A.class.getName());", + " private final Object o = new Object();", + " private final String s = o.toString();", + " private final Throwable t = new Throwable();", + "", + " void m() {", + " LOG.error(FMT_ERR, o);", + " LOG.error(\"format-string-with-'{}'-placeholder\", o);", + " LOG.error(\"format-string-with-\\\"{}\\\"-placeholder\", o);", + " LOG.error(\"format-string-with-{}\" + \"-placeholder\", o);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +}