diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java index 6ac8a9d0d..40aaf69de 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormat.java @@ -35,7 +35,7 @@ link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = LinkType.CUSTOM, severity = SeverityLevel.ERROR, - summary = "logsafe Preconditions.checkX() methods should not have print-f style formatting.") + summary = "logsafe Preconditions.checkX() methods should not have print-f or slf4j style formatting.") public final class LogSafePreconditionsMessageFormat extends PreconditionsMessageFormat { private static final long serialVersionUID = 1L; @@ -50,11 +50,20 @@ public LogSafePreconditionsMessageFormat() { @Override protected Description matchMessageFormat(MethodInvocationTree tree, String message, VisitorState state) { - if (!message.contains("%s")) { - return Description.NO_MATCH; + if (message.contains("%s")) { + return buildDescription(tree) + .setMessage("Do not use printf-style formatting in logsafe Preconditions. " + + "Logsafe exceptions provide a simple message and key-value pairs of arguments, " + + "no interpolation is performed.") + .build(); } - - return buildDescription(tree).setMessage( - "Do not use printf-style formatting in logsafe Preconditions.").build(); + if (message.contains("{}")) { + return buildDescription(tree) + .setMessage("Do not use slf4j-style formatting in logsafe Preconditions. " + + "Logsafe exceptions provide a simple message and key-value pairs of arguments, " + + "no interpolation is performed.") + .build(); + } + return Description.NO_MATCH; } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java index 5b33d1fe2..f461d628b 100755 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LogSafePreconditionsMessageFormatTests.java @@ -22,7 +22,8 @@ public final class LogSafePreconditionsMessageFormatTests extends PreconditionsTests { - private static final String DIAGNOSTIC = "Do not use printf-style formatting"; + private static final String PRINTF_DIAGNOSTIC = "Do not use printf-style formatting"; + private static final String SLF4J_DIAGNOSTIC = "Do not use slf4j-style formatting"; private CompilationTestHelper compilationHelper; @@ -37,38 +38,74 @@ public CompilationTestHelper compilationHelper() { } @Test - public void testCheckArgument() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s\"," + public void testCheckArgument_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s\"," + " UnsafeArg.of(\"long\", 123L));"); } @Test - public void testCheckArgument_multipleArgs() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s %s\"," + public void testCheckArgument_multipleArgs_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message %s %s\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); } @Test - public void testCheckState() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s\"," + public void testCheckState_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s\"," + " UnsafeArg.of(\"long\", 123L));"); } @Test - public void testCheckState_multipleArgs() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s %s\"," + public void testCheckState_multipleArgs_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message %s %s\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); } @Test - public void testCheckNotNull() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s\"," + public void testCheckNotNull_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s\"," + " UnsafeArg.of(\"long\", 123L));"); } @Test - public void testCheckNotNull_multipleArgs() { - failLogSafe(DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s %s\"," + public void testCheckNotNull_multipleArgs_printf() { + failLogSafe(PRINTF_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message %s %s\"," + + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); + } + + @Test + public void testCheckArgument_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {}\"," + + " UnsafeArg.of(\"long\", 123L));"); + } + + @Test + public void testCheckArgument_multipleArgs_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkArgument(param != \"string\", \"message {} {}\"," + + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); + } + + @Test + public void testCheckState_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {}\"," + + " UnsafeArg.of(\"long\", 123L));"); + } + + @Test + public void testCheckState_multipleArgs_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkState(param != \"string\", \"message {} {}\"," + + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); + } + + @Test + public void testCheckNotNull_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {}\"," + + " UnsafeArg.of(\"long\", 123L));"); + } + + @Test + public void testCheckNotNull_multipleArgs_slf4j() { + failLogSafe(SLF4J_DIAGNOSTIC, "Preconditions.checkNotNull(param, \"message {} {}\"," + " UnsafeArg.of(\"char1\", 'a'), UnsafeArg.of(\"char2\", 'b'));"); } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java index 4a62aeb55..1ec39e8b5 100755 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreconditionsTests.java @@ -156,137 +156,18 @@ public final void validLogSafe() throws Exception { " void f(boolean bArg, int iArg, Object oArg) {", " Preconditions.checkArgument(bArg);", " Preconditions.checkArgument(bArg, \"message\");", - " Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"int\", 123));", - " Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkArgument(bArg, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char1\", 'a')," + " Preconditions.checkArgument(bArg, \"message {char}\", UnsafeArg.of(\"char\", 'a'));", + " Preconditions.checkArgument(bArg, \"message {char1 {char2}\", UnsafeArg.of(\"char1\", 'a')," + " UnsafeArg.of(\"char2\", 'b'));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkArgument(bArg, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\")," - + " UnsafeArg.of(\"string2\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {} {}\"," - + " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\")," - + " UnsafeArg.of(\"string3\", \"msg\"));", - " Preconditions.checkArgument(bArg, \"message {} {} {} {}\"," - + " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\")," - + " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));", "", " Preconditions.checkState(iArg > 0);", " Preconditions.checkState(iArg > 0, \"message\");", - " Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"int\", 123));", - " Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkState(iArg > 0, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char1\", 'a')," + " Preconditions.checkState(iArg > 0, \"message {char}\", UnsafeArg.of(\"char\", 'a'));", + " Preconditions.checkState(iArg > 0, \"message {char1} {char2}\", UnsafeArg.of(\"char1\", 'a')," + " UnsafeArg.of(\"char2\", 'b'));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkState(iArg > 0, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\")," - + " UnsafeArg.of(\"string2\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {} {}\"," - + " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\")," - + " UnsafeArg.of(\"string3\", \"msg\"));", - " Preconditions.checkState(iArg > 0, \"message {} {} {} {}\"," - + " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\")," - + " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));", "", " Preconditions.checkNotNull(oArg);", " Preconditions.checkNotNull(oArg, \"message\");", - " Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"int\", 123));", - " Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkNotNull(oArg, \"message {}\", UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char1\", 'a')," - + " UnsafeArg.of(\"char2\", 'b'));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"char\", 'a')," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"int\", 123)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"long\", 123L)," - + " UnsafeArg.of(\"string\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"char\", 'a'));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"int\", 123));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string\", \"msg\")," - + " UnsafeArg.of(\"long\", 123L));", - " Preconditions.checkNotNull(oArg, \"message {} {}\", UnsafeArg.of(\"string1\", \"msg\")," - + " UnsafeArg.of(\"string2\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {} {}\", UnsafeArg.of(\"string1\", \"msg\")," - + " UnsafeArg.of(\"string2\", \"msg\"), UnsafeArg.of(\"string3\", \"msg\"));", - " Preconditions.checkNotNull(oArg, \"message {} {} {} {}\"," - + " UnsafeArg.of(\"string1\", \"msg\"), UnsafeArg.of(\"string2\", \"msg\")," - + " UnsafeArg.of(\"string3\", \"msg\"), UnsafeArg.of(\"string4\", \"msg\"));", " }", "}") .doTest(); diff --git a/changelog/@unreleased/pr-761.v2.yml b/changelog/@unreleased/pr-761.v2.yml new file mode 100644 index 000000000..67d7e63ed --- /dev/null +++ b/changelog/@unreleased/pr-761.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: LogSafePreconditionsMessageFormat disallows slf4j-style format characters + links: + - https://github.com/palantir/gradle-baseline/pull/761