From 0c94940609b82f3fdb2bcb250956f586f8c2845d Mon Sep 17 00:00:00 2001 From: mohamedsamehsalah Date: Sat, 23 Sep 2023 19:50:28 +0200 Subject: [PATCH] Address comments: Identify suggestion blockers --- .../CanonicalConstantFieldName.java | 63 ++++++++++++++++--- .../CanonicalConstantFieldNameTest.java | 56 ++++++++++++++++- 2 files changed, 107 insertions(+), 12 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 3c3a8284935..6e776420921 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 @@ -1,5 +1,6 @@ package tech.picnic.errorprone.bugpatterns; +import static com.google.common.collect.ImmutableList.toImmutableList; 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; @@ -14,19 +15,22 @@ 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.bugpatterns.BugChecker.ClassTreeMatcher; 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.Tree; +import com.sun.source.tree.Tree.Kind; import com.sun.source.tree.VariableTree; 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 tech.picnic.errorprone.bugpatterns.util.Flags; /** @@ -62,7 +66,7 @@ linkType = CUSTOM, severity = WARNING, tags = LIKELY_ERROR) -public final class CanonicalConstantFieldName extends BugChecker implements VariableTreeMatcher { +public final class CanonicalConstantFieldName extends BugChecker implements ClassTreeMatcher { private static final long serialVersionUID = 1L; private static final Matcher IS_CONSTANT = allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL)); @@ -90,26 +94,65 @@ public CanonicalConstantFieldName() { @Inject CanonicalConstantFieldName(ErrorProneFlags flags) { optionalExcludedConstantFliedNames = getCanonicalizedLoggerName(flags); - includePublicConstantFieldNames = getIncludePrivateConstantFieldNames(flags); + includePublicConstantFieldNames = isIncludePrivateConstantFieldNames(flags); } @Override - public Description matchVariable(VariableTree tree, VisitorState state) { - if (!(IS_CONSTANT.matches(tree, state) && isFieldAccessModifierApplicable(tree, state))) { + 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()) { return Description.NO_MATCH; } + 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; + } - VarSymbol variableSymbol = ASTHelpers.getSymbol(tree); - String variableName = variableSymbol.getSimpleName().toString(); + String variableName = variableSymbol.getSimpleName().toString(); + if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) { + String replacement = toUpperSnakeCase(variableName); - if (!isVariableUpperSnakeCase(variableName) && !isVariableNameExcluded(variableName)) { - fixBuilder.merge(SuggestedFixes.renameVariable(tree, toUpperSnakeCase(variableName), state)); + if (!classVariables.contains(replacement)) { + fixBuilder.merge(SuggestedFixes.renameVariable(variableTree, replacement, state)); + } else { + reportConstantRenameBlocker(variableTree, replacement, state); + } + } + } } return fixBuilder.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fixBuilder.build()); } + private void reportConstantRenameBlocker( + VariableTree tree, String replacement, VisitorState state) { + state.reportMatch( + buildDescription(tree) + .setMessage( + String.format( + "Suggested fix for constant name conflicts with an already defined variable `%s`.", + replacement)) + .build()); + } + private boolean isFieldAccessModifierApplicable(VariableTree tree, VisitorState state) { return includePublicConstantFieldNames || IS_PRIVATE.matches(tree, state); } @@ -131,7 +174,7 @@ private static ImmutableList getCanonicalizedLoggerName(ErrorProneFlags return Flags.getList(flags, EXCLUDED_CONSTANT_FIELD_NAMES); } - private static boolean getIncludePrivateConstantFieldNames(ErrorProneFlags flags) { + private static boolean isIncludePrivateConstantFieldNames(ErrorProneFlags flags) { return flags.getBoolean(IS_INCLUDE_PUBLIC_CONSTANT_FIELDS).orElse(false); } } 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 36c29667ba5..553f7011d24 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 @@ -2,9 +2,25 @@ import com.google.errorprone.BugCheckerRefactoringTestHelper; import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; final class CanonicalConstantFieldNameTest { + @Test + void identification() { + CompilationTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) + .addSourceLines( + "A.java", + "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 = 2;", + "}") + .doTest(); + } + @Test void replacement() { BugCheckerRefactoringTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) @@ -13,7 +29,13 @@ void replacement() { "class A {", " private static final int number = 1;", "", - " static int getNumber() {", + " static int getConstantNumber() {", + " return number;", + " }", + "", + " static int getLocalNumber() {", + " int number = 2;", + "", " return number;", " }", "}") @@ -22,9 +44,39 @@ void replacement() { "class A {", " private static final int NUMBER = 1;", "", - " static int getNumber() {", + " static int getConstantNumber() {", " return NUMBER;", " }", + "", + " static int getLocalNumber() {", + " int number = 2;", + "", + " return number;", + " }", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void doNotReplaceExcludedOrPublicConstantsByDefault() { + BugCheckerRefactoringTestHelper.newInstance(CanonicalConstantFieldName.class, getClass()) + .addInputLines( + "A.java", + "class A {", + " private static final long serialVersionUID = 1L;", + "", + " public static final int number = 1;", + "", + " static final int anotherNumber = 2;", + "}") + .addOutputLines( + "A.java", + "class A {", + " private static final long serialVersionUID = 1L;", + "", + " public static final int number = 1;", + "", + " static final int anotherNumber = 2;", "}") .doTest(TestMode.TEXT_MATCH); }