@SideEffectFree annotations - #282
Open
mernst wants to merge 16 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR activates Checker Framework Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/java.base/share/classes/sun/net/www/HeaderParser.java (1)
212-216: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnforce the
Iteratorcontract by throwingNoSuchElementException.While the annotation additions are correct, the
next()method itself violates the standardIteratorcontract. If called when the iterator is exhausted, it throws an unexpectedArrayIndexOutOfBoundsExceptioninstead of aNoSuchElementException.Add a boundary check to throw the correct exception.
🐛 Proposed fix
`@SideEffectsOnly`("this") `@DoesNotUnrefineReceiver`("modifiability") public String next () { + if (!hasNext()) { + throw new java.util.NoSuchElementException(); + } return tab[index++][returnsValue?1:0]; }🤖 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/sun/net/www/HeaderParser.java` around lines 212 - 216, Update HeaderParser.next() to check whether the iterator is exhausted before indexing tab, and throw NoSuchElementException when index has reached the available entries. Preserve the existing return and index-advance behavior for valid elements.src/java.base/share/classes/java/util/regex/Matcher.java (1)
1359-1369: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude the enclosing instance in these
@SideEffectsOnlylists.
"this"only covers the iterator object; these methods also mutate the outer collection.
src/java.base/share/classes/java/util/regex/Matcher.java#L1359-L1369: addMatcher.thissrc/java.base/share/classes/java/util/RegularEnumSet.java#L138-L145: addRegularEnumSet.thissrc/java.base/share/classes/java/util/PriorityQueue.java#L572-L594: addPriorityQueue.this🤖 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/regex/Matcher.java` around lines 1359 - 1369, Update the `@SideEffectsOnly` annotations on the affected iterator methods to include both the iterator receiver and its enclosing collection instance: add Matcher.this in src/java.base/share/classes/java/util/regex/Matcher.java:1359-1369, RegularEnumSet.this in src/java.base/share/classes/java/util/RegularEnumSet.java:138-145, and PriorityQueue.this in src/java.base/share/classes/java/util/PriorityQueue.java:572-594.src/java.base/share/classes/java/util/concurrent/BlockingQueue.java (1)
380-409: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
cindrainTo’s side-effect spec. Both overloads can mutatecas well asthis, so@SideEffectsOnly("this")is incomplete; list both targets with the argument-expression form (for example,@SideEffectsOnly({"this", "#1"})).🤖 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/BlockingQueue.java` around lines 380 - 409, Update both drainTo overloads in BlockingQueue so their `@SideEffectsOnly` annotations include both the receiver and collection argument c, using the argument-expression target form "`#1`". Leave the existing `@DoesNotUnrefineReceiver` annotations and method signatures unchanged.src/java.base/share/classes/java/util/concurrent/SynchronousQueue.java (1)
1125-1138: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude the collection parameter in the
@SideEffectsOnlyannotation.The
drainTomethods mutate the provided collectioncby adding elements to it (c.add(e)). Therefore, the@SideEffectsOnly("this")annotation is incorrect as it claims only the receiver (this) is modified.Consider updating the annotation to include the first parameter, for example,
@SideEffectsOnly({"this", "#1"}), so that the static analysis correctly tracks the mutation on the passed collection.🐛 Proposed fix
- `@SideEffectsOnly`("this") + `@SideEffectsOnly`({"this", "`#1`"}) `@DoesNotUnrefineReceiver`("modifiability") public int drainTo(`@GuardSatisfied` `@CanShrink` SynchronousQueue<E> this, Collection<? super E> c) {And similarly for the other
drainTooverload:- `@SideEffectsOnly`("this") + `@SideEffectsOnly`({"this", "`#1`"}) `@DoesNotUnrefineReceiver`("modifiability") public int drainTo(`@GuardSatisfied` `@CanShrink` SynchronousQueue<E> this, Collection<? super E> c, int maxElements) {Also applies to: 1143-1156
🤖 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/SynchronousQueue.java` around lines 1125 - 1138, Update the `@SideEffectsOnly` annotations on both drainTo overloads in SynchronousQueue to include the collection parameter alongside "this" (for example, "this" and "`#1`"), so the mutation performed by c.add(e) is correctly tracked.src/java.base/share/classes/java/util/Collections.java (1)
1204-1215: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove incorrect
@SideEffectsOnlyfrom read-only methods. Read-only view methods and stream-creation methods were erroneously annotated with@SideEffectsOnly("this")and@DoesNotUnrefineReceiver("modifiability"). This incorrectly flags them as mutators and breaks static analysis by preventing their use on immutable references. The shared root cause is likely a copy-paste error during the mass annotation update.
src/java.base/share/classes/java/util/Collections.java#L1204-L1215: remove the mutator annotations fromstream()andparallelStream().src/java.base/share/classes/java/util/Collections.java#L1271-L1275: remove the mutator annotations fromreversed().src/java.base/share/classes/java/util/Collections.java#L1401-L1405: remove the mutator annotations fromreversed().src/java.base/share/classes/java/util/Collections.java#L1450-L1464: remove the mutator annotations fromsubSet,headSet, andtailSet.src/java.base/share/classes/java/util/Collections.java#L1550-L1577: remove the mutator annotations fromdescendingSet,subSet,headSet, andtailSet.src/java.base/share/classes/java/util/Collections.java#L1671-L1673: remove the mutator annotations fromlistIterator(int).🤖 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/Collections.java` around lines 1204 - 1215, Remove `@SideEffectsOnly`("this") and `@DoesNotUnrefineReceiver`("modifiability") from the read-only methods in Collections.java: stream() and parallelStream() at lines 1204-1215, reversed() at 1271-1275 and 1401-1405, subSet(), headSet(), and tailSet() at 1450-1464, descendingSet(), subSet(), headSet(), and tailSet() at 1550-1577, and listIterator(int) at 1671-1673. Leave their existing implementations and other annotations unchanged.
🤖 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/lang/StringBuilder.java`:
- Around line 502-505: Remove the contradictory `@SideEffectFree` annotation from
StringBuilder.repeat(int codePoint, int count), keeping `@SideEffectsOnly`("this")
and the existing method behavior unchanged.
In `@src/java.base/share/classes/java/util/concurrent/DelayQueue.java`:
- Around line 406-408: Update the `@SideEffectsOnly` annotations on both drainTo
overloads in src/java.base/share/classes/java/util/concurrent/DelayQueue.java at
lines 406-408 and 418-420, and both drainTo overloads in
src/java.base/share/classes/java/util/concurrent/PriorityBlockingQueue.java at
lines 751-753 and 763-765, to declare mutations of both the receiver and the
first parameter using {"this", "`#1`"}.
In `@src/java.base/share/classes/java/util/concurrent/LinkedBlockingQueue.java`:
- Around line 728-730: Update both LinkedBlockingQueue.drainTo overloads at
src/java.base/share/classes/java/util/concurrent/LinkedBlockingQueue.java lines
728-730 and 740-742: expand `@SideEffectsOnly` to include both "this" and the
first parameter ("`#1`" or "c"), preserving the existing `@DoesNotUnrefineReceiver`
annotation.
In `@src/java.base/share/classes/java/util/Deque.java`:
- Line 385: Remove the incorrect `@SideEffectsOnly`("this") annotations from the
read-only methods at src/java.base/share/classes/java/util/Deque.java lines 385,
396, 537, 552, 692, and 706; use `@SideEffectFree` instead at lines 692 and 706
where appropriate. Remove the annotation from
src/java.base/share/classes/java/util/SequencedSet.java line 60 and consider
`@SideEffectFree`, and remove it from
src/java.base/share/classes/java/util/TreeMap.java lines 313, 988, and 1182,
using `@SideEffectFree` at lines 988 and 1182 where appropriate. Keep existing
`@Pure` annotations and method behavior unchanged.
In `@src/java.base/share/classes/java/util/DualPivotQuicksort.java`:
- Around line 186-187: Update all four Sorter-based sort overloads in
src/java.base/share/classes/java/util/DualPivotQuicksort.java at lines 186-187,
942-943, 2490-2491, and 3298-3299: include both arguments in `@SideEffectsOnly`,
and bind low/high to the array parameter (`#2`) rather than the Sorter parameter
(`#1`). Apply the identical annotation correction to the int[], long[], float[],
and double[] overloads.
In `@src/java.base/share/classes/java/util/LinkedHashSet.java`:
- Around line 333-335: Replace the mutator annotations on
LinkedHashSet.reversed() in
src/java.base/share/classes/java/util/LinkedHashSet.java:333-335 with
`@SideEffectFree`. Apply the same annotation change to TreeSet.reversed() in
src/java.base/share/classes/java/util/TreeSet.java:222-224, removing
`@SideEffectsOnly`("this") and `@DoesNotUnrefineReceiver`("modifiability") at both
sites.
In `@src/java.base/share/classes/java/util/Queue.java`:
- Line 235: Remove the contradictory `@SideEffectsOnly`("this") annotation from
the Queue.peek() method, preserving its existing `@Pure` annotation and
non-mutating behavior.
In `@src/java.base/share/classes/java/util/SortedMap.java`:
- Around line 351-353: Update SortedMap.reversed() to replace
`@SideEffectsOnly`("this") and `@DoesNotUnrefineReceiver`("modifiability") with
`@SideEffectFree`, matching the annotations used by other view-creating methods
such as subMap and headMap.
In `@src/java.base/share/classes/java/util/SortedSet.java`:
- Around line 403-404: Update the annotations on the SortedSet.reversed()
view-factory method: replace `@SideEffectsOnly`("this") with `@SideEffectFree` and
remove the redundant `@DoesNotUnrefineReceiver`("modifiability") annotation,
matching the annotation pattern used by subSet and headSet.
---
Outside diff comments:
In `@src/java.base/share/classes/java/util/Collections.java`:
- Around line 1204-1215: Remove `@SideEffectsOnly`("this") and
`@DoesNotUnrefineReceiver`("modifiability") from the read-only methods in
Collections.java: stream() and parallelStream() at lines 1204-1215, reversed()
at 1271-1275 and 1401-1405, subSet(), headSet(), and tailSet() at 1450-1464,
descendingSet(), subSet(), headSet(), and tailSet() at 1550-1577, and
listIterator(int) at 1671-1673. Leave their existing implementations and other
annotations unchanged.
In `@src/java.base/share/classes/java/util/concurrent/BlockingQueue.java`:
- Around line 380-409: Update both drainTo overloads in BlockingQueue so their
`@SideEffectsOnly` annotations include both the receiver and collection argument
c, using the argument-expression target form "`#1`". Leave the existing
`@DoesNotUnrefineReceiver` annotations and method signatures unchanged.
In `@src/java.base/share/classes/java/util/concurrent/SynchronousQueue.java`:
- Around line 1125-1138: Update the `@SideEffectsOnly` annotations on both drainTo
overloads in SynchronousQueue to include the collection parameter alongside
"this" (for example, "this" and "`#1`"), so the mutation performed by c.add(e) is
correctly tracked.
In `@src/java.base/share/classes/java/util/regex/Matcher.java`:
- Around line 1359-1369: Update the `@SideEffectsOnly` annotations on the affected
iterator methods to include both the iterator receiver and its enclosing
collection instance: add Matcher.this in
src/java.base/share/classes/java/util/regex/Matcher.java:1359-1369,
RegularEnumSet.this in
src/java.base/share/classes/java/util/RegularEnumSet.java:138-145, and
PriorityQueue.this in
src/java.base/share/classes/java/util/PriorityQueue.java:572-594.
In `@src/java.base/share/classes/sun/net/www/HeaderParser.java`:
- Around line 212-216: Update HeaderParser.next() to check whether the iterator
is exhausted before indexing tab, and throw NoSuchElementException when index
has reached the available entries. Preserve the existing return and
index-advance behavior for valid elements.
🪄 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: 99f555c6-6920-448f-91af-ac1798aa1ab4
⛔ Files ignored due to path filters (1)
checker-qual.jaris excluded by!**/*.jar
📒 Files selected for processing (122)
README.mdsrc/java.base/share/classes/java/io/BufferedReader.javasrc/java.base/share/classes/java/lang/AbstractStringBuilder.javasrc/java.base/share/classes/java/lang/CharSequence.javasrc/java.base/share/classes/java/lang/StackStreamFactory.javasrc/java.base/share/classes/java/lang/String.javasrc/java.base/share/classes/java/lang/StringBuffer.javasrc/java.base/share/classes/java/lang/StringBuilder.javasrc/java.base/share/classes/java/lang/StringLatin1.javasrc/java.base/share/classes/java/lang/StringUTF16.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/text/EntryPair.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/BitSet.javasrc/java.base/share/classes/java/util/Collection.javasrc/java.base/share/classes/java/util/Collections.javasrc/java.base/share/classes/java/util/ComparableTimSort.javasrc/java.base/share/classes/java/util/Deque.javasrc/java.base/share/classes/java/util/DualPivotQuicksort.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/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/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/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/zip/ZipFile.javasrc/java.base/share/classes/javax/security/auth/Subject.javasrc/java.base/share/classes/jdk/internal/icu/text/Replaceable.javasrc/java.base/share/classes/jdk/internal/icu/text/ReplaceableString.javasrc/java.base/share/classes/org/checkerframework/checker/builder/qual/CalledMethods.javasrc/java.base/share/classes/org/checkerframework/checker/formatter/qual/ConversionCategory.javasrc/java.base/share/classes/org/checkerframework/checker/formatter/qual/FormatBottom.javasrc/java.base/share/classes/org/checkerframework/checker/i18nformatter/qual/I18nConversionCategory.javasrc/java.base/share/classes/org/checkerframework/checker/i18nformatter/qual/I18nFormatBottom.javasrc/java.base/share/classes/org/checkerframework/checker/index/qual/IndexOrHigh.javasrc/java.base/share/classes/org/checkerframework/checker/index/qual/LengthOf.javasrc/java.base/share/classes/org/checkerframework/checker/interning/qual/InternedDistinct.javasrc/java.base/share/classes/org/checkerframework/checker/nonempty/qual/EnsuresNonEmptyIf.javasrc/java.base/share/classes/org/checkerframework/checker/optional/qual/RequiresPresent.javasrc/java.base/share/classes/org/checkerframework/checker/regex/qual/RegexBottom.javasrc/java.base/share/classes/org/checkerframework/checker/signature/qual/README.mdsrc/java.base/share/classes/org/checkerframework/checker/sqlquotes/qual/SqlQuotesBottom.javasrc/java.base/share/classes/org/checkerframework/common/aliasing/qual/NonLeaked.javasrc/java.base/share/classes/org/checkerframework/common/returnsreceiver/qual/UnknownThis.javasrc/java.base/share/classes/org/checkerframework/dataflow/qual/SideEffectFree.javasrc/java.base/share/classes/org/checkerframework/dataflow/qual/SideEffectsOnly.javasrc/java.base/share/classes/org/checkerframework/framework/qual/ConditionalPostconditionAnnotation.javasrc/java.base/share/classes/org/checkerframework/framework/qual/DoesNotUnrefineReceiver.javasrc/java.base/share/classes/sun/net/www/HeaderParser.javasrc/java.base/share/classes/sun/net/www/MessageHeader.javasrc/java.base/share/classes/sun/security/jca/ProviderList.javasrc/java.base/share/classes/sun/util/PreHashedMap.javasrc/java.base/share/classes/sun/util/locale/StringTokenIterator.javasrc/java.base/share/classes/sun/util/resources/ParallelListResourceBundle.java
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 (2)
src/java.base/share/classes/java/util/concurrent/PriorityBlockingQueue.java (1)
913-914: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
@DoesNotUnrefineReceiver("modifiability")toItr.next.
PriorityBlockingQueue.Itr.nextis side-effecting and should match the same receiver refinement contract used byDelayQueueandLinkedBlockingQueue.Suggested fix
`@SideEffectsOnly`("this") + `@DoesNotUnrefineReceiver`("modifiability") public E next(`@NonEmpty` Itr this) {🤖 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/PriorityBlockingQueue.java` around lines 913 - 914, Add `@DoesNotUnrefineReceiver`("modifiability") to PriorityBlockingQueue.Itr.next alongside its existing `@SideEffectsOnly`("this") annotation, matching the receiver refinement contract used by DelayQueue and LinkedBlockingQueue.src/java.base/share/classes/java/util/Collections.java (1)
904-906: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix invalid receiver annotations on a static method.
The
replaceAllmethod isstaticand therefore has no receiver (this). The@SideEffectsOnly("this")and@DoesNotUnrefineReceiverannotations are intended for instance methods and are invalid here. The method mutates its first parameter (list), so the correct annotation is@SideEffectsOnly("#1").🐛 Proposed fix
- `@SideEffectsOnly`("this") - `@DoesNotUnrefineReceiver`("modifiability") + `@SideEffectsOnly`("`#1`") public static <T> boolean replaceAll(List<T> list, `@Nullable` T oldVal, T newVal) {🤖 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/Collections.java` around lines 904 - 906, Update the static Collections.replaceAll method annotations to reference its first parameter rather than the nonexistent receiver: change the side-effect target to "`#1`" and remove or replace the receiver-specific `@DoesNotUnrefineReceiver` annotation according to the project’s parameter-annotation conventions.
🤖 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/Collections.java`:
- Around line 904-906: Update the static Collections.replaceAll method
annotations to reference its first parameter rather than the nonexistent
receiver: change the side-effect target to "`#1`" and remove or replace the
receiver-specific `@DoesNotUnrefineReceiver` annotation according to the project’s
parameter-annotation conventions.
In `@src/java.base/share/classes/java/util/concurrent/PriorityBlockingQueue.java`:
- Around line 913-914: Add `@DoesNotUnrefineReceiver`("modifiability") to
PriorityBlockingQueue.Itr.next alongside its existing `@SideEffectsOnly`("this")
annotation, matching the receiver refinement contract used by DelayQueue and
LinkedBlockingQueue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8ae828d-ce9b-4c74-a888-6cf4df156d28
📒 Files selected for processing (10)
src/java.base/share/classes/java/lang/StringBuilder.javasrc/java.base/share/classes/java/util/Collections.javasrc/java.base/share/classes/java/util/concurrent/ArrayBlockingQueue.javasrc/java.base/share/classes/java/util/concurrent/BlockingQueue.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/SynchronousQueue.java
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 (9)
src/java.base/share/classes/java/util/ArrayDeque.java (2)
347-349: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not claim these methods only modify the deque.
addAlldelegates through caller-provided iteration,removeFirstOccurrence()/removeLastOccurrence()/remove()callo.equals(...), andremoveIf()/removeAll()/retainAll()call caller-supplied code (filter.test(...)orc.contains(e)). Those can mutate receiver arguments,c, or other reachable state, so@SideEffectsOnly("this")is too narrow for these contracts. Remove the annotations or include the affected arguments/predicates in the expression list if it is supported in the local Checker Framework setup.Also applies to: 485-487, 515-517, 980-982, 990-992, 1000-1002, 1118-1120
🤖 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/ArrayDeque.java` around lines 347 - 349, Remove the `@SideEffectsOnly`("this") annotations from ArrayDeque methods addAll, removeFirstOccurrence, removeLastOccurrence, remove, removeIf, removeAll, and retainAll, or expand their expressions to include all caller-provided arguments and predicates if supported by the local Checker Framework. Do not leave these methods claiming side effects are limited to the deque.Source: MCP tools
787-789: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAccount for the enclosing deque mutation in iterator
remove().
delete(lastRet)mutates the enclosingArrayDeque(elements,head, and/ortail), while@SideEffectsOnly("this")only permits side effects onDeqIterator. Include the enclosing receiver in the annotation, or remove it if that expression syntax is not supported for nested classes.🤖 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/ArrayDeque.java` around lines 787 - 789, Update the iterator remove() method’s `@SideEffectsOnly` annotation to permit mutation of the enclosing ArrayDeque receiver in addition to DeqIterator, using the supported nested-receiver expression; if that syntax is unsupported, remove the annotation rather than retaining an incorrect contract. Keep delete(lastRet) behavior unchanged.Source: MCP tools
src/java.base/share/classes/java/util/Arrays.java (1)
1077-1078: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRemove
@SideEffectsOnly("#1")from user-controlled sorting callbacks.
SideEffectsOnlyrecords an upper bound on the expressions a method may change; calling user-controlledComparable.compareToorComparator.comparecan mutate arbitrary state beyond#1. The side-effect annotation is unsound for lines 1077, 1143, 1268, and 1341 unless callback effects are explicitly excluded/defined.🤖 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/Arrays.java` around lines 1077 - 1078, Remove the `@SideEffectsOnly`("`#1`") annotation from the affected Arrays.sort overloads, including the overloads near the identified sort methods at lines 1077, 1143, 1268, and 1341. Leave the sorting implementations and callback behavior unchanged, and ensure no user-controlled Comparable.compareTo or Comparator.compare invocation remains annotated with this unsound side-effect bound.src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java (3)
1268-1282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
COWIterator.previous()is missing@DoesNotUnrefineReceiver("modifiability").
next()(lines 1267-1274) and the three throwing methodsremove()/set()/add()(lines 1299-1325) — all touched by this same diff hunk — carry both@SideEffectsOnly("this")and@DoesNotUnrefineReceiver("modifiability").previous()only got@SideEffectsOnly("this")activated, breaking the symmetry withnext()for what is otherwise an identical cursor-mutation operation.🐛 Proposed fix
`@SuppressWarnings`("unchecked") `@SideEffectsOnly`("this") + `@DoesNotUnrefineReceiver`("modifiability") public E previous() { if (! hasPrevious()) throw new NoSuchElementException(); return (E) snapshot[--cursor]; }🤖 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/CopyOnWriteArrayList.java` around lines 1268 - 1282, Add `@DoesNotUnrefineReceiver`("modifiability") to COWIterator.previous(), matching the existing annotations on next() and the throwing iterator mutation methods while retaining `@SideEffectsOnly`("this").
2013-2018: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reversed.add(E),add(int, E),addFirst,addLastall drop@EnsuresNonEmpty("this").See consolidated cross-file comment for the fix.
Also applies to: 2155-2173
🤖 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/CopyOnWriteArrayList.java` around lines 2013 - 2018, Restore `@EnsuresNonEmpty`("this") on Reversed.add(E) and the corresponding add(int, E), addFirst, and addLast methods around the referenced symbols, preserving the existing mutation behavior while declaring that each successful operation leaves this collection non-empty.
1602-1626: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
COWSubList.add(int, E),addFirst,addLastdrop@EnsuresNonEmpty("this")that the siblingadd(E)(line 1589) and the outerCopyOnWriteArrayListequivalents keep.See consolidated cross-file comment for the fix.
🤖 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/CopyOnWriteArrayList.java` around lines 1602 - 1626, Add `@EnsuresNonEmpty`("this") to COWSubList.add(int, E), addFirst(E), and addLast(E), matching the existing contract annotations on sibling add(E) and the outer CopyOnWriteArrayList methods.src/java.base/share/classes/java/util/LinkedList.java (2)
1628-1641: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ReverseOrderLinkedListView.add(int, E)and.add(E)drop the@EnsuresNonEmpty("this")postcondition present on theListcontract.
List.add(int, E)andList.add(E)(this file's main-class overrides too) both declare@EnsuresNonEmpty("this")since a normal return always increases size. Here the reverse-view overrides only got@SideEffectsOnly("this")/@DoesNotUnrefineReceiver("modifiability")activated, silently dropping the postcondition. See consolidated comment for the fix and other affected sites.Also applies to: 1664-1680
🤖 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/LinkedList.java` around lines 1628 - 1641, The reverse-view methods add(int, E) and add(E) must preserve the List contract’s `@EnsuresNonEmpty`("this") postcondition. Add that annotation to both overrides in ReverseOrderLinkedListView, alongside their existing side-effect and receiver annotations, without changing their delegation behavior.
1692-1702: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ReverseOrderLinkedListView.addLast/addFirstalso drop@EnsuresNonEmpty("this").Same gap as
add(int, E)/add(E)above —List.addFirst/addLastdeclare@EnsuresNonEmpty("this"), but these overrides only carry@SideEffectsOnly("this"). See consolidated comment.🤖 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/LinkedList.java` around lines 1692 - 1702, Update ReverseOrderLinkedListView.addLast and addFirst to include the `@EnsuresNonEmpty`("this") contract alongside their existing annotations, matching the List.addLast/addFirst API declarations and the related add methods.src/java.base/share/classes/java/util/NavigableSet.java (1)
207-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMark
descendingSet()as side-effect-free.The returned
NavigableSetis a reverse-order view and does not mutate the receiver, sosrc/java.base/share/classes/java/util/NavigableSet.java:207-208should use@SideEffectFreeand@DoesNotUnrefineReceiver("modifiability")instead of@SideEffectsOnly("this"), matchingdescendingIterator(),reversed(), and the existing view-factory methods in this interface.🤖 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/NavigableSet.java` around lines 207 - 208, Update the annotations on NavigableSet.descendingSet() by replacing `@SideEffectsOnly`("this") with `@SideEffectFree` while retaining `@DoesNotUnrefineReceiver`("modifiability"), matching the side-effect contract used by the related view-factory methods.
🤖 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/ArrayDeque.java`:
- Around line 347-349: Remove the `@SideEffectsOnly`("this") annotations from
ArrayDeque methods addAll, removeFirstOccurrence, removeLastOccurrence, remove,
removeIf, removeAll, and retainAll, or expand their expressions to include all
caller-provided arguments and predicates if supported by the local Checker
Framework. Do not leave these methods claiming side effects are limited to the
deque.
- Around line 787-789: Update the iterator remove() method’s `@SideEffectsOnly`
annotation to permit mutation of the enclosing ArrayDeque receiver in addition
to DeqIterator, using the supported nested-receiver expression; if that syntax
is unsupported, remove the annotation rather than retaining an incorrect
contract. Keep delete(lastRet) behavior unchanged.
In `@src/java.base/share/classes/java/util/Arrays.java`:
- Around line 1077-1078: Remove the `@SideEffectsOnly`("`#1`") annotation from the
affected Arrays.sort overloads, including the overloads near the identified sort
methods at lines 1077, 1143, 1268, and 1341. Leave the sorting implementations
and callback behavior unchanged, and ensure no user-controlled
Comparable.compareTo or Comparator.compare invocation remains annotated with
this unsound side-effect bound.
In `@src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java`:
- Around line 1268-1282: Add `@DoesNotUnrefineReceiver`("modifiability") to
COWIterator.previous(), matching the existing annotations on next() and the
throwing iterator mutation methods while retaining `@SideEffectsOnly`("this").
- Around line 2013-2018: Restore `@EnsuresNonEmpty`("this") on Reversed.add(E) and
the corresponding add(int, E), addFirst, and addLast methods around the
referenced symbols, preserving the existing mutation behavior while declaring
that each successful operation leaves this collection non-empty.
- Around line 1602-1626: Add `@EnsuresNonEmpty`("this") to COWSubList.add(int, E),
addFirst(E), and addLast(E), matching the existing contract annotations on
sibling add(E) and the outer CopyOnWriteArrayList methods.
In `@src/java.base/share/classes/java/util/LinkedList.java`:
- Around line 1628-1641: The reverse-view methods add(int, E) and add(E) must
preserve the List contract’s `@EnsuresNonEmpty`("this") postcondition. Add that
annotation to both overrides in ReverseOrderLinkedListView, alongside their
existing side-effect and receiver annotations, without changing their delegation
behavior.
- Around line 1692-1702: Update ReverseOrderLinkedListView.addLast and addFirst
to include the `@EnsuresNonEmpty`("this") contract alongside their existing
annotations, matching the List.addLast/addFirst API declarations and the related
add methods.
In `@src/java.base/share/classes/java/util/NavigableSet.java`:
- Around line 207-208: Update the annotations on NavigableSet.descendingSet() by
replacing `@SideEffectsOnly`("this") with `@SideEffectFree` while retaining
`@DoesNotUnrefineReceiver`("modifiability"), matching the side-effect contract
used by the related view-factory methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eeeea339-2ea5-4bd7-8dcd-c9bdbc5185d1
📒 Files selected for processing (33)
src/java.base/share/classes/java/util/AbstractCollection.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/Collections.javasrc/java.base/share/classes/java/util/Comparator.javasrc/java.base/share/classes/java/util/Comparators.javasrc/java.base/share/classes/java/util/Deque.javasrc/java.base/share/classes/java/util/EnumMap.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/LinkedList.javasrc/java.base/share/classes/java/util/List.javasrc/java.base/share/classes/java/util/NavigableMap.javasrc/java.base/share/classes/java/util/NavigableSet.javasrc/java.base/share/classes/java/util/Queue.javasrc/java.base/share/classes/java/util/ReverseOrderDequeView.javasrc/java.base/share/classes/java/util/ReverseOrderSortedMapView.javasrc/java.base/share/classes/java/util/ReverseOrderSortedSetView.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/SortedMap.javasrc/java.base/share/classes/java/util/SortedSet.javasrc/java.base/share/classes/java/util/TreeMap.javasrc/java.base/share/classes/java/util/Vector.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/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/LinkedBlockingDeque.java
💤 Files with no reviewable changes (6)
- src/java.base/share/classes/java/util/Queue.java
- src/java.base/share/classes/java/util/EnumMap.java
- src/java.base/share/classes/java/util/AbstractCollection.java
- src/java.base/share/classes/java/util/ArrayList.java
- src/java.base/share/classes/java/util/Vector.java
- src/java.base/share/classes/java/util/concurrent/ConcurrentHashMap.java
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/Enumeration.java (1)
138-141: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse the enclosing
Enumerationreceiver in the adapter contract.
@SideEffectsOnly("this")limits mutations to the anonymousIteratorobject. This override delegates toEnumeration.nextElement(), which changes the enclosingEnumerationstate, not the adapter object.Specify the enclosing receiver, or change the contract only if the adapter is intentionally modeled as owning iterator state. Add a Checker Framework test for this path.
🤖 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/Enumeration.java` around lines 138 - 141, Update the `next` override in the `Enumeration`-to-`Iterator` adapter so its `@SideEffectsOnly` contract names the enclosing `Enumeration` receiver whose state is mutated, rather than only the anonymous iterator adapter. Preserve the existing `nextElement()` delegation and add a Checker Framework test covering this receiver-effect path.
🤖 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/Enumeration.java`:
- Around line 138-141: Update the `next` override in the
`Enumeration`-to-`Iterator` adapter so its `@SideEffectsOnly` contract names the
enclosing `Enumeration` receiver whose state is mutated, rather than only the
anonymous iterator adapter. Preserve the existing `nextElement()` delegation and
add a Checker Framework test covering this receiver-effect path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 31efa1b2-1361-4237-8111-5703f6c10fd1
📒 Files selected for processing (1)
src/java.base/share/classes/java/util/Enumeration.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/checker-framework#7818 .