From e79584ee563f65add7e7faa92bbadbe52dd66e89 Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sun, 1 Oct 2023 14:29:29 +0200 Subject: [PATCH] Address comments: Match on compilation unit and bug fixes --- .../CanonicalConstantFieldName.java | 84 ++++++++++--------- .../CanonicalConstantFieldNameTest.java | 72 +++++++++++++++- 2 files changed, 114 insertions(+), 42 deletions(-) diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java index 6e77642092..80a5614fec 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java @@ -15,22 +15,24 @@ 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.CompilationUnitTreeMatcher; 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.CompilationUnitTree; 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.VarSymbol; import java.util.Locale; 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.bugpatterns.util.Flags; /** @@ -66,7 +68,8 @@ linkType = CUSTOM, severity = WARNING, tags = LIKELY_ERROR) -public final class CanonicalConstantFieldName extends BugChecker implements ClassTreeMatcher { +public final class CanonicalConstantFieldName extends BugChecker + implements CompilationUnitTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher IS_CONSTANT = allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL)); @@ -98,46 +101,51 @@ public CanonicalConstantFieldName() { } @Override - public Description matchClass(ClassTree tree, VisitorState state) { - ImmutableList classVariables = - tree.getMembers().stream() - .filter(member -> member.getKind() == Kind.VARIABLE) - .map(VariableTree.class::cast) - .map(ASTHelpers::getSymbol) - .map(VarSymbol::getSimpleName) - .map(Name::toString) - .collect(toImmutableList()); - - if (classVariables.isEmpty()) { + public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) { + ImmutableList.Builder variablesInCompilationUnitBuilder = ImmutableList.builder(); + new TreeScanner<@Nullable Void, @Nullable Void>() { + @Override + public @Nullable Void visitClass(ClassTree classTree, @Nullable Void unused) { + for (Tree member : classTree.getMembers()) { + if (member.getKind() == Kind.VARIABLE) { + variablesInCompilationUnitBuilder.add((VariableTree) member); + } + } + return super.visitClass(classTree, unused); + } + }.scan(tree, null); + + ImmutableList variables = variablesInCompilationUnitBuilder.build(); + if (variables.isEmpty()) { return Description.NO_MATCH; } + ImmutableList variableSymbols = + variables.stream().map(ASTHelpers::getSymbol).collect(toImmutableList()); SuggestedFix.Builder fixBuilder = SuggestedFix.builder(); - for (Tree member : tree.getMembers()) { - if (member.getKind() == Kind.VARIABLE) { - VariableTree variableTree = (VariableTree) member; - if (!(IS_CONSTANT.matches(variableTree, state) - && isFieldAccessModifierApplicable(variableTree, state))) { - return Description.NO_MATCH; - } - - VarSymbol variableSymbol = ASTHelpers.getSymbol(variableTree); - if (variableSymbol == null) { - return Description.NO_MATCH; - } - - String variableName = variableSymbol.getSimpleName().toString(); - if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) { - String replacement = toUpperSnakeCase(variableName); - - if (!classVariables.contains(replacement)) { - fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, replacement, state)); - } else { - reportConstantRenameBlocker(variableTree, replacement, state); + variables.forEach( + variableTree -> { + if (IS_CONSTANT.matches(variableTree, state) + && isFieldAccessModifierApplicable(variableTree, state)) { + VarSymbol variableSymbol = ASTHelpers.getSymbol(variableTree); + + if (variableSymbol != null) { + String variableName = variableSymbol.getSimpleName().toString(); + + if (!isVariableUpperSnakeCase(variableName) + && !isVariableNameExcluded(variableName)) { + String replacement = toUpperSnakeCase(variableName); + + if (variableSymbols.stream() + .noneMatch(s -> s.getSimpleName().toString().equals(replacement))) { + fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, replacement, state)); + } else { + reportConstantRenameBlocker(variableTree, replacement, state); + } + } + } } - } - } - } + }); return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java index 553f7011d2..09415f681c 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java @@ -14,9 +14,11 @@ void identification() { "class A {", " // BUG: Diagnostic contains: Suggested fix for constant name conflicts with an already defined", " // variable `NUMBER`.", - " private static final int number = 1;", + " private static final int number = B.NUMBER;", "", - " private static final int NUMBER = 2;", + " class B {", + " private static final int NUMBER = 1;", + " }", "}") .doTest(); } @@ -29,30 +31,50 @@ void replacement() { "class A {", " private static final int number = 1;", "", + " int nonConstantNumber = 2;", + "", + " int referenceToNumberFromAnotherClass = B.numberFromAnotherClass;", + "", " static int getConstantNumber() {", " return number;", " }", "", " static int getLocalNumber() {", - " int number = 2;", + " int number = 3;", "", " return number;", " }", + "", + " class B {", + " private static final int number = 4;", + "", + " private static final int numberFromAnotherClass = 5;", + " }", "}") .addOutputLines( "A.java", "class A {", " private static final int NUMBER = 1;", "", + " int nonConstantNumber = 2;", + "", + " int referenceToNumberFromAnotherClass = B.NUMBER_FROM_ANOTHER_CLASS;", + "", " static int getConstantNumber() {", " return NUMBER;", " }", "", " static int getLocalNumber() {", - " int number = 2;", + " int number = 3;", "", " return number;", " }", + "", + " class B {", + " private static final int NUMBER = 4;", + "", + " private static final int NUMBER_FROM_ANOTHER_CLASS = 5;", + " }", "}") .doTest(TestMode.TEXT_MATCH); } @@ -80,4 +102,46 @@ void doNotReplaceExcludedOrPublicConstantsByDefault() { "}") .doTest(TestMode.TEXT_MATCH); } + + @Test + void excludeFlaggedConstants() { + BugCheckerRefactoringTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) + .setArgs("-XepOpt:CanonicalConstantFieldName:ExcludedConstantFliedNames=excludedField") + .addInputLines( + "A.java", + "class A {", + " private static final int number = 1;", + "", + " private static final int excludedField = 3;", + "}") + .addOutputLines( + "A.java", + "class A {", + " private static final int NUMBER = 1;", + "", + " private static final int excludedField = 3;", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void IncludePublicConstants() { + BugCheckerRefactoringTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) + .setArgs("-XepOpt:CanonicalConstantFieldName:IncludePublicConstantFields=true") + .addInputLines( + "A.java", + "class A {", + " int nonConstantNumber = 1;", + "", + " public static final int number = 2;", + "}") + .addOutputLines( + "A.java", + "class A {", + " int nonConstantNumber = 1;", + "", + " public static final int NUMBER = 2;", + "}") + .doTest(TestMode.TEXT_MATCH); + } }