Introduce `CanonicalConstantFieldName` bug checker #794

mohamedsamehsalah · 2023-09-17T17:27:46Z

Suggested commit message:

Introduce `CanonicalConstantFieldName` check (#794)

github-actions · 2023-09-17T17:31:10Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	5	13

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

mohamedsamehsalah

In addition to class variables within the same compilation unit, should we also match on constants imported from other classes ?
Ideally, the checker will suggest the fix in that class 🤔

Which raises the other question, do we really need to suggest a fix for the other classes referencing the constants in this compilation unit ?

@Stephan202 suggested the use of TreeScanner as in this example but I could not seem to make this work 😓

Help appreciated 🙏

pom.xml

github-actions · 2023-09-17T17:35:44Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	5	13

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

Stephan202

Tnx for working on this @mohamedsamehsalah; it'll allow for some nice cleanup I expect. Did leave some comments based on a quick skim :)

pom.xml

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

...contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

Stephan202 · 2023-09-17T18:52:51Z

(I didn't check the comments before reviewing; sorry.)

In addition to class variables within the same compilation unit, should we also match on constants imported from other classes ?
Ideally, the checker will suggest the fix in that class 🤔

Error Prone doesn't support this; for cases like that OpenRewrite would be better suited. (But for now, let's just focus on the private fields.)

@Stephan202 suggested the use of TreeScanner as in this example but I could not seem to make this work 😓

Anywhere we can see the code that doesn't work? Perhaps I can take a look.

github-actions · 2023-09-19T09:29:44Z

Surviving mutants in this change: 11
Killed mutants in this change: 14

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	11	14

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

github-actions · 2023-09-23T17:53:36Z

Surviving mutants in this change: 14
Killed mutants in this change: 28

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	14	28

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

github-actions · 2023-09-23T20:17:02Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	8	34
🧟`tech.picnic.errorprone.bugpatterns.RefasterMethodParameterOrder`	1	20
🧟`tech.picnic.errorprone.bugpatterns.RefasterMethodParameterOrder$1`	1	2

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

github-actions · 2023-09-23T20:23:43Z

Surviving mutants in this change: 8
Killed mutants in this change: 34

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	8	34

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

mohamedsamehsalah · 2023-09-23T20:27:01Z

@Stephan202 I covered your first pass comments, however, I still could not make use of the TreeScanner. Or in other words, I couldn't find what advantage does the TreeScanner give that the TreeMatcher does not.

AFAIU, the suggested fix builder used in this PR will execute what you are suggesting in this comment:

IIUC this change will suggest many distinct fixes. It might work in practice, but it'll be nicer if there's a single report against the field, which, when applied, renames all references to that Symbol. (That's why offline I suggested using a TreeScanner :)

Happy to hear your thoughts (again? 😅) 🙏

github-actions · 2023-09-23T20:31:44Z

Surviving mutants in this change: 8
Killed mutants in this change: 34

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	8	34

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

Stephan202

@Stephan202 I covered your first pass comments, however, I still could not make use of the TreeScanner. Or in other words, I couldn't find what advantage does the TreeScanner give that the TreeMatcher does not.

Actually, you are using a TreeScanner now, just through a method I didn't know existed: have a close look at SuggestedFixes.renameVariable 👀 😄 This should work!

Before I dive into a more thorough review: would you like to have a crack at resolving the surviving mutants?

Stephan202 · 2023-09-24T07:49:55Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+        if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) {
+          String replacement = toUpperSnakeCase(variableName);
+
+          if (!classVariables.contains(replacement)) {


IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner may be employed.

would you like to have a crack at resolving the surviving mutants?

Yes, sir 🖖

IIUC, here we avoid renaming a variable if the target name is already declared in the exact same scope. Due to the global search and replace that we do, I suspect that any usage of the target name in this CompilationUnit could cause ambiguity, shadowing or confusion. So for this case, too, a TreeScanner may be employed.

Will look into this 👀

Thanks.

github-actions · 2023-10-01T13:04:28Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	3	37
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName$1`	1	2

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

github-actions · 2023-10-02T06:51:21Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	2	34
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName$1`	1	2

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

github-actions · 2023-10-02T06:55:47Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	2	34
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName$1`	1	2

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

sonarcloud · 2023-10-02T06:56:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

94.9% Coverage
0.0% Duplication

github-actions · 2023-10-02T06:56:17Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	2	34
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName$1`	1	2

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

mohamedsamehsalah

Sorry for the slow progress on this @Stephan202 , PTAL 🙏

rickie

As a result of dropping, we can simplify the code, pushing a suggestion!

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

github-actions · 2024-04-05T09:09:02Z

Surviving mutants in this change: 1
Killed mutants in this change: 25

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.CanonicalConstantFieldName`	0	25

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

rickie · 2024-04-05T09:28:34Z

/integration-test

mohamedsamehsalah · 2024-04-05T12:25:59Z

Thanks @rickie , changes LGMT 🚀

github-actions · 2024-04-08T05:36:33Z

Surviving mutants in this change: 1
Killed mutants in this change: 28

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantNaming$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.CanonicalConstantNaming`	0	28

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

rickie

Hey @mohamedsamehsalah , thanks a lot for pushing this! It looks really nice and am curious to see this getting rolled out on Picnic's codebase 🔥 !!!

rickie · 2024-04-05T15:36:33Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+    linkType = CUSTOM,
+    severity = WARNING,
+    tags = LIKELY_ERROR)
+public final class CanonicalConstantFieldName extends BugChecker implements VariableTreeMatcher {


I think we can omit the word "field" from most things in this class. As a constant is actually always a field right? 🤔

rickie · 2024-04-05T15:37:20Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+  private static final Matcher<Tree> IS_CONSTANT =
+      allOf(
+          hasModifier(Modifier.STATIC), hasModifier(Modifier.PRIVATE), hasModifier(Modifier.FINAL));
+  private static final Pattern TO_SNAKE_CASE = Pattern.compile("([a-z])([A-Z])");


This is actually not the TO_ snake case, it's the pattern for SNAKE_CASE, right?

rickie · 2024-04-05T15:43:06Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+  private static final Pattern TO_SNAKE_CASE = Pattern.compile("([a-z])([A-Z])");
+  private static final ImmutableSet<String> DEFAULT_EXCLUDED_CONSTANT_FIELD_NAMES =
+      ImmutableSet.of("serialVersionUID");
+  private static final String EXCLUDED_CONSTANT_FIELD_NAMES =


Suggested change

private static final String EXCLUDED_CONSTANT_FIELD_NAMES =

private static final String ALLOWED_CONSTANT_NAMES_FLAG =

Let's add the FLAG suffix and maybe instead of excluded maybe we should use the word "allowed", WDYT?

rickie · 2024-04-08T05:26:16Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+  @Override
+  public Description matchVariable(VariableTree tree, VisitorState state) {
+    String variableName = tree.getName().toString();
+    if (IS_CONSTANT.matches(tree, state)


If we flip the conditions here and do an early return, we have one level of indentation less. Additionally, we generally try to have early returns if possible :).

rickie · 2024-04-08T05:31:43Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+    return Description.NO_MATCH;
+  }
+
+  private static ImmutableList<VariableTree> getVariablesInCompilationUnit(


Changed the order of static methods in the class based on which one is used first :).

rickie · 2024-04-08T05:32:09Z

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

+      ImmutableList<VariableTree> variablesInCompilationUnit =
+          getVariablesInCompilationUnit(state.getPath().getCompilationUnit());
+      String replacement = toUpperSnakeCase(variableName);
+      if (variablesInCompilationUnit.stream()


Just a suggestion but what if we extract this to a method, I think it reads nicely. Additionally if we use anyMatch instead of noneMatch it reads as "isVariableNameInUse`, instead of the negated variant, could be a bit easier to understand.

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

github-actions · 2024-04-08T06:32:05Z

Surviving mutants in this change: 1
Killed mutants in this change: 28

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.CanonicalConstantNaming$1`	1	0
🎉`tech.picnic.errorprone.bugpatterns.CanonicalConstantNaming`	0	28

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

mohamedsamehsalah · 2024-04-08T06:59:32Z

Thanks for suggestions @rickie . LGTM 🚀

mohamedsamehsalah requested a review from Stephan202 September 17, 2023 17:27

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

Canonical constant naming #396

Open

2 tasks

mohamedsamehsalah commented Sep 17, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from 31a9e4d to 7fff49f Compare September 17, 2023 17:33

Stephan202 reviewed Sep 17, 2023

View reviewed changes

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from 4acc2bc to ecdccc3 Compare September 23, 2023 20:14

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from ecdccc3 to 40a2a6f Compare September 23, 2023 20:21

mohamedsamehsalah requested a review from Stephan202 September 23, 2023 20:27

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from 40a2a6f to 0c94940 Compare September 23, 2023 20:29

Stephan202 reviewed Sep 24, 2023

View reviewed changes

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch 2 times, most recently from 1c55636 to 12e51ab Compare October 2, 2023 06:53

mohamedsamehsalah requested a review from Stephan202 October 2, 2023 07:03

mohamedsamehsalah commented Oct 2, 2023

View reviewed changes

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

mohamedsamehsalah force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from 12e51ab to a7f652e Compare November 25, 2023 20:42

rickie reviewed Apr 5, 2024

View reviewed changes

...one-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java Outdated Show resolved Hide resolved

rickie approved these changes Apr 8, 2024

View reviewed changes

mohamedsamehsalah and others added 16 commits April 8, 2024 08:29

Introduce CanonicalConstantFieldName bug checker

88c40e2

Address comments: Use Pattern matching instead of replaceAll

ea2ee7f

Address comments: Exclude serialVersionUID field name by default

f12a2aa

Address comments: Conditionally include public constant fields

3ea7959

Address comments: Identify suggestion blockers

fce8827

Address comments: Match on compilation unit and bug fixes

686a81f

Address comments: Fix mutation tests

c6a0e84

Small tweaks

07b8cd2

Add some tweaks and failing test cases

f635f5c

Second round of suggestions

0ced369

Report and fix match on variable level

33c274e

Simplify and drop public support

d3f55fb

Suggestions

7711a7e

Re-order and flip for early return

0c82378

Add some test cases

dbb63c5

Format

0728480

rickie force-pushed the mohamedsamehsalah/396-canonical-constant-naming branch from 9605dfe to 0728480 Compare April 8, 2024 06:29

rickie added this to the 0.17.0 milestone Apr 8, 2024

rickie added the new feature label Apr 8, 2024

+                void identification() {
+                  CompilationTestHelper.newInstance(CanonicalConstantFieldName.class, getClass())
+                      .addSourceLines(
+                          "A.java",

	private static final String EXCLUDED_CONSTANT_FIELD_NAMES =
	private static final String ALLOWED_CONSTANT_NAMES_FLAG =

Introduce CanonicalConstantFieldName bug checker #794

Are you sure you want to change the base?

Introduce CanonicalConstantFieldName bug checker #794

Conversation

mohamedsamehsalah commented Sep 17, 2023 • edited by rickie

github-actions bot commented Sep 17, 2023

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 commented Sep 17, 2023 • edited

github-actions bot commented Sep 19, 2023

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 23, 2023

github-actions bot commented Sep 23, 2023

mohamedsamehsalah commented Sep 23, 2023

github-actions bot commented Sep 23, 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

github-actions bot commented Oct 1, 2023

github-actions bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

sonarcloud bot commented Oct 2, 2023

github-actions bot commented Oct 2, 2023

mohamedsamehsalah left a comment

Choose a reason for hiding this comment

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024

rickie commented Apr 5, 2024

mohamedsamehsalah commented Apr 5, 2024

github-actions bot commented Apr 8, 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

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

github-actions bot commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

sonarcloud bot commented Apr 8, 2024

Quality Gate passed

github-actions bot commented Apr 8, 2024

mohamedsamehsalah commented Apr 8, 2024

Introduce `CanonicalConstantFieldName` bug checker #794

Introduce `CanonicalConstantFieldName` bug checker #794

mohamedsamehsalah commented Sep 17, 2023 •

edited by rickie

Stephan202 commented Sep 17, 2023 •

edited