From 2f8a673af836fc2c28b453be50f20f12e93a83a9 Mon Sep 17 00:00:00 2001 From: Bastien Diederichs Date: Thu, 6 Oct 2022 15:28:21 +0200 Subject: [PATCH 1/3] PSM-1465 Fix sorting of annotation nodes --- .../LexicographicalAnnotationAttributeListing.java | 4 +++- .../LexicographicalAnnotationAttributeListingTest.java | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) 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..eaf1697cb9 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 @@ -203,8 +203,10 @@ private ImmutableList tokenize(Tree node) { /* * Tokens are split on `=` so that e.g. inline Spring property declarations are properly * sorted by key, then value. + * Leading and trailing double quotes are ignored for sorting purposes. */ - return ImmutableList.copyOf(SourceCode.treeToString(node, state).split("=", -1)); + return ImmutableList.copyOf( + SourceCode.treeToString(node, state).replaceAll("^\"|\"$", "").split("=", -1)); } }.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})", From 84f75ecaf0b4da6999e74d471568ded658d0bec6 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Thu, 6 Oct 2022 21:02:21 +0200 Subject: [PATCH 2/3] Suggestion --- ...cographicalAnnotationAttributeListing.java | 41 ++++++++++--------- 1 file changed, 22 insertions(+), 19 deletions(-) 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 eaf1697cb9..5a38b4fda2 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; @@ -180,33 +188,28 @@ private static ImmutableList> getStructure( 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(SourceCode.treeToString(node, state))); + 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. - * Leading and trailing double quotes are ignored for sorting purposes. - */ - return ImmutableList.copyOf( - SourceCode.treeToString(node, state).replaceAll("^\"|\"$", "").split("=", -1)); + public Void visitPrimitiveType(PrimitiveTypeTree node, @Nullable Void unused) { + nodes.add(ImmutableList.of(SourceCode.treeToString(node, state))); + return super.visitPrimitiveType(node, unused); } }.scan(array, null); From cdd7e312df88bf160bae92dc0043c4b93fa30eb8 Mon Sep 17 00:00:00 2001 From: Stephan Schroevers Date: Fri, 7 Oct 2022 08:17:25 +0200 Subject: [PATCH 3/3] Simpler --- .../LexicographicalAnnotationAttributeListing.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) 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 5a38b4fda2..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 @@ -134,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(); @@ -164,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())))), @@ -181,15 +181,14 @@ 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 unused) { - nodes.add(ImmutableList.of(SourceCode.treeToString(node, state))); + nodes.add(ImmutableList.of(node.getName().toString())); return super.visitIdentifier(node, unused); } @@ -208,7 +207,7 @@ public Void visitLiteral(LiteralTree node, @Nullable Void unused) { @Nullable @Override public Void visitPrimitiveType(PrimitiveTypeTree node, @Nullable Void unused) { - nodes.add(ImmutableList.of(SourceCode.treeToString(node, state))); + nodes.add(ImmutableList.of(node.getPrimitiveTypeKind().toString())); return super.visitPrimitiveType(node, unused); } }.scan(array, null);