Skip to content

Commit

Permalink
Slf4jLogsafeArgs fixes safe-log wrapped throwables
Browse files Browse the repository at this point in the history 
Follow-up to #997
  • Loading branch information
@carterkozak
carterkozak committed Oct 29, 2019
1 parent b560b2e commit 51721aa
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 7 deletions.
56 changes: 49 additions & 7 deletions baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,27 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.ChildMultiMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.matchers.method.MethodMatchers;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.util.SimpleTreeVisitor;
import com.sun.tools.javac.code.Type;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;

@AutoService(BugChecker.class)
@BugPattern(
name = "Slf4jLogsafeArgs",
link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
linkType = BugPattern.LinkType.CUSTOM,
providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
severity = SeverityLevel.WARNING,
summary = "Allow only com.palantir.logsafe.Arg types as parameter inputs to slf4j log messages.")
public final class Slf4jLogsafeArgs extends BugChecker implements MethodInvocationTreeMatcher {
Expand All @@ -47,6 +54,8 @@ public final class Slf4jLogsafeArgs extends BugChecker implements MethodInvocati
.onDescendantOf("org.slf4j.Logger")
.withNameMatching(Pattern.compile("trace|debug|info|warn|error"));

private static final Matcher<ExpressionTree> THROWABLE = Matchers.isSubtypeOf(Throwable.class);

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (!LOG_METHOD.matches(tree, state)) {
Expand All @@ -59,20 +68,28 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
ASTHelpers.getType(allArgs.get(0)),
state.getTypeFromString("org.slf4j.Marker"),
state) ? 2 : 1;
int endArg = ASTHelpers.isCastable(
ASTHelpers.getType(allArgs.get(lastIndex)),
state.getTypeFromString("java.lang.Throwable"),
state) ? lastIndex - 1 : lastIndex;
ExpressionTree lastArg = allArgs.get(lastIndex);
boolean lastArgIsThrowable = THROWABLE.matches(lastArg, state);
int endArg = lastArgIsThrowable ? lastIndex - 1 : lastIndex;

ImmutableList.Builder<Integer> badArgsBuilder = ImmutableList.builder();
Type argType = state.getTypeFromString("com.palantir.logsafe.Arg");
for (int i = startArg; i <= endArg; i++) {
if (!ASTHelpers.isCastable(ASTHelpers.getType(allArgs.get(i)),
state.getTypeFromString("com.palantir.logsafe.Arg"), state)) {
if (!ASTHelpers.isCastable(ASTHelpers.getType(allArgs.get(i)), argType, state)) {
badArgsBuilder.add(i);
}
}
List<Integer> badArgs = badArgsBuilder.build();

if (!lastArgIsThrowable && !badArgs.contains(lastIndex)) {
Optional<Description> description = lastArg.accept(ThrowableArgVisitor.INSTANCE, state)
.map(node -> buildDescription(tree)
.setMessage("slf4j log statement should not use logsafe wrappers for throwables")
.addFix(SuggestedFix.replace(lastArg, state.getSourceForNode(node)))
.build());
if (description.isPresent()) {
return description.get();
}
}
if (badArgs.isEmpty() || TestCheckUtils.isTestCode(state)) {
return Description.NO_MATCH;
} else {
Expand All @@ -81,4 +98,29 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
.build();
}
}

/** Returns the throwable argument from SafeArg.of(name, throwable) or UnsafeArg.of(name, throwable). */
private static final class ThrowableArgVisitor extends SimpleTreeVisitor<Optional<ExpressionTree>, VisitorState> {
private static final ThrowableArgVisitor INSTANCE = new ThrowableArgVisitor();

private static final Matcher<ExpressionTree> ARG = Matchers.staticMethod()
.onClassAny("com.palantir.logsafe.SafeArg", "com.palantir.logsafe.UnsafeArg")
.named("of")
.withParameters(String.class.getName(), Object.class.getName());

private static final Matcher<ExpressionTree> THROWABLE_ARG = Matchers.methodInvocation(
ARG, ChildMultiMatcher.MatchType.LAST, THROWABLE);

private ThrowableArgVisitor() {
super(Optional.empty());
}

@Override
public Optional<ExpressionTree> visitMethodInvocation(MethodInvocationTree node, VisitorState state) {
if (THROWABLE_ARG.matches(node, state)) {
return Optional.of(node.getArguments().get(1));
}
return Optional.empty();
}
}
}
37 changes: 37 additions & 0 deletions ...line-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLogsafeArgsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.palantir.baseline.errorprone;

import com.google.common.collect.ImmutableList;
import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -171,4 +172,40 @@ public void testPassingLogsafeArgs() throws Exception {
"}")
.doTest());
}

@Test
public void testLastArgThrowable() {
fix()
.addInputLines("Test.java",
"import org.slf4j.Logger;",
"import org.slf4j.LoggerFactory;",
"import com.palantir.logsafe.SafeArg;",
"import com.palantir.logsafe.UnsafeArg;",
"class Test {",
" private static final Logger log = LoggerFactory.getLogger(Test.class);",
" void f(Throwable t) {",
" log.warn(\"1\", SafeArg.of(\"extra\", t), t);",
" log.warn(\"2\", SafeArg.of(\"extra\", t), SafeArg.of(\"name\", t));",
" log.warn(\"3\", SafeArg.of(\"extra\", t), UnsafeArg.of(\"name\", t));",
" }",
"}")
.addOutputLines("Test.java",
"import org.slf4j.Logger;",
"import org.slf4j.LoggerFactory;",
"import com.palantir.logsafe.SafeArg;",
"import com.palantir.logsafe.UnsafeArg;",
"class Test {",
" private static final Logger log = LoggerFactory.getLogger(Test.class);",
" void f(Throwable t) {",
" log.warn(\"1\", SafeArg.of(\"extra\", t), t);",
" log.warn(\"2\", SafeArg.of(\"extra\", t), t);",
" log.warn(\"3\", SafeArg.of(\"extra\", t), t);",
" }",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
}

private BugCheckerRefactoringTestHelper fix() {
return BugCheckerRefactoringTestHelper.newInstance(new Slf4jLogsafeArgs(), getClass());
}
}
1 change: 1 addition & 0 deletions ...ne-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ public class BaselineErrorProneExtension {
"PreferSafeLoggingPreconditions",
"ReadReturnValueIgnored",
"Slf4jLevelCheck",
"Slf4jLogsafeArgs",
"StrictUnusedVariable",
"StringBuilderConstantParameters",
"ThrowError",
Expand Down

0 comments on commit 51721aa

Please sign in to comment.