Introduce `Slf4jLogDeclaration` to canonicalize slf4j's variable's declaration #783

mohamedsamehsalah · 2023-09-08T20:09:00Z

Suggested commit message:

Introduce `Slf4jLogDeclaration` check (#783)

github-actions · 2023-09-08T20:11:27Z

Surviving mutants in this change: 5
Killed mutants in this change: 16

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	5	16

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

github-actions · 2023-09-08T20:18:05Z

Surviving mutants in this change: 5
Killed mutants in this change: 16

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	5	16

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-09-08T20:27:00Z

Surviving mutants in this change: 5
Killed mutants in this change: 16

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	5	16

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarcloud · 2023-09-08T20:45:39Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

91.4% Coverage
0.0% Duplication

github-actions · 2023-09-08T20:46:02Z

Surviving mutants in this change: 5
Killed mutants in this change: 16

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	5	16

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Quick skim; hope this helps :)

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

Stephan202 · 2023-10-17T11:48:27Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+
+  @Override
+  public Description matchClass(ClassTree tree, VisitorState state) {
+    if (tree.getKind() == Kind.INTERFACE || !TEST_CLASS_WITH_LOGGER.matches(tree, state)) {


I don't think we need to skip interfaces: for those we can still suggest (but not auto-fix) the canonical field name.

(Separately from that we should drop redundant modifiers, but that should be a separate check, modeled after this Checkstyle rule.)

I don't think we need to skip interfaces

The intention was to not apply modifiers to inteface variables but still suggest/fix field name.

: for those we can still suggest (but not auto-fix) the canonical field name.

Why only suggest and not auto-fix ?

(Separately from that we should drop redundant modifiers, but that should be a separate check, modeled after this Checkstyle rule.)

Should this check be part of this PR ?

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

Stephan202 · 2023-10-17T12:10:12Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    if (!GET_LOGGER_METHOD.matches(tree, state)) {


We're now creating separate fixes for the field declaration (modifiers + name) and for the referenced class. I wonder whether this means that we should have two separate checks. I can imagine that some users will appreciate the second, while finding the first too conservative.

~~Pushed a change using TreeScanner (Conceptually still two checks usingvisitClass & visitMethodInvocationTree).~~

What do you think about making the separation into an EP flag ?

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

Stephan202 · 2023-10-17T12:17:38Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+      if (LOGGER.matches(member, state)) {
+        VariableTree variable = (VariableTree) member;
+        SuggestedFixes.addModifiers(
+                member, state, Modifier.PRIVATE, Modifier.STATIC, Modifier.FINAL)


Making the field final or private could make the code fail to compile. Likely this is still the right trade-off, but perhaps something worth calling out.

Does it make sense to make the suggested modifiers EP flags instead ?

github-actions · 2023-11-25T20:38:43Z

Surviving mutants in this change: 6
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	3	4
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	15

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarcloud · 2023-11-25T21:23:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

89.5% Coverage
0.0% Duplication

github-actions · 2023-11-25T21:23:13Z

Surviving mutants in this change: 6
Killed mutants in this change: 19

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	3	4
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	15

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Some comments we discussed offline :).

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

mohamedsamehsalah

Sorry for the slow progress 🐢

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

github-actions · 2024-03-03T16:29:55Z

Surviving mutants in this change: 10
Killed mutants in this change: 15

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	7	2
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	13

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-03-03T16:34:23Z

Surviving mutants in this change: 10
Killed mutants in this change: 15

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	7	2
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	13

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-06T17:13:56Z

Surviving mutants in this change: 6
Killed mutants in this change: 22

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	4	16
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	2	6

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-06T18:53:27Z

Surviving mutants in this change: 6
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	4	8
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	2	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-06T18:58:38Z

Surviving mutants in this change: 5
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	3	8
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	2	18

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah

Thanks @rickie , I resolved the comments from the pair programming sessions and tried to clean up some of the mutants 🐞. PTAL 🙏 .

mohamedsamehsalah · 2024-04-06T18:59:48Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+    }
+
+    Symbol enclosingElement = ASTHelpers.getSymbol(tree).getEnclosingElement();
+    if (enclosingElement == null) {


Either we do this check or we suppress NullAway warning

mohamedsamehsalah · 2024-04-06T19:03:15Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+          String argumentClassName;
+          if (arg.getKind() == Kind.STRING_LITERAL) {
+            java.util.regex.Matcher matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName);
+            checkArgument(matcher.matches(), "Invalid argument name.");


Here and below, this check is used to invoke the matching.

rickie

Did another pass, some small improvements here and there :).

rickie · 2024-04-17T06:32:07Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+          ExpressionTree arg = Iterables.getOnlyElement(tree.getArguments());
+          String argumentName = SourceCode.treeToString(arg, state);
+
+          String argumentClassName;


We can simplify the code a little:

java.util.regex.Matcher matcher; if (arg.getKind() == Kind.STRING_LITERAL) { matcher = STRING_LITERAL_ARGUMENT.matcher(argumentName); } else { matcher = CLASS_ARGUMENT.matcher(argumentName); } checkArgument(matcher.matches(), "Invalid argument name."); String argumentClassName = matcher.group(1);

rickie · 2024-04-17T06:54:21Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+    }
+  }
+
+  private static void updateGetLoggerArgument(


I'm not seeing why we need a TreeScanner to walk over a VariableTree, I think we can improve this :).

We do this because we need to match on the variable declaration and not in the class level (Which was flagged here). In addition, in case the variable is in a nested class, without this, the match will be on the outer class level .

github-actions · 2024-04-17T06:57:28Z

Surviving mutants in this change: 4
Killed mutants in this change: 27

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration$1`	2	8
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	2	19

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2024-04-19T16:07:31Z

Surviving mutants in this change: 3
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2024-04-20T18:22:03Z

/integration-test

rickie

I think there is only one open question for me that we should answer before approving. Apart from that, I have almost nothing to add here!

Really cool stuff @mohamedsamehsalah 🚀 !

rickie · 2024-04-20T18:19:09Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java

+      matcher = CLASS_ARGUMENT_PATTERN.matcher(argumentName);
+    }
+
+    checkArgument(matcher.matches(), "Invalid argument name.");


Hmm interesting. Maybe a verify or checkState, would be better?

rickie · 2024-05-02T07:09:56Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

+            "",
+            "  static class WrongArgumentOverloadedGetLogger {",
+            "    // BUG: Diagnostic contains:",
+            "    private static final Logger LOG = LoggerFactory.getLogger(\"OverloadedGetLogger\");",


On a second look, I'm not really sure how we should handle these string overloads... There are quite some different ways of using them, if we look outside of Picnic to usages. Within Picnic we have a few like Something.NameOfclass, so not sure what we want to do there...

CC: @Stephan202 What do you think?

rickie · 2024-05-02T07:12:05Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

+            "  }",
+            "",
+            "  static class WrongArgumentOverloadedGetLogger {",
+            "    // BUG: Diagnostic contains:",


We can drop this one in a replacement test 😉.

rickie · 2024-05-02T07:12:50Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java

+            "class A {",
+            "  private static final long serialVersionUID = 1L;",
+            "",
+            "  private static final Logger LOG = LoggerFactory.getLogger(A.class);",


I put the "positive" examples at the top of the file :).

sonarcloud · 2024-05-02T07:18:26Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
92.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-05-02T07:19:01Z

Surviving mutants in this change: 3
Killed mutants in this change: 26

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.Slf4jLogDeclaration`	3	26

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah added this to the 0.14.0 milestone Sep 8, 2023

mohamedsamehsalah linked an issue Sep 8, 2023 that may be closed by this pull request

Canonical SLF4J Logger usage #632

Open

3 tasks

mohamedsamehsalah commented Sep 8, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclaration.java Outdated Show resolved Hide resolved

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from 536811a to fc4d717 Compare September 8, 2023 20:15

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from fc4d717 to 3c70f11 Compare September 8, 2023 20:24

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from 3c70f11 to 5947396 Compare September 8, 2023 20:43

mohamedsamehsalah requested a review from Stephan202 September 10, 2023 16:58

rickie modified the milestones: 0.14.0, 0.15.0 Oct 4, 2023

mohamedsamehsalah requested a review from rickie October 15, 2023 19:33

Stephan202 reviewed Oct 17, 2023

View reviewed changes

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from 5947396 to 61626b7 Compare November 25, 2023 20:36

mohamedsamehsalah requested a review from Stephan202 November 25, 2023 20:42

Stephan202 added the new feature label Dec 24, 2023

rickie modified the milestones: 0.15.0, 0.16.0 Feb 10, 2024

rickie reviewed Feb 26, 2024

View reviewed changes

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from 4142606 to a54db09 Compare March 3, 2024 16:27

mohamedsamehsalah commented Mar 3, 2024

View reviewed changes

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/Slf4jLogDeclarationTest.java Outdated Show resolved Hide resolved

mohamedsamehsalah requested a review from rickie March 3, 2024 16:28

mohamedsamehsalah and others added 12 commits April 6, 2024 19:11

Assume one logger per class, check nested classes and use TreeScanner

d3439fa

Don't skip interfaces

ba1bb9b

Ommit modifying interface variables modifiers (heh)

5eb8fcf

Fix checkstyles

78bda3c

TIL: Illegal static declaration in inner class

e8579bf

Address comments: Add test case for bug checker's flags

779b756

Fix nested class import checkstyle

59bc76b

Suggestions

0ef12cd

Suggestions part 2

d1e6318

Suggestions part 3

647be6f

Pair programming

14f46b5

Comments

b29f243

mohamedsamehsalah force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from 7109c5d to b29f243 Compare April 6, 2024 17:11

Kill mutants & handle getLogger overload

4d7c776

Some mutants are just meant to live on among us...

e5209ff

mohamedsamehsalah commented Apr 6, 2024

View reviewed changes

mohamedsamehsalah requested a review from rickie April 6, 2024 19:03

Suggestions

ccc6611

rickie reviewed Apr 17, 2024

View reviewed changes

Simplify some code, forgot to push

72a6871

Suggestions

a5995c8

rickie reviewed May 2, 2024

View reviewed changes

Introduce Slf4jLogDeclaration to canonicalize slf4j's variable's declaration #783

Are you sure you want to change the base?

Introduce Slf4jLogDeclaration to canonicalize slf4j's variable's declaration #783

Conversation

mohamedsamehsalah commented Sep 8, 2023 • edited by rickie

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

sonarcloud bot commented Sep 8, 2023

github-actions bot commented Sep 8, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah Nov 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 25, 2023

sonarcloud bot commented Nov 25, 2023

github-actions bot commented Nov 25, 2023

rickie left a comment

Choose a reason for hiding this comment

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 3, 2024

github-actions bot commented Mar 3, 2024

github-actions bot commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mohamedsamehsalah Apr 26, 2024 • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2024

github-actions bot commented Apr 19, 2024

rickie commented Apr 20, 2024

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented May 2, 2024

Quality Gate passed

github-actions bot commented May 2, 2024

Introduce `Slf4jLogDeclaration` to canonicalize slf4j's variable's declaration #783

Introduce `Slf4jLogDeclaration` to canonicalize slf4j's variable's declaration #783

mohamedsamehsalah commented Sep 8, 2023 •

edited by rickie

mohamedsamehsalah Nov 25, 2023 •

edited

mohamedsamehsalah Apr 26, 2024 •

edited