From b560b2e6de1b8a283eecd3f9ddb4d32eca19ace5 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 13:54:05 -0400 Subject: [PATCH] Implement a suggested fix for CatchBlockLogException (#998) Implement a suggested fix for CatchBlockLogException --- .../errorprone/CatchBlockLogException.java | 101 +++++++++++++++++ .../CatchBlockLogExceptionTest.java | 105 ++++++++++++++++++ changelog/@unreleased/pr-998.v2.yml | 5 + .../BaselineErrorProneExtension.java | 1 + 4 files changed, 212 insertions(+) create mode 100644 changelog/@unreleased/pr-998.v2.yml diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java index 4651d2098..d2e294de3 100755 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java @@ -17,11 +17,13 @@ package com.palantir.baseline.errorprone; import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableList; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.LinkType; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.matchers.ChildMultiMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -29,14 +31,22 @@ import com.google.errorprone.matchers.method.MethodMatchers; import com.sun.source.tree.CatchTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MemberSelectTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; +import com.sun.source.util.SimpleTreeVisitor; +import com.sun.source.util.TreeScanner; +import java.util.List; +import java.util.Optional; import java.util.regex.Pattern; +import javax.annotation.Nullable; @AutoService(BugChecker.class) @BugPattern( name = "CatchBlockLogException", link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", linkType = LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, severity = SeverityLevel.ERROR, summary = "log statement in catch block does not log the caught exception.") public final class CatchBlockLogException extends BugChecker implements BugChecker.CatchTreeMatcher { @@ -60,10 +70,101 @@ public final class CatchBlockLogException extends BugChecker implements BugCheck public Description matchCatch(CatchTree tree, VisitorState state) { if (containslogMethod.matches(tree, state) && !containslogException.matches(tree, state)) { return buildDescription(tree) + .addFix(attemptFix(tree, state)) .setMessage("Catch block contains log statements but thrown exception is never logged.") .build(); } return Description.NO_MATCH; } + private static Optional attemptFix(CatchTree tree, VisitorState state) { + List matchingLoggingStatements = + tree.getBlock().accept(LogStatementScanner.INSTANCE, state); + if (matchingLoggingStatements == null || matchingLoggingStatements.size() != 1) { + return Optional.empty(); + } + MethodInvocationTree loggingInvocation = matchingLoggingStatements.get(0); + if (containslogException.matches(loggingInvocation, state)) { + return Optional.empty(); + } + List loggingArguments = loggingInvocation.getArguments(); + // There are no valid log invocations without at least a single argument. + ExpressionTree lastArgument = loggingArguments.get(loggingArguments.size() - 1); + return Optional.of(SuggestedFix.builder() + .replace(lastArgument, lastArgument.accept(ThrowableFromArgVisitor.INSTANCE, state) + .orElseGet(() -> state.getSourceForNode(lastArgument) + ", " + tree.getParameter().getName())) + .build()); + } + + private static final class ThrowableFromArgVisitor extends SimpleTreeVisitor, VisitorState> { + private static final ThrowableFromArgVisitor INSTANCE = new ThrowableFromArgVisitor(); + + private static final Matcher throwableMessageInvocation = Matchers.instanceMethod() + .onDescendantOf(Throwable.class.getName()) + .named("getMessage"); + + ThrowableFromArgVisitor() { + super(Optional.empty()); + } + + @Override + public Optional visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + if (throwableMessageInvocation.matches(node, state)) { + return node.getMethodSelect().accept(ThrowableFromInvocationVisitor.INSTANCE, state); + } + return Optional.empty(); + } + } + + private static final class ThrowableFromInvocationVisitor + extends SimpleTreeVisitor, VisitorState> { + private static final ThrowableFromInvocationVisitor INSTANCE = new ThrowableFromInvocationVisitor(); + + ThrowableFromInvocationVisitor() { + super(Optional.empty()); + } + + @Override + public Optional visitMemberSelect(MemberSelectTree node, VisitorState state) { + if (node.getIdentifier().contentEquals("getMessage")) { + return Optional.ofNullable(state.getSourceForNode(node.getExpression())); + } + return Optional.empty(); + } + } + + private static final class LogStatementScanner extends TreeScanner, VisitorState> { + private static final LogStatementScanner INSTANCE = new LogStatementScanner(); + + @Override + public List visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + if (logMethod.matches(node, state)) { + return ImmutableList.of(node); + } + return super.visitMethodInvocation(node, state); + } + + @Override + public List visitCatch(CatchTree node, VisitorState state) { + // Do not flag logging from a nested catch, it's handled separately + return ImmutableList.of(); + } + + @Override + public List reduce( + @Nullable List left, + @Nullable List right) { + // Unfortunately there's no way to provide default initial values, so we must handle nulls. + if (left == null) { + return right; + } + if (right == null) { + return left; + } + return ImmutableList.builder() + .addAll(left) + .addAll(right) + .build(); + } + } } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java index a4c3f11f7..59543e555 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java @@ -16,6 +16,7 @@ package com.palantir.baseline.errorprone; +import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.CompilationTestHelper; import java.util.Optional; import org.junit.jupiter.api.BeforeEach; @@ -71,6 +72,106 @@ public void testNoLogStatement() { test(RuntimeException.class, "// Do nothing", Optional.empty()); } + @Test + public void testFix_simple() { + fix() + .addInputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\");", + " }", + " }", + "}") + .addOutputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\", t);", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void testFix_ambiguous() { + // In this case there are multiple options, no fixes should be suggested. + fix() + .addInputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\");", + " log.warn(\"bar\");", + " }", + " }", + "}") + .addOutputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\");", + " log.warn(\"bar\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + public void testFix_getMessage() { + // In this case there are multiple options, no fixes should be suggested. + fix() + .addInputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\", t.getMessage());", + " }", + " }", + "}") + .addOutputLines("Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f(String param) {", + " try {", + " log.info(\"hello\");", + " } catch (Throwable t) {", + " log.error(\"foo\", t);", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + private void test(Class exceptionClass, String catchStatement, Optional error) { compilationHelper .addSourceLines( @@ -90,4 +191,8 @@ private void test(Class exceptionClass, String catchStateme "}") .doTest(); } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new CatchBlockLogException(), getClass()); + } } diff --git a/changelog/@unreleased/pr-998.v2.yml b/changelog/@unreleased/pr-998.v2.yml new file mode 100644 index 000000000..bd5ed606b --- /dev/null +++ b/changelog/@unreleased/pr-998.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Implement a suggested fix for CatchBlockLogException + links: + - https://github.com/palantir/gradle-baseline/pull/998 diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java index 592e8cb65..b88f55f0d 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java @@ -23,6 +23,7 @@ public class BaselineErrorProneExtension { private static final ImmutableList DEFAULT_PATCH_CHECKS = ImmutableList.of( // Baseline checks + "CatchBlockLogException", "ExecutorSubmitRunnableFutureIgnored", "LambdaMethodReference", "OptionalOrElseMethodInvocation",