Add @DoesNotUnrefineReceiver("modifiability") annotations#276
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates Checker Framework annotations across core JDK classes, interfaces, collection implementations, concurrent collections, iterators, views, and helper utilities. The changes add Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java (1)
1342-1352:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
@Pureis unsound on methods reading concurrently mutable state; use@SideEffectFreeinstead.Methods at lines 1342, 4801, 4990, 5386, 5431, 5476, and 5521 are annotated
@Purebut read from volatile/concurrent fields (table,result.get()from AtomicReference). Since@Purerequires both side-effect-freedom and determinism (same return value for same receiver/args/environment), and determinism cannot be guaranteed under concurrent mutation, these methods violate the@Purecontract. Replace with@SideEffectFree, which is already imported and used elsewhere in the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java` around lines 1342 - 1352, The methods annotated `@Pure` (notably hashCode() and the other methods that read concurrent/volatile state or call AtomicReference.get()) are not deterministic under concurrent mutation; replace the `@Pure` annotation with `@SideEffectFree` for each such method (e.g., hashCode and the methods referenced at lines 4801, 4990, 5386, 5431, 5476, 5521) so the annotation accurately reflects side-effect freedom without implying determinism—ensure you import/use the existing SideEffectFree annotation already present in the file.src/java.base/share/classes/java/util/IdentityHashMap.java (1)
795-806:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
@Pureannotation—nextIndexmutates receiver state.The
nextIndexmethod modifies three receiver fields (indexValidon line 802,lastReturnedIndexon line 803, andindexon line 804). The@Pureannotation contradicts this behavior. Use@SideEffectsOnly("this")instead, as indicated by the commented-out annotation on line 794.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/util/IdentityHashMap.java` around lines 795 - 806, Replace the incorrect `@Pure` annotation on method nextIndex of IdentityHashMapIterator<T> with `@SideEffectsOnly`("this") because nextIndex mutates receiver state (it updates indexValid, lastReturnedIndex, and index) and therefore is not pure; locate the nextIndex method and the commented-out `@SideEffectsOnly`("this") and re-enable that annotation in place of `@Pure` so the annotation accurately reflects the method’s side effects.src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java (1)
1645-1650:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
@SideEffectFreefrom memoizing methods that mutate receiver fields.Methods
keySet()andvalues()perform lazy initialization by assigning to receiver fields (keySet,values,keySetView,valuesView). The@SideEffectFreeannotation is incorrect—these methods mutate receiver state and violate the annotation's contract, causing unsound checker assumptions.Affected methods:
keySet()at 1645–1650 and 3027–3032values()at 1678–1683 and 3041–3046🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java` around lines 1645 - 1650, Remove the incorrect `@SideEffectFree` annotation from the lazy-memoizing accessor methods that mutate receiver fields—specifically the keySet() and values() methods (both overloads/occurrences) which assign to the cached fields keySet, values, keySetView and valuesView; update the method declarations to drop `@SideEffectFree` so the checker no longer assumes they are side-effect-free and no other behavioral changes are made.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/java.base/share/classes/java/util/concurrent/ConcurrentLinkedDeque.java`:
- Around line 932-943: The annotated purity is incorrect because getFirst(),
getLast(), toString(), Itr.startNode(), and DescendingItr.startNode() call
first()/last() which can mutate head/tail via
HEAD.compareAndSet()/TAIL.compareAndSet(); remove the `@Pure` and `@SideEffectFree`
annotations from these methods (getFirst, getLast, toString, Itr.startNode,
DescendingItr.startNode) so they no longer claim to be side-effect-free and
update any method-level annotations/comments accordingly to reflect that these
calls may modify head/tail through CAS.
In `@src/java.base/share/classes/java/util/ImmutableCollections.java`:
- Around line 1360-1363: The method nextIndex() mutates the receiver (it assigns
to this.idx) so the `@Pure` annotation is incorrect; remove the `@Pure` annotation
from the nextIndex() method declaration (leave other annotations like
`@DoesNotUnrefineReceiver`("modifiability") intact) so the method's metadata
correctly reflects that it has visible side effects.
In `@src/java.base/share/classes/java/util/LinkedHashMap.java`:
- Around line 1129-1132: The `@Pure` annotation on
LinkedHashMap.LinkedKeyIterator.next() is incorrect because nextNode() mutates
iterator state; remove the `@Pure` annotation from the LinkedKeyIterator.next(...)
declaration (leaving `@SideEffectsOnly`("this") and
`@DoesNotUnrefineReceiver`("modifiability") intact) so its contract matches
LinkedValueIterator.next() and LinkedEntryIterator.next() which are not marked
`@Pure`.
- Around line 1230-1233: Remove the `@Pure` annotation from the
ReversedLinkedHashMapView methods public V get(Object key) and public V
getOrDefault(Object key, V defaultValue) because they call base.get(key) /
base.getOrDefault(key, defaultValue) which may invoke afterNodeAccess(e) when
the underlying LinkedHashMap has accessOrder=true and thus mutate internal state
and modCount; locate the methods by their signatures (get(Object) and
getOrDefault(Object, V)) and simply delete the `@Pure` annotations so the
annotations no longer claim immutability.
In `@src/java.base/share/classes/java/util/TreeSet.java`:
- Around line 638-641: Remove the redundant
`@DoesNotUnrefineReceiver`("modifiability") annotation from the spliterator()
method (and the other 8 similar occurrences); keep the `@SideEffectFree`
annotation only, since it already guarantees no visible side effects and thus
preserves receiver modifiability—update the annotations on public Spliterator<E>
spliterator() (and the matching method declarations) to drop
`@DoesNotUnrefineReceiver` while leaving `@SideEffectFree` intact.
---
Outside diff comments:
In `@src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java`:
- Around line 1342-1352: The methods annotated `@Pure` (notably hashCode() and the
other methods that read concurrent/volatile state or call AtomicReference.get())
are not deterministic under concurrent mutation; replace the `@Pure` annotation
with `@SideEffectFree` for each such method (e.g., hashCode and the methods
referenced at lines 4801, 4990, 5386, 5431, 5476, 5521) so the annotation
accurately reflects side-effect freedom without implying determinism—ensure you
import/use the existing SideEffectFree annotation already present in the file.
In `@src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java`:
- Around line 1645-1650: Remove the incorrect `@SideEffectFree` annotation from
the lazy-memoizing accessor methods that mutate receiver fields—specifically the
keySet() and values() methods (both overloads/occurrences) which assign to the
cached fields keySet, values, keySetView and valuesView; update the method
declarations to drop `@SideEffectFree` so the checker no longer assumes they are
side-effect-free and no other behavioral changes are made.
In `@src/java.base/share/classes/java/util/IdentityHashMap.java`:
- Around line 795-806: Replace the incorrect `@Pure` annotation on method
nextIndex of IdentityHashMapIterator<T> with `@SideEffectsOnly`("this") because
nextIndex mutates receiver state (it updates indexValid, lastReturnedIndex, and
index) and therefore is not pure; locate the nextIndex method and the
commented-out `@SideEffectsOnly`("this") and re-enable that annotation in place of
`@Pure` so the annotation accurately reflects the method’s side effects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f532fdfd-0288-4225-91b4-af69dfbd20d8
📒 Files selected for processing (94)
src/java.base/share/classes/java/io/BufferedReader.javasrc/java.base/share/classes/java/lang/CharSequence.javasrc/java.base/share/classes/java/lang/Class.javasrc/java.base/share/classes/java/lang/String.javasrc/java.base/share/classes/java/lang/invoke/AbstractConstantGroup.javasrc/java.base/share/classes/java/net/URL.javasrc/java.base/share/classes/java/nio/charset/Charset.javasrc/java.base/share/classes/java/nio/file/FileTreeIterator.javasrc/java.base/share/classes/java/nio/file/Files.javasrc/java.base/share/classes/java/nio/file/Path.javasrc/java.base/share/classes/java/security/Provider.javasrc/java.base/share/classes/java/util/AbstractCollection.javasrc/java.base/share/classes/java/util/AbstractList.javasrc/java.base/share/classes/java/util/AbstractMap.javasrc/java.base/share/classes/java/util/AbstractQueue.javasrc/java.base/share/classes/java/util/AbstractSequentialList.javasrc/java.base/share/classes/java/util/AbstractSet.javasrc/java.base/share/classes/java/util/ArrayDeque.javasrc/java.base/share/classes/java/util/ArrayList.javasrc/java.base/share/classes/java/util/Arrays.javasrc/java.base/share/classes/java/util/Collection.javasrc/java.base/share/classes/java/util/Collections.javasrc/java.base/share/classes/java/util/Deque.javasrc/java.base/share/classes/java/util/EnumMap.javasrc/java.base/share/classes/java/util/EnumSet.javasrc/java.base/share/classes/java/util/Enumeration.javasrc/java.base/share/classes/java/util/HashMap.javasrc/java.base/share/classes/java/util/HashSet.javasrc/java.base/share/classes/java/util/Hashtable.javasrc/java.base/share/classes/java/util/IdentityHashMap.javasrc/java.base/share/classes/java/util/ImmutableCollections.javasrc/java.base/share/classes/java/util/Iterator.javasrc/java.base/share/classes/java/util/JumboEnumSet.javasrc/java.base/share/classes/java/util/LinkedHashMap.javasrc/java.base/share/classes/java/util/LinkedHashSet.javasrc/java.base/share/classes/java/util/LinkedList.javasrc/java.base/share/classes/java/util/List.javasrc/java.base/share/classes/java/util/ListIterator.javasrc/java.base/share/classes/java/util/Map.javasrc/java.base/share/classes/java/util/NavigableMap.javasrc/java.base/share/classes/java/util/NavigableSet.javasrc/java.base/share/classes/java/util/Objects.javasrc/java.base/share/classes/java/util/PrimitiveIterator.javasrc/java.base/share/classes/java/util/PriorityQueue.javasrc/java.base/share/classes/java/util/Properties.javasrc/java.base/share/classes/java/util/Queue.javasrc/java.base/share/classes/java/util/RegularEnumSet.javasrc/java.base/share/classes/java/util/ReverseOrderDequeView.javasrc/java.base/share/classes/java/util/ReverseOrderListView.javasrc/java.base/share/classes/java/util/ReverseOrderSortedMapView.javasrc/java.base/share/classes/java/util/ReverseOrderSortedSetView.javasrc/java.base/share/classes/java/util/Scanner.javasrc/java.base/share/classes/java/util/SequencedCollection.javasrc/java.base/share/classes/java/util/SequencedMap.javasrc/java.base/share/classes/java/util/SequencedSet.javasrc/java.base/share/classes/java/util/ServiceLoader.javasrc/java.base/share/classes/java/util/Set.javasrc/java.base/share/classes/java/util/SortedMap.javasrc/java.base/share/classes/java/util/SortedSet.javasrc/java.base/share/classes/java/util/Spliterators.javasrc/java.base/share/classes/java/util/Stack.javasrc/java.base/share/classes/java/util/TreeMap.javasrc/java.base/share/classes/java/util/TreeSet.javasrc/java.base/share/classes/java/util/Vector.javasrc/java.base/share/classes/java/util/WeakHashMap.javasrc/java.base/share/classes/java/util/concurrent/ArrayBlockingQueue.javasrc/java.base/share/classes/java/util/concurrent/BlockingDeque.javasrc/java.base/share/classes/java/util/concurrent/BlockingQueue.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentLinkedDeque.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentLinkedQueue.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentMap.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentNavigableMap.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentSkipListSet.javasrc/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.javasrc/java.base/share/classes/java/util/concurrent/CopyOnWriteArraySet.javasrc/java.base/share/classes/java/util/concurrent/DelayQueue.javasrc/java.base/share/classes/java/util/concurrent/LinkedBlockingDeque.javasrc/java.base/share/classes/java/util/concurrent/LinkedBlockingQueue.javasrc/java.base/share/classes/java/util/concurrent/LinkedTransferQueue.javasrc/java.base/share/classes/java/util/concurrent/PriorityBlockingQueue.javasrc/java.base/share/classes/java/util/concurrent/ScheduledThreadPoolExecutor.javasrc/java.base/share/classes/java/util/concurrent/SynchronousQueue.javasrc/java.base/share/classes/java/util/jar/Attributes.javasrc/java.base/share/classes/java/util/regex/Matcher.javasrc/java.base/share/classes/java/util/regex/Pattern.javasrc/java.base/share/classes/java/util/stream/SpinedBuffer.javasrc/java.base/share/classes/java/util/stream/Stream.javasrc/java.base/share/classes/java/util/zip/ZipFile.javasrc/java.base/share/classes/jdk/internal/org/objectweb/asm/tree/InsnList.javasrc/java.base/share/classes/sun/net/www/HeaderParser.javasrc/java.base/share/classes/sun/net/www/MessageHeader.javasrc/jdk.httpserver/share/classes/com/sun/net/httpserver/Headers.java
💤 Files with no reviewable changes (1)
- src/java.base/share/classes/java/util/stream/Stream.java
Member
Author
|
@CodeRabbit The Java method annotation |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/java.base/share/classes/java/util/LinkedHashMap.java (1)
556-572:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove
@Pureannotations from access-order-sensitive methodsLines 557 and 571 mark
get()andgetOrDefault()as@Pure, but both methods mutate receiver state viaafterNodeAccess(...)whenaccessOrder==true(lines 562–563 and 576–577). This violates the@Purecontract, which requires methods to be side-effect-free. The existingCFCommentalready documents that these methods are "not strictly pure" when access order is enabled.Suggested fix
`@CFComment`("`get()` is not strictly pure: if `accessOrder==true`, it changes the access order") -@Pure public `@Nullable` V get(`@GuardSatisfied` LinkedHashMap<K, V> this, `@UnknownSignedness` `@GuardSatisfied` `@Nullable` Object key) { Node<K,V> e; if ((e = getNode(key)) == null) return null; if (accessOrder) afterNodeAccess(e); return e.value; } /** * {`@inheritDoc`} */ `@CFComment`("`getOrDefault()` is not strictly pure: if `accessOrder==true`, it changes the access order") -@Pure public V getOrDefault(`@Nullable` Object key, V defaultValue) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/java.base/share/classes/java/util/LinkedHashMap.java` around lines 556 - 572, Remove the incorrect `@Pure` annotation from the access-order-sensitive methods: delete the `@Pure` annotation on the get(`@GuardSatisfied` LinkedHashMap<K,V> this, `@Nullable` Object key) method and the `@Pure` annotation on getOrDefault(`@Nullable` Object key, V defaultValue), since both call afterNodeAccess(e) when accessOrder is true and therefore can mutate receiver state; leave the existing CFComment lines intact to document the conditional impurity and ensure no other annotations or method signatures are changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/java.base/share/classes/java/util/LinkedHashMap.java`:
- Around line 556-572: Remove the incorrect `@Pure` annotation from the
access-order-sensitive methods: delete the `@Pure` annotation on the
get(`@GuardSatisfied` LinkedHashMap<K,V> this, `@Nullable` Object key) method and
the `@Pure` annotation on getOrDefault(`@Nullable` Object key, V defaultValue),
since both call afterNodeAccess(e) when accessOrder is true and therefore can
mutate receiver state; leave the existing CFComment lines intact to document the
conditional impurity and ensure no other annotations or method signatures are
changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93bae187-324c-4934-a24a-e03c112a979b
📒 Files selected for processing (11)
src/java.base/share/classes/java/util/Collections.javasrc/java.base/share/classes/java/util/HashSet.javasrc/java.base/share/classes/java/util/IdentityHashMap.javasrc/java.base/share/classes/java/util/ImmutableCollections.javasrc/java.base/share/classes/java/util/LinkedHashMap.javasrc/java.base/share/classes/java/util/LinkedHashSet.javasrc/java.base/share/classes/java/util/RegularEnumSet.javasrc/java.base/share/classes/java/util/ReverseOrderListView.javasrc/java.base/share/classes/java/util/SortedSet.javasrc/java.base/share/classes/java/util/TreeSet.javasrc/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
💤 Files with no reviewable changes (7)
- src/java.base/share/classes/java/util/HashSet.java
- src/java.base/share/classes/java/util/LinkedHashSet.java
- src/java.base/share/classes/java/util/RegularEnumSet.java
- src/java.base/share/classes/java/util/SortedSet.java
- src/java.base/share/classes/java/util/ImmutableCollections.java
- src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
- src/java.base/share/classes/java/util/IdentityHashMap.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge with typetools/jdk21u#8