From e92922483260e0facf51a191ec043afe4ccf8a4c Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sun, 3 Sep 2023 18:32:04 +0200 Subject: [PATCH 01/23] Introduce `Slf4jLogDeclaration` to canonicalize slf4j's variable's declaration --- .../bugpatterns/Slf4jLogDeclaration.java | 129 ++++++++++++++++++ .../bugpatterns/Slf4jLogDeclarationTest.java | 93 +++++++++++++ 2 files changed, 222 insertions(+) create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java new file mode 100644 index 0000000000..867d33b926 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -0,0 +1,129 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.hasIdentifier; +import static com.google.errorprone.matchers.Matchers.isSubtypeOf; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +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.sun.source.tree.ClassTree; +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.Tree.Kind; +import com.sun.source.tree.VariableTree; +import javax.inject.Inject; +import javax.lang.model.element.Modifier; +import javax.lang.model.element.Name; +import javax.tools.JavaFileObject; +import tech.picnic.errorprone.bugpatterns.util.SourceCode; + +/** + * A {@link BugChecker} that warns when SLF4J declarations are not canonicalized across the project. + * + * @apiNote The default canonicalized logger name can be overriden through {@link ErrorProneFlags + * flag arguments}. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Make sure SLF4J log declarations follow the SLF4J conventions inside classes.", + link = BUG_PATTERNS_BASE_URL + "Slf4jLogDeclaration", + linkType = CUSTOM, + severity = WARNING, + tags = LIKELY_ERROR) +public final class Slf4jLogDeclaration extends BugChecker + implements ClassTreeMatcher, MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher TEST_CLASS_WITH_LOGGER = + allOf(hasIdentifier(isSubtypeOf("org.slf4j.Logger"))); + private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); + private static final String CANONICALIZED_LOGGER_NAME_FLAG = + "Slf4jLogDeclaration:CanonicalizedLoggerName"; + private static final String DEFAULT_CANONICALIZED_LOGGER_NAME = "LOG"; + private static final Matcher GET_LOGGER_METHOD = + staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); + + private final String canonicalizedLoggerName; + + /** Instantiates a default {@link Slf4jLogDeclaration} instance. */ + public Slf4jLogDeclaration() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link Slf4jLogDeclaration}. + * + * @param flags Any provided command line flags. + */ + @Inject + Slf4jLogDeclaration(ErrorProneFlags flags) { + canonicalizedLoggerName = getCanonicalizedLoggerName(flags); + } + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + if (tree.getKind() == Kind.INTERFACE || !TEST_CLASS_WITH_LOGGER.matches(tree, state)) { + return Description.NO_MATCH; + } + + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + for (Tree member : tree.getMembers()) { + if (LOGGER.matches(member, state)) { + VariableTree variable = (VariableTree) member; + SuggestedFixes.addModifiers( + member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + .ifPresent(fixBuilder::merge); + if (!variable.getName().toString().startsWith(canonicalizedLoggerName)) { + fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); + } + } + } + + return describeMatch(tree, fixBuilder.build()); + } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!GET_LOGGER_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + + for (ExpressionTree arg : tree.getArguments()) { + MemberSelectTree memberArgument = (MemberSelectTree) arg; + + for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { + if (typeDeclaration instanceof ClassTree) { + Name className = ((ClassTree) typeDeclaration).getSimpleName(); + String argumentName = SourceCode.treeToString(memberArgument.getExpression(), state); + + if (!className.contentEquals(argumentName)) { + fixBuilder.merge( + SuggestedFix.replace(arg, className + JavaFileObject.Kind.CLASS.extension)); + } + } + } + } + + return describeMatch(tree, fixBuilder.build()); + } + + private static String getCanonicalizedLoggerName(ErrorProneFlags flags) { + return flags.get(CANONICALIZED_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICALIZED_LOGGER_NAME); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java new file mode 100644 index 0000000000..1fe61b2ac8 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -0,0 +1,93 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import org.junit.jupiter.api.Test; + +final class Slf4jLogDeclarationTest { + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " Logger LOG1 = LoggerFactory.getLogger(A.class);", + "", + " private Logger LOG2 = LoggerFactory.getLogger(A.class);", + "", + " private static Logger LOG3 = LoggerFactory.getLogger(A.class);", + "", + " static final Logger LOG4 = LoggerFactory.getLogger(A.class);", + "", + " private final Logger LOG5 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG6 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", + " LoggerFactory.getLogger(B.class);", + "", + " class B {}", + "}") + .addOutputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " private static final Logger LOG1 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG2 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG3 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG4 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG5 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG6 = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", + " LoggerFactory.getLogger(A.class);", + "", + " class B {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void doNotAddModifiersToDeclarationsInsideInterfaces() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "interface A {", + " Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " Logger LOG1 = LoggerFactory.getLogger(B.class);", + "", + " class B {}", + "}") + .addOutputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "interface A {", + " Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " Logger LOG1 = LoggerFactory.getLogger(A.class);", + "", + " class B {}", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} From 2949d86c090a47267db02184139c07456cc8e5de Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 16:15:38 +0100 Subject: [PATCH 02/23] Drop redundant check --- .../picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 867d33b926..64bd0998e7 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -3,8 +3,6 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; -import static com.google.errorprone.matchers.Matchers.allOf; -import static com.google.errorprone.matchers.Matchers.hasIdentifier; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; @@ -49,8 +47,6 @@ public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher, MethodInvocationTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher TEST_CLASS_WITH_LOGGER = - allOf(hasIdentifier(isSubtypeOf("org.slf4j.Logger"))); private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); private static final String CANONICALIZED_LOGGER_NAME_FLAG = "Slf4jLogDeclaration:CanonicalizedLoggerName"; @@ -77,7 +73,7 @@ public Slf4jLogDeclaration() { @Override public Description matchClass(ClassTree tree, VisitorState state) { - if (tree.getKind() == Kind.INTERFACE || !TEST_CLASS_WITH_LOGGER.matches(tree, state)) { + if (tree.getKind() == Kind.INTERFACE) { return Description.NO_MATCH; } From 9f99689fb7b83b43355bb0dcb6d8e4ac4f898ede Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 19:58:19 +0100 Subject: [PATCH 03/23] Assume one logger per class, check nested classes and use TreeScanner --- .../bugpatterns/Slf4jLogDeclaration.java | 59 +++++++++------- .../bugpatterns/Slf4jLogDeclarationTest.java | 67 +++++++++++++------ 2 files changed, 80 insertions(+), 46 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 64bd0998e7..1a78454727 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -13,22 +13,23 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; -import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFix.Builder; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.sun.source.tree.ClassTree; 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.Tree.Kind; import com.sun.source.tree.VariableTree; +import com.sun.source.util.TreeScanner; import javax.inject.Inject; import javax.lang.model.element.Modifier; import javax.lang.model.element.Name; import javax.tools.JavaFileObject; +import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.bugpatterns.util.SourceCode; /** @@ -44,8 +45,7 @@ linkType = CUSTOM, severity = WARNING, tags = LIKELY_ERROR) -public final class Slf4jLogDeclaration extends BugChecker - implements ClassTreeMatcher, MethodInvocationTreeMatcher { +public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); private static final String CANONICALIZED_LOGGER_NAME_FLAG = @@ -78,45 +78,52 @@ public Description matchClass(ClassTree tree, VisitorState state) { } SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + + fixLoggerVariableModifiers(tree, state, fixBuilder); + fixLoggerVariableDeclaration(state, fixBuilder); + + return describeMatch(tree, fixBuilder.build()); + } + + private void fixLoggerVariableModifiers(ClassTree tree, VisitorState state, Builder fixBuilder) { for (Tree member : tree.getMembers()) { if (LOGGER.matches(member, state)) { VariableTree variable = (VariableTree) member; SuggestedFixes.addModifiers( member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) .ifPresent(fixBuilder::merge); - if (!variable.getName().toString().startsWith(canonicalizedLoggerName)) { + if (!variable.getName().toString().equals(canonicalizedLoggerName)) { fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); } } } - - return describeMatch(tree, fixBuilder.build()); } - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (!GET_LOGGER_METHOD.matches(tree, state)) { - return Description.NO_MATCH; - } - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - - for (ExpressionTree arg : tree.getArguments()) { - MemberSelectTree memberArgument = (MemberSelectTree) arg; + private static void fixLoggerVariableDeclaration(VisitorState state, Builder fixBuilder) { + for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { + if (typeDeclaration instanceof ClassTree) { + new TreeScanner<@Nullable Void, Name>() { + @Override + public @Nullable Void visitClass(ClassTree classTree, Name className) { + return super.visitClass(classTree, classTree.getSimpleName()); + } - for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { - if (typeDeclaration instanceof ClassTree) { - Name className = ((ClassTree) typeDeclaration).getSimpleName(); - String argumentName = SourceCode.treeToString(memberArgument.getExpression(), state); + public @Nullable Void visitMethodInvocation( + MethodInvocationTree methodTree, Name className) { + if (GET_LOGGER_METHOD.matches(methodTree, state)) { + ExpressionTree arg1 = methodTree.getArguments().get(0); + String argumentName = SourceCode.treeToString(arg1, state); - if (!className.contentEquals(argumentName)) { - fixBuilder.merge( - SuggestedFix.replace(arg, className + JavaFileObject.Kind.CLASS.extension)); + if (!className.contentEquals(argumentName)) { + fixBuilder.merge( + SuggestedFix.replace(arg1, className + JavaFileObject.Kind.CLASS.extension)); + } + } + return super.visitMethodInvocation(methodTree, className); } - } + }.scan(typeDeclaration, null); } } - - return describeMatch(tree, fixBuilder.build()); } private static String getCanonicalizedLoggerName(ErrorProneFlags flags) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 1fe61b2ac8..7c81de5902 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -14,24 +14,38 @@ void replacement() { "import org.slf4j.LoggerFactory;", "", "class A {", - " Logger LOG1 = LoggerFactory.getLogger(A.class);", + " Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", "", - " private Logger LOG2 = LoggerFactory.getLogger(A.class);", + " class B {", + " private Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(B.class);", + " }", "", - " private static Logger LOG3 = LoggerFactory.getLogger(A.class);", + " class C {", + " private static Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(C.class);", + " }", "", - " static final Logger LOG4 = LoggerFactory.getLogger(A.class);", + " class D {", + " static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(D.class);", + " }", "", - " private final Logger LOG5 = LoggerFactory.getLogger(A.class);", + " class E {", + " private final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(E.class);", + " }", "", - " private static final Logger LOG6 = LoggerFactory.getLogger(A.class);", + " class F {", + " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(F.class);", + " }", "", - " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", + " class G {", + " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(G.class);", + " }", "", - " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", - " LoggerFactory.getLogger(B.class);", + " class H {", + " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", + " LoggerFactory.getLogger(J.class);", + " }", "", - " class B {}", + " class J {}", "}") .addOutputLines( "A.java", @@ -39,24 +53,37 @@ void replacement() { "import org.slf4j.LoggerFactory;", "", "class A {", - " private static final Logger LOG1 = LoggerFactory.getLogger(A.class);", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", - " private static final Logger LOG2 = LoggerFactory.getLogger(A.class);", + " class B {", + " private static final Logger LOG = LoggerFactory.getLogger(B.class);", + " }", "", - " private static final Logger LOG3 = LoggerFactory.getLogger(A.class);", + " class C {", + " private static final Logger LOG = LoggerFactory.getLogger(C.class);", + " }", "", - " private static final Logger LOG4 = LoggerFactory.getLogger(A.class);", + " class D {", + " private static final Logger LOG = LoggerFactory.getLogger(D.class);", + " }", "", - " private static final Logger LOG5 = LoggerFactory.getLogger(A.class);", + " class E {", + " private static final Logger LOG = LoggerFactory.getLogger(E.class);", + " }", "", - " private static final Logger LOG6 = LoggerFactory.getLogger(A.class);", + " class F {", + " private static final Logger LOG = LoggerFactory.getLogger(F.class);", + " }", "", - " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + " class G {", + " private static final Logger LOG = LoggerFactory.getLogger(G.class);", + " }", "", - " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", - " LoggerFactory.getLogger(A.class);", + " class H {", + " private static final Logger LOG = LoggerFactory.getLogger(H.class);", + " }", "", - " class B {}", + " class J {}", "}") .doTest(TestMode.TEXT_MATCH); } From 1ac3568cdc2bccfb751fc7ee61157f4e8b778d4d Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 20:08:29 +0100 Subject: [PATCH 04/23] Don't skip interfaces --- .../errorprone/bugpatterns/Slf4jLogDeclaration.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 1a78454727..381fce18f1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -73,16 +73,14 @@ public Slf4jLogDeclaration() { @Override public Description matchClass(ClassTree tree, VisitorState state) { - if (tree.getKind() == Kind.INTERFACE) { - return Description.NO_MATCH; - } - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); fixLoggerVariableModifiers(tree, state, fixBuilder); fixLoggerVariableDeclaration(state, fixBuilder); - return describeMatch(tree, fixBuilder.build()); + return tree.getKind() == Kind.INTERFACE + ? describeMatch(tree) + : describeMatch(tree, fixBuilder.build()); } private void fixLoggerVariableModifiers(ClassTree tree, VisitorState state, Builder fixBuilder) { From afdc61a7ac635dc70708d3c3065185753cd81b0e Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 21:01:13 +0100 Subject: [PATCH 05/23] Ommit modifying interface variables modifiers (heh) --- .../bugpatterns/Slf4jLogDeclaration.java | 33 +++++++++-------- .../bugpatterns/Slf4jLogDeclarationTest.java | 36 ++++--------------- 2 files changed, 25 insertions(+), 44 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 381fce18f1..fc8dcac3e6 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -75,26 +75,29 @@ public Slf4jLogDeclaration() { public Description matchClass(ClassTree tree, VisitorState state) { SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - fixLoggerVariableModifiers(tree, state, fixBuilder); - fixLoggerVariableDeclaration(state, fixBuilder); - - return tree.getKind() == Kind.INTERFACE - ? describeMatch(tree) - : describeMatch(tree, fixBuilder.build()); - } - - private void fixLoggerVariableModifiers(ClassTree tree, VisitorState state, Builder fixBuilder) { for (Tree member : tree.getMembers()) { if (LOGGER.matches(member, state)) { - VariableTree variable = (VariableTree) member; - SuggestedFixes.addModifiers( - member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) - .ifPresent(fixBuilder::merge); - if (!variable.getName().toString().equals(canonicalizedLoggerName)) { - fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); + if (tree.getKind() != Kind.INTERFACE) { + fixLoggerVariableModifiers(member, state, fixBuilder); } + canonicalizeLoggerVariable(member, state, fixBuilder); } } + fixLoggerVariableDeclaration(state, fixBuilder); + + return describeMatch(tree, fixBuilder.build()); + } + + private void fixLoggerVariableModifiers(Tree member, VisitorState state, Builder fixBuilder) { + SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + .ifPresent(fixBuilder::merge); + } + + private void canonicalizeLoggerVariable(Tree member, VisitorState state, Builder fixBuilder) { + VariableTree variable = (VariableTree) member; + if (!variable.getName().toString().equals(canonicalizedLoggerName)) { + fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); + } } private static void fixLoggerVariableDeclaration(VisitorState state, Builder fixBuilder) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 7c81de5902..42ab513ad1 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -46,6 +46,10 @@ void replacement() { " }", "", " class J {}", + "", + " interface K {", + " Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", + " }", "}") .addOutputLines( "A.java", @@ -84,36 +88,10 @@ void replacement() { " }", "", " class J {}", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void doNotAddModifiersToDeclarationsInsideInterfaces() { - BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .addInputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", "", - "interface A {", - " Logger LOG = LoggerFactory.getLogger(A.class);", - "", - " Logger LOG1 = LoggerFactory.getLogger(B.class);", - "", - " class B {}", - "}") - .addOutputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "interface A {", - " Logger LOG = LoggerFactory.getLogger(A.class);", - "", - " Logger LOG1 = LoggerFactory.getLogger(A.class);", - "", - " class B {}", + " interface K {", + " Logger LOG = LoggerFactory.getLogger(K.class);", + " }", "}") .doTest(TestMode.TEXT_MATCH); } From 9cebce1051cc3aba0ef5c195f6bf5af23bd24cc5 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 21:19:44 +0100 Subject: [PATCH 06/23] Fix checkstyles --- .../errorprone/bugpatterns/Slf4jLogDeclaration.java | 13 ++++++++----- .../bugpatterns/Slf4jLogDeclarationTest.java | 4 ++-- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index fc8dcac3e6..0c78359718 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -14,7 +14,6 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.fixes.SuggestedFix.Builder; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -85,22 +84,25 @@ public Description matchClass(ClassTree tree, VisitorState state) { } fixLoggerVariableDeclaration(state, fixBuilder); - return describeMatch(tree, fixBuilder.build()); + return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } - private void fixLoggerVariableModifiers(Tree member, VisitorState state, Builder fixBuilder) { + private static void fixLoggerVariableModifiers( + Tree member, VisitorState state, SuggestedFix.Builder fixBuilder) { SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) .ifPresent(fixBuilder::merge); } - private void canonicalizeLoggerVariable(Tree member, VisitorState state, Builder fixBuilder) { + private void canonicalizeLoggerVariable( + Tree member, VisitorState state, SuggestedFix.Builder fixBuilder) { VariableTree variable = (VariableTree) member; if (!variable.getName().toString().equals(canonicalizedLoggerName)) { fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); } } - private static void fixLoggerVariableDeclaration(VisitorState state, Builder fixBuilder) { + private static void fixLoggerVariableDeclaration( + VisitorState state, SuggestedFix.Builder fixBuilder) { for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { if (typeDeclaration instanceof ClassTree) { new TreeScanner<@Nullable Void, Name>() { @@ -109,6 +111,7 @@ private static void fixLoggerVariableDeclaration(VisitorState state, Builder fix return super.visitClass(classTree, classTree.getSimpleName()); } + @Override public @Nullable Void visitMethodInvocation( MethodInvocationTree methodTree, Name className) { if (GET_LOGGER_METHOD.matches(methodTree, state)) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 42ab513ad1..6f6355191c 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -42,7 +42,7 @@ void replacement() { "", " class H {", " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", - " LoggerFactory.getLogger(J.class);", + " LoggerFactory.getLogger(J.class);", " }", "", " class J {}", @@ -60,7 +60,7 @@ void replacement() { " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", " class B {", - " private static final Logger LOG = LoggerFactory.getLogger(B.class);", + " private static final Logger LOG = LoggerFactory.getLogger(B.class);", " }", "", " class C {", From c1156b8407220437141e5df9b31b59ce45c5fc83 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 25 Nov 2023 22:21:16 +0100 Subject: [PATCH 07/23] TIL: Illegal static declaration in inner class --- .../bugpatterns/Slf4jLogDeclaration.java | 6 ++-- .../bugpatterns/Slf4jLogDeclarationTest.java | 28 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 0c78359718..99052b6712 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -115,12 +115,12 @@ private static void fixLoggerVariableDeclaration( public @Nullable Void visitMethodInvocation( MethodInvocationTree methodTree, Name className) { if (GET_LOGGER_METHOD.matches(methodTree, state)) { - ExpressionTree arg1 = methodTree.getArguments().get(0); - String argumentName = SourceCode.treeToString(arg1, state); + ExpressionTree arg = methodTree.getArguments().get(0); + String argumentName = SourceCode.treeToString(arg, state); if (!className.contentEquals(argumentName)) { fixBuilder.merge( - SuggestedFix.replace(arg1, className + JavaFileObject.Kind.CLASS.extension)); + SuggestedFix.replace(arg, className + JavaFileObject.Kind.CLASS.extension)); } } return super.visitMethodInvocation(methodTree, className); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 6f6355191c..2a07f01811 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -16,31 +16,31 @@ void replacement() { "class A {", " Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", "", - " class B {", + " static class B {", " private Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(B.class);", " }", "", - " class C {", + " static class C {", " private static Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(C.class);", " }", "", - " class D {", + " static class D {", " static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(D.class);", " }", "", - " class E {", + " static class E {", " private final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(E.class);", " }", "", - " class F {", + " static class F {", " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(F.class);", " }", "", - " class G {", + " static class G {", " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(G.class);", " }", "", - " class H {", + " static class H {", " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", " LoggerFactory.getLogger(J.class);", " }", @@ -59,31 +59,31 @@ void replacement() { "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", - " class B {", + " static class B {", " private static final Logger LOG = LoggerFactory.getLogger(B.class);", " }", "", - " class C {", + " static class C {", " private static final Logger LOG = LoggerFactory.getLogger(C.class);", " }", "", - " class D {", + " static class D {", " private static final Logger LOG = LoggerFactory.getLogger(D.class);", " }", "", - " class E {", + " static class E {", " private static final Logger LOG = LoggerFactory.getLogger(E.class);", " }", "", - " class F {", + " static class F {", " private static final Logger LOG = LoggerFactory.getLogger(F.class);", " }", "", - " class G {", + " static class G {", " private static final Logger LOG = LoggerFactory.getLogger(G.class);", " }", "", - " class H {", + " static class H {", " private static final Logger LOG = LoggerFactory.getLogger(H.class);", " }", "", From f0fe44fc1d4ee4a2fd92a4aac39c15930632e455 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sun, 3 Mar 2024 11:47:01 +0100 Subject: [PATCH 08/23] Address comments: Add test case for bug checker's flags --- .../bugpatterns/Slf4jLogDeclaration.java | 52 ++++----- .../bugpatterns/Slf4jLogDeclarationTest.java | 106 ++++++++++-------- 2 files changed, 83 insertions(+), 75 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 99052b6712..8aedf779c8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -5,7 +5,8 @@ import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; -import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; +import static javax.tools.JavaFileObject.Kind.CLASS; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; @@ -14,6 +15,7 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFix.Builder; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -27,14 +29,13 @@ import javax.inject.Inject; import javax.lang.model.element.Modifier; import javax.lang.model.element.Name; -import javax.tools.JavaFileObject; import org.jspecify.annotations.Nullable; -import tech.picnic.errorprone.bugpatterns.util.SourceCode; +import tech.picnic.errorprone.utils.SourceCode; /** * A {@link BugChecker} that warns when SLF4J declarations are not canonicalized across the project. * - * @apiNote The default canonicalized logger name can be overriden through {@link ErrorProneFlags + * @apiNote The default canonicalized logger name can be overridden through {@link ErrorProneFlags * flag arguments}. */ @AutoService(BugChecker.class) @@ -82,7 +83,7 @@ public Description matchClass(ClassTree tree, VisitorState state) { canonicalizeLoggerVariable(member, state, fixBuilder); } } - fixLoggerVariableDeclaration(state, fixBuilder); + fixLoggerVariableDeclaration(tree, state, fixBuilder); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } @@ -96,38 +97,33 @@ private static void fixLoggerVariableModifiers( private void canonicalizeLoggerVariable( Tree member, VisitorState state, SuggestedFix.Builder fixBuilder) { VariableTree variable = (VariableTree) member; - if (!variable.getName().toString().equals(canonicalizedLoggerName)) { + if (!variable.getName().contentEquals(canonicalizedLoggerName)) { fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); } } private static void fixLoggerVariableDeclaration( - VisitorState state, SuggestedFix.Builder fixBuilder) { - for (Tree typeDeclaration : state.getPath().getCompilationUnit().getTypeDecls()) { - if (typeDeclaration instanceof ClassTree) { - new TreeScanner<@Nullable Void, Name>() { - @Override - public @Nullable Void visitClass(ClassTree classTree, Name className) { - return super.visitClass(classTree, classTree.getSimpleName()); - } + ClassTree tree, VisitorState state, Builder fixBuilder) { + new TreeScanner<@Nullable Void, Name>() { + @Override + public @Nullable Void visitClass(ClassTree classTree, Name className) { + return super.visitClass(classTree, classTree.getSimpleName()); + } - @Override - public @Nullable Void visitMethodInvocation( - MethodInvocationTree methodTree, Name className) { - if (GET_LOGGER_METHOD.matches(methodTree, state)) { - ExpressionTree arg = methodTree.getArguments().get(0); - String argumentName = SourceCode.treeToString(arg, state); + @Override + public @Nullable Void visitMethodInvocation(MethodInvocationTree methodTree, Name className) { + if (GET_LOGGER_METHOD.matches(methodTree, state)) { + ExpressionTree arg = methodTree.getArguments().get(0); + String argumentName = SourceCode.treeToString(arg, state); - if (!className.contentEquals(argumentName)) { - fixBuilder.merge( - SuggestedFix.replace(arg, className + JavaFileObject.Kind.CLASS.extension)); - } - } - return super.visitMethodInvocation(methodTree, className); + if (!className.contentEquals( + argumentName.substring(0, argumentName.indexOf(CLASS.extension)))) { + fixBuilder.merge(SuggestedFix.replace(arg, className + CLASS.extension)); } - }.scan(typeDeclaration, null); + } + return super.visitMethodInvocation(methodTree, className); } - } + }.scan(tree, tree.getSimpleName()); } private static String getCanonicalizedLoggerName(ErrorProneFlags flags) { diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 2a07f01811..99c1148e12 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -1,56 +1,79 @@ package tech.picnic.errorprone.bugpatterns; +import com.google.common.collect.ImmutableList; import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; final class Slf4jLogDeclarationTest { @Test - void replacement() { - BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .addInputLines( + void identification() { + CompilationTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) + .addSourceLines( "A.java", "import org.slf4j.Logger;", "import org.slf4j.LoggerFactory;", "", + "// BUG: Diagnostic contains: SLF4J", "class A {", - " Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", + " Logger FOO = LoggerFactory.getLogger(A.class);", "", + " // BUG: Diagnostic contains: SLF4J", " static class B {", - " private Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(B.class);", + " private Logger BAR = LoggerFactory.getLogger(B.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class C {", - " private static Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(C.class);", + " private static Logger BAZ = LoggerFactory.getLogger(C.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class D {", - " static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(D.class);", + " static final Logger QUX = LoggerFactory.getLogger(D.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class E {", - " private final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(E.class);", + " private final Logger QUUX = LoggerFactory.getLogger(E.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class F {", - " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(F.class);", + " private static final Logger CORGE = LoggerFactory.getLogger(F.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class G {", - " private static final Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(G.class);", + " private static final Logger GRAPLY = LoggerFactory.getLogger(G.class);", " }", "", + " // BUG: Diagnostic contains: SLF4J", " static class H {", - " private static final Logger LOGGER_WITH_WRONG_CLASS_AS_ARGUMENT =", - " LoggerFactory.getLogger(J.class);", + " private static final Logger WALDO = LoggerFactory.getLogger(J.class);", " }", "", " class J {}", "", + " // BUG: Diagnostic contains: SLF4J", " interface K {", - " Logger NOT_PROPER_LOGGER_NAME = LoggerFactory.getLogger(A.class);", + " Logger FRED = LoggerFactory.getLogger(A.class);", " }", "}") + .doTest(); + } + + @Test + void replacementWithDefaultCanonicalizedLoggerName() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " Logger FOO = LoggerFactory.getLogger(A.class);", + "}") .addOutputLines( "A.java", "import org.slf4j.Logger;", @@ -58,41 +81,30 @@ void replacement() { "", "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "}") + .doTest(); + } + + @Test + void replacementWithOverriddenCanonicalizedLoggerName() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) + .setArgs(ImmutableList.of("-XepOpt:Slf4jLogDeclaration:CanonicalizedLoggerName=BAR")) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", "", - " static class B {", - " private static final Logger LOG = LoggerFactory.getLogger(B.class);", - " }", - "", - " static class C {", - " private static final Logger LOG = LoggerFactory.getLogger(C.class);", - " }", - "", - " static class D {", - " private static final Logger LOG = LoggerFactory.getLogger(D.class);", - " }", - "", - " static class E {", - " private static final Logger LOG = LoggerFactory.getLogger(E.class);", - " }", - "", - " static class F {", - " private static final Logger LOG = LoggerFactory.getLogger(F.class);", - " }", - "", - " static class G {", - " private static final Logger LOG = LoggerFactory.getLogger(G.class);", - " }", - "", - " static class H {", - " private static final Logger LOG = LoggerFactory.getLogger(H.class);", - " }", - "", - " class J {}", + "class A {", + " Logger FOO = LoggerFactory.getLogger(A.class);", + "}") + .addOutputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", "", - " interface K {", - " Logger LOG = LoggerFactory.getLogger(K.class);", - " }", + "class A {", + " private static final Logger BAR = LoggerFactory.getLogger(A.class);", "}") - .doTest(TestMode.TEXT_MATCH); + .doTest(); } } From b68016eeea7bb3bf12a6a29190b8c5b14103bbf4 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sun, 3 Mar 2024 17:31:31 +0100 Subject: [PATCH 09/23] Fix nested class import checkstyle --- .../picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 8aedf779c8..9e62c5ebc0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -15,7 +15,6 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; -import com.google.errorprone.fixes.SuggestedFix.Builder; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; @@ -103,7 +102,7 @@ private void canonicalizeLoggerVariable( } private static void fixLoggerVariableDeclaration( - ClassTree tree, VisitorState state, Builder fixBuilder) { + ClassTree tree, VisitorState state, SuggestedFix.Builder fixBuilder) { new TreeScanner<@Nullable Void, Name>() { @Override public @Nullable Void visitClass(ClassTree classTree, Name className) { From 5f334d062c71653c7b1d26048d179e5155cc4c95 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 21 Mar 2024 15:31:11 +0100 Subject: [PATCH 10/23] Suggestions --- .../bugpatterns/Slf4jLogDeclaration.java | 49 ++++++++++--------- .../bugpatterns/Slf4jLogDeclarationTest.java | 24 ++++----- 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 9e62c5ebc0..38d547d2d0 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -47,13 +47,13 @@ public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); - private static final String CANONICALIZED_LOGGER_NAME_FLAG = - "Slf4jLogDeclaration:CanonicalizedLoggerName"; - private static final String DEFAULT_CANONICALIZED_LOGGER_NAME = "LOG"; + private static final String CANONICAL_LOGGER_NAME_FLAG = + "Slf4jLogDeclaration:CanonicalLoggerName"; + private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; private static final Matcher GET_LOGGER_METHOD = staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); - private final String canonicalizedLoggerName; + private final String canonicalLoggerName; /** Instantiates a default {@link Slf4jLogDeclaration} instance. */ public Slf4jLogDeclaration() { @@ -67,7 +67,7 @@ public Slf4jLogDeclaration() { */ @Inject Slf4jLogDeclaration(ErrorProneFlags flags) { - canonicalizedLoggerName = getCanonicalizedLoggerName(flags); + canonicalLoggerName = getCanonicalLoggerName(flags); } @Override @@ -77,32 +77,33 @@ public Description matchClass(ClassTree tree, VisitorState state) { for (Tree member : tree.getMembers()) { if (LOGGER.matches(member, state)) { if (tree.getKind() != Kind.INTERFACE) { - fixLoggerVariableModifiers(member, state, fixBuilder); + suggestCanonicalModifiers(member, fixBuilder, state); } - canonicalizeLoggerVariable(member, state, fixBuilder); + canonicalizeLoggerVariable((VariableTree) member, fixBuilder, state); } } - fixLoggerVariableDeclaration(tree, state, fixBuilder); + fixLoggerVariableDeclarations(tree, fixBuilder, state); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } - private static void fixLoggerVariableModifiers( - Tree member, VisitorState state, SuggestedFix.Builder fixBuilder) { + private void suggestCanonicalModifiers( + Tree member, SuggestedFix.Builder fixBuilder, VisitorState state) { SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) .ifPresent(fixBuilder::merge); } private void canonicalizeLoggerVariable( - Tree member, VisitorState state, SuggestedFix.Builder fixBuilder) { - VariableTree variable = (VariableTree) member; - if (!variable.getName().contentEquals(canonicalizedLoggerName)) { - fixBuilder.merge(SuggestedFixes.renameVariable(variable, canonicalizedLoggerName, state)); + VariableTree variableTree, SuggestedFix.Builder fixBuilder, VisitorState state) { + if (!variableTree.getName().contentEquals(canonicalLoggerName)) { + fixBuilder + .merge(SuggestedFixes.renameVariable(variableTree, canonicalLoggerName, state)) + .build(); } } - private static void fixLoggerVariableDeclaration( - ClassTree tree, VisitorState state, SuggestedFix.Builder fixBuilder) { + private static void fixLoggerVariableDeclarations( + ClassTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { new TreeScanner<@Nullable Void, Name>() { @Override public @Nullable Void visitClass(ClassTree classTree, Name className) { @@ -110,22 +111,22 @@ private static void fixLoggerVariableDeclaration( } @Override - public @Nullable Void visitMethodInvocation(MethodInvocationTree methodTree, Name className) { - if (GET_LOGGER_METHOD.matches(methodTree, state)) { - ExpressionTree arg = methodTree.getArguments().get(0); + public @Nullable Void visitMethodInvocation(MethodInvocationTree tree, Name className) { + if (GET_LOGGER_METHOD.matches(tree, state)) { + ExpressionTree arg = tree.getArguments().get(0); String argumentName = SourceCode.treeToString(arg, state); - if (!className.contentEquals( - argumentName.substring(0, argumentName.indexOf(CLASS.extension)))) { + String findAName = argumentName.substring(0, argumentName.indexOf(CLASS.extension)); + if (!className.contentEquals(findAName)) { fixBuilder.merge(SuggestedFix.replace(arg, className + CLASS.extension)); } } - return super.visitMethodInvocation(methodTree, className); + return super.visitMethodInvocation(tree, className); } }.scan(tree, tree.getSimpleName()); } - private static String getCanonicalizedLoggerName(ErrorProneFlags flags) { - return flags.get(CANONICALIZED_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICALIZED_LOGGER_NAME); + private static String getCanonicalLoggerName(ErrorProneFlags flags) { + return flags.get(CANONICAL_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 99c1148e12..695291c85f 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -14,48 +14,48 @@ void identification() { "import org.slf4j.Logger;", "import org.slf4j.LoggerFactory;", "", - "// BUG: Diagnostic contains: SLF4J", + "// BUG: Diagnostic contains:", "class A {", " Logger FOO = LoggerFactory.getLogger(A.class);", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class B {", " private Logger BAR = LoggerFactory.getLogger(B.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class C {", " private static Logger BAZ = LoggerFactory.getLogger(C.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class D {", " static final Logger QUX = LoggerFactory.getLogger(D.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class E {", " private final Logger QUUX = LoggerFactory.getLogger(E.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class F {", " private static final Logger CORGE = LoggerFactory.getLogger(F.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class G {", " private static final Logger GRAPLY = LoggerFactory.getLogger(G.class);", " }", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " static class H {", " private static final Logger WALDO = LoggerFactory.getLogger(J.class);", " }", "", " class J {}", "", - " // BUG: Diagnostic contains: SLF4J", + " // BUG: Diagnostic contains:", " interface K {", " Logger FRED = LoggerFactory.getLogger(A.class);", " }", @@ -64,7 +64,7 @@ void identification() { } @Test - void replacementWithDefaultCanonicalizedLoggerName() { + void replacementWithDefaultCanonicalLoggerName() { BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) .addInputLines( "A.java", @@ -86,9 +86,9 @@ void replacementWithDefaultCanonicalizedLoggerName() { } @Test - void replacementWithOverriddenCanonicalizedLoggerName() { + void replacementWithOverriddenCanonicalLoggerName() { BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .setArgs(ImmutableList.of("-XepOpt:Slf4jLogDeclaration:CanonicalizedLoggerName=BAR")) + .setArgs(ImmutableList.of("-XepOpt:Slf4jLogDeclaration:CanonicalLoggerName=BAR")) .addInputLines( "A.java", "import org.slf4j.Logger;", From d3e471853a4e1c795e618d8f14f35440d7a78cc0 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 21 Mar 2024 15:54:52 +0100 Subject: [PATCH 11/23] Suggestions part 2 --- .../bugpatterns/Slf4jLogDeclaration.java | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 38d547d2d0..8728556225 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -2,7 +2,7 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.BugPattern.StandardTags.LIKELY_ERROR; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; import static javax.tools.JavaFileObject.Kind.CLASS; @@ -32,10 +32,10 @@ import tech.picnic.errorprone.utils.SourceCode; /** - * A {@link BugChecker} that warns when SLF4J declarations are not canonicalized across the project. + * A {@link BugChecker} that warns when SLF4J declarations are not canonical. * - * @apiNote The default canonicalized logger name can be overridden through {@link ErrorProneFlags - * flag arguments}. + * @apiNote The default canonical logger name can be overridden through {@link ErrorProneFlags flag + * arguments}. */ @AutoService(BugChecker.class) @BugPattern( @@ -43,7 +43,7 @@ link = BUG_PATTERNS_BASE_URL + "Slf4jLogDeclaration", linkType = CUSTOM, severity = WARNING, - tags = LIKELY_ERROR) + tags = STYLE) public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); @@ -87,12 +87,6 @@ public Description matchClass(ClassTree tree, VisitorState state) { return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } - private void suggestCanonicalModifiers( - Tree member, SuggestedFix.Builder fixBuilder, VisitorState state) { - SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) - .ifPresent(fixBuilder::merge); - } - private void canonicalizeLoggerVariable( VariableTree variableTree, SuggestedFix.Builder fixBuilder, VisitorState state) { if (!variableTree.getName().contentEquals(canonicalLoggerName)) { @@ -126,6 +120,12 @@ private static void fixLoggerVariableDeclarations( }.scan(tree, tree.getSimpleName()); } + private static void suggestCanonicalModifiers( + Tree member, SuggestedFix.Builder fixBuilder, VisitorState state) { + SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + .ifPresent(fixBuilder::merge); + } + private static String getCanonicalLoggerName(ErrorProneFlags flags) { return flags.get(CANONICAL_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME); } From 27e8544fbad7e1286d7f4b715962dcf43e983b6f Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 21 Mar 2024 17:06:05 +0100 Subject: [PATCH 12/23] Suggestions part 3 --- .../bugpatterns/Slf4jLogDeclaration.java | 18 +++---- .../bugpatterns/Slf4jLogDeclarationTest.java | 49 ++++++++++--------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 8728556225..4a11911b30 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -9,6 +9,7 @@ import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.collect.Iterables; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; @@ -82,7 +83,7 @@ public Description matchClass(ClassTree tree, VisitorState state) { canonicalizeLoggerVariable((VariableTree) member, fixBuilder, state); } } - fixLoggerVariableDeclarations(tree, fixBuilder, state); + updateLoggerVariableDeclarations(tree, fixBuilder, state); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } @@ -90,13 +91,11 @@ public Description matchClass(ClassTree tree, VisitorState state) { private void canonicalizeLoggerVariable( VariableTree variableTree, SuggestedFix.Builder fixBuilder, VisitorState state) { if (!variableTree.getName().contentEquals(canonicalLoggerName)) { - fixBuilder - .merge(SuggestedFixes.renameVariable(variableTree, canonicalLoggerName, state)) - .build(); + fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, canonicalLoggerName, state)); } } - private static void fixLoggerVariableDeclarations( + private static void updateLoggerVariableDeclarations( ClassTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { new TreeScanner<@Nullable Void, Name>() { @Override @@ -107,11 +106,12 @@ private static void fixLoggerVariableDeclarations( @Override public @Nullable Void visitMethodInvocation(MethodInvocationTree tree, Name className) { if (GET_LOGGER_METHOD.matches(tree, state)) { - ExpressionTree arg = tree.getArguments().get(0); - String argumentName = SourceCode.treeToString(arg, state); + ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); + String argumentSource = SourceCode.treeToString(arg, state); - String findAName = argumentName.substring(0, argumentName.indexOf(CLASS.extension)); - if (!className.contentEquals(findAName)) { + String argumentClassName = + argumentSource.substring(0, argumentSource.indexOf(CLASS.extension)); + if (!className.contentEquals(argumentClassName)) { fixBuilder.merge(SuggestedFix.replace(arg, className + CLASS.extension)); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 695291c85f..11f65aa264 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -2,6 +2,7 @@ import com.google.common.collect.ImmutableList; import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; @@ -14,50 +15,44 @@ void identification() { "import org.slf4j.Logger;", "import org.slf4j.LoggerFactory;", "", + // XXX: This one should not be here... "// BUG: Diagnostic contains:", "class A {", - " Logger FOO = LoggerFactory.getLogger(A.class);", - "", - " // BUG: Diagnostic contains:", - " static class B {", - " private Logger BAR = LoggerFactory.getLogger(B.class);", - " }", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", " // BUG: Diagnostic contains:", - " static class C {", - " private static Logger BAZ = LoggerFactory.getLogger(C.class);", + " static class VariableMissingStaticFinal {", + " private Logger LOG = LoggerFactory.getLogger(VariableMissingStaticFinal.class);", " }", "", " // BUG: Diagnostic contains:", - " static class D {", - " static final Logger QUX = LoggerFactory.getLogger(D.class);", + " static class VariableMissingFinal {", + " private static Logger LOG = LoggerFactory.getLogger(VariableMissingFinal.class);", " }", "", " // BUG: Diagnostic contains:", - " static class E {", - " private final Logger QUUX = LoggerFactory.getLogger(E.class);", + " static class VariableMissingPrivate {", + " static final Logger LOG = LoggerFactory.getLogger(VariableMissingPrivate.class);", " }", "", " // BUG: Diagnostic contains:", - " static class F {", - " private static final Logger CORGE = LoggerFactory.getLogger(F.class);", + " static class VariableMissingStatic {", + " private final Logger LOG = LoggerFactory.getLogger(VariableMissingStatic.class);", " }", "", " // BUG: Diagnostic contains:", - " static class G {", - " private static final Logger GRAPLY = LoggerFactory.getLogger(G.class);", + " static class WrongVariableName {", + " private static final Logger GRAPLY = LoggerFactory.getLogger(WrongVariableName.class);", " }", "", " // BUG: Diagnostic contains:", - " static class H {", - " private static final Logger WALDO = LoggerFactory.getLogger(J.class);", + " static class WrongArgumentGetLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(String.class);", " }", "", - " class J {}", - "", " // BUG: Diagnostic contains:", " interface K {", - " Logger FRED = LoggerFactory.getLogger(A.class);", + " Logger FOO = LoggerFactory.getLogger(A.class);", " }", "}") .doTest(); @@ -73,6 +68,10 @@ void replacementWithDefaultCanonicalLoggerName() { "", "class A {", " Logger FOO = LoggerFactory.getLogger(A.class);", + "", + " static class WrongArgumentGetLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(String.class);", + " }", "}") .addOutputLines( "A.java", @@ -81,8 +80,12 @@ void replacementWithDefaultCanonicalLoggerName() { "", "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " static class WrongArgumentGetLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(WrongArgumentGetLogger.class);", + " }", "}") - .doTest(); + .doTest(TestMode.TEXT_MATCH); } @Test @@ -105,6 +108,6 @@ void replacementWithOverriddenCanonicalLoggerName() { "class A {", " private static final Logger BAR = LoggerFactory.getLogger(A.class);", "}") - .doTest(); + .doTest(TestMode.TEXT_MATCH); } } From c2ed5e2e8055c017176975f7baaff5070e5b9bc8 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 5 Apr 2024 14:51:42 +0200 Subject: [PATCH 13/23] Pair programming --- .../errorprone/bugpatterns/Slf4jLogDeclaration.java | 1 + .../bugpatterns/Slf4jLogDeclarationTest.java | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 4a11911b30..18cc7ce840 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -45,6 +45,7 @@ linkType = CUSTOM, severity = WARNING, tags = STYLE) +// XXX: Do not match on `Class` but on `VariableTree`. That improves on the reporting. public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 11f65aa264..c776d29790 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -7,6 +7,7 @@ import org.junit.jupiter.api.Test; final class Slf4jLogDeclarationTest { + // XXX: Add testcase that we flag lowercase logger names. @Test void identification() { CompilationTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) @@ -20,6 +21,10 @@ void identification() { "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", + " void m() {", + " LOG.trace(\"foo\");", + " }", + "", " // BUG: Diagnostic contains:", " static class VariableMissingStaticFinal {", " private Logger LOG = LoggerFactory.getLogger(VariableMissingStaticFinal.class);", @@ -58,6 +63,11 @@ void identification() { .doTest(); } + // XXX: Introduce test case where we actually use the incorrect log name and check that it is + // correctly renamed. So e.g. a usage of `LOG` in normal a method, but also nested class. + // And in an interface, if you have a default method that uses the incorrect `log` method, is it + // also renamed. + // XXX: This will kill some of the mutants, and take a look to the remaining ones. @Test void replacementWithDefaultCanonicalLoggerName() { BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) @@ -67,7 +77,7 @@ void replacementWithDefaultCanonicalLoggerName() { "import org.slf4j.LoggerFactory;", "", "class A {", - " Logger FOO = LoggerFactory.getLogger(A.class);", + " Logger foo = LoggerFactory.getLogger(A.class);", "", " static class WrongArgumentGetLogger {", " private static final Logger LOG = LoggerFactory.getLogger(String.class);", From 92fc7e7a3b294059e81e385511f63339549e837a Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 6 Apr 2024 19:00:32 +0200 Subject: [PATCH 14/23] Comments --- .../bugpatterns/Slf4jLogDeclaration.java | 65 ++++++++------- .../bugpatterns/Slf4jLogDeclarationTest.java | 80 +++++++++++++------ 2 files changed, 91 insertions(+), 54 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 18cc7ce840..713ab406b5 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -14,21 +14,22 @@ import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; 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.sun.source.tree.ClassTree; +import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; -import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.util.Name; import javax.inject.Inject; +import javax.lang.model.element.ElementKind; import javax.lang.model.element.Modifier; -import javax.lang.model.element.Name; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.utils.SourceCode; @@ -46,14 +47,14 @@ severity = WARNING, tags = STYLE) // XXX: Do not match on `Class` but on `VariableTree`. That improves on the reporting. -public final class Slf4jLogDeclaration extends BugChecker implements ClassTreeMatcher { +public final class Slf4jLogDeclaration extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); + private static final Matcher GET_LOGGER_METHOD = + staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); private static final String CANONICAL_LOGGER_NAME_FLAG = "Slf4jLogDeclaration:CanonicalLoggerName"; private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; - private static final Matcher GET_LOGGER_METHOD = - staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); private final String canonicalLoggerName; @@ -73,18 +74,23 @@ public Slf4jLogDeclaration() { } @Override - public Description matchClass(ClassTree tree, VisitorState state) { + public Description matchVariable(VariableTree tree, VisitorState state) { + if (!LOGGER.matches(tree, state)) { + return Description.NO_MATCH; + } + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - for (Tree member : tree.getMembers()) { - if (LOGGER.matches(member, state)) { - if (tree.getKind() != Kind.INTERFACE) { - suggestCanonicalModifiers(member, fixBuilder, state); - } - canonicalizeLoggerVariable((VariableTree) member, fixBuilder, state); - } + Symbol enclosingElement = ASTHelpers.getSymbol(tree).getEnclosingElement(); + if (enclosingElement == null) { + return Description.NO_MATCH; + } + + if (enclosingElement.getKind() != ElementKind.INTERFACE) { + suggestCanonicalModifiers(tree, fixBuilder, state); } - updateLoggerVariableDeclarations(tree, fixBuilder, state); + canonicalizeLoggerVariable(tree, fixBuilder, state); + updateGetLoggerArgument(tree, enclosingElement.getSimpleName(), fixBuilder, state); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } @@ -96,29 +102,28 @@ private void canonicalizeLoggerVariable( } } - private static void updateLoggerVariableDeclarations( - ClassTree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { + private static void updateGetLoggerArgument( + VariableTree tree, + Name enclosingElementName, + SuggestedFix.Builder fixBuilder, + VisitorState state) { new TreeScanner<@Nullable Void, Name>() { @Override - public @Nullable Void visitClass(ClassTree classTree, Name className) { - return super.visitClass(classTree, classTree.getSimpleName()); - } - - @Override - public @Nullable Void visitMethodInvocation(MethodInvocationTree tree, Name className) { + public @Nullable Void visitMethodInvocation( + MethodInvocationTree tree, Name enclosingElementName) { if (GET_LOGGER_METHOD.matches(tree, state)) { ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); - String argumentSource = SourceCode.treeToString(arg, state); + String argumentName = SourceCode.treeToString(arg, state); String argumentClassName = - argumentSource.substring(0, argumentSource.indexOf(CLASS.extension)); - if (!className.contentEquals(argumentClassName)) { - fixBuilder.merge(SuggestedFix.replace(arg, className + CLASS.extension)); + argumentName.substring(0, argumentName.indexOf(CLASS.extension)); + if (!enclosingElementName.contentEquals(argumentClassName)) { + fixBuilder.merge(SuggestedFix.replace(arg, enclosingElementName + CLASS.extension)); } } - return super.visitMethodInvocation(tree, className); + return super.visitMethodInvocation(tree, enclosingElementName); } - }.scan(tree, tree.getSimpleName()); + }.scan(tree, enclosingElementName); } private static void suggestCanonicalModifiers( diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index c776d29790..11dbeb5845 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -7,7 +7,6 @@ import org.junit.jupiter.api.Test; final class Slf4jLogDeclarationTest { - // XXX: Add testcase that we flag lowercase logger names. @Test void identification() { CompilationTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) @@ -16,60 +15,61 @@ void identification() { "import org.slf4j.Logger;", "import org.slf4j.LoggerFactory;", "", - // XXX: This one should not be here... - "// BUG: Diagnostic contains:", "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", - " void m() {", - " LOG.trace(\"foo\");", - " }", - "", " // BUG: Diagnostic contains:", + " private static final Logger foo = LoggerFactory.getLogger(A.class);", + "", " static class VariableMissingStaticFinal {", + " // BUG: Diagnostic contains:", " private Logger LOG = LoggerFactory.getLogger(VariableMissingStaticFinal.class);", " }", "", - " // BUG: Diagnostic contains:", " static class VariableMissingFinal {", + " // BUG: Diagnostic contains:", " private static Logger LOG = LoggerFactory.getLogger(VariableMissingFinal.class);", " }", "", - " // BUG: Diagnostic contains:", " static class VariableMissingPrivate {", + " // BUG: Diagnostic contains:", " static final Logger LOG = LoggerFactory.getLogger(VariableMissingPrivate.class);", " }", "", - " // BUG: Diagnostic contains:", " static class VariableMissingStatic {", + " // BUG: Diagnostic contains:", " private final Logger LOG = LoggerFactory.getLogger(VariableMissingStatic.class);", " }", "", - " // BUG: Diagnostic contains:", " static class WrongVariableName {", - " private static final Logger GRAPLY = LoggerFactory.getLogger(WrongVariableName.class);", + " // BUG: Diagnostic contains:", + " private static final Logger BAR = LoggerFactory.getLogger(WrongVariableName.class);", " }", "", - " // BUG: Diagnostic contains:", " static class WrongArgumentGetLogger {", + " // BUG: Diagnostic contains:", " private static final Logger LOG = LoggerFactory.getLogger(String.class);", " }", "", - " // BUG: Diagnostic contains:", - " interface K {", - " Logger FOO = LoggerFactory.getLogger(A.class);", + " interface InterfaceWithNoCanonicalModifiers {", + " Logger LOG = LoggerFactory.getLogger(InterfaceWithNoCanonicalModifiers.class);", + " }", + "", + " interface InterfaceWithWrongVariableName {", + " // BUG: Diagnostic contains:", + " Logger BAZ = LoggerFactory.getLogger(InterfaceWithWrongVariableName.class);", + " }", + "", + " interface WrongArgumentGetLoggerInterface {", + " // BUG: Diagnostic contains:", + " Logger LOG = LoggerFactory.getLogger(A.class);", " }", "}") .doTest(); } - // XXX: Introduce test case where we actually use the incorrect log name and check that it is - // correctly renamed. So e.g. a usage of `LOG` in normal a method, but also nested class. - // And in an interface, if you have a default method that uses the incorrect `log` method, is it - // also renamed. - // XXX: This will kill some of the mutants, and take a look to the remaining ones. @Test - void replacementWithDefaultCanonicalLoggerName() { + void replacement() { BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) .addInputLines( "A.java", @@ -77,11 +77,27 @@ void replacementWithDefaultCanonicalLoggerName() { "import org.slf4j.LoggerFactory;", "", "class A {", - " Logger foo = LoggerFactory.getLogger(A.class);", + " static Logger foo = LoggerFactory.getLogger(A.class);", + "", + " void m() {", + " foo.trace(\"foo\");", + " }", + "", + " static class NestedClass {", + " void m() {", + " foo.trace(\"foo\");", + " }", + " }", "", " static class WrongArgumentGetLogger {", " private static final Logger LOG = LoggerFactory.getLogger(String.class);", " }", + "", + " interface InterfaceWithDefaultMethod {", + " default void m() {", + " foo.trace(\"foo\");", + " }", + " }", "}") .addOutputLines( "A.java", @@ -91,9 +107,25 @@ void replacementWithDefaultCanonicalLoggerName() { "class A {", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", + " void m() {", + " LOG.trace(\"foo\");", + " }", + "", + " static class NestedClass {", + " void m() {", + " LOG.trace(\"foo\");", + " }", + " }", + "", " static class WrongArgumentGetLogger {", " private static final Logger LOG = LoggerFactory.getLogger(WrongArgumentGetLogger.class);", " }", + "", + " interface InterfaceWithDefaultMethod {", + " default void m() {", + " LOG.trace(\"foo\");", + " }", + " }", "}") .doTest(TestMode.TEXT_MATCH); } @@ -108,7 +140,7 @@ void replacementWithOverriddenCanonicalLoggerName() { "import org.slf4j.LoggerFactory;", "", "class A {", - " Logger FOO = LoggerFactory.getLogger(A.class);", + " Logger LOG = LoggerFactory.getLogger(A.class);", "}") .addOutputLines( "A.java", From 10c73687d8e0f1096a763119ebc8030bcb911927 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 6 Apr 2024 19:32:17 +0200 Subject: [PATCH 15/23] Kill mutants & handle getLogger overload --- .../bugpatterns/Slf4jLogDeclaration.java | 24 +++++++++++++++---- .../bugpatterns/Slf4jLogDeclarationTest.java | 21 ++++++++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 713ab406b5..c4811ec6fe 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -1,11 +1,11 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.STYLE; import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; -import static javax.tools.JavaFileObject.Kind.CLASS; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -23,10 +23,12 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.util.Name; +import java.util.regex.Pattern; import javax.inject.Inject; import javax.lang.model.element.ElementKind; import javax.lang.model.element.Modifier; @@ -52,9 +54,12 @@ public final class Slf4jLogDeclaration extends BugChecker implements VariableTre private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); private static final Matcher GET_LOGGER_METHOD = staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); + private static final String CANONICAL_LOGGER_NAME_FLAG = "Slf4jLogDeclaration:CanonicalLoggerName"; private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; + private static final Pattern STRING_LITERAL_ARGUMENT = Pattern.compile("\"(.*?)\""); + private static final Pattern CLASS_ARGUMENT = Pattern.compile("(.*?)\\.class"); private final String canonicalLoggerName; @@ -115,10 +120,21 @@ private static void updateGetLoggerArgument( ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); String argumentName = SourceCode.treeToString(arg, state); - String argumentClassName = - argumentName.substring(0, argumentName.indexOf(CLASS.extension)); + String argumentClassName; + if (arg.getKind() == Kind.STRING_LITERAL) { + java.util.regex.Matcher matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName); + checkArgument(matcher.matches(), "Invalid argument name."); + argumentClassName = matcher.group(1); + } else { + java.util.regex.Matcher matcher = CLASS_ARGUMENT.matcher(argumentName); + checkArgument(matcher.matches(), "Invalid argument name."); + argumentClassName = matcher.group(1); + } + if (!enclosingElementName.contentEquals(argumentClassName)) { - fixBuilder.merge(SuggestedFix.replace(arg, enclosingElementName + CLASS.extension)); + fixBuilder.merge( + SuggestedFix.replace( + arg, argumentName.replace(argumentClassName, enclosingElementName))); } } return super.visitMethodInvocation(tree, enclosingElementName); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 11dbeb5845..1037164864 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -16,11 +16,22 @@ void identification() { "import org.slf4j.LoggerFactory;", "", "class A {", + " private static final long serialVersionUID = 1L;", + "", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", " // BUG: Diagnostic contains:", " private static final Logger foo = LoggerFactory.getLogger(A.class);", "", + " static class OverloadedGetLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", + " }", + "", + " static class WrongOverloadedGetLogger {", + " // BUG: Diagnostic contains:", + " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", + " }", + "", " static class VariableMissingStaticFinal {", " // BUG: Diagnostic contains:", " private Logger LOG = LoggerFactory.getLogger(VariableMissingStaticFinal.class);", @@ -93,6 +104,11 @@ void replacement() { " private static final Logger LOG = LoggerFactory.getLogger(String.class);", " }", "", + " static class WrongArgumentOverloadedGetLogger {", + " // BUG: Diagnostic contains:", + " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", + " }", + "", " interface InterfaceWithDefaultMethod {", " default void m() {", " foo.trace(\"foo\");", @@ -121,6 +137,11 @@ void replacement() { " private static final Logger LOG = LoggerFactory.getLogger(WrongArgumentGetLogger.class);", " }", "", + " static class WrongArgumentOverloadedGetLogger {", + " // BUG: Diagnostic contains:", + " private static final Logger LOG = LoggerFactory.getLogger(\"WrongArgumentOverloadedGetLogger\");", + " }", + "", " interface InterfaceWithDefaultMethod {", " default void m() {", " LOG.trace(\"foo\");", From e037073530f5bbd8ff5573ab686cdf71324a70b0 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 6 Apr 2024 20:56:07 +0200 Subject: [PATCH 16/23] Some mutants are just meant to live on among us... --- .../picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index c4811ec6fe..586049f76e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -84,13 +84,13 @@ public Description matchVariable(VariableTree tree, VisitorState state) { return Description.NO_MATCH; } - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - Symbol enclosingElement = ASTHelpers.getSymbol(tree).getEnclosingElement(); if (enclosingElement == null) { return Description.NO_MATCH; } + SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); + if (enclosingElement.getKind() != ElementKind.INTERFACE) { suggestCanonicalModifiers(tree, fixBuilder, state); } @@ -137,7 +137,7 @@ private static void updateGetLoggerArgument( arg, argumentName.replace(argumentClassName, enclosingElementName))); } } - return super.visitMethodInvocation(tree, enclosingElementName); + return null; } }.scan(tree, enclosingElementName); } From e2c62a70503a14fb257f737aefb3c3606db7a6b3 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Wed, 17 Apr 2024 08:43:05 +0200 Subject: [PATCH 17/23] Suggestions --- .../bugpatterns/Slf4jLogDeclaration.java | 44 ++++++++----------- .../bugpatterns/Slf4jLogDeclarationTest.java | 2 + 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 586049f76e..7fe4ba9739 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -4,7 +4,6 @@ import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.STYLE; -import static com.google.errorprone.matchers.Matchers.isSubtypeOf; import static com.google.errorprone.matchers.Matchers.staticMethod; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; @@ -19,19 +18,17 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; -import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.source.util.TreeScanner; -import com.sun.tools.javac.code.Symbol; -import com.sun.tools.javac.util.Name; import java.util.regex.Pattern; import javax.inject.Inject; -import javax.lang.model.element.ElementKind; import javax.lang.model.element.Modifier; +import javax.lang.model.element.Name; import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.utils.SourceCode; @@ -48,18 +45,15 @@ linkType = CUSTOM, severity = WARNING, tags = STYLE) -// XXX: Do not match on `Class` but on `VariableTree`. That improves on the reporting. public final class Slf4jLogDeclaration extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; - private static final Matcher LOGGER = isSubtypeOf("org.slf4j.Logger"); private static final Matcher GET_LOGGER_METHOD = staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); - private static final String CANONICAL_LOGGER_NAME_FLAG = "Slf4jLogDeclaration:CanonicalLoggerName"; private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; - private static final Pattern STRING_LITERAL_ARGUMENT = Pattern.compile("\"(.*?)\""); - private static final Pattern CLASS_ARGUMENT = Pattern.compile("(.*?)\\.class"); + private static final Pattern STRING_LITERAL_ARGUMENT_PATTERN = Pattern.compile("\"(.*?)\""); + private static final Pattern CLASS_ARGUMENT_PATTERN = Pattern.compile("(.*?)\\.class"); private final String canonicalLoggerName; @@ -80,27 +74,27 @@ public Slf4jLogDeclaration() { @Override public Description matchVariable(VariableTree tree, VisitorState state) { - if (!LOGGER.matches(tree, state)) { + if (!GET_LOGGER_METHOD.matches(tree.getInitializer(), state)) { return Description.NO_MATCH; } - Symbol enclosingElement = ASTHelpers.getSymbol(tree).getEnclosingElement(); - if (enclosingElement == null) { + ClassTree clazz = state.findEnclosing(ClassTree.class); + if (clazz == null) { return Description.NO_MATCH; } SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - if (enclosingElement.getKind() != ElementKind.INTERFACE) { + if (clazz.getKind() != Kind.INTERFACE) { suggestCanonicalModifiers(tree, fixBuilder, state); } - canonicalizeLoggerVariable(tree, fixBuilder, state); - updateGetLoggerArgument(tree, enclosingElement.getSimpleName(), fixBuilder, state); + canonicalizeLoggerVariableName(tree, fixBuilder, state); + updateGetLoggerArgument(tree, clazz.getSimpleName(), fixBuilder, state); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } - private void canonicalizeLoggerVariable( + private void canonicalizeLoggerVariableName( VariableTree variableTree, SuggestedFix.Builder fixBuilder, VisitorState state) { if (!variableTree.getName().contentEquals(canonicalLoggerName)) { fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, canonicalLoggerName, state)); @@ -120,17 +114,15 @@ private static void updateGetLoggerArgument( ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); String argumentName = SourceCode.treeToString(arg, state); - String argumentClassName; + java.util.regex.Matcher matcher; if (arg.getKind() == Kind.STRING_LITERAL) { - java.util.regex.Matcher matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName); - checkArgument(matcher.matches(), "Invalid argument name."); - argumentClassName = matcher.group(1); + matcher = STRING_LITERAL_ARGUMENT_PATTERN.matcher(argumentName); } else { - java.util.regex.Matcher matcher = CLASS_ARGUMENT.matcher(argumentName); - checkArgument(matcher.matches(), "Invalid argument name."); - argumentClassName = matcher.group(1); + matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); } + checkArgument(matcher.matches(), "Invalid argument name."); + String argumentClassName = matcher.group(1); if (!enclosingElementName.contentEquals(argumentClassName)) { fixBuilder.merge( SuggestedFix.replace( @@ -143,8 +135,8 @@ private static void updateGetLoggerArgument( } private static void suggestCanonicalModifiers( - Tree member, SuggestedFix.Builder fixBuilder, VisitorState state) { - SuggestedFixes.addModifiers(member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) + Tree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { + SuggestedFixes.addModifiers(tree, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) .ifPresent(fixBuilder::merge); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 1037164864..4baf9b39ea 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -91,6 +91,7 @@ void replacement() { " static Logger foo = LoggerFactory.getLogger(A.class);", "", " void m() {", + " String unused = toString();", " foo.trace(\"foo\");", " }", "", @@ -124,6 +125,7 @@ void replacement() { " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", " void m() {", + " String unused = toString();", " LOG.trace(\"foo\");", " }", "", From b58cfd8a72df0960f752dadf550bd3c6e8d4b87c Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Fri, 19 Apr 2024 18:04:35 +0200 Subject: [PATCH 18/23] Simplify some code, forgot to push --- .../bugpatterns/Slf4jLogDeclaration.java | 52 +++++++------------ 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 7fe4ba9739..6efcbab7b1 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -24,12 +24,10 @@ import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; -import com.sun.source.util.TreeScanner; import java.util.regex.Pattern; import javax.inject.Inject; import javax.lang.model.element.Modifier; import javax.lang.model.element.Name; -import org.jspecify.annotations.Nullable; import tech.picnic.errorprone.utils.SourceCode; /** @@ -84,12 +82,12 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - if (clazz.getKind() != Kind.INTERFACE) { suggestCanonicalModifiers(tree, fixBuilder, state); } canonicalizeLoggerVariableName(tree, fixBuilder, state); - updateGetLoggerArgument(tree, clazz.getSimpleName(), fixBuilder, state); + updateGetLoggerArgument( + (MethodInvocationTree) tree.getInitializer(), clazz.getSimpleName(), fixBuilder, state); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } @@ -102,36 +100,26 @@ private void canonicalizeLoggerVariableName( } private static void updateGetLoggerArgument( - VariableTree tree, - Name enclosingElementName, + MethodInvocationTree tree, + Name className, SuggestedFix.Builder fixBuilder, VisitorState state) { - new TreeScanner<@Nullable Void, Name>() { - @Override - public @Nullable Void visitMethodInvocation( - MethodInvocationTree tree, Name enclosingElementName) { - if (GET_LOGGER_METHOD.matches(tree, state)) { - ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); - String argumentName = SourceCode.treeToString(arg, state); - - java.util.regex.Matcher matcher; - if (arg.getKind() == Kind.STRING_LITERAL) { - matcher = STRING_LITERAL_ARGUMENT_PATTERN.matcher(argumentName); - } else { - matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); - } - - checkArgument(matcher.matches(), "Invalid argument name."); - String argumentClassName = matcher.group(1); - if (!enclosingElementName.contentEquals(argumentClassName)) { - fixBuilder.merge( - SuggestedFix.replace( - arg, argumentName.replace(argumentClassName, enclosingElementName))); - } - } - return null; - } - }.scan(tree, enclosingElementName); + ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); + String argumentName = SourceCode.treeToString(arg, state); + + java.util.regex.Matcher matcher; + if (arg.getKind() == Kind.STRING_LITERAL) { + matcher = STRING_LITERAL_ARGUMENT_PATTERN.matcher(argumentName); + } else { + matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); + } + + checkArgument(matcher.matches(), "Invalid argument name."); + String argumentClassName = matcher.group(1); + if (!className.contentEquals(argumentClassName)) { + fixBuilder.merge( + SuggestedFix.replace(arg, argumentName.replace(argumentClassName, className))); + } } private static void suggestCanonicalModifiers( From 7e1a372ff1c103af9b7c863ee04e451b8922bf10 Mon Sep 17 00:00:00 2001 From: Rick Ossendrijver Date: Thu, 2 May 2024 09:16:14 +0200 Subject: [PATCH 19/23] Suggestions --- .../bugpatterns/Slf4jLogDeclaration.java | 4 ++-- .../bugpatterns/Slf4jLogDeclarationTest.java | 16 +++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java index 6efcbab7b1..7c57fdf5dc 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java @@ -1,6 +1,6 @@ package tech.picnic.errorprone.bugpatterns; -import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkState; import static com.google.errorprone.BugPattern.LinkType.CUSTOM; import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; import static com.google.errorprone.BugPattern.StandardTags.STYLE; @@ -114,7 +114,7 @@ private static void updateGetLoggerArgument( matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); } - checkArgument(matcher.matches(), "Invalid argument name."); + checkState(matcher.matches()); String argumentClassName = matcher.group(1); if (!className.contentEquals(argumentClassName)) { fixBuilder.merge( diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java index 4baf9b39ea..6f67a2a16b 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java @@ -20,13 +20,17 @@ void identification() { "", " private static final Logger LOG = LoggerFactory.getLogger(A.class);", "", - " // BUG: Diagnostic contains:", - " private static final Logger foo = LoggerFactory.getLogger(A.class);", - "", " static class OverloadedGetLogger {", " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", " }", "", + " interface InterfaceWithNoCanonicalModifiers {", + " Logger LOG = LoggerFactory.getLogger(InterfaceWithNoCanonicalModifiers.class);", + " }", + "", + " // BUG: Diagnostic contains:", + " private static final Logger foo = LoggerFactory.getLogger(A.class);", + "", " static class WrongOverloadedGetLogger {", " // BUG: Diagnostic contains:", " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", @@ -62,10 +66,6 @@ void identification() { " private static final Logger LOG = LoggerFactory.getLogger(String.class);", " }", "", - " interface InterfaceWithNoCanonicalModifiers {", - " Logger LOG = LoggerFactory.getLogger(InterfaceWithNoCanonicalModifiers.class);", - " }", - "", " interface InterfaceWithWrongVariableName {", " // BUG: Diagnostic contains:", " Logger BAZ = LoggerFactory.getLogger(InterfaceWithWrongVariableName.class);", @@ -106,7 +106,6 @@ void replacement() { " }", "", " static class WrongArgumentOverloadedGetLogger {", - " // BUG: Diagnostic contains:", " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", " }", "", @@ -140,7 +139,6 @@ void replacement() { " }", "", " static class WrongArgumentOverloadedGetLogger {", - " // BUG: Diagnostic contains:", " private static final Logger LOG = LoggerFactory.getLogger(\"WrongArgumentOverloadedGetLogger\");", " }", "", From 1722d0a0fafb97438cf893368ff7fe1388f41188 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 27 Oct 2024 19:50:28 +0100 Subject: [PATCH 20/23] Suggestions --- .../errorprone/bugpatterns/EmptyMethod.java | 2 +- .../bugpatterns/Slf4jLogDeclaration.java | 134 ----------- .../bugpatterns/Slf4jLoggerDeclaration.java | 172 ++++++++++++++ .../Slf4JLoggerDeclarationTest.java | 210 ++++++++++++++++++ .../bugpatterns/Slf4jLogDeclarationTest.java | 176 --------------- 5 files changed, 383 insertions(+), 311 deletions(-) delete mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java create mode 100644 error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java create mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java delete mode 100644 error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java index 306a27c58f..b88eb2e63b 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/EmptyMethod.java @@ -62,7 +62,7 @@ public Description matchMethod(MethodTree tree, VisitorState state) { } private static boolean isInPossibleTestHelperClass(VisitorState state) { - return Optional.ofNullable(ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class)) + return Optional.ofNullable(state.findEnclosing(ClassTree.class)) .map(ClassTree::getSimpleName) .filter(name -> name.toString().contains("Test")) .isPresent(); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java deleted file mode 100644 index 7c57fdf5dc..0000000000 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java +++ /dev/null @@ -1,134 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import static com.google.common.base.Preconditions.checkState; -import static com.google.errorprone.BugPattern.LinkType.CUSTOM; -import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; -import static com.google.errorprone.BugPattern.StandardTags.STYLE; -import static com.google.errorprone.matchers.Matchers.staticMethod; -import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; - -import com.google.auto.service.AutoService; -import com.google.common.collect.Iterables; -import com.google.errorprone.BugPattern; -import com.google.errorprone.ErrorProneFlags; -import com.google.errorprone.VisitorState; -import com.google.errorprone.bugpatterns.BugChecker; -import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; -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.sun.source.tree.ClassTree; -import com.sun.source.tree.ExpressionTree; -import com.sun.source.tree.MethodInvocationTree; -import com.sun.source.tree.Tree; -import com.sun.source.tree.Tree.Kind; -import com.sun.source.tree.VariableTree; -import java.util.regex.Pattern; -import javax.inject.Inject; -import javax.lang.model.element.Modifier; -import javax.lang.model.element.Name; -import tech.picnic.errorprone.utils.SourceCode; - -/** - * A {@link BugChecker} that warns when SLF4J declarations are not canonical. - * - * @apiNote The default canonical logger name can be overridden through {@link ErrorProneFlags flag - * arguments}. - */ -@AutoService(BugChecker.class) -@BugPattern( - summary = "Make sure SLF4J log declarations follow the SLF4J conventions inside classes.", - link = BUG_PATTERNS_BASE_URL + "Slf4jLogDeclaration", - linkType = CUSTOM, - severity = WARNING, - tags = STYLE) -public final class Slf4jLogDeclaration extends BugChecker implements VariableTreeMatcher { - private static final long serialVersionUID = 1L; - private static final Matcher GET_LOGGER_METHOD = - staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); - private static final String CANONICAL_LOGGER_NAME_FLAG = - "Slf4jLogDeclaration:CanonicalLoggerName"; - private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; - private static final Pattern STRING_LITERAL_ARGUMENT_PATTERN = Pattern.compile("\"(.*?)\""); - private static final Pattern CLASS_ARGUMENT_PATTERN = Pattern.compile("(.*?)\\.class"); - - private final String canonicalLoggerName; - - /** Instantiates a default {@link Slf4jLogDeclaration} instance. */ - public Slf4jLogDeclaration() { - this(ErrorProneFlags.empty()); - } - - /** - * Instantiates a customized {@link Slf4jLogDeclaration}. - * - * @param flags Any provided command line flags. - */ - @Inject - Slf4jLogDeclaration(ErrorProneFlags flags) { - canonicalLoggerName = getCanonicalLoggerName(flags); - } - - @Override - public Description matchVariable(VariableTree tree, VisitorState state) { - if (!GET_LOGGER_METHOD.matches(tree.getInitializer(), state)) { - return Description.NO_MATCH; - } - - ClassTree clazz = state.findEnclosing(ClassTree.class); - if (clazz == null) { - return Description.NO_MATCH; - } - - SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - if (clazz.getKind() != Kind.INTERFACE) { - suggestCanonicalModifiers(tree, fixBuilder, state); - } - canonicalizeLoggerVariableName(tree, fixBuilder, state); - updateGetLoggerArgument( - (MethodInvocationTree) tree.getInitializer(), clazz.getSimpleName(), fixBuilder, state); - - return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); - } - - private void canonicalizeLoggerVariableName( - VariableTree variableTree, SuggestedFix.Builder fixBuilder, VisitorState state) { - if (!variableTree.getName().contentEquals(canonicalLoggerName)) { - fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, canonicalLoggerName, state)); - } - } - - private static void updateGetLoggerArgument( - MethodInvocationTree tree, - Name className, - SuggestedFix.Builder fixBuilder, - VisitorState state) { - ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments()); - String argumentName = SourceCode.treeToString(arg, state); - - java.util.regex.Matcher matcher; - if (arg.getKind() == Kind.STRING_LITERAL) { - matcher = STRING_LITERAL_ARGUMENT_PATTERN.matcher(argumentName); - } else { - matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName); - } - - checkState(matcher.matches()); - String argumentClassName = matcher.group(1); - if (!className.contentEquals(argumentClassName)) { - fixBuilder.merge( - SuggestedFix.replace(arg, argumentName.replace(argumentClassName, className))); - } - } - - private static void suggestCanonicalModifiers( - Tree tree, SuggestedFix.Builder fixBuilder, VisitorState state) { - SuggestedFixes.addModifiers(tree, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL) - .ifPresent(fixBuilder::merge); - } - - private static String getCanonicalLoggerName(ErrorProneFlags flags) { - return flags.get(CANONICAL_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME); - } -} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java new file mode 100644 index 0000000000..ae1851898e --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java @@ -0,0 +1,172 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.common.base.Verify.verify; +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.classLiteral; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static com.google.errorprone.matchers.Matchers.staticMethod; +import static com.google.errorprone.matchers.Matchers.toType; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.base.CaseFormat; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +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.util.ASTHelpers; +import com.sun.source.tree.ClassTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.Tree.Kind; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Symbol; +import java.util.EnumSet; +import java.util.Objects; +import javax.inject.Inject; +import javax.lang.model.element.Modifier; + +/** A {@link BugChecker} that flags non-canonical SLF4J logger declarations. */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "SLF4J logger declarations should follow established best-practices", + link = BUG_PATTERNS_BASE_URL + "Slf4jLoggerDeclaration", + linkType = CUSTOM, + severity = WARNING, + tags = STYLE) +public final class Slf4jLoggerDeclaration extends BugChecker implements VariableTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher IS_GET_LOGGER = + staticMethod().onDescendantOf("org.slf4j.LoggerFactory").named("getLogger"); + private static final String CANONICAL_STATIC_LOGGER_NAME_FLAG = + "Slf4jLogDeclaration:CanonicalStaticLoggerName"; + private static final String DEFAULT_CANONICAL_LOGGER_NAME = "LOG"; + private static final Matcher IS_STATIC_ENCLOSING_CLASS_REFERENCE = + classLiteral(Slf4jLoggerDeclaration::isEnclosingClassReference); + private static final Matcher IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE = + toType( + MethodInvocationTree.class, + allOf( + instanceMethod().anyClass().named("getClass").withNoParameters(), + Slf4jLoggerDeclaration::receiverIsEnclosingClassInstance)); + private static final ImmutableSet INSTANCE_DECLARATION_MODIFIERS = + Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.FINAL); + private static final ImmutableSet STATIC_DECLARATION_MODIFIERS = + Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL); + + private final String canonicalStaticName; + private final String canonicalInstanceName; + + /** Instantiates a default {@link Slf4jLoggerDeclaration} instance. */ + public Slf4jLoggerDeclaration() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link Slf4jLoggerDeclaration}. + * + * @param flags Any provided command line flags. + */ + @Inject + Slf4jLoggerDeclaration(ErrorProneFlags flags) { + canonicalStaticName = + flags.get(CANONICAL_STATIC_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME); + canonicalInstanceName = + CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticName); + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + ExpressionTree initializer = tree.getInitializer(); + if (!IS_GET_LOGGER.matches(initializer, state)) { + return Description.NO_MATCH; + } + + ClassTree clazz = getEnclosingClass(state); + ExpressionTree factoryArg = + Iterables.getOnlyElement(((MethodInvocationTree) initializer).getArguments()); + + SuggestedFix.Builder fix = SuggestedFix.builder(); + + if (clazz.getModifiers().getFlags().contains(Modifier.ABSTRACT) + && IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) { + suggestModifiers(tree, INSTANCE_DECLARATION_MODIFIERS, fix, state); + suggestRename(tree, canonicalInstanceName, fix, state); + } else { + suggestModifiers( + tree, + clazz.getKind() == Kind.INTERFACE ? ImmutableSet.of() : STATIC_DECLARATION_MODIFIERS, + fix, + state); + suggestRename(tree, canonicalStaticName, fix, state); + + if (!ASTHelpers.isSameType( + ASTHelpers.getType(factoryArg), state.getSymtab().stringType, state) + && !IS_STATIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) { + fix.merge(SuggestedFix.replace(factoryArg, clazz.getSimpleName() + ".class")); + } + } + + return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix.build()); + } + + private static void suggestModifiers( + Tree tree, + ImmutableSet modifiers, + SuggestedFix.Builder fixBuilder, + VisitorState state) { + ModifiersTree modifiersTree = ASTHelpers.getModifiers(tree); + verify(modifiersTree != null, "Given tree does not support modifiers"); + SuggestedFixes.addModifiers(tree, modifiersTree, state, modifiers).ifPresent(fixBuilder::merge); + SuggestedFixes.removeModifiers( + modifiersTree, state, Sets.difference(EnumSet.allOf(Modifier.class), modifiers)) + .ifPresent(fixBuilder::merge); + } + + private static void suggestRename( + VariableTree variableTree, String name, SuggestedFix.Builder fixBuilder, VisitorState state) { + if (!variableTree.getName().contentEquals(name)) { + fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, name, state)); + } + } + + private static boolean isEnclosingClassReference(ExpressionTree tree, VisitorState state) { + ClassTree enclosing = state.findEnclosing(ClassTree.class); + return enclosing != null + && Objects.equals(ASTHelpers.getSymbol(tree), ASTHelpers.getSymbol(enclosing)); + } + + private static boolean receiverIsEnclosingClassInstance( + MethodInvocationTree tree, VisitorState state) { + ExpressionTree receiver = ASTHelpers.getReceiver(tree); + if (receiver == null) { + return true; + } + + Symbol symbol = ASTHelpers.getSymbol(receiver); + return symbol != null + && symbol.asType().tsym.equals(ASTHelpers.getSymbol(getEnclosingClass(state))); + } + + private static ClassTree getEnclosingClass(VisitorState state) { + ClassTree clazz = state.findEnclosing(ClassTree.class); + // XXX: Review whether we should relax this constraint in the face of so-called anonymous + // classes. See + // https://docs.oracle.com/en/java/javase/23/language/implicitly-declared-classes-and-instance-main-methods.html + verify(clazz != null, "Variable not defined inside class"); + return clazz; + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java new file mode 100644 index 0000000000..35d217c5bd --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java @@ -0,0 +1,210 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class Slf4JLoggerDeclarationTest { + @Test + void identification() { + CompilationTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass()) + .addSourceLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " private static final long serialVersionUID = 1L;", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " abstract static class DynamicLogger {", + " private final Logger log = LoggerFactory.getLogger(getClass());", + " }", + "", + " abstract static class DynamicLoggerWithExplicitThis {", + " private final Logger log = LoggerFactory.getLogger(this.getClass());", + " }", + "", + " static final class StaticLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(StaticLogger.class);", + " }", + "", + " static final class StaticLoggerWithCustomIdentifier {", + " private static final Logger LOG = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " interface StaticLoggerForInterface {", + " Logger LOG = LoggerFactory.getLogger(StaticLoggerForInterface.class);", + " }", + "", + " abstract static class DynamicLoggerForWrongTypeWithoutSymbol {", + " // BUG: Diagnostic contains:", + " private final Logger log = LoggerFactory.getLogger(\"foo\".getClass());", + " }", + "", + " abstract static class DynamicLoggerForWrongTypeWithSymbol {", + " // BUG: Diagnostic contains:", + " private final Logger log = LoggerFactory.getLogger(new A().getClass());", + " }", + "", + " static final class NonAbstractDynamicLogger {", + " // BUG: Diagnostic contains:", + " private final Logger log = LoggerFactory.getLogger(getClass());", + " }", + "", + " abstract static class DynamicLoggerWithMissingModifier {", + " // BUG: Diagnostic contains:", + " final Logger log = LoggerFactory.getLogger(getClass());", + " }", + "", + " abstract static class DynamicLoggerWithExcessModifier {", + " // BUG: Diagnostic contains:", + " private final transient Logger log = LoggerFactory.getLogger(getClass());", + " }", + "", + " abstract static class MisnamedDynamicLogger {", + " // BUG: Diagnostic contains:", + " private final Logger LOG = LoggerFactory.getLogger(getClass());", + " }", + "", + " static final class StaticLoggerWithMissingModifier {", + " // BUG: Diagnostic contains:", + " static final Logger LOG = LoggerFactory.getLogger(StaticLoggerWithMissingModifier.class);", + " }", + "", + " static final class StaticLoggerWithExcessModifier {", + " // BUG: Diagnostic contains:", + " private static final transient Logger LOG =", + " LoggerFactory.getLogger(StaticLoggerWithExcessModifier.class);", + " }", + "", + " static final class MisnamedStaticLogger {", + " // BUG: Diagnostic contains:", + " private static final Logger log = LoggerFactory.getLogger(MisnamedStaticLogger.class);", + " }", + "", + " static final class StaticLoggerWithIncorrectIdentifier {", + " // BUG: Diagnostic contains:", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + " }", + "", + " static final class StaticLoggerWithCustomIdentifierAndMissingModifier {", + " // BUG: Diagnostic contains:", + " static final Logger LOG = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " static final class StaticLoggerWithCustomIdentifierAndExcessModifier {", + " // BUG: Diagnostic contains:", + " private static final transient Logger LOG = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " static final class MisnamedStaticLoggerWithCustomIdentifier {", + " // BUG: Diagnostic contains:", + " private static final Logger log = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " interface StaticLoggerForInterfaceWithExcessModifier {", + " // BUG: Diagnostic contains:", + " static Logger LOG = LoggerFactory.getLogger(StaticLoggerForInterfaceWithExcessModifier.class);", + " }", + "", + " interface MisnamedStaticLoggerForInterface {", + " // BUG: Diagnostic contains:", + " Logger log = LoggerFactory.getLogger(MisnamedStaticLoggerForInterface.class);", + " }", + "", + " interface StaticLoggerForInterfaceWithIncorrectIdentifier {", + " // BUG: Diagnostic contains:", + " Logger LOG = LoggerFactory.getLogger(A.class);", + " }", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass()) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " static Logger foo = LoggerFactory.getLogger(Logger.class);", + "", + " abstract static class DynamicLogger {", + " transient Logger BAR = LoggerFactory.getLogger(getClass());", + " }", + "", + " static final class StaticLogger {", + " transient Logger baz = LoggerFactory.getLogger(LoggerFactory.class);", + " }", + "", + " static final class StaticLoggerWithCustomIdentifier {", + " transient Logger qux = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " interface StaticLoggerForInterface {", + " public static final Logger quux = LoggerFactory.getLogger(A.class);", + " }", + "}") + .addOutputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " private static final Logger LOG = LoggerFactory.getLogger(A.class);", + "", + " abstract static class DynamicLogger {", + " private final Logger log = LoggerFactory.getLogger(getClass());", + " }", + "", + " static final class StaticLogger {", + " private static final Logger LOG = LoggerFactory.getLogger(StaticLogger.class);", + " }", + "", + " static final class StaticLoggerWithCustomIdentifier {", + " private static final Logger LOG = LoggerFactory.getLogger(\"custom-identifier\");", + " }", + "", + " interface StaticLoggerForInterface {", + " Logger LOG = LoggerFactory.getLogger(StaticLoggerForInterface.class);", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void replacementWithCustomLoggerName() { + BugCheckerRefactoringTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass()) + .setArgs(ImmutableList.of("-XepOpt:Slf4jLogDeclaration:CanonicalStaticLoggerName=FOO_BAR")) + .addInputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " transient Logger LOG = LoggerFactory.getLogger(Logger.class);", + "", + " abstract static class DynamicLogger {", + " transient Logger log = LoggerFactory.getLogger(getClass());", + " }", + "}") + .addOutputLines( + "A.java", + "import org.slf4j.Logger;", + "import org.slf4j.LoggerFactory;", + "", + "class A {", + " private static final Logger FOO_BAR = LoggerFactory.getLogger(A.class);", + "", + " abstract static class DynamicLogger {", + " private final Logger fooBar = LoggerFactory.getLogger(getClass());", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java deleted file mode 100644 index 6f67a2a16b..0000000000 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java +++ /dev/null @@ -1,176 +0,0 @@ -package tech.picnic.errorprone.bugpatterns; - -import com.google.common.collect.ImmutableList; -import com.google.errorprone.BugCheckerRefactoringTestHelper; -import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; -import com.google.errorprone.CompilationTestHelper; -import org.junit.jupiter.api.Test; - -final class Slf4jLogDeclarationTest { - @Test - void identification() { - CompilationTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .addSourceLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "class A {", - " private static final long serialVersionUID = 1L;", - "", - " private static final Logger LOG = LoggerFactory.getLogger(A.class);", - "", - " static class OverloadedGetLogger {", - " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", - " }", - "", - " interface InterfaceWithNoCanonicalModifiers {", - " Logger LOG = LoggerFactory.getLogger(InterfaceWithNoCanonicalModifiers.class);", - " }", - "", - " // BUG: Diagnostic contains:", - " private static final Logger foo = LoggerFactory.getLogger(A.class);", - "", - " static class WrongOverloadedGetLogger {", - " // BUG: Diagnostic contains:", - " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", - " }", - "", - " static class VariableMissingStaticFinal {", - " // BUG: Diagnostic contains:", - " private Logger LOG = LoggerFactory.getLogger(VariableMissingStaticFinal.class);", - " }", - "", - " static class VariableMissingFinal {", - " // BUG: Diagnostic contains:", - " private static Logger LOG = LoggerFactory.getLogger(VariableMissingFinal.class);", - " }", - "", - " static class VariableMissingPrivate {", - " // BUG: Diagnostic contains:", - " static final Logger LOG = LoggerFactory.getLogger(VariableMissingPrivate.class);", - " }", - "", - " static class VariableMissingStatic {", - " // BUG: Diagnostic contains:", - " private final Logger LOG = LoggerFactory.getLogger(VariableMissingStatic.class);", - " }", - "", - " static class WrongVariableName {", - " // BUG: Diagnostic contains:", - " private static final Logger BAR = LoggerFactory.getLogger(WrongVariableName.class);", - " }", - "", - " static class WrongArgumentGetLogger {", - " // BUG: Diagnostic contains:", - " private static final Logger LOG = LoggerFactory.getLogger(String.class);", - " }", - "", - " interface InterfaceWithWrongVariableName {", - " // BUG: Diagnostic contains:", - " Logger BAZ = LoggerFactory.getLogger(InterfaceWithWrongVariableName.class);", - " }", - "", - " interface WrongArgumentGetLoggerInterface {", - " // BUG: Diagnostic contains:", - " Logger LOG = LoggerFactory.getLogger(A.class);", - " }", - "}") - .doTest(); - } - - @Test - void replacement() { - BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .addInputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "class A {", - " static Logger foo = LoggerFactory.getLogger(A.class);", - "", - " void m() {", - " String unused = toString();", - " foo.trace(\"foo\");", - " }", - "", - " static class NestedClass {", - " void m() {", - " foo.trace(\"foo\");", - " }", - " }", - "", - " static class WrongArgumentGetLogger {", - " private static final Logger LOG = LoggerFactory.getLogger(String.class);", - " }", - "", - " static class WrongArgumentOverloadedGetLogger {", - " private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");", - " }", - "", - " interface InterfaceWithDefaultMethod {", - " default void m() {", - " foo.trace(\"foo\");", - " }", - " }", - "}") - .addOutputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "class A {", - " private static final Logger LOG = LoggerFactory.getLogger(A.class);", - "", - " void m() {", - " String unused = toString();", - " LOG.trace(\"foo\");", - " }", - "", - " static class NestedClass {", - " void m() {", - " LOG.trace(\"foo\");", - " }", - " }", - "", - " static class WrongArgumentGetLogger {", - " private static final Logger LOG = LoggerFactory.getLogger(WrongArgumentGetLogger.class);", - " }", - "", - " static class WrongArgumentOverloadedGetLogger {", - " private static final Logger LOG = LoggerFactory.getLogger(\"WrongArgumentOverloadedGetLogger\");", - " }", - "", - " interface InterfaceWithDefaultMethod {", - " default void m() {", - " LOG.trace(\"foo\");", - " }", - " }", - "}") - .doTest(TestMode.TEXT_MATCH); - } - - @Test - void replacementWithOverriddenCanonicalLoggerName() { - BugCheckerRefactoringTestHelper.newInstance(Slf4jLogDeclaration.class, getClass()) - .setArgs(ImmutableList.of("-XepOpt:Slf4jLogDeclaration:CanonicalLoggerName=BAR")) - .addInputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "class A {", - " Logger LOG = LoggerFactory.getLogger(A.class);", - "}") - .addOutputLines( - "A.java", - "import org.slf4j.Logger;", - "import org.slf4j.LoggerFactory;", - "", - "class A {", - " private static final Logger BAR = LoggerFactory.getLogger(A.class);", - "}") - .doTest(TestMode.TEXT_MATCH); - } -} From 2f538d8f7bf2af6ca4579e82a5d8b67741b95d80 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Sun, 27 Oct 2024 21:48:53 +0100 Subject: [PATCH 21/23] Suppress SonarCloud warning --- .../picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java | 1 + 1 file changed, 1 insertion(+) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java index ae1851898e..ce31cb0e3d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java @@ -47,6 +47,7 @@ linkType = CUSTOM, severity = WARNING, tags = STYLE) +@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */) public final class Slf4jLoggerDeclaration extends BugChecker implements VariableTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher IS_GET_LOGGER = From 9681655c5a9b4690344445bc90f2051c38437d76 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Mon, 28 Oct 2024 07:22:08 +0100 Subject: [PATCH 22/23] More suggestions --- .../FormatStringConcatenation.java | 9 ++-- .../bugpatterns/Slf4jLoggerDeclaration.java | 54 +++++++++++-------- .../Slf4JLoggerDeclarationTest.java | 9 ++++ .../errorprone/utils/MoreASTHelpers.java | 12 +++++ .../errorprone/utils/MoreASTHelpersTest.java | 46 ++++++++++++++++ 5 files changed, 103 insertions(+), 27 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java index 744c6e732c..e80d69611f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/FormatStringConcatenation.java @@ -36,6 +36,7 @@ import java.util.List; import java.util.Optional; import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.utils.MoreASTHelpers; import tech.picnic.errorprone.utils.SourceCode; /** @@ -203,14 +204,10 @@ private static boolean hasNonConstantStringConcatenationArgument( ExpressionTree argument = ASTHelpers.stripParentheses(arguments.get(argPosition)); return argument instanceof BinaryTree - && isStringTyped(argument, state) + && MoreASTHelpers.isStringTyped(argument, state) && ASTHelpers.constValue(argument, String.class) == null; } - private static boolean isStringTyped(ExpressionTree tree, VisitorState state) { - return ASTHelpers.isSameType(ASTHelpers.getType(tree), state.getSymtab().stringType, state); - } - private static class ReplacementArgumentsConstructor extends SimpleTreeVisitor<@Nullable Void, VisitorState> { private final StringBuilder formatString = new StringBuilder(); @@ -223,7 +220,7 @@ private static class ReplacementArgumentsConstructor @Override public @Nullable Void visitBinary(BinaryTree tree, VisitorState state) { - if (tree.getKind() == Kind.PLUS && isStringTyped(tree, state)) { + if (tree.getKind() == Kind.PLUS && MoreASTHelpers.isStringTyped(tree, state)) { tree.getLeftOperand().accept(this, state); tree.getRightOperand().accept(this, state); } else { diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java index ce31cb0e3d..8435860751 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclaration.java @@ -9,6 +9,7 @@ import static com.google.errorprone.matchers.Matchers.instanceMethod; import static com.google.errorprone.matchers.Matchers.staticMethod; import static com.google.errorprone.matchers.Matchers.toType; +import static java.util.Objects.requireNonNull; import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; @@ -30,14 +31,13 @@ import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.ModifiersTree; -import com.sun.source.tree.Tree; import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; import com.sun.tools.javac.code.Symbol; import java.util.EnumSet; -import java.util.Objects; import javax.inject.Inject; import javax.lang.model.element.Modifier; +import tech.picnic.errorprone.utils.MoreASTHelpers; /** A {@link BugChecker} that flags non-canonical SLF4J logger declarations. */ @AutoService(BugChecker.class) @@ -62,14 +62,14 @@ public final class Slf4jLoggerDeclaration extends BugChecker implements Variable MethodInvocationTree.class, allOf( instanceMethod().anyClass().named("getClass").withNoParameters(), - Slf4jLoggerDeclaration::receiverIsEnclosingClassInstance)); + Slf4jLoggerDeclaration::getClassReceiverIsEnclosingClassInstance)); private static final ImmutableSet INSTANCE_DECLARATION_MODIFIERS = Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.FINAL); private static final ImmutableSet STATIC_DECLARATION_MODIFIERS = Sets.immutableEnumSet(Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL); - private final String canonicalStaticName; - private final String canonicalInstanceName; + private final String canonicalStaticFieldName; + private final String canonicalInstanceFieldName; /** Instantiates a default {@link Slf4jLoggerDeclaration} instance. */ public Slf4jLoggerDeclaration() { @@ -83,10 +83,10 @@ public Slf4jLoggerDeclaration() { */ @Inject Slf4jLoggerDeclaration(ErrorProneFlags flags) { - canonicalStaticName = + canonicalStaticFieldName = flags.get(CANONICAL_STATIC_LOGGER_NAME_FLAG).orElse(DEFAULT_CANONICAL_LOGGER_NAME); - canonicalInstanceName = - CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticName); + canonicalInstanceFieldName = + CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_CAMEL, canonicalStaticFieldName); } @Override @@ -104,19 +104,27 @@ public Description matchVariable(VariableTree tree, VisitorState state) { if (clazz.getModifiers().getFlags().contains(Modifier.ABSTRACT) && IS_DYNAMIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) { + /* + * While generally we prefer `Logger` declarations to be static and named after their + * enclosing class, we allow one exception: loggers in abstract classes with a name derived + * from `getClass()`. + */ suggestModifiers(tree, INSTANCE_DECLARATION_MODIFIERS, fix, state); - suggestRename(tree, canonicalInstanceName, fix, state); + suggestRename(tree, canonicalInstanceFieldName, fix, state); } else { suggestModifiers( tree, clazz.getKind() == Kind.INTERFACE ? ImmutableSet.of() : STATIC_DECLARATION_MODIFIERS, fix, state); - suggestRename(tree, canonicalStaticName, fix, state); + suggestRename(tree, canonicalStaticFieldName, fix, state); - if (!ASTHelpers.isSameType( - ASTHelpers.getType(factoryArg), state.getSymtab().stringType, state) + if (!MoreASTHelpers.isStringTyped(factoryArg, state) && !IS_STATIC_ENCLOSING_CLASS_REFERENCE.matches(factoryArg, state)) { + /* + * Loggers with a custom string name are generally "special", but those with a name derived + * from a class other than the one that encloses it are likely in error. + */ fix.merge(SuggestedFix.replace(factoryArg, clazz.getSimpleName() + ".class")); } } @@ -125,12 +133,12 @@ public Description matchVariable(VariableTree tree, VisitorState state) { } private static void suggestModifiers( - Tree tree, + VariableTree tree, ImmutableSet modifiers, SuggestedFix.Builder fixBuilder, VisitorState state) { - ModifiersTree modifiersTree = ASTHelpers.getModifiers(tree); - verify(modifiersTree != null, "Given tree does not support modifiers"); + ModifiersTree modifiersTree = + requireNonNull(ASTHelpers.getModifiers(tree), "`VariableTree` must have modifiers"); SuggestedFixes.addModifiers(tree, modifiersTree, state, modifiers).ifPresent(fixBuilder::merge); SuggestedFixes.removeModifiers( modifiersTree, state, Sets.difference(EnumSet.allOf(Modifier.class), modifiers)) @@ -145,15 +153,19 @@ private static void suggestRename( } private static boolean isEnclosingClassReference(ExpressionTree tree, VisitorState state) { - ClassTree enclosing = state.findEnclosing(ClassTree.class); - return enclosing != null - && Objects.equals(ASTHelpers.getSymbol(tree), ASTHelpers.getSymbol(enclosing)); + return ASTHelpers.getSymbol(getEnclosingClass(state)).equals(ASTHelpers.getSymbol(tree)); } - private static boolean receiverIsEnclosingClassInstance( - MethodInvocationTree tree, VisitorState state) { - ExpressionTree receiver = ASTHelpers.getReceiver(tree); + private static boolean getClassReceiverIsEnclosingClassInstance( + MethodInvocationTree getClassInvocationTree, VisitorState state) { + ExpressionTree receiver = ASTHelpers.getReceiver(getClassInvocationTree); if (receiver == null) { + /* + * Method invocations without an explicit receiver either involve static methods (possibly + * statically imported), or instance methods invoked on the enclosing class. As the given + * `getClassInvocationTree` is guaranteed to be a nullary `#getClass()` invocation, the latter + * must be the case. + */ return true; } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java index 35d217c5bd..d14c3dd145 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java @@ -12,6 +12,8 @@ void identification() { CompilationTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass()) .addSourceLines( "A.java", + "import static java.lang.Class.forName;", + "", "import org.slf4j.Logger;", "import org.slf4j.LoggerFactory;", "", @@ -39,6 +41,13 @@ void identification() { " Logger LOG = LoggerFactory.getLogger(StaticLoggerForInterface.class);", " }", "", + " abstract static class DynamicLoggerForWrongTypeWithoutReceiver {", + " // BUG: Diagnostic contains:", + " private final Logger log = LoggerFactory.getLogger(forName(\"A.class\"));", + "", + " DynamicLoggerForWrongTypeWithoutReceiver() throws ClassNotFoundException {}", + " }", + "", " abstract static class DynamicLoggerForWrongTypeWithoutSymbol {", " // BUG: Diagnostic contains:", " private final Logger log = LoggerFactory.getLogger(\"foo\".getClass());", diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/MoreASTHelpers.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/MoreASTHelpers.java index 30c12d3c0d..0caa0002fe 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/MoreASTHelpers.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/MoreASTHelpers.java @@ -78,4 +78,16 @@ public static Optional findMethodExitedOnReturn(VisitorState state) public static boolean areSameType(Tree treeA, Tree treeB, VisitorState state) { return ASTHelpers.isSameType(ASTHelpers.getType(treeA), ASTHelpers.getType(treeB), state); } + + /** + * Tells whether the given tree is of type {@link String}. + * + * @param tree The tree of interest. + * @param state The {@link VisitorState} describing the context in which the given tree was found. + * @return Whether the specified tree has the same type as {@link + * com.sun.tools.javac.code.Symtab#stringType}. + */ + public static boolean isStringTyped(Tree tree, VisitorState state) { + return ASTHelpers.isSameType(ASTHelpers.getType(tree), state.getSymtab().stringType, state); + } } diff --git a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/MoreASTHelpersTest.java b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/MoreASTHelpersTest.java index efd0a33bff..905023bbaf 100644 --- a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/MoreASTHelpersTest.java +++ b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/MoreASTHelpersTest.java @@ -9,10 +9,13 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.ExpressionStatementTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.ReturnTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; import com.google.errorprone.matchers.Description; import com.sun.source.tree.ExpressionStatementTree; +import com.sun.source.tree.MethodInvocationTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.Tree; @@ -137,6 +140,25 @@ void areSameType() { .doTest(); } + @Test + void isStringTyped() { + CompilationTestHelper.newInstance(IsStringTypedTestChecker.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " void m() {", + " int foo = 1;", + " // BUG: Diagnostic contains:", + " String s = \"foo\";", + "", + " hashCode();", + " // BUG: Diagnostic contains:", + " toString();", + " }", + "}") + .doTest(); + } + private static String createMethodSearchDiagnosticsMessage( BiFunction valueFunction, VisitorState state) { return Maps.toMap(ImmutableSet.of("foo", "bar", "baz"), key -> valueFunction.apply(key, state)) @@ -224,4 +246,28 @@ public Description matchMethod(MethodTree tree, VisitorState state) { : Description.NO_MATCH; } } + + /** + * A {@link BugChecker} that delegates to {@link MoreASTHelpers#isStringTyped(Tree, + * VisitorState)}. + */ + @BugPattern(summary = "Interacts with `MoreASTHelpers` for testing purposes", severity = ERROR) + public static final class IsStringTypedTestChecker extends BugChecker + implements MethodInvocationTreeMatcher, VariableTreeMatcher { + private static final long serialVersionUID = 1L; + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return getDescription(tree, state); + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + return getDescription(tree, state); + } + + private Description getDescription(Tree tree, VisitorState state) { + return MoreASTHelpers.isStringTyped(tree, state) ? describeMatch(tree) : Description.NO_MATCH; + } + } } From b7621f2b637917b6232828cf0ed1ba3a2dd36952 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Mon, 28 Oct 2024 08:32:12 +0100 Subject: [PATCH 23/23] Rename test --- ...ggerDeclarationTest.java => Slf4jLoggerDeclarationTest.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/{Slf4JLoggerDeclarationTest.java => Slf4jLoggerDeclarationTest.java} (99%) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclarationTest.java similarity index 99% rename from error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java rename to error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclarationTest.java index d14c3dd145..ada419cc4c 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4JLoggerDeclarationTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLoggerDeclarationTest.java @@ -6,7 +6,7 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; -final class Slf4JLoggerDeclarationTest { +final class Slf4jLoggerDeclarationTest { @Test void identification() { CompilationTestHelper.newInstance(Slf4jLoggerDeclaration.class, getClass())