From bb9cc209ba3f2d8288e2f42e880b031c76bf0a30 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 10:55:05 -0400 Subject: [PATCH 1/6] Implement a suggested fix for CatchBlockLogException --- .../errorprone/CatchBlockLogException.java | 61 ++++++++++++++++ .../CatchBlockLogExceptionTest.java | 72 +++++++++++++++++++ 2 files changed, 133 insertions(+) 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..78a70f0bd 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,8 +31,13 @@ import com.google.errorprone.matchers.method.MethodMatchers; import com.sun.source.tree.CatchTree; import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; +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( @@ -60,10 +67,64 @@ 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(MostSevereLogStatementScanner.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, state.getSourceForNode(lastArgument) + ", " + tree.getParameter().getName()) + .build()); + } + + private static final class MostSevereLogStatementScanner + extends TreeScanner, VisitorState> { + private static final MostSevereLogStatementScanner INSTANCE = new MostSevereLogStatementScanner(); + + @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..96db59129 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,73 @@ 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.info(\"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.info(\"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.info(\"foo\");", + " log.debug(\"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.info(\"foo\");", + " log.debug(\"bar\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + private void test(Class exceptionClass, String catchStatement, Optional error) { compilationHelper .addSourceLines( @@ -90,4 +158,8 @@ private void test(Class exceptionClass, String catchStateme "}") .doTest(); } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new CatchBlockLogException(), getClass()); + } } From 40cb52248ec6797685c02871cb42147458e6b8dd Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 14:55:05 +0000 Subject: [PATCH 2/6] Add generated changelog entries --- changelog/@unreleased/pr-998.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-998.v2.yml 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 From 1f76d552012c672b378235609d386bfd9e2fc2e6 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 11:12:11 -0400 Subject: [PATCH 3/6] providesFix annotation arg --- .../com/palantir/baseline/errorprone/CatchBlockLogException.java | 1 + 1 file changed, 1 insertion(+) 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 78a70f0bd..c63ce8fe1 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 @@ -44,6 +44,7 @@ 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 { From ab6fd42fc0fc7aaf70a5f46cbfdc5082dd52d5e4 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 11:12:49 -0400 Subject: [PATCH 4/6] apply fixes by default --- .../baseline/extensions/BaselineErrorProneExtension.java | 1 + 1 file changed, 1 insertion(+) 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", From 663e46733b4c21c49cc038610c5a4ea462e1976f Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 12:05:56 -0400 Subject: [PATCH 5/6] Fix last arg if it uses getMessage, apply to warn/error --- .../errorprone/CatchBlockLogException.java | 51 ++++++++++++++- .../CatchBlockLogExceptionTest.java | 65 +++++++++++++++++-- 2 files changed, 108 insertions(+), 8 deletions(-) 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 c63ce8fe1..a139b4b51 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 @@ -31,8 +31,10 @@ 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; @@ -55,6 +57,10 @@ public final class CatchBlockLogException extends BugChecker implements BugCheck .onDescendantOf("org.slf4j.Logger") .withNameMatching(Pattern.compile("trace|debug|info|warn|error")); + private static final Matcher severeLogMethod = MethodMatchers.instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .withNameMatching(Pattern.compile("warn|error")); + private static final Matcher containslogMethod = Matchers.contains( Matchers.toType(ExpressionTree.class, logMethod)); @@ -82,17 +88,58 @@ private static Optional attemptFix(CatchTree tree, VisitorState st return Optional.empty(); } MethodInvocationTree loggingInvocation = matchingLoggingStatements.get(0); - if (containslogException.matches(loggingInvocation, state)) { + if (containslogException.matches(loggingInvocation, state) + // Only suggest fixes for warnings and errors to begin with. A future change + // should expand this to other statements if it goes well. + || !severeLogMethod.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, state.getSourceForNode(lastArgument) + ", " + tree.getParameter().getName()) + .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 MostSevereLogStatementScanner extends TreeScanner, VisitorState> { private static final MostSevereLogStatementScanner INSTANCE = new MostSevereLogStatementScanner(); 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 96db59129..62319fcad 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 @@ -84,7 +84,7 @@ public void testFix_simple() { " try {", " log.info(\"hello\");", " } catch (Throwable t) {", - " log.info(\"foo\");", + " log.error(\"foo\");", " }", " }", "}") @@ -97,7 +97,7 @@ public void testFix_simple() { " try {", " log.info(\"hello\");", " } catch (Throwable t) {", - " log.info(\"foo\", t);", + " log.error(\"foo\", t);", " }", " }", "}") @@ -117,8 +117,8 @@ public void testFix_ambiguous() { " try {", " log.info(\"hello\");", " } catch (Throwable t) {", - " log.info(\"foo\");", - " log.debug(\"bar\");", + " log.error(\"foo\");", + " log.warn(\"bar\");", " }", " }", "}") @@ -131,14 +131,67 @@ public void testFix_ambiguous() { " try {", " log.info(\"hello\");", " } catch (Throwable t) {", - " log.info(\"foo\");", - " log.debug(\"bar\");", + " 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); + } + + @Test + public void testFix_doesNotApplyToInfo() { + 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.info(\"foo\");", + " }", + " }", + "}") + .expectUnchanged() + .doTest(); + } + private void test(Class exceptionClass, String catchStatement, Optional error) { compilationHelper .addSourceLines( From 758e37934692fad0f7372f4121d6c4a6852b897c Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Thu, 24 Oct 2019 12:40:42 -0400 Subject: [PATCH 6/6] Do not limit to warn+error --- .../errorprone/CatchBlockLogException.java | 16 ++++----------- .../CatchBlockLogExceptionTest.java | 20 ------------------- 2 files changed, 4 insertions(+), 32 deletions(-) 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 a139b4b51..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 @@ -57,10 +57,6 @@ public final class CatchBlockLogException extends BugChecker implements BugCheck .onDescendantOf("org.slf4j.Logger") .withNameMatching(Pattern.compile("trace|debug|info|warn|error")); - private static final Matcher severeLogMethod = MethodMatchers.instanceMethod() - .onDescendantOf("org.slf4j.Logger") - .withNameMatching(Pattern.compile("warn|error")); - private static final Matcher containslogMethod = Matchers.contains( Matchers.toType(ExpressionTree.class, logMethod)); @@ -83,15 +79,12 @@ public Description matchCatch(CatchTree tree, VisitorState state) { private static Optional attemptFix(CatchTree tree, VisitorState state) { List matchingLoggingStatements = - tree.getBlock().accept(MostSevereLogStatementScanner.INSTANCE, state); + 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) - // Only suggest fixes for warnings and errors to begin with. A future change - // should expand this to other statements if it goes well. - || !severeLogMethod.matches(loggingInvocation, state)) { + if (containslogException.matches(loggingInvocation, state)) { return Optional.empty(); } List loggingArguments = loggingInvocation.getArguments(); @@ -140,9 +133,8 @@ public Optional visitMemberSelect(MemberSelectTree node, VisitorState st } } - private static final class MostSevereLogStatementScanner - extends TreeScanner, VisitorState> { - private static final MostSevereLogStatementScanner INSTANCE = new MostSevereLogStatementScanner(); + private static final class LogStatementScanner extends TreeScanner, VisitorState> { + private static final LogStatementScanner INSTANCE = new LogStatementScanner(); @Override public List visitMethodInvocation(MethodInvocationTree node, VisitorState state) { 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 62319fcad..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 @@ -172,26 +172,6 @@ public void testFix_getMessage() { .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); } - @Test - public void testFix_doesNotApplyToInfo() { - 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.info(\"foo\");", - " }", - " }", - "}") - .expectUnchanged() - .doTest(); - } - private void test(Class exceptionClass, String catchStatement, Optional error) { compilationHelper .addSourceLines(