diff --git a/error-prone-experimental/pom.xml b/error-prone-experimental/pom.xml index 719e27cbb5..68730960f9 100644 --- a/error-prone-experimental/pom.xml +++ b/error-prone-experimental/pom.xml @@ -73,4 +73,16 @@ test + + + + + org.apache.maven.plugins + maven-checkstyle-plugin + + true + + + + diff --git a/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrder.java b/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrder.java index 18e5afc6c0..84f69e74e7 100644 --- a/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrder.java +++ b/error-prone-experimental/src/main/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrder.java @@ -10,12 +10,14 @@ 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; @@ -23,6 +25,7 @@ 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,6 +58,7 @@ * {@code DeclarationOrderCheck} */ // 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", @@ -62,17 +66,42 @@ 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 members = getAllTypeMembers(tree, bodyStartPos, state); - ImmutableList 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 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 members, VisitorState state) { - CharSequence sourceCode = requireNonNull(state.getSourceCode(), "Source code"); + ImmutableList members, ImmutableMap 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 { 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()); - } } } diff --git a/error-prone-experimental/src/test/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrderClassTest.java b/error-prone-experimental/src/test/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrderClassTest.java index 4828bcee2d..f1b50aaf91 100644 --- a/error-prone-experimental/src/test/java/tech/picnic/errorprone/experimental/bugpatterns/TypeMemberOrderClassTest.java +++ b/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 {};", diff --git a/error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestInput.java b/error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestInput.java new file mode 100644 index 0000000000..52d2b6971b --- /dev/null +++ b/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; +} diff --git a/error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestOutput.java b/error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestOutput.java new file mode 100644 index 0000000000..445b76220e --- /dev/null +++ b/error-prone-experimental/src/test/resources/TypeMemberOrderNestedCompilationUnitTestOutput.java @@ -0,0 +1,67 @@ +class A { + + private static final int foo = 1; + + static int baz = 3; + private final int bar = 2; + + class AInner2 { + private static final int foo = 1; + int bar = 2; + + class AInner2Inner1 { + private static final int foo = 1; + int bar = 2; + } + + class AInner2Inner2 { + private static final int foo = 1; + int bar = 2; + } + } + + class OnlyTopLevelSortedClass { + private static final int foo = 1; + int bar = 2; + + class UnsortedNestedClass { + private static final int foo = 1; + int bar = 2; + } + } + + class AInner1 { + + private static final int foo = 1; + + int bar = 2; + + class AInner1Inner1 { + private static final int foo = 1; + int bar = 2; + } + + enum DeeplyNestedEnum { + /** FOO's JavaDoc */ + FOO, + /* Dangling comment trailing enumerations. */ ; + + static final int BAR = 1; + + // `baz` method's comment + final int baz = 2; + + /** `quz` method's dangling comment */ + ; + + /** `quz` method's comment */ + void qux() {} + // trailing comment + } + + class AInner1Inner2 { + private static final int foo = 1; + int bar = 2; + } + } +}