diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java index 0ba0a18712..21ded55dfd 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListing.java @@ -1,5 +1,6 @@ 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; @@ -9,6 +10,7 @@ import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; import com.google.auto.service.AutoService; +import com.google.common.base.Splitter; import com.google.common.collect.Comparators; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -71,6 +73,12 @@ public final class LexicographicalAnnotationAttributeListing extends BugChecker private static final String FLAG_PREFIX = "LexicographicalAnnotationAttributeListing:"; private static final String INCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Includes"; private static final String EXCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Excludes"; + /** + * The splitter applied to string-typed annotation arguments prior to lexicographical sorting. By + * splitting on {@code =}, strings that represent e.g. inline Spring property declarations are + * properly sorted by key, then value. + */ + private static final Splitter STRING_ARGUMENT_SPLITTER = Splitter.on('='); private final AnnotationAttributeMatcher matcher; @@ -126,7 +134,7 @@ private static Optional suggestSorting( } List actualOrdering = array.getInitializers(); - ImmutableList desiredOrdering = doSort(actualOrdering, state); + ImmutableList desiredOrdering = doSort(actualOrdering); if (actualOrdering.equals(desiredOrdering)) { /* In the (presumably) common case the elements are already sorted. */ return Optional.empty(); @@ -156,12 +164,12 @@ private static boolean canSort(Tree array, VisitorState state) { } private static ImmutableList doSort( - Iterable elements, VisitorState state) { + Iterable elements) { // XXX: Perhaps we should use `Collator` with `.setStrength(Collator.PRIMARY)` and // `getCollationKey`. Not clear whether that's worth the hassle at this point. return ImmutableList.sortedCopyOf( comparing( - e -> getStructure(e, state), + LexicographicalAnnotationAttributeListing::getStructure, Comparators.lexicographical( Comparators.lexicographical( String.CASE_INSENSITIVE_ORDER.thenComparing(naturalOrder())))), @@ -173,38 +181,34 @@ private static ImmutableList doSort( * performed. This approach disregards e.g. irrelevant whitespace. It also allows special * structure within string literals to be respected. */ - private static ImmutableList> getStructure( - ExpressionTree array, VisitorState state) { + private static ImmutableList> getStructure(ExpressionTree array) { ImmutableList.Builder> nodes = ImmutableList.builder(); new TreeScanner() { @Nullable @Override - public Void visitIdentifier(IdentifierTree node, @Nullable Void ctx) { - nodes.add(tokenize(node)); - return super.visitIdentifier(node, ctx); + public Void visitIdentifier(IdentifierTree node, @Nullable Void unused) { + nodes.add(ImmutableList.of(node.getName().toString())); + return super.visitIdentifier(node, unused); } @Nullable @Override - public Void visitLiteral(LiteralTree node, @Nullable Void ctx) { - nodes.add(tokenize(node)); - return super.visitLiteral(node, ctx); + public Void visitLiteral(LiteralTree node, @Nullable Void unused) { + Object value = ASTHelpers.constValue(node); + nodes.add( + value instanceof String + ? STRING_ARGUMENT_SPLITTER.splitToStream((String) value).collect(toImmutableList()) + : ImmutableList.of(String.valueOf(value))); + + return super.visitLiteral(node, unused); } @Nullable @Override - public Void visitPrimitiveType(PrimitiveTypeTree node, @Nullable Void ctx) { - nodes.add(tokenize(node)); - return super.visitPrimitiveType(node, ctx); - } - - private ImmutableList tokenize(Tree node) { - /* - * Tokens are split on `=` so that e.g. inline Spring property declarations are properly - * sorted by key, then value. - */ - return ImmutableList.copyOf(SourceCode.treeToString(node, state).split("=", -1)); + public Void visitPrimitiveType(PrimitiveTypeTree node, @Nullable Void unused) { + nodes.add(ImmutableList.of(node.getPrimitiveTypeKind().toString())); + return super.visitPrimitiveType(node, unused); } }.scan(array, null); diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListingTest.java index e7740423c7..b7a204c457 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListingTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationAttributeListingTest.java @@ -188,7 +188,7 @@ void replacement() { " String[] value() default {};", " }", "", - " @Foo({\"b\", \"a\"})", + " @Foo({\" \", \"\", \"b\", \"a\"})", " A unsortedString();", "", " @Foo(cls = {long.class, int.class})", @@ -225,7 +225,7 @@ void replacement() { " String[] value() default {};", " }", "", - " @Foo({\"a\", \"b\"})", + " @Foo({\"\", \" \", \"a\", \"b\"})", " A unsortedString();", "", " @Foo(cls = {int.class, long.class})",