From fbaa55b54e79ef05ca7f69a4e5ff3f85d7ce897e Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Mon, 24 Jan 2022 17:26:13 -0800 Subject: [PATCH] Update `OrphanedFormatString` to warn on `log("hello %s")` and remove the corresponding error from `FloggerFormatString`. PiperOrigin-RevId: 423950862 --- .../bugpatterns/OrphanedFormatString.java | 4 +++ .../flogger/FloggerFormatString.java | 13 ++------ .../bugpatterns/OrphanedFormatStringTest.java | 31 +++++++++++++++++++ .../flogger/FloggerFormatStringTest.java | 22 ++----------- docs/bugpattern/OrphanedFormatString.md | 12 +++++++ 5 files changed, 52 insertions(+), 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 index dc75f9ced4a..8f255bfd1b6 100644 --- 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 index 8aaf45055f1..a99a845c4e9 100644 --- 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 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 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 index 411eef33ad4..361ff11c185 100644 --- 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 index 08a7979a439..c431f76c6aa 100644 --- 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 index 08c54c870d9..0c0ffac1c4e 100644 --- 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