Added support for logsafe Arg arrays in Slf4jLogsafeArgs. (#1394)

Slf4jLogsafeArgs now allows Arg<?>[] to be passed as vararg argument to logging methods.

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<ExpressionTree> THROWABLE = MoreMatchers.isSubtypeOf(Throwable.class);
     private static final Matcher<ExpressionTree> ARG = MoreMatchers.isSubtypeOf("com.palantir.logsafe.Arg");
     private static final Matcher<ExpressionTree> MARKER = MoreMatchers.isSubtypeOf("org.slf4j.Marker");
+    private static final Matcher<Tree> 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<? extends ExpressionTree> 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<Integer> 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<Integer> getBadArgIndices(VisitorState state, List<? extends ExpressionTree> args, int from, int to) {
         ImmutableList.Builder<Integer> 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<Integer> 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<Description> checkThrowableArgumentNotWrapped(MethodInvocationTree tree, VisitorState state) {

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<Marker> 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,19 +153,35 @@ public void testPassingLogsafeArgs() throws Exception {
                         "        UnsafeArg.of(\"name2\", \"string2\"),",
                         "        new Throwable());",
                         "",
+                        "    // log.<>(String, Arg<T>[])",
+                        "    log." + logLevel + "(\"constant {}\",",
+                        "                         // BUG: Diagnostic contains: varargs method with inexact argument",
+                        "                         new Arg<?>[] { SafeArg.of(\"name\", \"string\") });",
+                        "",
+                        "    // log.<>(String, SafeArg<T>[])",
+                        "    log." + logLevel + "(\"constant {}\",",
+                        "                         // BUG: Diagnostic contains: varargs method with inexact argument",
+                        "                         new SafeArg<?>[] { SafeArg.of(\"name\", \"string\") });",
+                        "",
+                        "    // log.<>(String, (Object[]) SafeArg<T>[])",
+                        "    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<Marker> iterator() { return null; }",
                         "    public boolean contains(Marker other) { return false; }",
                         "    public boolean contains(String name) { return false; }",
                         "  }",
                         "}")
+                .matchAllDiagnostics()
                 .doTest());
     }
 

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