diff --git a/README.md b/README.md index 45e4e7b1b..2ebbcc133 100644 --- a/README.md +++ b/README.md @@ -186,6 +186,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `UnsafeGaugeRegistration`: Use TaggedMetricRegistry.registerWithReplacement over TaggedMetricRegistry.gauge. - `BracesRequired`: Require braces for loops and if expressions. - `CollectionStreamForEach`: Collection.forEach is more efficient than Collection.stream().forEach. +- `LoggerEnclosingClass`: Loggers created using getLogger(Class) must reference their enclosing class. ### Programmatic Application diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LoggerEnclosingClass.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LoggerEnclosingClass.java new file mode 100644 index 000000000..869343380 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/LoggerEnclosingClass.java @@ -0,0 +1,90 @@ +/* + * (c) Copyright 2020 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.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +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.google.errorprone.util.ASTHelpers; +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.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; + +@AutoService(BugChecker.class) +@BugPattern( + name = "LoggerEnclosingClass", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION, + severity = BugPattern.SeverityLevel.WARNING, + summary = "Loggers created using getLogger(Class) must reference their enclosing class.") +public final class LoggerEnclosingClass extends BugChecker implements BugChecker.VariableTreeMatcher { + + private static final Matcher matcher = Matchers.allOf( + Matchers.isField(), + Matchers.isSubtypeOf("org.slf4j.Logger"), + Matchers.variableInitializer(MethodMatchers.staticMethod() + .onClass("org.slf4j.LoggerFactory") + .named("getLogger") + // Only match the 'class' constructor + .withParameters(Class.class.getName()))); + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (!matcher.matches(tree, state)) { + return Description.NO_MATCH; + } + + MethodInvocationTree getLoggerInvocation = (MethodInvocationTree) tree.getInitializer(); + ExpressionTree classArgument = getLoggerInvocation.getArguments().get(0); + + if (!(classArgument instanceof MemberSelectTree)) { + return Description.NO_MATCH; + } + MemberSelectTree memberSelectTree = (MemberSelectTree) classArgument; + if (!memberSelectTree.getIdentifier().contentEquals("class") + || memberSelectTree.getExpression().getKind() != Tree.Kind.IDENTIFIER) { + return Description.NO_MATCH; + } + + Symbol targetSymbol = ASTHelpers.getSymbol(memberSelectTree.getExpression()); + Symbol.ClassSymbol enclosingClassSymbol = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); + if (targetSymbol == null || enclosingClassSymbol == null) { + return Description.NO_MATCH; + } + if (state.getTypes().isSameType(targetSymbol.type, enclosingClassSymbol.type)) { + return Description.NO_MATCH; + } + SuggestedFix.Builder fix = SuggestedFix.builder(); + return buildDescription(classArgument) + .addFix(fix.replace( + classArgument, + SuggestedFixes.prettyType(state, fix, enclosingClassSymbol.type) + ".class") + .build()) + .build(); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LoggerEnclosingClassTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LoggerEnclosingClassTest.java new file mode 100644 index 000000000..f598cf429 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/LoggerEnclosingClassTest.java @@ -0,0 +1,101 @@ +/* + * (c) Copyright 2020 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 org.junit.jupiter.api.Test; + +class LoggerEnclosingClassTest { + + @Test + void testFix() { + fix().addInputLines( + "Test.java", + "import org.slf4j.*;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(String.class);", + "}") + .addOutputLines( + "Test.java", + "import org.slf4j.*;", + "class Test {", + " private static final Logger log = LoggerFactory.getLogger(Test.class);", + "}") + .doTest(); + } + + @Test + void testFix_interface() { + fix().addInputLines( + "Test.java", + "import org.slf4j.*;", + "interface Test {", + " Logger log = LoggerFactory.getLogger(String.class);", + "}") + .addOutputLines( + "Test.java", + "import org.slf4j.*;", + "interface Test {", + " Logger log = LoggerFactory.getLogger(Test.class);", + "}") + .doTest(); + } + + @Test + void testFix_nested() { + fix().addInputLines( + "Test.java", + "import org.slf4j.*;", + "class Test {", + " interface Nested {", + " Logger log = LoggerFactory.getLogger(Test.class);", + " }", + "}") + .addOutputLines( + "Test.java", + "import org.slf4j.*;", + "class Test {", + " interface Nested {", + " Logger log = LoggerFactory.getLogger(Nested.class);", + " }", + "}") + .doTest(); + } + + @Test + void testNegative() { + fix().addInputLines( + "Test.java", + "import org.slf4j.*;", + "class Test {", + // Not great, but it's out of scope for this check to validate dynamic cases. + " private static final Logger log = LoggerFactory.getLogger(dynamic());", + " private static Class dynamic() {", + " return String.class;", + " }", + " private static void func() {", + " LoggerFactory.getLogger(String.class);", + " Logger local = LoggerFactory.getLogger(String.class);", + " }", + "}") + .expectUnchanged() + .doTest(); + } + + private RefactoringValidator fix() { + return RefactoringValidator.of(new LoggerEnclosingClass(), getClass()); + } +} diff --git a/changelog/@unreleased/pr-1163.v2.yml b/changelog/@unreleased/pr-1163.v2.yml new file mode 100644 index 000000000..38b60377f --- /dev/null +++ b/changelog/@unreleased/pr-1163.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Add error prone LoggerEnclosingClass check. Loggers created using getLogger(Class) + must reference their enclosing class. + links: + - https://github.com/palantir/gradle-baseline/pull/1163 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 14134e055..f1a365e46 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 @@ -31,6 +31,7 @@ public class BaselineErrorProneExtension { "ExecutorSubmitRunnableFutureIgnored", "FinalClass", "LambdaMethodReference", + "LoggerEnclosingClass", "OptionalOrElseMethodInvocation", "PreferBuiltInConcurrentKeySet", "PreferCollectionTransform",