Introduce LexicographicalPermitsListing check

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

@@ -1,6 +1,5 @@
 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.SUGGESTION;
 import static com.google.errorprone.BugPattern.StandardTags.STYLE;
@@ -11,14 +10,10 @@
 import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
 
 import com.google.auto.service.AutoService;
-import com.google.common.base.VerifyException;
-import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Streams;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.bugpatterns.BugChecker;
 import com.google.errorprone.bugpatterns.BugChecker.ModifiersTreeMatcher;
-import com.google.errorprone.fixes.Fix;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.ASTHelpers;
@@ -73,50 +68,24 @@
 
   @Override
   public Description matchModifiers(ModifiersTree tree, VisitorState state) {
-    List<? extends AnnotationTree> originalOrdering = tree.getAnnotations();
-    if (originalOrdering.size() < 2) {
+    List<? extends AnnotationTree> annotations = tree.getAnnotations();
+    if (annotations.size() < 2) {
+      /* Fast path: there's at most one annotation. */
       return Description.NO_MATCH;
     }
 
     Symbol symbol =
         requireNonNull(
             ASTHelpers.getSymbol(ASTHelpers.findEnclosingNode(state.getPath(), Tree.class)),
             "Cannot find enclosing symbol");
+    Comparator<AnnotationTree> comparator =
+        comparing(
+                (AnnotationTree annotation) ->
+                    ASTHelpers.getAnnotationType(annotation, symbol, state),
+                BY_ANNOTATION_TYPE)
+            .thenComparing(annotation -> SourceCode.treeToString(annotation, state));
 
-    ImmutableList<? extends AnnotationTree> sortedAnnotations =
-        sort(originalOrdering, symbol, state);
-    if (originalOrdering.equals(sortedAnnotations)) {
-      return Description.NO_MATCH;
-    }
-
-    return describeMatch(
-        originalOrdering.getFirst(), fixOrdering(originalOrdering, sortedAnnotations, state));
-  }
-
-  private static ImmutableList<? extends AnnotationTree> sort(
-      List<? extends AnnotationTree> annotations, Symbol symbol, VisitorState state) {
-    return annotations.stream()
-        .sorted(
-            comparing(
-                    (AnnotationTree annotation) ->
-                        ASTHelpers.getAnnotationType(annotation, symbol, state),
-                    BY_ANNOTATION_TYPE)
-                .thenComparing(annotation -> SourceCode.treeToString(annotation, state)))
-        .collect(toImmutableList());
-  }
-
-  private static Fix fixOrdering(
-      List<? extends AnnotationTree> originalAnnotations,
-      ImmutableList<? extends AnnotationTree> sortedAnnotations,
-      VisitorState state) {
-    return Streams.zip(
-            originalAnnotations.stream(),
-            sortedAnnotations.stream(),
-            (original, replacement) ->
-                SuggestedFix.builder()
-                    .replace(original, SourceCode.treeToString(replacement, state)))
-        .reduce(SuggestedFix.Builder::merge)
-        .map(SuggestedFix.Builder::build)
-        .orElseThrow(() -> new VerifyException("No annotations were provided"));
+    SuggestedFix fix = SourceCode.sortTrees(annotations, comparator, state);
+    return fix.isEmpty() ? Description.NO_MATCH : describeMatch(annotations.getFirst(), fix);
   }
 }

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

@@ -0,0 +1,50 @@
+package tech.picnic.errorprone.bugpatterns;
+
+import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
+import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
+import static com.google.errorprone.BugPattern.StandardTags.STYLE;
+import static java.util.Comparator.comparing;
+import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
+import com.google.errorprone.fixes.SuggestedFix;
+import com.google.errorprone.matchers.Description;
+import com.sun.source.tree.ClassTree;
+import tech.picnic.errorprone.utils.SourceCode;
+
+/**
+ * A {@link BugChecker} that flags enumerations of permitted subtypes that are not lexicographically
+ * sorted.
+ *
+ * <p>The idea behind this checker is that maintaining a sorted sequence simplifies conflict
+ * resolution.
+ */
+@AutoService(BugChecker.class)
+@BugPattern(
+    summary = "Sort permitted subtypes lexicographically where possible",
+    link = BUG_PATTERNS_BASE_URL + "LexicographicalPermitsListing",
+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = STYLE)
+public final class LexicographicalPermitsListing extends BugChecker implements ClassTreeMatcher {
+  private static final long serialVersionUID = 1L;
+
+  /** Instantiates a new {@link LexicographicalPermitsListing} instance. */
+  public LexicographicalPermitsListing() {}
+
+  @Override
+  public Description matchClass(ClassTree tree, VisitorState state) {
+    SuggestedFix fix =
+        SourceCode.sortTrees(
+            tree.getPermitsClause(),
+            comparing(annotation -> SourceCode.treeToString(annotation, state)),
+            state);
+    return fix.isEmpty()
+        ? Description.NO_MATCH
+        : describeMatch(tree.getPermitsClause().getFirst(), fix);
+  }
+}

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalPermitsListingTest.java

@@ -0,0 +1,70 @@
+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 LexicographicalPermitsListingTest {
+  @Test
+  void identification() {
+    CompilationTestHelper.newInstance(LexicographicalPermitsListing.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "interface A {",
+            "  non-sealed class X extends UnsortedPermitsClass",
+            "      implements SinglePermits, SortedPermits, UnsortedPermits {}",
+            "",
+            "  non-sealed class Y extends UnsortedPermitsClass implements SortedPermits, UnsortedPermits {}",
+            "",
+            "  non-sealed class Z extends UnsortedPermitsClass {}",
+            "",
+            "  sealed interface NoPermits {",
+            "    record R() implements NoPermits {}",
+            "  }",
+            "",
+            "  sealed interface SinglePermits permits X {}",
+            "",
+            "  sealed interface SortedPermits permits X, Y {}",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  sealed interface UnsortedPermits permits Y, X {}",
+            "",
+            "  // BUG: Diagnostic contains:",
+            "  sealed class UnsortedPermitsClass permits Z, X, Y {}",
+            "}")
+        .doTest();
+  }
+
+  @Test
+  void replacement() {
+    BugCheckerRefactoringTestHelper.newInstance(LexicographicalPermitsListing.class, getClass())
+        .addInputLines(
+            "A.java",
+            "interface A {",
+            "  non-sealed class X extends UnsortedPermitsClass implements UnsortedPermits {}",
+            "",
+            "  non-sealed class Y extends UnsortedPermitsClass implements UnsortedPermits {}",
+            "",
+            "  non-sealed class Z extends UnsortedPermitsClass {}",
+            "",
+            "  sealed interface UnsortedPermits permits Y, X {}",
+            "",
+            "  sealed class UnsortedPermitsClass permits Z, X, Y {}",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "interface A {",
+            "  non-sealed class X extends UnsortedPermitsClass implements UnsortedPermits {}",
+            "",
+            "  non-sealed class Y extends UnsortedPermitsClass implements UnsortedPermits {}",
+            "",
+            "  non-sealed class Z extends UnsortedPermitsClass {}",
+            "",
+            "  sealed interface UnsortedPermits permits X, Y {}",
+            "",
+            "  sealed class UnsortedPermitsClass permits X, Y, Z {}",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+}

error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java

@@ -6,6 +6,7 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.CharMatcher;
+import com.google.common.collect.Comparators;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Streams;
 import com.google.errorprone.VisitorState;
@@ -16,6 +17,8 @@
 import com.sun.source.tree.MethodInvocationTree;
 import com.sun.source.tree.Tree;
 import com.sun.tools.javac.util.Position;
+import java.util.Collection;
+import java.util.Comparator;
 import java.util.Optional;
 import javax.lang.model.SourceVersion;
 
@@ -155,4 +158,34 @@
         tree,
         tree.getArguments().stream().map(arg -> treeToString(arg, state)).collect(joining(", ")));
   }
+
+  /**
+   * Creates a {@link SuggestedFix} for the replacement of the given collection of {@link Tree}s
+   * with the same collection, sorted according to the given {@link Comparator}.
+   *
+   * @param <T> The type of the given {@link Tree}s.
+   * @param trees The AST nodes to be sorted.
+   * @param comparator The {@link Comparator} according to which the given {@link Tree}s should be
+   *     sorted.
+   * @param state A {@link VisitorState} describing the context in which the given {@link Tree}s are
+   *     found.
+   * @return A non-{@code null} {@link SuggestedFix} that replaces the given collection of {@link
+   *     Tree}s with the same collection, sorted according to the given {@link Comparator}; {@link
+   *     SuggestedFix#emptyFix() empty} if the trees are already sorted.
+   */
+  public static <T extends Tree> SuggestedFix sortTrees(
+      Collection<? extends T> trees, Comparator<? super T> comparator, VisitorState state) {
+    if (Comparators.isInOrder(trees, comparator)) {
+      /* Fast path: the trees are already sorted. */
+      return SuggestedFix.emptyFix();
+    }
+
+    return Streams.zip(
+            trees.stream(),
+            trees.stream().sorted(comparator),
+            (original, replacement) ->
+                SuggestedFix.replace(original, treeToString(replacement, state)))
+        .reduce(SuggestedFix.builder(), SuggestedFix.Builder::merge, SuggestedFix.Builder::merge)
+        .build();
+  }
 }

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

@@ -1,6 +1,7 @@
 package tech.picnic.errorprone.utils;
 
 import static com.google.errorprone.BugPattern.SeverityLevel.ERROR;
+import static java.util.Comparator.comparing;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.params.provider.Arguments.arguments;
 
@@ -23,6 +24,8 @@
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
+import java.util.Collection;
+import java.util.Comparator;
 import java.util.Optional;
 import java.util.stream.Stream;
 import javax.lang.model.element.Name;
@@ -292,6 +295,40 @@ UnwrapMethodInvocationDroppingWhitespaceAndCommentsTestChecker.class, getClass()
         .doTest(TestMode.TEXT_MATCH);
   }
 
+  @Test
+  void sortTrees() {
+    BugCheckerRefactoringTestHelper.newInstance(SortTreesTestChecker.class, getClass())
+        .addInputLines(
+            "A.java",
+            "import java.util.stream.Stream;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Stream.of();",
+            "    Stream.of(\"a\");",
+            "    Stream.of(\"a\", \"b\");",
+            "    Stream.of(\"b\", \"a\");",
+            "    Stream.of(\"a\", \"b\", \"c\");",
+            "    Stream.of(\"c\", \"a\", \"b\");",
+            "  }",
+            "}")
+        .addOutputLines(
+            "A.java",
+            "import java.util.stream.Stream;",
+            "",
+            "class A {",
+            "  void m() {",
+            "    Stream.of();",
+            "    Stream.of(\"a\");",
+            "    Stream.of(\"a\", \"b\");",
+            "    Stream.of(\"a\", \"b\");",
+            "    Stream.of(\"a\", \"b\", \"c\");",
+            "    Stream.of(\"a\", \"b\", \"c\");",
+            "  }",
+            "}")
+        .doTest(TestMode.TEXT_MATCH);
+  }
+
   /**
    * A {@link BugChecker} that applies {@link SourceCode#toStringConstantExpression(Object,
    * VisitorState)} to string literals.
@@ -383,4 +420,23 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState
           tree, SourceCode.unwrapMethodInvocationDroppingWhitespaceAndComments(tree, state));
     }
   }
+
+  /**
+   * A {@link BugChecker} that applies {@link SourceCode#sortTrees(Collection, Comparator,
+   * VisitorState)} to all method invocation arguments with the aim of sorting them
+   * lexicographically.
+   */
+  @BugPattern(severity = ERROR, summary = "Interacts with `SourceCode` for testing purposes")
+  public static final class SortTreesTestChecker extends BugChecker
+      implements MethodInvocationTreeMatcher {
+    private static final long serialVersionUID = 1L;
+
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+      SuggestedFix fix =
+          SourceCode.sortTrees(
+              tree.getArguments(), comparing(arg -> SourceCode.treeToString(arg, state)), state);
+      return fix.isEmpty() ? Description.NO_MATCH : describeMatch(tree, fix);
+    }
+  }
 }