diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ConstantNaming.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ConstantNaming.java new file mode 100644 index 0000000000..a0354a83c6 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ConstantNaming.java @@ -0,0 +1,125 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.STYLE; +import static com.google.errorprone.matchers.Matchers.allOf; +import static com.google.errorprone.matchers.Matchers.hasModifier; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.google.errorprone.BugPattern; +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.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.VariableTree; +import com.sun.source.util.TreeScanner; +import java.util.Locale; +import java.util.regex.Pattern; +import javax.inject.Inject; +import javax.lang.model.element.Modifier; +import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.utils.Flags; + +/** + * A {@link BugChecker} that flags static constants that do not follow the upper snake case naming + * convention. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Constant variables should adhere to the `UPPER_SNAKE_CASE` naming convention", + link = BUG_PATTERNS_BASE_URL + "ConstantNaming", + linkType = CUSTOM, + severity = WARNING, + tags = STYLE) +@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */) +public final class ConstantNaming extends BugChecker implements VariableTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher IS_CONSTANT = + allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL)); + private static final Matcher IS_PRIVATE = hasModifier(Modifier.PRIVATE); + private static final Pattern SNAKE_CASE = Pattern.compile("([a-z])([A-Z])"); + private static final ImmutableSet DEFAULT_EXEMPTED_NAMES = + ImmutableSet.of("serialVersionUID"); + + /** + * Flag using which constant names that must not be flagged (in addition to those defined by + * {@link #DEFAULT_EXEMPTED_NAMES}) can be specified. + */ + private static final String ADDITIONAL_EXEMPTED_NAMES_FLAG = + "CanonicalConstantNaming:ExemptedNames"; + + private final ImmutableSet exemptedNames; + + /** Instantiates a default {@link ConstantNaming} instance. */ + public ConstantNaming() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link ConstantNaming}. + * + * @param flags Any provided command line flags. + */ + @Inject + ConstantNaming(ErrorProneFlags flags) { + exemptedNames = + Sets.union(DEFAULT_EXEMPTED_NAMES, Flags.getSet(flags, ADDITIONAL_EXEMPTED_NAMES_FLAG)) + .immutableCopy(); + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + String variableName = tree.getName().toString(); + if (!IS_CONSTANT.matches(tree, state) || exemptedNames.contains(variableName)) { + return Description.NO_MATCH; + } + + String replacement = toUpperSnakeCase(variableName); + if (replacement.equals(variableName)) { + return Description.NO_MATCH; + } + + Description.Builder description = buildDescription(tree); + if (!IS_PRIVATE.matches(tree, state)) { + description.setMessage( + "%s; consider renaming to '%s', though note that this is not a private constant" + .formatted(message(), replacement)); + } else if (isVariableNameInUse(replacement, state)) { + description.setMessage( + "%s; consider renaming to '%s', though note that a variable with this name is already declared" + .formatted(message(), replacement)); + } else { + description.addFix(SuggestedFixes.renameVariable(tree, replacement, state)); + } + + return description.build(); + } + + private static String toUpperSnakeCase(String variableName) { + return SNAKE_CASE.matcher(variableName).replaceAll("$1_$2").toUpperCase(Locale.ROOT); + } + + private static boolean isVariableNameInUse(String name, VisitorState state) { + return Boolean.TRUE.equals( + new TreeScanner() { + @Override + public Boolean visitVariable(VariableTree tree, @Nullable Void unused) { + return ASTHelpers.getSymbol(tree).getSimpleName().toString().equals(name) + || super.visitVariable(tree, null); + } + + @Override + public Boolean reduce(Boolean r1, Boolean r2) { + return Boolean.TRUE.equals(r1) || Boolean.TRUE.equals(r2); + } + }.scan(state.getPath().getCompilationUnit(), null)); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java index fca196b464..a788c6991f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java @@ -14,6 +14,7 @@ import com.google.common.collect.Comparators; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; @@ -34,10 +35,8 @@ import com.sun.source.util.TreeScanner; import com.sun.tools.javac.code.Symtab; import com.sun.tools.javac.code.Type; -import java.util.HashSet; import java.util.List; import java.util.Optional; -import java.util.Set; import java.util.stream.Stream; import javax.inject.Inject; import org.jspecify.annotations.Nullable; @@ -230,10 +229,8 @@ private static AnnotationAttributeMatcher createAnnotationAttributeMatcher( excludedAnnotations(flags)); } - private static ImmutableList excludedAnnotations(ErrorProneFlags flags) { - Set exclusions = new HashSet<>(); - exclusions.addAll(Flags.getList(flags, EXCLUDED_ANNOTATIONS_FLAG)); - exclusions.addAll(BLACKLISTED_ANNOTATIONS); - return ImmutableList.copyOf(exclusions); + private static ImmutableSet excludedAnnotations(ErrorProneFlags flags) { + return Sets.union(BLACKLISTED_ANNOTATIONS, Flags.getSet(flags, EXCLUDED_ANNOTATIONS_FLAG)) + .immutableCopy(); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java index bc6dadbd2f..689996a06c 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java @@ -378,11 +378,11 @@ private Description createDescription(Tree tree, Optional private static Matcher createConversionMethodMatcher( ErrorProneFlags flags) { - // XXX: ErrorProneFlags#getList splits by comma, but method signatures may also contain commas. - // For this class methods accepting more than one argument are not valid, but still: not nice. + // XXX: `Flags#getSet` splits by comma, but method signatures may also contain commas. For this + // class methods accepting more than one argument are not valid, but still: not nice. return anyOf( WELL_KNOWN_STRING_CONVERSION_METHODS, new MethodMatcherFactory() - .create(Flags.getList(flags, EXTRA_STRING_CONVERSION_METHODS_FLAG))); + .create(Flags.getSet(flags, EXTRA_STRING_CONVERSION_METHODS_FLAG))); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java index ee736da113..3d2aef0c92 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RequestParamType.java @@ -15,8 +15,8 @@ import com.google.auto.service.AutoService; import com.google.common.collect.ImmutableCollection; -import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.BugPattern; import com.google.errorprone.ErrorProneFlags; import com.google.errorprone.VisitorState; @@ -74,10 +74,10 @@ private static Matcher hasUnsupportedRequestParamType(ErrorProneFl return allOf( annotations(AT_LEAST_ONE, isType("org.springframework.web.bind.annotation.RequestParam")), anyOf(isSubtypeOf(ImmutableCollection.class), isSubtypeOf(ImmutableMap.class)), - not(isSubtypeOfAny(Flags.getList(flags, SUPPORTED_CUSTOM_TYPES_FLAG)))); + not(isSubtypeOfAny(Flags.getSet(flags, SUPPORTED_CUSTOM_TYPES_FLAG)))); } - private static Matcher isSubtypeOfAny(ImmutableList inclusions) { + private static Matcher isSubtypeOfAny(ImmutableSet inclusions) { return anyOf( inclusions.stream() .map(inclusion -> isSubtypeOf(Suppliers.typeFromString(inclusion))) diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ConstantNamingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ConstantNamingTest.java new file mode 100644 index 0000000000..889f8599a3 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ConstantNamingTest.java @@ -0,0 +1,78 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class ConstantNamingTest { + @Test + void identification() { + CompilationTestHelper.newInstance(ConstantNaming.class, getClass()) + .addSourceLines( + "A.java", + "class A {", + " private static final long serialVersionUID = 1L;", + " private static final int FOO = 1;", + " // BUG: Diagnostic contains: consider renaming to 'BAR', though note that this is not a private", + " // constant", + " static final int bar = 2;", + " // BUG: Diagnostic contains:", + " private static final int baz = 3;", + " // BUG: Diagnostic contains: consider renaming to 'QUX_QUUX', though note that a variable with", + " // this name is already declared", + " private static final int qux_QUUX = 4;", + " // BUG: Diagnostic contains: consider renaming to 'QUUZ', though note that a variable with", + " // this name is already declared", + " private static final int quuz = 3;", + "", + " private final int foo = 4;", + " private final Runnable QUX_QUUX =", + " new Runnable() {", + " private static final int QUUZ = 1;", + "", + " @Override", + " public void run() {}", + " };", + "}") + .doTest(); + } + + @Test + void identificationWithCustomExemption() { + CompilationTestHelper.newInstance(ConstantNaming.class, getClass()) + .setArgs("-XepOpt:CanonicalConstantNaming:ExemptedNames=foo,baz") + .addSourceLines( + "A.java", + "class A {", + " private static final long serialVersionUID = 1L;", + " private static final int foo = 1;", + " // BUG: Diagnostic contains:", + " private static final int bar = 2;", + " private static final int baz = 3;", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance(ConstantNaming.class, getClass()) + .addInputLines( + "A.java", + "class A {", + " static final int foo = 1;", + " private static final int bar = 2;", + " private static final int baz = 3;", + " private static final int BAZ = 4;", + "}") + .addOutputLines( + "A.java", + "class A {", + " static final int foo = 1;", + " private static final int BAR = 2;", + " private static final int baz = 3;", + " private static final int BAZ = 4;", + "}") + .doTest(TestMode.TEXT_MATCH); + } +} diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/Flags.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/Flags.java index df2787c7f4..b8e263404f 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/Flags.java +++ b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/Flags.java @@ -1,6 +1,7 @@ package tech.picnic.errorprone.utils; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.ErrorProneFlags; /** Helper methods for working with {@link ErrorProneFlags}. */ @@ -19,4 +20,19 @@ public static ImmutableList getList(ErrorProneFlags errorProneFlags, Str ImmutableList list = errorProneFlags.getListOrEmpty(name); return list.equals(ImmutableList.of("")) ? ImmutableList.of() : list; } + + /** + * Returns the set of (comma-separated) arguments passed using the given Error Prone flag. + * + * @param errorProneFlags The full set of flags provided. + * @param name The name of the flag of interest. + * @return A non-{@code null} set of provided arguments; this set is empty if the flag was not + * provided, or if the flag's value is the empty string. + * @implNote This method does not delegate to {@link ErrorProneFlags#getSetOrEmpty(String)}, as + * that method wouldn't allow us to identify a non-singleton set of empty strings; such a set + * should not be treated as empty. + */ + public static ImmutableSet getSet(ErrorProneFlags errorProneFlags, String name) { + return ImmutableSet.copyOf(getList(errorProneFlags, name)); + } } diff --git a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/FlagsTest.java b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/FlagsTest.java index bc7658ce84..dcc217c8f7 100644 --- a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/FlagsTest.java +++ b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/FlagsTest.java @@ -4,6 +4,7 @@ import static org.junit.jupiter.params.provider.Arguments.arguments; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.errorprone.ErrorProneOptions; import java.util.stream.Stream; import org.junit.jupiter.params.ParameterizedTest; @@ -11,21 +12,29 @@ import org.junit.jupiter.params.provider.MethodSource; final class FlagsTest { - private static Stream getListTestCases() { - /* { args, flag, expected } */ + private static Stream getCollectionTestCases() { + /* { args, flag, listed } */ return Stream.of( arguments(ImmutableList.of(), "Foo", ImmutableList.of()), arguments(ImmutableList.of("-XepOpt:Foo=bar,baz"), "Qux", ImmutableList.of()), arguments(ImmutableList.of("-XepOpt:Foo="), "Foo", ImmutableList.of()), arguments(ImmutableList.of("-XepOpt:Foo=bar"), "Foo", ImmutableList.of("bar")), + arguments(ImmutableList.of("-XepOpt:Foo=bar,bar"), "Foo", ImmutableList.of("bar", "bar")), arguments(ImmutableList.of("-XepOpt:Foo=bar,baz"), "Foo", ImmutableList.of("bar", "baz")), arguments(ImmutableList.of("-XepOpt:Foo=,"), "Foo", ImmutableList.of("", ""))); } - @MethodSource("getListTestCases") + @MethodSource("getCollectionTestCases") @ParameterizedTest - void getList(ImmutableList args, String flag, ImmutableList expected) { + void getList(ImmutableList args, String flag, ImmutableList listed) { assertThat(Flags.getList(ErrorProneOptions.processArgs(args).getFlags(), flag)) - .containsExactlyElementsOf(expected); + .containsExactlyElementsOf(listed); + } + + @MethodSource("getCollectionTestCases") + @ParameterizedTest + void getSet(ImmutableList args, String flag, ImmutableList listed) { + assertThat(Flags.getSet(ErrorProneOptions.processArgs(args).getFlags(), flag)) + .containsExactlyElementsOf(ImmutableSet.copyOf(listed)); } }