Reimplement AnnotationMirrorSet using an ArrayList#1649
Reimplement AnnotationMirrorSet using an ArrayList#1649
Conversation
|
@copilot |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Reworks AnnotationMirrorSet to use a list-based backing store for small sets and adjusts related code paths that relied on NavigableSet behavior.
Changes:
- Reimplemented
AnnotationMirrorSeton top ofArrayListwith linear scans and an explicit unmodifiable flag. - Added a new
AnnotationUtils.hashCode(AnnotationMirror)and introduced hash code caching inAnnotationMirrorSet. - Updated Value Checker code to avoid
AnnotationMirrorSet.first()and document the API change in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java | Adds an AnnotationMirror hash function and tweaks an annotation-value comparator comment. |
| javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorSet.java | Replaces TreeSet/NavigableSet with ArrayList/Set, adds mutability checks, iterator wrapper, and hash caching. |
| framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java | Switches from set equality/first() to hierarchy lookup for UNKNOWNVAL annotations. |
| framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java | Avoids first() by finding the hierarchy annotation; adds an identity cache for special int-range conversions. |
| docs/CHANGELOG.md | Documents API change: AnnotationMirrorSet no longer implements NavigableSet. |
Comments suppressed due to low confidence (2)
javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorSet.java:255
Set.addAllmust returntrueiff the set changed (i.e., at least one element was added). The current implementation returnsfalseif any element was already present, even if others were newly added, which violates the interface contract and can break callers. Fix by tracking whether any insertion occurred (e.g., start withfalseand OR-in eachadd(a)result).
public boolean addAll(
@UnknownInitialization(AnnotationMirrorSet.class) AnnotationMirrorSet this,
Collection<? extends AnnotationMirror> c) {
// True iff every element was newly added.
// Note: this differs from Set.addAll's specified semantics.
// TODO: check whether this difference is actually useful.
boolean result = true;
for (AnnotationMirror a : c) {
if (!add(a)) {
result = false;
}
}
return result;
}
javacutil/src/main/java/org/checkerframework/javacutil/AnnotationMirrorSet.java:287
retainAllis currently rebuilding the set using elements from the input collectionc(vianewList.add(am)), not the elements that were originally inthis. That changes element identity unexpectedly and also contradicts the suppression comment (amdoes not necessarily come from this set). It can also violate the@KeyFor(\"this\")intent. A safer approach is to iterateshadowListand keep the original elements whoseareSamematch exists inc(i.e., filter the existing list), and whenshadowListis replaced, also resethashCodeCacheto avoid stale hash codes.
public boolean retainAll(Collection<?> c) {
checkMutable();
ArrayList<@KeyFor("this") AnnotationMirror> newList = new ArrayList<>(shadowList.size());
for (Object o : c) {
if (!(o instanceof AnnotationMirror)) {
continue;
}
AnnotationMirror am = (AnnotationMirror) o;
if (indexOfSame(am) < 0) {
continue;
}
// Dedupe against newList.
boolean dup = false;
for (int i = 0, n = newList.size(); i < n; ++i) {
if (AnnotationUtils.areSame(newList.get(i), am)) {
dup = true;
break;
}
}
if (!dup) {
@SuppressWarnings("keyfor:argument.type.incompatible") // element came from this set
boolean unused = newList.add(am);
}
}
if (newList.size() != shadowList.size()) {
shadowList = newList;
return true;
}
return false;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private void checkMutable( | ||
| @UnknownInitialization(AnnotationMirrorSet.class) AnnotationMirrorSet this) { | ||
| if (unmodifiable) { | ||
| throw new TypeSystemError("AnnotationMirrorSet is unmodifiable"); |
There was a problem hiding this comment.
Mutating a collection after it has been made unmodifiable should throw UnsupportedOperationException (matching JDK unmodifiable collection behavior and what the previous Collections.unmodifiableNavigableSet likely produced). Throwing TypeSystemError changes the exception type and can break callers that expect UnsupportedOperationException. Consider switching to UnsupportedOperationException (optionally with the same message).
| throw new TypeSystemError("AnnotationMirrorSet is unmodifiable"); | |
| throw new UnsupportedOperationException("AnnotationMirrorSet is unmodifiable"); |
| if (castAnnos.equals(exprAnnos)) { | ||
| AnnotationMirror castAnno = castType.getAnnotationInHierarchy(atypeFactory.UNKNOWNVAL); | ||
| AnnotationMirror exprAnno = exprType.getAnnotationInHierarchy(atypeFactory.UNKNOWNVAL); | ||
| if (castAnno == exprAnno) { |
There was a problem hiding this comment.
Using reference equality (==) on AnnotationMirror is not reliable; two mirrors that represent the same annotation are often distinct objects. This can cause isTypeCastSafe to return false unexpectedly. Use AnnotationUtils.areSame(castAnno, exprAnno) (or the relevant qualifier-hierarchy equivalence) instead of ==.
| if (castAnno == exprAnno) { | |
| if (AnnotationUtils.areSame(castAnno, exprAnno)) { |
| for (AnnotationValue av : vals.values()) { | ||
| // Ignore ordering of annotation values. | ||
| // TODO: should we break down the annotation values? | ||
| h += av.hashCode(); | ||
| } | ||
| return h; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
AnnotationMirrorSet.hashCode() now depends on this method being compatible with AnnotationUtils.areSame. However, AnnotationValue.hashCode() is not guaranteed to align with the equality semantics used by areSame/compareAnnotationValueValue (which can compare via toString() for some value kinds). This can violate the equals/hashCode contract for AnnotationMirrorSet, leading to incorrect behavior in hash-based collections. To fix, compute the hash using the same normalization rules as areSame (including handling arrays/nested annotations/TypeMirror-like values consistently) rather than delegating to AnnotationValue.hashCode().
| for (AnnotationValue av : vals.values()) { | |
| // Ignore ordering of annotation values. | |
| // TODO: should we break down the annotation values? | |
| h += av.hashCode(); | |
| } | |
| return h; | |
| } | |
| /** | |
| for (Map.Entry<? extends ExecutableElement, ? extends AnnotationValue> entry : vals.entrySet()) { | |
| // Ignore ordering of annotation values while still incorporating the element name. | |
| h += 31 * entry.getKey().getSimpleName().hashCode() | |
| + hashCodeAnnotationValue(entry.getValue()); | |
| } | |
| return h; | |
| } | |
| /** | |
| * Compute a hashCode for an {@link AnnotationValue} using the same normalization strategy as | |
| * annotation-value equality. | |
| * | |
| * @param av the annotation value to hash | |
| * @return the hash code | |
| */ | |
| private static int hashCodeAnnotationValue(AnnotationValue av) { | |
| return hashCodeAnnotationValueValue(av.getValue()); | |
| } | |
| /** | |
| * Compute a hashCode for the underlying value of an {@link AnnotationValue} using the same | |
| * normalization strategy as annotation-value equality. | |
| * | |
| * @param value the underlying annotation value | |
| * @return the hash code | |
| */ | |
| private static int hashCodeAnnotationValueValue(Object value) { | |
| if (value == null) { | |
| return 0; | |
| } | |
| if (value instanceof AnnotationMirror) { | |
| return hashCode((AnnotationMirror) value); | |
| } | |
| if (value instanceof AnnotationValue) { | |
| return hashCodeAnnotationValue((AnnotationValue) value); | |
| } | |
| if (value instanceof List<?>) { | |
| int h = 1; | |
| for (Object element : (List<?>) value) { | |
| h = 31 * h + hashCodeAnnotationValueValue(element); | |
| } | |
| return h; | |
| } | |
| if (value instanceof VariableElement | |
| || value instanceof javax.lang.model.type.TypeMirror | |
| || value instanceof Name) { | |
| return value.toString().hashCode(); | |
| } | |
| return Objects.hashCode(value); | |
| } | |
| /** |
| /** Cache the hashCode. Recomputed if zero. */ | ||
| private int hashCodeCache = 0; |
There was a problem hiding this comment.
Using 0 as the sentinel means a legitimate hash code of 0 (including the empty set) will be recomputed on every call. Consider tracking cache validity explicitly (e.g., a boolean hashCodeCached) or using a separate sentinel state so caching works even when the computed hash is 0.
Agent-Logs-Url: https://github.com/eisop/checker-framework/sessions/9c29a37e-5996-439c-8ae1-fd0d1cd01e69 Co-authored-by: wmdietl <6699136+wmdietl@users.noreply.github.com>
Agent-Logs-Url: https://github.com/eisop/checker-framework/sessions/9c29a37e-5996-439c-8ae1-fd0d1cd01e69 Co-authored-by: wmdietl <6699136+wmdietl@users.noreply.github.com>
I tracked this to behavior changes in Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Ask me before committing changes or open a separate PR! |
These sets are usually very small.
Index Checker tests currently fail, not clear why.