diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListing.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListing.java new file mode 100644 index 0000000000..14a20c270a --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListing.java @@ -0,0 +1,257 @@ +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.stream.Collectors.joining; +import static javax.lang.model.element.ElementKind.CLASS; +import static javax.lang.model.element.ElementKind.INTERFACE; +import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.ErrorProneFlags; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.AnnotationTree; +import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.IdentifierTree; +import com.sun.source.tree.LiteralTree; +import com.sun.source.tree.NewArrayTree; +import com.sun.source.tree.PrimitiveTypeTree; +import com.sun.source.tree.Tree; +import com.sun.source.util.TreeScanner; +import com.sun.tools.javac.code.Symbol; +import java.util.HashSet; +import java.util.List; +import java.util.Optional; +import java.util.Set; +import javax.inject.Inject; +import javax.tools.JavaFileObject.Kind; +import org.jspecify.annotations.Nullable; +import tech.picnic.errorprone.utils.AnnotationAttributeMatcher; +import tech.picnic.errorprone.utils.Flags; +import tech.picnic.errorprone.utils.SourceCode; + +/** + * A {@link BugChecker} that flags and removes duplicate listings inside declared annotations. + * + *

Example: + * + * 

+ *   {@code @JsonPropertyOrder({ "a", "b", "a" })}
+ *
+ * + *

Will be flagged and fixed to the following: + * + * 

+ *   {@code @JsonPropertyOrder({ "a", "b"})}
+ *
+ */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Duplicate listings within an annotation are often a mistake.", + link = BUG_PATTERNS_BASE_URL + "DuplicateAnnotationAttributeListing", + linkType = CUSTOM, + severity = SUGGESTION, + tags = STYLE) +@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */) +public final class DuplicateAnnotationAttributeListing extends BugChecker + implements AnnotationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final String FLAG_PREFIX = "DuplicateAnnotationAttributeListing:"; + private static final String INCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Includes"; + private static final String EXCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Excludes"; + + private final AnnotationAttributeMatcher matcher; + + /** Instantiates a default {@link DuplicateAnnotationAttributeListing} instance. */ + public DuplicateAnnotationAttributeListing() { + this(ErrorProneFlags.empty()); + } + + /** + * Instantiates a customized {@link DuplicateAnnotationAttributeListing}. + * + * @param flags Any provided command line flags. + */ + @Inject + DuplicateAnnotationAttributeListing(ErrorProneFlags flags) { + matcher = createAnnotationAttributeMatcher(flags); + } + + @Override + public Description matchAnnotation(AnnotationTree tree, VisitorState state) { + return removeDuplicateAnnotationEntries(tree, state) + .map(fix -> describeMatch(tree, fix)) + .orElse(Description.NO_MATCH); + } + + private Optional removeDuplicateAnnotationEntries(AnnotationTree tree, VisitorState state) { + return matcher + .extractMatchingArguments(tree) + .map(expr -> extractArray(expr).flatMap(arr -> removeDuplicates(arr, tree, state))) + .flatMap(Optional::stream) + .reduce(SuggestedFix.Builder::merge) + .map(SuggestedFix.Builder::build); + } + + private static Optional extractArray(ExpressionTree expr) { + return expr instanceof AssignmentTree assignment + ? extractArray(assignment.getExpression()) + : Optional.of(expr).filter(NewArrayTree.class::isInstance).map(NewArrayTree.class::cast); + } + + private static Optional removeDuplicates( + NewArrayTree array, AnnotationTree tree, VisitorState state) { + if (array.getInitializers().size() < 2) { + /* There's only one element, no duplicates are expected. */ + return Optional.empty(); + } + + List actualEntries = array.getInitializers(); + + ImmutableSet nonDuplicateEntries = getNonDuplicateEntries(array, state); + + if (actualEntries.size() == nonDuplicateEntries.size()) { + /* In the (presumably) common case that no duplicates are found. */ + return Optional.empty(); + } + + String prefix = shouldOmitBrackets(tree, nonDuplicateEntries) ? "" : "{"; + String suffix = prefix.isBlank() ? "" : "}"; + + String suggestion = + nonDuplicateEntries.stream() + .map(expr -> extractName(state, expr)) + .collect(joining(", ", prefix, suffix)); + return Optional.of(SuggestedFix.builder().replace(array, suggestion)); + } + + /** + * Extracts from the given array element expression a distinct set of {@link Tree nodes} omitting + * the duplicate entries. + * + * @implNote Annotations are a special case in which we want to make sure we get the distinct + * value of the full annotation expression instead of the annotation name. For example, we + * want to flag the following as a duplicate annotation entry: + * 
+   *       {@code @Foo(anns = { @Bar("a"), @Bar("a") })}
+   *
+ * but not: + * 
+   *       {@code @Foo(anns = { @Bar("a"), @Bar("b") })}
+   *
+ * To do this, we conditionally visit the identifiers, literals and primitive types of the + * original annotation but not those of the annotation entry being visited, given that the + * listings of each nested annotation will be the visited through the {@link + * AnnotationTreeMatcher}. + */ + private static ImmutableSet getNonDuplicateEntries( + ExpressionTree array, VisitorState state) { + // Helper collections to identify when a node has been visited by its identifier, to avoid + // including duplicate nodes. + Set visitedAnnotations = new HashSet<>(); + Set visitedIdentifiers = new HashSet<>(); + Set visitedLiterals = new HashSet<>(); + Set visitPrimitiveType = new HashSet<>(); + + ImmutableSet.Builder nodes = ImmutableSet.builder(); + + new TreeScanner<@Nullable Void, Set>() { + @Override + public @Nullable Void visitAnnotation(AnnotationTree node, Set visitedAnnotations) { + String annotation = SourceCode.treeToString(node, state); + if (!visitedAnnotations.contains(annotation)) { + nodes.add(node); + } + visitedAnnotations.add(annotation); + + return super.visitAnnotation(node, visitedAnnotations); + } + + @Override + public @Nullable Void visitIdentifier(IdentifierTree node, Set visitedAnnotations) { + String identifier = node.getName().toString(); + + if (visitedAnnotations.isEmpty() && !visitedIdentifiers.contains(identifier)) { + nodes.add(node); + } + visitedIdentifiers.add(identifier); + + return super.visitIdentifier(node, visitedAnnotations); + } + + @Override + public @Nullable Void visitLiteral(LiteralTree node, Set visitedAnnotations) { + String literal = String.valueOf(ASTHelpers.constValue(node)); + + if (visitedAnnotations.isEmpty() && !visitedLiterals.contains(literal)) { + nodes.add(node); + } + visitedLiterals.add(literal); + + return super.visitLiteral(node, visitedAnnotations); + } + + @Override + public @Nullable Void visitPrimitiveType( + PrimitiveTypeTree node, Set visitedAnnotations) { + String primitiveTypeKind = node.getPrimitiveTypeKind().toString(); + + if (visitedAnnotations.isEmpty() && !visitPrimitiveType.contains(primitiveTypeKind)) { + nodes.add(node); + } + visitPrimitiveType.add(primitiveTypeKind); + + return super.visitPrimitiveType(node, visitedAnnotations); + } + }.scan(array, visitedAnnotations); + + return nodes.build(); + } + + // In the case of entries defined for the special attribute name value, if after removing the + // duplicates in a listing array, there's only one element left, then we can omit the brackets. + private static boolean shouldOmitBrackets( + AnnotationTree tree, ImmutableSet nonDuplicateEntries) { + boolean hasValueKeyword = + tree.getArguments().stream() + .filter(arg -> arg.getKind() == Tree.Kind.ASSIGNMENT) + .map(AssignmentTree.class::cast) + .map(AssignmentTree::getVariable) + .map(IdentifierTree.class::cast) + .map(IdentifierTree::getName) + .anyMatch(name -> name.contentEquals("value")); + return nonDuplicateEntries.size() == 1 && hasValueKeyword; + } + + private static String extractName(VisitorState state, Tree expr) { + String exprString = SourceCode.treeToString(expr, state); + + Symbol symbol = ASTHelpers.getSymbol(expr); + if (symbol != null) { + return (symbol.getKind() == INTERFACE || symbol.getKind() == CLASS) + ? exprString + Kind.CLASS.extension + : exprString; + } + + return exprString; + } + + private static AnnotationAttributeMatcher createAnnotationAttributeMatcher( + ErrorProneFlags flags) { + return AnnotationAttributeMatcher.create( + flags.get(INCLUDED_ANNOTATIONS_FLAG).isPresent() + ? Optional.of(flags.getListOrEmpty(INCLUDED_ANNOTATIONS_FLAG)) + : Optional.empty(), + Flags.getList(flags, EXCLUDED_ANNOTATIONS_FLAG)); + } +} 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 d608cd79f0..037d1c53bb 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 @@ -38,8 +38,6 @@ */ // XXX: Currently this checker only flags method-level annotations. It should likely also flag // type-, field- and parameter-level annotations. -// XXX: Duplicate entries are often a mistake. Consider introducing a similar `BugChecker` that -// flags duplicates. @AutoService(BugChecker.class) @BugPattern( summary = "Sort annotations lexicographically where possible", diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListingTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListingTest.java new file mode 100644 index 0000000000..fdfc493560 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListingTest.java @@ -0,0 +1,266 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode; +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class DuplicateAnnotationAttributeListingTest { + @Test + void identification() { + CompilationTestHelper.newInstance(DuplicateAnnotationAttributeListing.class, getClass()) + .addSourceLines( + "A.java", + "import static java.math.RoundingMode.DOWN;", + "import static java.math.RoundingMode.UP;", + "", + "import java.math.RoundingMode;", + "", + "interface A {", + " @interface Foo {", + " String[] value() default {};", + "", + " int[] ints() default {};", + "", + " Class[] cls() default {};", + "", + " RoundingMode[] enums() default {};", + "", + " Bar[] anns() default {};", + " }", + "", + " @interface Bar {", + " String[] value() default {};", + " }", + "", + " @Foo({})", + " A noString();", + "", + " @Foo({\"a\"})", + " A oneString();", + "", + " @Foo({\"a\", \"b\"})", + " A distinctStrings();", + "", + " @Foo({\"a\", \"A\"})", + " A distinctStringCaseInsensitive();", + "", + " // BUG: Diagnostic contains:", + " @Foo({\"a\", \"a\"})", + " A duplicateString();", + "", + " @Foo(ints = {})", + " A noInts();", + "", + " @Foo(ints = {0})", + " A oneInt();", + "", + " @Foo(ints = {0, 1})", + " A distinctInts();", + "", + " // BUG: Diagnostic contains:", + " @Foo(ints = {0, 0})", + " A duplicateInts();", + "", + " @Foo(cls = {})", + " A noClasses();", + "", + " @Foo(cls = {int.class})", + " A oneClass();", + "", + " @Foo(cls = {int.class, long.class})", + " A distinctClasses();", + "", + " // BUG: Diagnostic contains:", + " @Foo(cls = {int.class, int.class})", + " A duplicateClasses();", + "", + " @Foo(enums = {})", + " A noEnums();", + "", + " @Foo(enums = {DOWN})", + " A oneEnum();", + "", + " @Foo(enums = {DOWN, UP})", + " A distinctEnums();", + "", + " // BUG: Diagnostic contains:", + " @Foo(enums = {DOWN, DOWN})", + " A duplicateEnums();", + "", + " @Foo(anns = {})", + " A noAnns();", + "", + " @Foo(anns = {@Bar(\"a\")})", + " A oneAnn();", + "", + " @Foo(anns = {@Bar(\"a\"), @Bar(\"b\")})", + " A distinctAnns();", + "", + " // BUG: Diagnostic contains:", + " @Foo(anns = {@Bar(\"a\"), @Bar(\"a\")})", + " A duplicateAnns();", + "", + " // BUG: Diagnostic contains:", + " @Foo(anns = {@Bar({\"a\", \"a\"}), @Bar({\"b\", \"b\"})})", + " A duplicateListingsInnerAnns();", + "", + " // BUG: Diagnostic contains:", + " @Foo(anns = {@Bar({\"a\", \"b\"}), @Bar({\"a\", \"b\"})})", + " A duplicateInnerAnns();", + "", + " @Foo({\"a=foo\", \"a.b=bar\", \"a.b=baz\"})", + " A hierarchicallyDuplicate();", + "}") + .doTest(); + } + + @Test + void replacement() { + BugCheckerRefactoringTestHelper.newInstance( + DuplicateAnnotationAttributeListing.class, getClass()) + .addInputLines( + "A.java", + "import static java.math.RoundingMode.DOWN;", + "", + "import java.math.RoundingMode;", + "", + "interface A {", + " @interface Foo {", + " String[] value() default {};", + "", + " Class[] cls() default {};", + "", + " RoundingMode[] enums() default {};", + "", + " Bar[] anns() default {};", + " }", + "", + " @interface Bar {", + " String[] value() default {};", + " }", + "", + " @interface Baz {", + " String[] declaredValue() default {};", + " }", + "", + " @Foo({\"\", \" \", \"a\", \"a\"})", + " A duplicateString();", + "", + " @Baz(declaredValue = {\"a\", \"a\"})", + " A duplicateStringWithNoValueKeyword();", + "", + " @Foo(cls = {A.class, A.class})", + " A duplicateClasses();", + "", + " @Foo(enums = {DOWN, DOWN})", + " A duplicateEnums();", + "", + " @Foo(anns = {@Bar(\"a\"), @Bar(\"a\")})", + " A duplicateAnns();", + "", + " @Foo(anns = {@Bar({\"a\", \"a\"}), @Bar({\"b\", \"b\"})})", + " A duplicateListingsInnerAnns();", + "}") + .addOutputLines( + "A.java", + "import static java.math.RoundingMode.DOWN;", + "", + "import java.math.RoundingMode;", + "", + "interface A {", + " @interface Foo {", + " String[] value() default {};", + "", + " Class[] cls() default {};", + "", + " RoundingMode[] enums() default {};", + "", + " Bar[] anns() default {};", + " }", + "", + " @interface Bar {", + " String[] value() default {};", + " }", + "", + " @interface Baz {", + " String[] declaredValue() default {};", + " }", + "", + " @Foo({\"\", \" \", \"a\"})", + " A duplicateString();", + "", + " @Baz(declaredValue = {\"a\"})", + " A duplicateStringWithNoValueKeyword();", + "", + " @Foo(cls = {A.class})", + " A duplicateClasses();", + "", + " @Foo(enums = {DOWN})", + " A duplicateEnums();", + "", + " @Foo(anns = {@Bar(\"a\")})", + " A duplicateAnns();", + "", + " @Foo(anns = {@Bar(\"a\"), @Bar(\"b\")})", + " A duplicateListingsInnerAnns();", + "}") + .doTest(TestMode.TEXT_MATCH); + } + + @Test + void filtering() { + /* Some violations are not flagged because they are not in- or excluded. */ + CompilationTestHelper.newInstance(DuplicateAnnotationAttributeListing.class, getClass()) + .setArgs( + ImmutableList.of( + "-XepOpt:DuplicateAnnotationAttributeListing:Includes=pkg.A.Foo,pkg.A.Bar", + "-XepOpt:DuplicateAnnotationAttributeListing:Excludes=pkg.A.Bar#value")) + .addSourceLines( + "pkg/A.java", + "package pkg;", + "", + "interface A {", + " @interface Foo {", + " String[] value() default {};", + "", + " String[] value2() default {};", + " }", + "", + " @interface Bar {", + " String[] value() default {};", + "", + " String[] value2() default {};", + " }", + "", + " @interface Baz {", + " String[] value() default {};", + "", + " String[] value2() default {};", + " }", + "", + " // BUG: Diagnostic contains:", + " @Foo({\"a\", \"a\"})", + " A fooValue();", + "", + " // BUG: Diagnostic contains:", + " @Foo(value2 = {\"a\", \"a\"})", + " A fooValue2();", + "", + " @Bar({\"a\", \"a\"})", + " A barValue();", + "", + " // BUG: Diagnostic contains:", + " @Bar(value2 = {\"a\", \"a\"})", + " A barValue2();", + "", + " @Baz({\"a\", \"a\"})", + " A bazValue();", + "", + " @Baz(value2 = {\"a\", \"a\"})", + " A bazValue2();", + "}") + .doTest(); + } +} 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 da842fd92a..029592c419 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 @@ -122,7 +122,7 @@ void identification() { "", " // BUG: Diagnostic contains:", " @Foo(anns = {@Bar(\"a\"), @Bar({\"b\", \"a\"})})", - " A unsortedInnderAnns();", + " A unsortedInnerAnns();", "", " @Foo({\"a=foo\", \"a.b=bar\", \"a.c=baz\"})", " A hierarchicallySorted();", @@ -197,7 +197,7 @@ LexicographicalAnnotationAttributeListing.class, getClass()) " A unsortedAnns();", "", " @Foo(anns = {@Bar(\"a\"), @Bar({\"b\", \"a\"})})", - " A unsortedInnderAnns();", + " A unsortedInnerAnns();", "}") .addOutputLines( "A.java", @@ -234,7 +234,7 @@ LexicographicalAnnotationAttributeListing.class, getClass()) " A unsortedAnns();", "", " @Foo(anns = {@Bar(\"a\"), @Bar({\"a\", \"b\"})})", - " A unsortedInnderAnns();", + " A unsortedInnerAnns();", "}") .doTest(TestMode.TEXT_MATCH); }