Skip to content

Commit

Permalink
Implement a suggested fix for CatchBlockLogException (#998)
Browse files Browse the repository at this point in the history 
Implement a suggested fix for CatchBlockLogException
  • Loading branch information
@carterkozak @bulldozer-bot
carterkozak authored and bulldozer-bot[bot] committed Oct 24, 2019
1 parent 60f04d6 commit b560b2e
Show file tree
Hide file tree
Showing 4 changed files with 212 additions and 0 deletions.
101 changes: 101 additions & 0 deletions ...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/CatchBlockLogException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,36 @@
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;
import com.google.errorprone.matchers.Matchers;
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 {
Expand All @@ -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<SuggestedFix> attemptFix(CatchTree tree, VisitorState state) {
List<MethodInvocationTree> 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<? extends ExpressionTree> 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<Optional<String>, VisitorState> {
private static final ThrowableFromArgVisitor INSTANCE = new ThrowableFromArgVisitor();

private static final Matcher<ExpressionTree> throwableMessageInvocation = Matchers.instanceMethod()
.onDescendantOf(Throwable.class.getName())
.named("getMessage");

ThrowableFromArgVisitor() {
super(Optional.empty());
}

@Override
public Optional<String> 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<Optional<String>, VisitorState> {
private static final ThrowableFromInvocationVisitor INSTANCE = new ThrowableFromInvocationVisitor();

ThrowableFromInvocationVisitor() {
super(Optional.empty());
}

@Override
public Optional<String> 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<List<MethodInvocationTree>, VisitorState> {
private static final LogStatementScanner INSTANCE = new LogStatementScanner();

@Override
public List<MethodInvocationTree> visitMethodInvocation(MethodInvocationTree node, VisitorState state) {
if (logMethod.matches(node, state)) {
return ImmutableList.of(node);
}
return super.visitMethodInvocation(node, state);
}

@Override
public List<MethodInvocationTree> visitCatch(CatchTree node, VisitorState state) {
// Do not flag logging from a nested catch, it's handled separately
return ImmutableList.of();
}

@Override
public List<MethodInvocationTree> reduce(
@Nullable List<MethodInvocationTree> left,
@Nullable List<MethodInvocationTree> 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.<MethodInvocationTree>builder()
.addAll(left)
.addAll(right)
.build();
}
}
}
105 changes: 105 additions & 0 deletions ...rror-prone/src/test/java/com/palantir/baseline/errorprone/CatchBlockLogExceptionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<? extends Throwable> exceptionClass, String catchStatement, Optional<String> error) {
compilationHelper
.addSourceLines(
Expand All @@ -90,4 +191,8 @@ private void test(Class<? extends Throwable> exceptionClass, String catchStateme
"}")
.doTest();
}

private BugCheckerRefactoringTestHelper fix() {
return BugCheckerRefactoringTestHelper.newInstance(new CatchBlockLogException(), getClass());
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-998.v2.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Implement a suggested fix for CatchBlockLogException
links:
- https://github.com/palantir/gradle-baseline/pull/998
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 @@ -23,6 +23,7 @@
public class BaselineErrorProneExtension {
private static final ImmutableList<String> DEFAULT_PATCH_CHECKS = ImmutableList.of(
// Baseline checks
"CatchBlockLogException",
"ExecutorSubmitRunnableFutureIgnored",
"LambdaMethodReference",
"OptionalOrElseMethodInvocation",
Expand Down

0 comments on commit b560b2e

Please sign in to comment.