diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java index 5976de277a..14fbebcc5a 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java +++ b/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,8 +68,9 @@ public LexicographicalAnnotationListing() {} @Override public Description matchModifiers(ModifiersTree tree, VisitorState state) { - List originalOrdering = tree.getAnnotations(); - if (originalOrdering.size() < 2) { + List annotations = tree.getAnnotations(); + if (annotations.size() < 2) { + /* Fast path: there's at most one annotation. */ return Description.NO_MATCH; } @@ -82,41 +78,14 @@ public Description matchModifiers(ModifiersTree tree, VisitorState state) { requireNonNull( ASTHelpers.getSymbol(ASTHelpers.findEnclosingNode(state.getPath(), Tree.class)), "Cannot find enclosing symbol"); + Comparator comparator = + comparing( + (AnnotationTree annotation) -> + ASTHelpers.getAnnotationType(annotation, symbol, state), + BY_ANNOTATION_TYPE) + .thenComparing(annotation -> SourceCode.treeToString(annotation, state)); - ImmutableList sortedAnnotations = - sort(originalOrdering, symbol, state); - if (originalOrdering.equals(sortedAnnotations)) { - return Description.NO_MATCH; - } - - return describeMatch( - originalOrdering.getFirst(), fixOrdering(originalOrdering, sortedAnnotations, state)); - } - - private static ImmutableList sort( - List 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 originalAnnotations, - ImmutableList 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); } } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalPermitsListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalPermitsListing.java new file mode 100644 index 0000000000..9c6f9254fd --- /dev/null +++ b/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. + * + *

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); + } +} diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalPermitsListingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalPermitsListingTest.java new file mode 100644 index 0000000000..e80d3dd0e2 --- /dev/null +++ b/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); + } +} diff --git a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java b/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java index 8fc963a50f..42b6999413 100644 --- a/error-prone-utils/src/main/java/tech/picnic/errorprone/utils/SourceCode.java +++ b/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 @@ static SuggestedFix unwrapMethodInvocationDroppingWhitespaceAndComments( 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 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 SuggestedFix sortTrees( + Collection trees, Comparator 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(); + } } diff --git a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java b/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java index 9fbfe2d274..45ee926bb2 100644 --- a/error-prone-utils/src/test/java/tech/picnic/errorprone/utils/SourceCodeTest.java +++ b/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); + } + } }