From 9081de300339135eb47d6fefb1474800a1caee4f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Tue, 10 Aug 2021 14:15:21 -0400 Subject: [PATCH 1/2] PreferSafeLogger produces suggestions which compile with >10 args --- .../baseline/errorprone/PreferSafeLogger.java | 33 +++++- .../errorprone/PreferSafeLoggerTest.java | 102 ++++++++++++++++++ 2 files changed, 131 insertions(+), 4 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 index 60435a63b..99589beb7 100644 --- 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 @@ -36,6 +36,7 @@ import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; +import java.util.Arrays; import java.util.List; import java.util.concurrent.atomic.AtomicBoolean; @@ -48,6 +49,7 @@ summary = "Prefer using type-safe safe-logging loggers rather than safety-oblivious implementations.") public final class PreferSafeLogger extends BugChecker implements BugChecker.VariableTreeMatcher { + private static final int MAX_SUPPORTED_ARGS = 10; private static final Matcher SLF4J_METHOD = MethodMatchers.instanceMethod().onDescendantOf("org.slf4j.Logger"); @@ -78,6 +80,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } Symbol.VarSymbol sym = ASTHelpers.getSymbol(tree); AtomicBoolean foundUnknownUsage = new AtomicBoolean(); + SuggestedFix.Builder fix = SuggestedFix.builder(); new TreeScanner() { @Override public Void visitIdentifier(IdentifierTree tree, Void _unused) { @@ -100,7 +103,7 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void _unused) { if (SLF4J_METHOD.matches(tree, state)) { ExpressionTree receiver = ASTHelpers.getReceiver(tree); if (sym.equals(ASTHelpers.getSymbol(receiver))) { - if (!isSafeSlf4jInteraction(tree, state)) { + if (!isSafeSlf4jInteraction(tree, fix, state)) { foundUnknownUsage.set(true); } else { // Scan arguments for findings that may not compile @@ -115,7 +118,6 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void _unused) { if (foundUnknownUsage.get()) { return Description.NO_MATCH; } - SuggestedFix.Builder fix = SuggestedFix.builder(); String qualifiedLogger = SuggestedFixes.qualifyType(state, fix, "com.palantir.logsafe.logger.SafeLogger"); String qualifiedFactory = SuggestedFixes.qualifyType(state, fix, "com.palantir.logsafe.logger.SafeLoggerFactory"); @@ -125,7 +127,9 @@ public Void visitMethodInvocation(MethodInvocationTree tree, Void _unused) { return buildDescription(tree).addFix(fix.build()).build(); } - private static boolean isSafeSlf4jInteraction(MethodInvocationTree tree, VisitorState state) { + @SuppressWarnings("checkstyle:CyclomaticComplexity") + private static boolean isSafeSlf4jInteraction( + MethodInvocationTree tree, SuggestedFix.Builder fix, VisitorState state) { if (!SLF4J_SAFE_METHODS.matches(tree, state)) { return false; } @@ -140,9 +144,20 @@ private static boolean isSafeSlf4jInteraction(MethodInvocationTree tree, Visitor } Type argType = state.getTypeFromString("com.palantir.logsafe.Arg"); Type throwableType = state.getTypeFromString(Throwable.class.getName()); + int args = 0; + int firstArgStartPosition = -1; + int lastArgEndPosition = -1; for (int i = 1; i < arguments.size(); i++) { - Type type = ASTHelpers.getType(arguments.get(i)); + Tree argument = arguments.get(i); + Type type = ASTHelpers.getType(argument); boolean isArg = ASTHelpers.isSubtype(type, argType, state); + if (isArg) { + if (i == 1) { + firstArgStartPosition = ASTHelpers.getStartPosition(argument); + } + args++; + lastArgEndPosition = state.getEndPosition(argument); + } boolean valid = isArg // Throwable is valid only in the last position || (ASTHelpers.isSubtype(type, throwableType, state) && i == arguments.size() - 1); @@ -150,6 +165,16 @@ private static boolean isSafeSlf4jInteraction(MethodInvocationTree tree, Visitor return false; } } + if (args > MAX_SUPPORTED_ARGS) { + // Update the call to wrap args with 'Arrays.asList' so the result will compile. + CharSequence argsSource = state.getSourceCode().subSequence(firstArgStartPosition, lastArgEndPosition); + String qualifiedArrays = SuggestedFixes.qualifyType(state, fix, Arrays.class.getName()); + fix.replace( + firstArgStartPosition, + lastArgEndPosition, + String.format("%s.asList(%s)", qualifiedArrays, argsSource)); + } + return true; } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java index 1c24b8f86..124753f86 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/PreferSafeLoggerTest.java @@ -254,4 +254,106 @@ void testIsTraceEnabledInArg() { "}") .doTest(); } + + @Test + void testTooManyArgs() { + 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(\"msg\",", + " SafeArg.of(\"1\", 1),", + " SafeArg.of(\"2\", 2),", + " SafeArg.of(\"3\", 3),", + " SafeArg.of(\"4\", 4),", + " SafeArg.of(\"5\", 5),", + " SafeArg.of(\"6\", 6),", + " SafeArg.of(\"7\", 7),", + " SafeArg.of(\"8\", 8),", + " SafeArg.of(\"9\", 9),", + " SafeArg.of(\"10\", 10),", + " SafeArg.of(\"11\", 11));", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.palantir.logsafe.logger.SafeLogger;", + "import com.palantir.logsafe.logger.SafeLoggerFactory;", + "import java.util.Arrays;", + "import org.slf4j.*;", + "class Test {", + " private static final SafeLogger log = SafeLoggerFactory.get(Test.class);", + " void action() {", + " log.info(\"msg\",", + " Arrays.asList(SafeArg.of(\"1\", 1),", + " SafeArg.of(\"2\", 2),", + " SafeArg.of(\"3\", 3),", + " SafeArg.of(\"4\", 4),", + " SafeArg.of(\"5\", 5),", + " SafeArg.of(\"6\", 6),", + " SafeArg.of(\"7\", 7),", + " SafeArg.of(\"8\", 8),", + " SafeArg.of(\"9\", 9),", + " SafeArg.of(\"10\", 10),", + " SafeArg.of(\"11\", 11)));", + " }", + "}") + .doTest(); + } + + @Test + void testTooManyArgsWithThrowable() { + fix().addInputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import org.slf4j.*;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void action(Throwable t) {", + " log.info(\"msg\",", + " SafeArg.of(\"1\", 1),", + " SafeArg.of(\"2\", 2),", + " SafeArg.of(\"3\", 3),", + " SafeArg.of(\"4\", 4),", + " SafeArg.of(\"5\", 5),", + " SafeArg.of(\"6\", 6),", + " SafeArg.of(\"7\", 7),", + " SafeArg.of(\"8\", 8),", + " SafeArg.of(\"9\", 9),", + " SafeArg.of(\"10\", 10),", + " SafeArg.of(\"11\", 11),", + " t);", + " }", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.palantir.logsafe.logger.SafeLogger;", + "import com.palantir.logsafe.logger.SafeLoggerFactory;", + "import java.util.Arrays;", + "import org.slf4j.*;", + "class Test {", + " private static final SafeLogger log = SafeLoggerFactory.get(Test.class);", + " void action(Throwable t) {", + " log.info(\"msg\",", + " Arrays.asList(SafeArg.of(\"1\", 1),", + " SafeArg.of(\"2\", 2),", + " SafeArg.of(\"3\", 3),", + " SafeArg.of(\"4\", 4),", + " SafeArg.of(\"5\", 5),", + " SafeArg.of(\"6\", 6),", + " SafeArg.of(\"7\", 7),", + " SafeArg.of(\"8\", 8),", + " SafeArg.of(\"9\", 9),", + " SafeArg.of(\"10\", 10),", + " SafeArg.of(\"11\", 11)),", + " t);", + " }", + "}") + .doTest(); + } } From 668c90e52411bcf95971eb6aef225cc8594a67c8 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Tue, 10 Aug 2021 18:17:42 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-1866.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-1866.v2.yml diff --git a/changelog/@unreleased/pr-1866.v2.yml b/changelog/@unreleased/pr-1866.v2.yml new file mode 100644 index 000000000..70b6eb6eb --- /dev/null +++ b/changelog/@unreleased/pr-1866.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: PreferSafeLogger produces suggestions which compile with >10 args + links: + - https://github.com/palantir/gradle-baseline/pull/1866