From 45acfe180445f4aefe35f7ee55bc910c38297066 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Sun, 13 Oct 2019 13:59:00 -0400 Subject: [PATCH 1/2] Implement Error Prone `Slf4jLevelCheck` Validate that slf4j level checks agree with contained logging. --- README.md | 1 + .../baseline/errorprone/Slf4jLevelCheck.java | 183 ++++++++++++++++++ .../errorprone/Slf4jLevelCheckTest.java | 137 +++++++++++++ .../BaselineErrorProneExtension.java | 1 + 4 files changed, 322 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLevelCheckTest.java diff --git a/README.md b/README.md index 9cb9528d3..52de67465 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,7 @@ checks](https://errorprone.info): - `DangerousParallelStreamUsage`: Discourage the use of Java parallel streams. - `Slf4jConstantLogMessage`: Allow only compile-time constant slf4j log message strings. +- `Slf4jLevelCheck`: Slf4j level checks (`if (log.isInfoEnabled()) {`) must match the most severe level in the containing block. - `Slf4jLogsafeArgs`: Allow only com.palantir.logsafe.Arg types as parameter inputs to slf4j log messages. More information on Safe Logging can be found at [github.com/palantir/safe-logging](https://github.com/palantir/safe-logging). - `PreferCollectionTransform`: Prefer Guava's Lists.transform or Collections2.transform instead of Iterables.transform when first argument's declared type is a List or Collection type for performance reasons. diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java new file mode 100644 index 000000000..683f0c61a --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/Slf4jLevelCheck.java @@ -0,0 +1,183 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.common.base.CaseFormat; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.IfTreeMatcher; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IfTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.ParenthesizedTree; +import com.sun.source.util.SimpleTreeVisitor; +import com.sun.source.util.TreeScanner; +import java.util.Locale; +import java.util.Optional; + +@AutoService(BugChecker.class) +@BugPattern( + name = "Slf4jLevelCheck", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = SeverityLevel.ERROR, + summary = "Slf4j logger.is[Level]Enabled level must match the most severe log statement") +public final class Slf4jLevelCheck extends BugChecker implements IfTreeMatcher { + + private static final long serialVersionUID = 1L; + + private static final Matcher LEVEL_CHECK_METHOD = MethodMatchers.instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .namedAnyOf("isTraceEnabled", "isDebugEnabled", "isInfoEnabled", "isWarnEnabled", "isErrorEnabled"); + + private static final Matcher LOG_METHOD = MethodMatchers.instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .namedAnyOf("trace", "debug", "info", "warn", "error"); + + @Override + public Description matchIf(IfTree tree, VisitorState state) { + // n.b. This check does not validate that the level check and logging occur on the same logger instance. + // It's possible to have multiple loggers in the same class used for different purposes, however we recommend + // against it. + Optional maybeCheckLevel = tree.getCondition().accept(ConditionVisitor.INSTANCE, state); + if (!maybeCheckLevel.isPresent()) { + return Description.NO_MATCH; + } + MethodInvocationTree levelCheckInvocation = maybeCheckLevel.get(); + LogLevel checkLevel = levelCheckLogLevel(levelCheckInvocation, state); + LogLevel mostSevere = tree.getThenStatement().accept(MostSevereLogStatementScanner.INSTANCE, state); + if (mostSevere == null) { + // Unable to find logging in this tree. This call likely delegates to something else which logs, + // but we cannot detect it. + return Description.NO_MATCH; + } + if (mostSevere == checkLevel) { + // The check matches the most severe log statement level. Keep up the great work! + return Description.NO_MATCH; + } + return buildDescription(tree) + .addFix(SuggestedFixes.renameMethodInvocation( + levelCheckInvocation, mostSevere.levelCheckMethodName(), state)) + .build(); + } + + @SuppressWarnings("PreferSafeLoggableExceptions") + private static LogLevel levelCheckLogLevel(MethodInvocationTree tree, VisitorState state) { + for (LogLevel level : LogLevel.values()) { + if (level.matchesLevelCheck(tree, state)) { + return level; + } + } + throw new IllegalStateException("Expected a level check, but was: " + state.getSourceForNode(tree)); + } + + // n.b. This check is only aware of conditionals of the form 'if (log.isWarnEnabled())', and does not currently + // match 'if (a != null && log.isWarnEnabled())', which may be worth including in the future. We should be careful + // to avoid anything beyond simple AND statement matching because some statements use complex boolean logic + // that's easy for automation to break. If implemented, we should avoid matching conditionals with level checks + // on both sides of a binary tree. + private static final class ConditionVisitor + extends SimpleTreeVisitor, VisitorState> { + + private static final ConditionVisitor INSTANCE = new ConditionVisitor(); + + private ConditionVisitor() { + super(Optional.empty()); + } + + @Override + public Optional visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + if (LEVEL_CHECK_METHOD.matches(node, state)) { + return Optional.of(node); + } + return Optional.empty(); + } + + @Override + public Optional visitParenthesized(ParenthesizedTree node, VisitorState state) { + return node.getExpression().accept(this, state); + } + } + + private static final class MostSevereLogStatementScanner extends TreeScanner { + private static final MostSevereLogStatementScanner INSTANCE = new MostSevereLogStatementScanner(); + + @Override + public LogLevel visitMethodInvocation(MethodInvocationTree node, VisitorState state) { + if (LOG_METHOD.matches(node, state)) { + for (LogLevel level : LogLevel.values()) { + if (level.matchesLogStatement(node, state)) { + return level; + } + } + } + return null; + } + + @Override + public LogLevel reduce(LogLevel r1, LogLevel r2) { + if (r1 == null) { + return r2; + } + if (r2 == null) { + return r1; + } + return r1.ordinal() > r2.ordinal() ? r1 : r2; + } + } + + @SuppressWarnings("unused") + private enum LogLevel { + TRACE, + DEBUG, + INFO, + WARN, + ERROR; + + private final String levelCheckMethodName = + "is" + CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.UPPER_CAMEL, name()) + "Enabled"; + + @SuppressWarnings("ImmutableEnumChecker") + private final Matcher levelCheckMatcher = MethodMatchers.instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .named(levelCheckMethodName); + + @SuppressWarnings("ImmutableEnumChecker") + private final Matcher logMatcher = MethodMatchers.instanceMethod() + .onDescendantOf("org.slf4j.Logger") + .named(name().toLowerCase(Locale.ENGLISH)); + + boolean matchesLevelCheck(ExpressionTree tree, VisitorState state) { + return levelCheckMatcher.matches(tree, state); + } + + boolean matchesLogStatement(ExpressionTree tree, VisitorState state) { + return logMatcher.matches(tree, state); + } + + String levelCheckMethodName() { + return levelCheckMethodName; + } + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLevelCheckTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLevelCheckTest.java new file mode 100644 index 000000000..187bbc895 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/Slf4jLevelCheckTest.java @@ -0,0 +1,137 @@ +/* + * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +class Slf4jLevelCheckTest { + + @Test + void testMessage() { + helper() + .addSourceLines( + "Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f() {", + " // BUG: Diagnostic contains: level must match the most severe", + " if (log.isInfoEnabled()) {", + " log.warn(\"foo\");", + " }", + " }", + "}") + .doTest(); + } + + @Test + void testCorrectLevel() { + helper() + .addSourceLines( + "Test.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + " void f() {", + " if (log.isInfoEnabled()) {", + " log.info(\"foo\");", + " }", + " }", + "}") + .doTest(); + } + + @Test + 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() {", + " if (log.isInfoEnabled()) {", + " log.warn(\"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() {", + " if (log.isWarnEnabled()) {", + " log.warn(\"foo\");", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + @Test + void testFix_nestedConditional() { + 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() {", + " if (log.isInfoEnabled()) {", + " if (this.getClass().getName().startsWith(\"c\")) {", + " log.info(\"foo\");", + " } else {", + " 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() {", + " if (log.isWarnEnabled()) {", + " if (this.getClass().getName().startsWith(\"c\")) {", + " log.info(\"foo\");", + " } else {", + " log.warn(\"bar\");", + " }", + " }", + " }", + "}") + .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(Slf4jLevelCheck.class, getClass()); + } + + private BugCheckerRefactoringTestHelper fix() { + return BugCheckerRefactoringTestHelper.newInstance(new Slf4jLevelCheck(), getClass()); + } +} 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 9fa021f08..dd951687c 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 @@ -32,6 +32,7 @@ public class BaselineErrorProneExtension { "PreferListsPartition", "PreferSafeLoggableExceptions", "PreferSafeLoggingPreconditions", + "Slf4jLevelCheck", "StrictUnusedVariable", "StringBuilderConstantParameters", From d6cafb84eba266ae467ae44ac147598a6783e72a Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Sun, 13 Oct 2019 17:59:00 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-960.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-960.v2.yml diff --git a/changelog/@unreleased/pr-960.v2.yml b/changelog/@unreleased/pr-960.v2.yml new file mode 100644 index 000000000..7b0a8c113 --- /dev/null +++ b/changelog/@unreleased/pr-960.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Implement Error Prone `Slf4jLevelCheck` to validate that slf4j level + checks agree with contained logging. + links: + - https://github.com/palantir/gradle-baseline/pull/960