Skip to content

Commit 1734b84

Browse files
mohamedsamehsalahmohamedsamehsalah
committed
Introduce DuplicateAnnotationAttributeListing 
1 parent 8847a15 commit 1734b84

File tree

4 files changed

+502
-5
lines changed

4 files changed

+502
-5
lines changed

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DuplicateAnnotationAttributeListing.java

+246
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,246 @@
1+
package tech.picnic.errorprone.bugpatterns;
2+
3+
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
4+
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
5+
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
6+
import static java.util.stream.Collectors.joining;
7+
import static javax.lang.model.element.ElementKind.CLASS;
8+
import static javax.lang.model.element.ElementKind.INTERFACE;
9+
import static tech.picnic.errorprone.utils.Documentation.BUG_PATTERNS_BASE_URL;
10+
11+
import com.google.auto.service.AutoService;
12+
import com.google.common.collect.ImmutableSet;
13+
import com.google.errorprone.BugPattern;
14+
import com.google.errorprone.ErrorProneFlags;
15+
import com.google.errorprone.VisitorState;
16+
import com.google.errorprone.bugpatterns.BugChecker;
17+
import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher;
18+
import com.google.errorprone.fixes.Fix;
19+
import com.google.errorprone.fixes.SuggestedFix;
20+
import com.google.errorprone.matchers.Description;
21+
import com.google.errorprone.util.ASTHelpers;
22+
import com.sun.source.tree.AnnotationTree;
23+
import com.sun.source.tree.AssignmentTree;
24+
import com.sun.source.tree.ExpressionTree;
25+
import com.sun.source.tree.IdentifierTree;
26+
import com.sun.source.tree.LiteralTree;
27+
import com.sun.source.tree.NewArrayTree;
28+
import com.sun.source.tree.PrimitiveTypeTree;
29+
import com.sun.source.tree.Tree;
30+
import com.sun.source.util.TreeScanner;
31+
import com.sun.tools.javac.code.Symbol;
32+
import java.util.HashSet;
33+
import java.util.List;
34+
import java.util.Optional;
35+
import java.util.Set;
36+
import javax.inject.Inject;
37+
import javax.tools.JavaFileObject.Kind;
38+
import org.jspecify.annotations.Nullable;
39+
import tech.picnic.errorprone.utils.AnnotationAttributeMatcher;
40+
import tech.picnic.errorprone.utils.Flags;
41+
import tech.picnic.errorprone.utils.SourceCode;
42+
43+
/**
44+
* A {@link BugChecker} that flags and removes duplicate listings inside declared annotations.
45+
*
46+
* <p><b>Example:</b>
47+
*
48+
* <pre>
49+
* {@code @JsonPropertyOrder({ "a", "b", "a" })}
50+
* </pre>
51+
*
52+
* <p><b>Will be flagged and fixed to the following:</b>
53+
*
54+
* <pre>
55+
* {@code @JsonPropertyOrder({ "a", "b"})}
56+
* </pre>
57+
*/
58+
@AutoService(BugChecker.class)
59+
@BugPattern(
60+
summary = "Duplicate listings within an annotation are often a mistake.",
61+
link = BUG_PATTERNS_BASE_URL + "DuplicateAnnotationAttributeListing",
62+
linkType = CUSTOM,
63+
severity = SUGGESTION,
64+
tags = STYLE)
65+
@SuppressWarnings("java:S2160" /* Super class equality definition suffices. */)
66+
public final class DuplicateAnnotationAttributeListing extends BugChecker
67+
implements AnnotationTreeMatcher {
68+
private static final long serialVersionUID = 1L;
69+
private static final String FLAG_PREFIX = "DuplicateAnnotationAttributeListing:";
70+
private static final String INCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Includes";
71+
private static final String EXCLUDED_ANNOTATIONS_FLAG = FLAG_PREFIX + "Excludes";
72+
73+
private final AnnotationAttributeMatcher matcher;
74+
75+
/** Instantiates a default {@link DuplicateAnnotationAttributeListing} instance. */
76+
public DuplicateAnnotationAttributeListing() {
77+
this(ErrorProneFlags.empty());
78+
}
79+
80+
/**
81+
* Instantiates a customized {@link DuplicateAnnotationAttributeListing}.
82+
*
83+
* @param flags Any provided command line flags.
84+
*/
85+
@Inject
86+
DuplicateAnnotationAttributeListing(ErrorProneFlags flags) {
87+
matcher = createAnnotationAttributeMatcher(flags);
88+
}
89+
90+
@Override
91+
public Description matchAnnotation(AnnotationTree tree, VisitorState state) {
92+
return removeDuplicateAnnotationEntries(tree, state)
93+
.map(fix -> describeMatch(tree, fix))
94+
.orElse(Description.NO_MATCH);
95+
}
96+
97+
private Optional<Fix> removeDuplicateAnnotationEntries(AnnotationTree tree, VisitorState state) {
98+
return matcher
99+
.extractMatchingArguments(tree)
100+
.map(expr -> extractArray(expr).flatMap(arr -> removeDuplicates(arr, state)))
101+
.flatMap(Optional::stream)
102+
.reduce(SuggestedFix.Builder::merge)
103+
.map(SuggestedFix.Builder::build);
104+
}
105+
106+
private static Optional<NewArrayTree> extractArray(ExpressionTree expr) {
107+
return expr instanceof AssignmentTree assignment
108+
? extractArray(assignment.getExpression())
109+
: Optional.of(expr).filter(NewArrayTree.class::isInstance).map(NewArrayTree.class::cast);
110+
}
111+
112+
private static Optional<SuggestedFix.Builder> removeDuplicates(
113+
NewArrayTree array, VisitorState state) {
114+
if (array.getInitializers().size() < 2) {
115+
/* There's only one element, no duplicates are expected. */
116+
return Optional.empty();
117+
}
118+
119+
List<? extends ExpressionTree> actualEntries = array.getInitializers();
120+
121+
ImmutableSet<Tree> nonDuplicateEntries = getNonDuplicateEntries(array, state);
122+
123+
if (actualEntries.size() == nonDuplicateEntries.size()) {
124+
/* In the (presumably) common case that no duplicates are found. */
125+
return Optional.empty();
126+
}
127+
128+
// In the case of String entries, if after removing the duplicates in a listing array, there's
129+
// only one element left, then we can omit the brackets.
130+
boolean stringEntries =
131+
nonDuplicateEntries.stream().map(Tree::getKind).allMatch(Tree.Kind.STRING_LITERAL::equals);
132+
String prefix = stringEntries && nonDuplicateEntries.size() == 1 ? "" : "{";
133+
String suffix = prefix.isBlank() ? "" : "}";
134+
135+
String suggestion =
136+
nonDuplicateEntries.stream()
137+
.map(expr -> extractName(state, expr))
138+
.collect(joining(", ", prefix, suffix));
139+
return Optional.of(SuggestedFix.builder().replace(array, suggestion));
140+
}
141+
142+
private static String extractName(VisitorState state, Tree expr) {
143+
String exprString = SourceCode.treeToString(expr, state);
144+
145+
Symbol symbol = ASTHelpers.getSymbol(expr);
146+
if (symbol != null) {
147+
return (symbol.getKind() == INTERFACE || symbol.getKind() == CLASS)
148+
? exprString + Kind.CLASS.extension
149+
: exprString;
150+
}
151+
152+
return exprString;
153+
}
154+
155+
/**
156+
* Extracts from the given array element expression a distinct set of {@link Tree nodes} omitting
157+
* the duplicate entries.
158+
*
159+
* @implNote Annotations are a special case in which we want to make sure we get the distinct
160+
* value of the full annotation expression instead of the annotation name. For example, we
161+
* want to flag the following as a duplicate annotation entry:
162+
* <pre>
163+
* {@code @Foo(anns = { @Bar("a"), @Bar("a") })}
164+
* </pre>
165+
* but not:
166+
* <pre>
167+
* {@code @Foo(anns = { @Bar("a"), @Bar("b") })}
168+
* </pre>
169+
* To do this, we conditionally visit the identifiers, literals and primitive types of the
170+
* original annotation but not those of the annotation entry being visited, given that the
171+
* listings of each nested annotation will be the visited through the {@link
172+
* AnnotationTreeMatcher}.
173+
*/
174+
private static ImmutableSet<Tree> getNonDuplicateEntries(
175+
ExpressionTree array, VisitorState state) {
176+
// Helper collections to identify when a node has been visited by its identifier, to avoid
177+
// including duplicate nodes.
178+
Set<String> visitedAnnotations = new HashSet<>();
179+
Set<String> visitedIdentifiers = new HashSet<>();
180+
Set<String> visitedLiterals = new HashSet<>();
181+
Set<String> visitPrimitiveType = new HashSet<>();
182+
183+
ImmutableSet.Builder<Tree> nodes = ImmutableSet.builder();
184+
185+
new TreeScanner<@Nullable Void, Set<String>>() {
186+
@Override
187+
public @Nullable Void visitAnnotation(AnnotationTree node, Set<String> visitedAnnotations) {
188+
String annotation = SourceCode.treeToString(node, state);
189+
if (!visitedAnnotations.contains(annotation)) {
190+
nodes.add(node);
191+
}
192+
visitedAnnotations.add(annotation);
193+
194+
return super.visitAnnotation(node, visitedAnnotations);
195+
}
196+
197+
@Override
198+
public @Nullable Void visitIdentifier(IdentifierTree node, Set<String> visitedAnnotations) {
199+
String identifier = node.getName().toString();
200+
201+
if (visitedAnnotations.isEmpty() && !visitedIdentifiers.contains(identifier)) {
202+
nodes.add(node);
203+
}
204+
visitedIdentifiers.add(identifier);
205+
206+
return super.visitIdentifier(node, visitedAnnotations);
207+
}
208+
209+
@Override
210+
public @Nullable Void visitLiteral(LiteralTree node, Set<String> visitedAnnotations) {
211+
String literal = String.valueOf(ASTHelpers.constValue(node));
212+
213+
if (visitedAnnotations.isEmpty() && !visitedLiterals.contains(literal)) {
214+
nodes.add(node);
215+
}
216+
visitedLiterals.add(literal);
217+
218+
return super.visitLiteral(node, visitedAnnotations);
219+
}
220+
221+
@Override
222+
public @Nullable Void visitPrimitiveType(
223+
PrimitiveTypeTree node, Set<String> visitedAnnotations) {
224+
String primitiveTypeKind = node.getPrimitiveTypeKind().toString();
225+
226+
if (visitedAnnotations.isEmpty() && !visitPrimitiveType.contains(primitiveTypeKind)) {
227+
nodes.add(node);
228+
}
229+
visitPrimitiveType.add(primitiveTypeKind);
230+
231+
return super.visitPrimitiveType(node, visitedAnnotations);
232+
}
233+
}.scan(array, visitedAnnotations);
234+
235+
return nodes.build();
236+
}
237+
238+
private static AnnotationAttributeMatcher createAnnotationAttributeMatcher(
239+
ErrorProneFlags flags) {
240+
return AnnotationAttributeMatcher.create(
241+
flags.get(INCLUDED_ANNOTATIONS_FLAG).isPresent()
242+
? Optional.of(flags.getListOrEmpty(INCLUDED_ANNOTATIONS_FLAG))
243+
: Optional.empty(),
244+
Flags.getList(flags, EXCLUDED_ANNOTATIONS_FLAG));
245+
}
246+
}

‎error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/LexicographicalAnnotationListing.java

-2
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@
3838
*/
3939
// XXX: Currently this checker only flags method-level annotations. It should likely also flag
4040
// type-, field- and parameter-level annotations.
41-
// XXX: Duplicate entries are often a mistake. Consider introducing a similar `BugChecker` that
42-
// flags duplicates.
4341
@AutoService(BugChecker.class)
4442
@BugPattern(
4543
summary = "Sort annotations lexicographically where possible",

0 commit comments

Comments
 (0)