Update OrphanedFormatString to warn on log("hello %s")

and remove the corresponding error from `FloggerFormatString`. PiperOrigin-RevId: 423950862
google · Jan 25, 2022 · fbaa55b · fbaa55b
1 parent e09ca6f
commit fbaa55b
Show file tree

Hide file tree

Showing 5 changed files with 52 additions and 30 deletions.
diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/OrphanedFormatString.java b/core/src/main/java/com/google/errorprone/bugpatterns/OrphanedFormatString.java
@@ -81,6 +81,10 @@ public class OrphanedFormatString extends BugChecker implements LiteralTreeMatch
                   .onClass("com.google.common.truth.Truth")
                   .named("assertWithMessage")
                   .withParameters("java.lang.String"),
+              instanceMethod()
+                  .onDescendantOf("com.google.common.flogger.LoggingApi")
+                  .named("log")
+                  .withParameters("java.lang.String"),
               (t, s) ->
                   t instanceof MethodInvocationTree
                       && !findMatchingMethods(

diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerFormatString.java b/core/src/main/java/com/google/errorprone/bugpatterns/flogger/FloggerFormatString.java
@@ -22,7 +22,6 @@
 import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod;
 
 import com.google.errorprone.BugPattern;
-import com.google.errorprone.ErrorProneFlags;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
@@ -55,21 +54,15 @@ public class FloggerFormatString extends BugChecker implements MethodInvocationT
   private static final Matcher<ExpressionTree> WITH_CAUSE =
       instanceMethod().onDescendantOf("com.google.common.flogger.LoggingApi").named("withCause");
 
-  private final int minArgs;
-
-  public FloggerFormatString(ErrorProneFlags flags) {
-    // TODO(b/183117069): Remove once FloggerLogString is fully enabled
-    this.minArgs = flags.getBoolean("FloggerLogStringDisabled").orElse(false) ? 1 : 2;
-  }
-
   @Override
   public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
     if (!FORMAT_METHOD.matches(tree, state)) {
       return NO_MATCH;
     }
     List<? extends ExpressionTree> args = tree.getArguments();
-    if (args.size() < minArgs) {
-      // log(String)'s argument isn't a format string and is checked by FloggerLogString instead.
+    if (args.size() <= 1) {
+      // log(String)'s argument isn't a format string, see FloggerLogString
+      // we also warn on mistakes like `log("hello %s")` in OrphanedFormatString
       return NO_MATCH;
     }
     MethodSymbol sym = ASTHelpers.getSymbol(tree);

diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/OrphanedFormatStringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/OrphanedFormatStringTest.java
@@ -134,4 +134,35 @@ public void assertWithMessage() {
             "}")
         .doTest();
   }
+
+  @Test
+  public void flogger() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.flogger.FluentLogger;",
+            "class Test {",
+            "  private static final FluentLogger logger = FluentLogger.forEnclosingClass();",
+            "  public void f() {",
+            "    // BUG: Diagnostic contains:",
+            "    logger.atInfo().log(\"hello %d\");",
+            "  }",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  public void negativeFlogger() {
+    testHelper
+        .addSourceLines(
+            "Test.java",
+            "import com.google.common.flogger.FluentLogger;",
+            "class Test {",
+            "  private static final FluentLogger logger = FluentLogger.forEnclosingClass();",
+            "  public void f(String arg) {",
+            "    logger.atInfo().log(\"hello %d\", arg);",
+            "  }",
+            "}")
+        .doTest();
+  }
 }
diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerFormatStringTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/flogger/FloggerFormatStringTest.java
@@ -111,27 +111,9 @@ public void varargs() {
         .doTest();
   }
 
+  // log(String)  takes a literal string, not a format string
   @Test
-  public void checksLogStringIfFloggerLogStringIsDisabled() {
-    // TODO(b/183117069): Remove once FloggerLogString is fully enabled
-    compilationHelper
-        .setArgs("-XepOpt:FloggerLogStringDisabled=true")
-        .addSourceLines(
-            "Test.java",
-            "package test;",
-            "import com.google.common.flogger.FluentLogger;",
-            "class Test {",
-            "  private static final FluentLogger logger = FluentLogger.forEnclosingClass();",
-            "  public void f() {",
-            "    // BUG: Diagnostic contains:",
-            "    logger.atSevere().log(\"hello %s\");",
-            "  }",
-            "}")
-        .doTest();
-  }
-
-  @Test
-  public void doesNotCheckLogStringIfFloggerLogStringIsEnabled() {
+  public void negativeLogString() {
     compilationHelper
         .addSourceLines(
             "Test.java",

diff --git a/docs/bugpattern/OrphanedFormatString.md b/docs/bugpattern/OrphanedFormatString.md
@@ -9,6 +9,12 @@ if (!isValid(arg)) {
 }
 ```
 
+or this:
+
+```java
+logger.atWarning().log("invalid arg: %s", arg);
+```
+
 Not this:
 
 ```java
@@ -17,6 +23,12 @@ if (!isValid(arg)) {
 }
 ```
 
+or this:
+
+```java
+logger.atWarning().log("invalid arg: %s");
+```
+
 If the method you're calling actually accepts a format string, you can annotate
 that method with [`@FormatMethod`][fm] to ensure that callers correctly pass
 format strings (and to inform Error Prone that the method call you're making