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.

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.

…claration

sonarcloud · 2024-09-24T08:20:57Z

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-09-24T08:21: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

With the final discussion resolved, it's actually time for the approval. Nice work again @mohamedsamehsalah ! 🚀 Can't wait to roll this out.

Badbond · 2024-09-26T10:18:20Z

While trying to introduce a similar ArchUnit rule (for the interim period), we spotted another interesting use-case that you might want to consider for this bug check. Essentially there is a use-case to not have a static logger and use this.getClass() for the logger class:

public abstract class AbstractHandler {
  /** A logger for the concrete class. */
  private final Logger log = LoggerFactory.getLogger(this.getClass());
  
  public void someMethod() {
    log.info("Doing something"); // Logs using the concrete class, MyHandler in this example.
  }
}

final class MyHandler extends AbstractHandler {}

We have several such cases in the internal code base and I think this is quite an elegant solution to make sure the right class is mentioned in the log lines. Might be nice to make sure this check works with this case.

Alternatively, but IMO not better, the abstract class can force the concrete class to provide the instance:

public abstract class AbstractHandler {
  public void doSomething() {
    getLogger().info("Doing something");
  }

  abstract Logger getLogger();
}

final class MyHandler extends AbstractHandler {
  private static final Logger LOG = LoggerFactory.getLogger(MyHandler.class);

  @Override
  Logger getLogger() {
    return LOG;
  }
}

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

Stephan202 modified the milestones: 0.17.0, 0.18.0 Jul 20, 2024

Stephan202 modified the milestones: 0.18.0, 0.19.0 Aug 11, 2024

mohamedsamehsalah and others added 19 commits September 24, 2024 10:18

Introduce Slf4jLogDeclaration to canonicalize slf4j's variable's de…

a1b03ed

…claration

Drop redundant check

d2bf6a5

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

af29373

Don't skip interfaces

23d3e4b

Ommit modifying interface variables modifiers (heh)

9d7d195

Fix checkstyles

7a5fb99

TIL: Illegal static declaration in inner class

eb2fa4b

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

aa65317

Fix nested class import checkstyle

4ba92bd

Suggestions

7df687b

Suggestions part 2

1adf468

Suggestions part 3

e2c2a04

Pair programming

df60311

Comments

7d2a645

Kill mutants & handle getLogger overload

9f64e18

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

40a5b58

Suggestions

b85b5b3

Simplify some code, forgot to push

4949658

Suggestions

7544755

rickie force-pushed the mohamedsamehsalah/632-canonical-slf4j-logger-usage branch from a5995c8 to 7544755 Compare September 24, 2024 08:18

rickie approved these changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Loading

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

Stephan202 Oct 17, 2023

mohamedsamehsalah Nov 25, 2023

Stephan202 Oct 17, 2023

mohamedsamehsalah Nov 25, 2023 •

edited

Loading

Stephan202 Oct 17, 2023

mohamedsamehsalah Nov 25, 2023

github-actions bot commented Nov 25, 2023

sonarcloud bot commented Nov 25, 2023

github-actions bot commented Nov 25, 2023

rickie left a comment

mohamedsamehsalah left a comment

github-actions bot commented Mar 3, 2024

github-actions bot commented Mar 3, 2024

sonarcloud bot commented May 2, 2024

github-actions bot commented May 2, 2024

sonarcloud bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024

rickie left a comment

Badbond commented Sep 26, 2024 •

edited

Loading

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 Loading

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

Stephan202 Oct 17, 2023

Choose a reason for hiding this comment

mohamedsamehsalah Nov 25, 2023

Choose a reason for hiding this comment

Stephan202 Oct 17, 2023

Choose a reason for hiding this comment

mohamedsamehsalah Nov 25, 2023 • edited Loading

Choose a reason for hiding this comment

Stephan202 Oct 17, 2023

Choose a reason for hiding this comment

mohamedsamehsalah Nov 25, 2023

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

sonarcloud bot commented May 2, 2024

Quality Gate passed

github-actions bot commented May 2, 2024

sonarcloud bot commented Sep 24, 2024

Quality Gate passed

github-actions bot commented Sep 24, 2024

rickie left a comment

Choose a reason for hiding this comment

Badbond commented Sep 26, 2024 • edited Loading

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

Loading

mohamedsamehsalah Nov 25, 2023 •

edited

Loading

Badbond commented Sep 26, 2024 •

edited

Loading