Merge fixes of inner type decls, enable sorting nested types in one-go

error-prone-experimental/pom.xml

@@ -73,4 +73,16 @@
             <scope>test</scope>
         </dependency>
     </dependencies>
+
+    <build>
+        <plugins>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-checkstyle-plugin</artifactId>
+                <configuration>
+                    <skip>true</skip>
+                </configuration>
+            </plugin>
+        </plugins>
+    </build>
 </project>

error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrder.java

@@ -10,19 +10,22 @@
 import com.google.auto.service.AutoService;
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Streams;
 import com.google.errorprone.BugPattern;
 import com.google.errorprone.VisitorState;
 import com.google.errorprone.annotations.Var;
 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.Replacement;
 import com.google.errorprone.fixes.SuggestedFix;
 import com.google.errorprone.matchers.Description;
 import com.google.errorprone.util.ASTHelpers;
 import com.google.errorprone.util.ErrorProneToken;
 import com.google.errorprone.util.ErrorProneTokens;
 import com.sun.source.tree.BlockTree;
 import com.sun.source.tree.ClassTree;
+import com.sun.source.tree.CompilationUnitTree;
 import com.sun.source.tree.MethodTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.Tree.Kind;
@@ -55,24 +58,50 @@
  *     {@code DeclarationOrderCheck}</a>
  */
 // XXX: Consider introducing support for ordering members in records or annotation definitions.
+// XXX: Exclude checkstyle from running against (this checker's) test files.
 @AutoService(BugChecker.class)
 @BugPattern(
     summary = "Type members should be defined in a canonical order",
     link = BUG_PATTERNS_BASE_URL + "TypeMemberOrder",
     linkType = CUSTOM,
     severity = WARNING,
     tags = STYLE)
-public final class TypeMemberOrder extends BugChecker implements ClassTreeMatcher {
+public final class TypeMemberOrder extends BugChecker implements CompilationUnitTreeMatcher {
   private static final long serialVersionUID = 1L;
 
   /** Instantiates a new {@link TypeMemberOrder} instance. */
   public TypeMemberOrder() {}
 
   @Override
-  public Description matchClass(ClassTree tree, VisitorState state) {
+  public Description matchCompilationUnit(
+      CompilationUnitTree compilationUnitTree, VisitorState state) {
+    SuggestedFix.Builder suggestedFixes = SuggestedFix.builder();
+    for (Tree tree : compilationUnitTree.getTypeDecls()) {
+      if (!isSuppressed(tree, state) && tree instanceof ClassTree classTree) {
+        suggestedFixes.merge(
+            matchClass(classTree, state, (JCTree.JCCompilationUnit) compilationUnitTree));
+      }
+    }
+    SuggestedFix suggestedFix = suggestedFixes.build();
+    if (suggestedFix.isEmpty()) {
+      return Description.NO_MATCH;
+    }
+    return describeMatch(compilationUnitTree, suggestedFix);
+  }
+
+  /**
+   * Matches a class.
+   *
+   * @param tree xxx
+   * @param state xxx
+   * @param compilationUnit xxx
+   * @return xxx
+   */
+  public SuggestedFix matchClass(
+      ClassTree tree, VisitorState state, JCTree.JCCompilationUnit compilationUnit) {
     Kind treeKind = tree.getKind();
     if (treeKind != Kind.CLASS && treeKind != Kind.INTERFACE && treeKind != Kind.ENUM) {
-      return Description.NO_MATCH;
+      return SuggestedFix.emptyFix();
     }
 
     int bodyStartPos = getBodyStartPos(tree, state);
@@ -82,17 +111,52 @@ public Description matchClass(ClassTree tree, VisitorState state) {
        * that (part of) its code was generated. Even if the source code for a subset of its members
        * is available, dealing with this edge case is not worth the trouble.
        */
-      return Description.NO_MATCH;
+      return SuggestedFix.emptyFix();
     }
 
     ImmutableList<TypeMember> members = getAllTypeMembers(tree, bodyStartPos, state);
-    ImmutableList<TypeMember> sortedMembers = ImmutableList.sortedCopyOf(members);
+    boolean topLevelSorted = members.equals(ImmutableList.sortedCopyOf(members));
 
-    if (members.equals(sortedMembers)) {
-      return Description.NO_MATCH;
+    if (topLevelSorted) {
+      SuggestedFix.Builder nestedSuggestedFixes = SuggestedFix.builder();
+      for (TypeMember member : members) {
+        if (member.tree() instanceof ClassTree memberClassTree) {
+          SuggestedFix other = matchClass(memberClassTree, state, compilationUnit);
+          nestedSuggestedFixes.merge(other);
+        }
+      }
+      return nestedSuggestedFixes.build();
     }
 
-    return describeMatch(tree, sortTypeMembers(members, state));
+    CharSequence source = requireNonNull(state.getSourceCode(), "Source code");
+    ImmutableMap.Builder<TypeMember, String> typeMemberSource = ImmutableMap.builder();
+
+    for (TypeMember member : members) {
+      if (member.tree() instanceof ClassTree memberClassTree) {
+        SuggestedFix suggestedFix = matchClass(memberClassTree, state, compilationUnit);
+        @Var
+        String memberSource =
+            source.subSequence(member.startPosition(), member.endPosition()).toString();
+        // Diff between memberSource and replacement positions.
+        @Var int diff = -member.startPosition();
+        for (Replacement replacement : suggestedFix.getReplacements(compilationUnit.endPositions)) {
+          memberSource =
+              memberSource.subSequence(0, replacement.startPosition() + diff)
+                  + replacement.replaceWith()
+                  + memberSource.subSequence(
+                      replacement.endPosition() + diff, memberSource.length());
+          diff +=
+              replacement.replaceWith().length()
+                  - (replacement.endPosition() - replacement.startPosition());
+        }
+        typeMemberSource.put(member, memberSource);
+      } else {
+        typeMemberSource.put(
+            member, source.subSequence(member.startPosition(), member.endPosition()).toString());
+      }
+    }
+
+    return sortTypeMembers(members, typeMemberSource.build());
   }
 
   /** Returns all members that can be moved or may lay between movable ones. */
@@ -177,7 +241,6 @@ private static int getBodyStartPos(ClassTree tree, VisitorState state) {
   /**
    * Returns true if {@link Tree} is an enum or an enumerator definition, false otherwise.
    *
-   * @see com.sun.tools.javac.tree.Pretty#isEnumerator(JCTree)
    * @see com.sun.tools.javac.code.Flags#ENUM
    */
   private static boolean isEnumeratorDefinition(Tree tree) {
@@ -195,12 +258,19 @@ private static boolean isEnumeratorDefinition(Tree tree) {
    *     resolve this.
    */
   private static SuggestedFix sortTypeMembers(
-      ImmutableList<TypeMember> members, VisitorState state) {
-    CharSequence sourceCode = requireNonNull(state.getSourceCode(), "Source code");
+      ImmutableList<TypeMember> members, ImmutableMap<TypeMember, String> sourceCode) {
     return Streams.zip(
             members.stream(),
             members.stream().sorted(),
-            (original, replacement) -> original.replaceWith(replacement, sourceCode))
+            (original, replacement) -> {
+              String replacementSource = requireNonNull(sourceCode.get(replacement), "replacement");
+              return original.equals(replacement)
+                  ? SuggestedFix.emptyFix()
+                  : SuggestedFix.replace(
+                      original.startPosition(),
+                      original.endPosition(),
+                      replacementSource.toString());
+            })
         .reduce(SuggestedFix.builder(), SuggestedFix.Builder::merge, SuggestedFix.Builder::merge)
         .build();
   }
@@ -232,14 +302,5 @@ abstract static class TypeMember implements Comparable<TypeMember> {
     public int compareTo(TypeMemberOrder.TypeMember o) {
       return preferredOrdinal().compareTo(o.preferredOrdinal());
     }
-
-    SuggestedFix replaceWith(TypeMember other, CharSequence fullSourceCode) {
-      return equals(other)
-          ? SuggestedFix.emptyFix()
-          : SuggestedFix.replace(
-              startPosition(),
-              endPosition(),
-              fullSourceCode.subSequence(other.startPosition(), other.endPosition()).toString());
-    }
   }
 }

error-prone-experimental/src/test/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrderClassTest.java

@@ -1,8 +1,12 @@
 package tech.picnic.errorprone.experimental.bugpatterns;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+
 import com.google.errorprone.BugCheckerRefactoringTestHelper;
 import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
 import com.google.errorprone.CompilationTestHelper;
+import java.io.BufferedReader;
+import java.io.InputStreamReader;
 import org.junit.jupiter.api.Test;
 
 final class TypeMemberOrderClassTest {
@@ -142,11 +146,47 @@ void replacement() {
             "    return foo;",
             "  }",
             "",
-            "  class Inner {}",
+            "  class Inner {",
+            "",
+            "    private final int innerBar = 2;",
+            "",
+            "    int innerFoo() {",
+            "      return 1;",
+            "    }",
+            "  }",
             "}")
         .doTest(TestMode.TEXT_MATCH);
   }
 
+  @Test
+  void replacementNestedClasses() {
+    BugCheckerRefactoringTestHelper.newInstance(TypeMemberOrder.class, getClass())
+        .addInputLines(
+            "A.java",
+            // XXX: Use dedicated helper method to read test files here and below.
+            new BufferedReader(
+                    new InputStreamReader(
+                        this.getClass()
+                            .getClassLoader()
+                            .getResourceAsStream(
+                                "TypeMemberOrderNestedCompilationUnitTestInput.java"),
+                        UTF_8))
+                .lines()
+                .toArray(String[]::new))
+        .addOutputLines(
+            "A.java",
+            new BufferedReader(
+                    new InputStreamReader(
+                        this.getClass()
+                            .getClassLoader()
+                            .getResourceAsStream(
+                                "TypeMemberOrderNestedCompilationUnitTestOutput.java"),
+                        UTF_8))
+                .lines()
+                .toArray(String[]::new))
+        .doTest(TestMode.TEXT_MATCH);
+  }
+
   @Test
   void replacementAbstractMethods() {
     BugCheckerRefactoringTestHelper.newInstance(TypeMemberOrder.class, getClass())
@@ -345,7 +385,7 @@ void replacementComplexAnnotation() {
     BugCheckerRefactoringTestHelper.newInstance(TypeMemberOrder.class, getClass())
         .addInputLines(
             "A.java",
-            "final class A {",
+            "class A {",
             "",
             "  @interface AnnotationWithClassReferences {",
             "    Class<?>[] value() default {};",
@@ -365,7 +405,7 @@ void replacementComplexAnnotation() {
             "}")
         .addOutputLines(
             "A.java",
-            "final class A {",
+            "class A {",
             "",
             "  @interface AnnotationWithClassReferences {",
             "    Class<?>[] value() default {};",

error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestInput.java

@@ -0,0 +1,65 @@
+class A {
+  class AInner2 {
+    private static final int foo = 1;
+    int bar = 2;
+
+    class AInner2Inner1 {
+      private static final int foo = 1;
+      int bar = 2;
+    }
+
+    class AInner2Inner2 {
+      int bar = 2;
+      private static final int foo = 1;
+    }
+  }
+
+  private static final int foo = 1;
+
+  class OnlyTopLevelSortedClass {
+    private static final int foo = 1;
+    int bar = 2;
+
+    class UnsortedNestedClass {
+      int bar = 2;
+      private static final int foo = 1;
+    }
+  }
+
+  class AInner1 {
+    class AInner1Inner1 {
+      int bar = 2;
+      private static final int foo = 1;
+    }
+
+    enum DeeplyNestedEnum {
+      /** FOO's JavaDoc */
+      FOO,
+    /* Dangling comment trailing enumerations. */ ;
+
+      /** `quz` method's dangling comment */
+      ;
+
+      /** `quz` method's comment */
+      void qux() {}
+
+      // `baz` method's comment
+      final int baz = 2;
+
+      static final int BAR = 1;
+      // trailing comment
+    }
+
+    private static final int foo = 1;
+
+    class AInner1Inner2 {
+      int bar = 2;
+      private static final int foo = 1;
+    }
+
+    int bar = 2;
+  }
+
+  static int baz = 3;
+  private final int bar = 2;
+}