Implement Error Prone Slf4jLevelCheck
#960
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<ExpressionTree> LEVEL_CHECK_METHOD = MethodMatchers.instanceMethod() | ||
| .onDescendantOf("org.slf4j.Logger") | ||
| .namedAnyOf("isTraceEnabled", "isDebugEnabled", "isInfoEnabled", "isWarnEnabled", "isErrorEnabled"); | ||
|
|
||
| private static final Matcher<ExpressionTree> 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<MethodInvocationTree> 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)) { | ||
|
There was a problem hiding this comment. 😮A ckozak snippet where we allocate an iterator and loop instead of pre-building a composed function! (just teasing) |
||
| 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<Optional<MethodInvocationTree>, VisitorState> { | ||
|
|
||
| private static final ConditionVisitor INSTANCE = new ConditionVisitor(); | ||
|
|
||
| private ConditionVisitor() { | ||
| super(Optional.empty()); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<MethodInvocationTree> visitMethodInvocation(MethodInvocationTree node, VisitorState state) { | ||
| if (LEVEL_CHECK_METHOD.matches(node, state)) { | ||
| return Optional.of(node); | ||
| } | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| @Override | ||
| public Optional<MethodInvocationTree> visitParenthesized(ParenthesizedTree node, VisitorState state) { | ||
| return node.getExpression().accept(this, state); | ||
| } | ||
| } | ||
|
|
||
| private static final class MostSevereLogStatementScanner extends TreeScanner<LogLevel, VisitorState> { | ||
| 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<ExpressionTree> levelCheckMatcher = MethodMatchers.instanceMethod() | ||
| .onDescendantOf("org.slf4j.Logger") | ||
| .named(levelCheckMethodName); | ||
|
|
||
| @SuppressWarnings("ImmutableEnumChecker") | ||
| private final Matcher<ExpressionTree> 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; | ||
| } | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/logger.is[Level]Enabled/log.is[Level]Enabled/just so people don't get the wrong ideas about what we recommend as best-practise for naming those variablesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call, updating shortly