Introduce ConstantNaming check

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<VariableTree> IS_CONSTANT =
+      allOf(hasModifier(Modifier.STATIC), hasModifier(Modifier.FINAL));
+  private static final Matcher<VariableTree> IS_PRIVATE = hasModifier(Modifier.PRIVATE);
+  private static final Pattern SNAKE_CASE = Pattern.compile("([a-z])([A-Z])");
+  private static final ImmutableSet<String> 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<String> 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<Boolean, @Nullable Void>() {
+          @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));
+  }
+}

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 @@
         excludedAnnotations(flags));
   }
 
-  private static ImmutableList<String> excludedAnnotations(ErrorProneFlags flags) {
-    Set<String> exclusions = new HashSet<>();
-    exclusions.addAll(Flags.getList(flags, EXCLUDED_ANNOTATIONS_FLAG));
-    exclusions.addAll(BLACKLISTED_ANNOTATIONS);
-    return ImmutableList.copyOf(exclusions);
+  private static ImmutableSet<String> excludedAnnotations(ErrorProneFlags flags) {
+    return Sets.union(BLACKLISTED_ANNOTATIONS, Flags.getSet(flags, EXCLUDED_ANNOTATIONS_FLAG))
+        .immutableCopy();
   }
 }

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/RedundantStringConversion.java

@@ -378,11 +378,11 @@ private Description createDescription(Tree tree, Optional<SuggestedFix.Builder>
 
   private static Matcher<MethodInvocationTree> 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)));
   }
 }

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<VariableTree> 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<Tree> isSubtypeOfAny(ImmutableList<String> inclusions) {
+  private static Matcher<Tree> isSubtypeOfAny(ImmutableSet<String> inclusions) {
     return anyOf(
         inclusions.stream()
             .map(inclusion -> isSubtypeOf(Suppliers.typeFromString(inclusion)))

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

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<String> getList(ErrorProneFlags errorProneFlags, Str
     ImmutableList<String> 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<String> getSet(ErrorProneFlags errorProneFlags, String name) {
+    return ImmutableSet.copyOf(getList(errorProneFlags, name));
+  }
 }

error-prone-utils/src/test/java/tech/picnic/errorprone/utils/FlagsTest.java

@@ -4,28 +4,37 @@
 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;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
 
 final class FlagsTest {
-  private static Stream<Arguments> getListTestCases() {
-    /* { args, flag, expected } */
+  private static Stream<Arguments> 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<String> args, String flag, ImmutableList<String> expected) {
+  void getList(ImmutableList<String> args, String flag, ImmutableList<String> listed) {
     assertThat(Flags.getList(ErrorProneOptions.processArgs(args).getFlags(), flag))
-        .containsExactlyElementsOf(expected);
+        .containsExactlyElementsOf(listed);
+  }
+
+  @MethodSource("getCollectionTestCases")
+  @ParameterizedTest
+  void getSet(ImmutableList<String> args, String flag, ImmutableList<String> listed) {
+    assertThat(Flags.getSet(ErrorProneOptions.processArgs(args).getFlags(), flag))
+        .containsExactlyElementsOf(ImmutableSet.copyOf(listed));
   }
 }