From d1daefb535b65c1a6da51169a29b1d0008249e97 Mon Sep 17 00:00:00 2001 From: Andreas Lundblad Date: Wed, 17 Jun 2020 13:17:41 -0700 Subject: [PATCH] Added support for logsafe Arg arrays in Slf4jLogsafeArgs. (#1394) Slf4jLogsafeArgs now allows Arg[] to be passed as vararg argument to logging methods. --- .../baseline/errorprone/Slf4jLogsafeArgs.java | 44 ++++++++++++------- .../errorprone/Slf4jLogsafeArgsTest.java | 18 ++++++++ changelog/@unreleased/pr-1394.v2.yml | 6 +++ 3 files changed, 53 insertions(+), 15 deletions(-) create mode 100644 changelog/@unreleased/pr-1394.v2.yml diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java index ad7b13310..d074d844a 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java @@ -31,7 +31,9 @@ import com.google.errorprone.matchers.method.MethodMatchers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.Tree; import com.sun.source.util.SimpleTreeVisitor; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.regex.Pattern; @@ -43,7 +45,8 @@ linkType = BugPattern.LinkType.CUSTOM, providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, severity = SeverityLevel.WARNING, - summary = "Allow only com.palantir.logsafe.Arg types as parameter inputs to slf4j log messages.") + summary = "Allow only com.palantir.logsafe.Arg types, or vararg arrays, as parameter inputs to slf4j log " + + "messages.") public final class Slf4jLogsafeArgs extends BugChecker implements MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; @@ -55,6 +58,8 @@ public final class Slf4jLogsafeArgs extends BugChecker implements MethodInvocati private static final Matcher THROWABLE = MoreMatchers.isSubtypeOf(Throwable.class); private static final Matcher ARG = MoreMatchers.isSubtypeOf("com.palantir.logsafe.Arg"); private static final Matcher MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker"); + private static final Matcher OBJECT_ARRAY = Matchers.isSubtypeOf( + s -> s.getType(s.getTypeFromString("java.lang.Object"), true, Collections.emptyList())); @Override public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { @@ -68,26 +73,35 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState } List allArgs = tree.getArguments(); - int lastIndex = allArgs.size() - 1; - int startArg = MARKER.matches(allArgs.get(0), state) ? 2 : 1; - ExpressionTree lastArg = allArgs.get(lastIndex); + int startArgIndex = MARKER.matches(allArgs.get(0), state) ? 2 : 1; + int lastArgIndex = allArgs.size() - 1; + ExpressionTree lastArg = allArgs.get(lastArgIndex); + + if (startArgIndex == lastArgIndex && OBJECT_ARRAY.matches(lastArg, state)) { + return Description.NO_MATCH; + } + boolean lastArgIsThrowable = THROWABLE.matches(lastArg, state); - int endArg = lastArgIsThrowable ? lastIndex - 1 : lastIndex; + int lastNonThrowableArgIndex = lastArgIsThrowable ? lastArgIndex - 1 : lastArgIndex; + List badArgs = getBadArgIndices(state, allArgs, startArgIndex, lastNonThrowableArgIndex); + if (badArgs.isEmpty() || TestCheckUtils.isTestCode(state)) { + return Description.NO_MATCH; + } + + return buildDescription(tree) + .setMessage("slf4j log statement does not use logsafe parameters for arguments " + badArgs) + .build(); + } + + private List getBadArgIndices(VisitorState state, List args, int from, int to) { ImmutableList.Builder badArgsBuilder = ImmutableList.builder(); - for (int i = startArg; i <= endArg; i++) { - if (!ARG.matches(allArgs.get(i), state)) { + for (int i = from; i <= to; i++) { + if (!ARG.matches(args.get(i), state)) { badArgsBuilder.add(i); } } - List badArgs = badArgsBuilder.build(); - if (badArgs.isEmpty() || TestCheckUtils.isTestCode(state)) { - return Description.NO_MATCH; - } else { - return buildDescription(tree) - .setMessage("slf4j log statement does not use logsafe parameters for arguments " + badArgs) - .build(); - } + return badArgsBuilder.build(); } private Optional checkThrowableArgumentNotWrapped(MethodInvocationTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java index 07260f3e6..a2fca8b66 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java @@ -47,6 +47,7 @@ private void test(String logArgs, String failingArgs) throws Exception { " public String getName() { return null; }", " public void add(Marker reference) {}", " public boolean remove(Marker reference) { return true; }", + " @SuppressWarnings(\"deprecation\")", " public boolean hasChildren() { return false; }", " public boolean hasReferences() { return false; }", " public Iterator iterator() { return null; }", @@ -89,6 +90,7 @@ public void testPassingLogsafeArgs() throws Exception { LOG_LEVELS.forEach(logLevel -> CompilationTestHelper.newInstance(Slf4jLogsafeArgs.class, getClass()) .addSourceLines( "Test.java", + "import com.palantir.logsafe.Arg;", "import com.palantir.logsafe.SafeArg;", "import com.palantir.logsafe.UnsafeArg;", "import org.slf4j.Logger;", @@ -151,12 +153,27 @@ public void testPassingLogsafeArgs() throws Exception { " UnsafeArg.of(\"name2\", \"string2\"),", " new Throwable());", "", + " // log.<>(String, Arg[])", + " log." + logLevel + "(\"constant {}\",", + " // BUG: Diagnostic contains: varargs method with inexact argument", + " new Arg[] { SafeArg.of(\"name\", \"string\") });", + "", + " // log.<>(String, SafeArg[])", + " log." + logLevel + "(\"constant {}\",", + " // BUG: Diagnostic contains: varargs method with inexact argument", + " new SafeArg[] { SafeArg.of(\"name\", \"string\") });", + "", + " // log.<>(String, (Object[]) SafeArg[])", + " log." + logLevel + "(\"constant {}\",", + " (Object[]) new SafeArg[] { SafeArg.of(\"name\", \"string\") });", + "", " }", "", " class DummyMarker implements Marker {", " public String getName() { return null; }", " public void add(Marker reference) {}", " public boolean remove(Marker reference) { return true; }", + " @SuppressWarnings(\"deprecation\")", " public boolean hasChildren() { return false; }", " public boolean hasReferences() { return false; }", " public Iterator iterator() { return null; }", @@ -164,6 +181,7 @@ public void testPassingLogsafeArgs() throws Exception { " public boolean contains(String name) { return false; }", " }", "}") + .matchAllDiagnostics() .doTest()); } diff --git a/changelog/@unreleased/pr-1394.v2.yml b/changelog/@unreleased/pr-1394.v2.yml new file mode 100644 index 000000000..e0d962159 --- /dev/null +++ b/changelog/@unreleased/pr-1394.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: `Slf4jLogsafeArgs` now allows object arrays be passed as vararg + argument to logging methods. + links: + - https://github.com/palantir/gradle-baseline/pull/1394