Skip to content

Commit

Permalink
Address comments: Match on compilation unit and bug fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
mohamedsamehsalah committed Oct 2, 2023
1 parent cdb5880 commit 532dcdd
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 42 deletions.
84 changes: 46 additions & 38 deletions ...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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<Tree> IS_CONSTANT =
allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL));
Expand Down Expand Up @@ -98,46 +101,51 @@ public CanonicalConstantFieldName() {
}

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
ImmutableList<String> 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<VariableTree> 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<VariableTree> variables = variablesInCompilationUnitBuilder.build();
if (variables.isEmpty()) {
return Description.NO_MATCH;
}

ImmutableList<VarSymbol> 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());
}
Expand Down
72 changes: 68 additions & 4 deletions ...trib/src/test/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldNameTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
}

0 comments on commit 532dcdd

Please sign in to comment.