-
Notifications
You must be signed in to change notification settings - Fork 331
Update jdk-javac-plugin to contain nested annotations #1432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
ad3a0f3
4358508
d66e0e4
c2fb5ff
4c9211b
582aade
e7e19be
b21c4d2
0d0f0de
4958718
d83bc26
981ec0c
d5cc811
8606f18
1ae87d2
42b3f41
e8b97f0
997a76b
000f06c
e879332
ad29bce
06c4233
e49e75c
c558007
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,153 @@ | ||||||
| package com.uber.nullaway.javacplugin; | ||||||
|
|
||||||
| import com.google.common.collect.ImmutableList; | ||||||
| import com.sun.tools.javac.code.Type; | ||||||
| import com.sun.tools.javac.code.Types; | ||||||
| import com.uber.nullaway.librarymodel.NestedAnnotationInfo; | ||||||
| import com.uber.nullaway.librarymodel.NestedAnnotationInfo.Annotation; | ||||||
| import com.uber.nullaway.librarymodel.NestedAnnotationInfo.TypePathEntry; | ||||||
| import java.util.ArrayDeque; | ||||||
| import java.util.HashSet; | ||||||
| import java.util.List; | ||||||
| import java.util.Set; | ||||||
| import javax.lang.model.element.AnnotationMirror; | ||||||
| import javax.lang.model.element.TypeElement; | ||||||
| import javax.lang.model.type.TypeMirror; | ||||||
| import org.jspecify.annotations.NullMarked; | ||||||
| import org.jspecify.annotations.Nullable; | ||||||
|
|
||||||
| @NullMarked | ||||||
| public class CreateNestedAnnotationInfoVisitor | ||||||
msridhar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| extends Types.DefaultTypeVisitor<Set<NestedAnnotationInfo>, @Nullable Void> { | ||||||
|
||||||
| extends Types.DefaultTypeVisitor<Set<NestedAnnotationInfo>, @Nullable Void> { | |
| extends Types.DefaultTypeVisitor<@Nullable Void, @Nullable Void> { |
Then, return null from every visitXXX method, but add a getter method getNestedAnnotationInfoSet() so that the client can retrieve the set at the end.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Make visitor state safer (final fields + clearer naming + avoid exposing mutable internals).
path/nestedAnnotationInfoList are mutable instance state and the returned Set is the same object (callers can mutate it). At minimum: mark fields final, rename nestedAnnotationInfoList → nestedAnnotationInfoSet, and consider returning an immutable copy at the end of the top-level call.
Proposed diff
- private ArrayDeque<TypePathEntry> path;
- private Set<NestedAnnotationInfo> nestedAnnotationInfoList;
+ private final ArrayDeque<TypePathEntry> path = new ArrayDeque<>();
+ private final Set<NestedAnnotationInfo> nestedAnnotationInfoSet = new HashSet<>();
public CreateNestedAnnotationInfoVisitor() {
- path = new ArrayDeque<>();
- nestedAnnotationInfoList = new HashSet<>();
}🤖 Prompt for AI Agents
In
@jdk-javac-plugin/src/main/java/com/uber/nullaway/javacplugin/CreateNestedAnnotationInfoVisitor.java
around lines 23 - 29, Make the visitor's state immutable and avoid exposing
mutable internals: mark the fields path and nestedAnnotationInfoList as final
and rename nestedAnnotationInfoList → nestedAnnotationInfoSet for clarity; keep
path private/final (ArrayDeque<TypePathEntry>) and do not expose it. Wherever
the visitor currently returns its set (e.g., a getNestedAnnotationInfo or the
top-level visit method), return an immutable copy (e.g.,
Set.copyOf(nestedAnnotationInfoSet) or Collections.unmodifiableSet(new
HashSet<>(nestedAnnotationInfoSet))) instead of the internal collection so
callers cannot mutate internal state.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not store this in a local variable here, since it will do a copy of path for every type variable, whether it is annotated or not. Instead, just call getTypePath() at lines 45 and 47
Uh oh!
There was an error while loading. Please reload this page.