Skip to content

Commit

Permalink
Address comments: Identify suggestion blockers
Browse files Browse the repository at this point in the history
  • Loading branch information
mohamedsamehsalah committed Sep 23, 2023
1 parent 42a7575 commit 40a2a6f
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 10 deletions.
59 changes: 51 additions & 8 deletions ...-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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;

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

@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<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()) {

Check warning on line 111 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 111 without causing a test to fail 

removed conditional - replaced equality check with false (covered by 3 tests RemoveConditionalMutator_EQUAL_ELSE)
return Description.NO_MATCH;

Check warning on line 112 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 112 without causing a test to fail 

replaced return value with null for matchClass (no tests cover this line NullReturnValsMutator)
}

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)

Check warning on line 119 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 119 without causing a test to fail 

removed conditional - replaced equality check with true (covered by 3 tests RemoveConditionalMutator_EQUAL_IF)
&& isFieldAccessModifierApplicable(variableTree, state))) {
return Description.NO_MATCH;
}

VarSymbol variableSymbol = ASTHelpers.getSymbol(variableTree);
if (variableSymbol == null) {

Check warning on line 125 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 125 without causing a test to fail 

removed conditional - replaced equality check with false (covered by 3 tests RemoveConditionalMutator_EQUAL_ELSE)
return Description.NO_MATCH;

Check warning on line 126 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 126 without causing a test to fail 

replaced return value with null for matchClass (no tests cover this line NullReturnValsMutator)
}

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);

Check warning on line 157 in error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CanonicalConstantFieldName.java

View workflow job for this annotation

GitHub Actions / pitest

A change can be made to line 157 without causing a test to fail 

removed conditional - replaced equality check with true (covered by 3 tests RemoveConditionalMutator_EQUAL_IF)
}
Expand Down
56 changes: 54 additions & 2 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 @@ -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())
Expand All @@ -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;",
" }",
"}")
Expand All @@ -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);
}
Expand Down

0 comments on commit 40a2a6f

Please sign in to comment.