Fix PreferSafeLogger edge case that produced non-compiling code (#1851)

Fix PreferSafeLogger edge case that produced suggested fixes that didn't compile without human interaction.
palantir · Jul 23, 2021 · 2ea1274 · 2ea1274
1 parent e78e392
commit 2ea1274
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 0 deletions.
diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLogger.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/PreferSafeLogger.java
@@ -102,6 +102,9 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void _unused) {
                     if (sym.equals(ASTHelpers.getSymbol(receiver))) {
                         if (!isSafeSlf4jInteraction(tree, state)) {
                             foundUnknownUsage.set(true);
+                        } else {
+                            // Scan arguments for findings that may not compile
+                            scan(tree.getArguments(), null);
                         }
                         return null;
                     }

diff --git a/...line-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java b/...line-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java
@@ -210,4 +210,48 @@ void testPassedLoggerUnmodified() {
                 .expectUnchanged()
                 .doTest();
     }
+
+    @Test
+    void testGetNameInArgUnmodified() {
+        // getName is not supported by SafeLogger, so we shouldn't make changes that won't compile.
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import org.slf4j.*;",
+                        "class Test {",
+                        "  private static final Logger log = LoggerFactory.getLogger(Test.class);",
+                        "  void action() {",
+                        "    log.info(\"foo\", SafeArg.of(\"name\", log.getName()));",
+                        "  }",
+                        "}")
+                .expectUnchanged()
+                .doTest();
+    }
+
+    @Test
+    void testIsTraceEnabledInArg() {
+        fix().addInputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import org.slf4j.*;",
+                        "class Test {",
+                        "  private static final Logger log = LoggerFactory.getLogger(Test.class);",
+                        "  void action() {",
+                        "    log.info(\"foo\", SafeArg.of(\"name\", log.isTraceEnabled()));",
+                        "  }",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.logsafe.*;",
+                        "import com.palantir.logsafe.logger.SafeLogger;",
+                        "import com.palantir.logsafe.logger.SafeLoggerFactory;",
+                        "import org.slf4j.*;",
+                        "class Test {",
+                        "  private static final SafeLogger log = SafeLoggerFactory.get(Test.class);",
+                        "  void action() {",
+                        "    log.info(\"foo\", SafeArg.of(\"name\", log.isTraceEnabled()));",
+                        "  }",
+                        "}")
+                .doTest();
+    }
 }
diff --git a/changelog/@unreleased/pr-1851.v2.yml b/changelog/@unreleased/pr-1851.v2.yml
@@ -0,0 +1,6 @@
+type: fix
+fix:
+  description: Fix PreferSafeLogger edge case that produced suggested fixes that didn't
+    compile without human interaction.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1851