[
https://issues.apache.org/jira/browse/GROOVY-12219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100575#comment-18100575
]
Paul King edited comment on GROOVY-12219 at 7/30/26 9:37 PM:
-------------------------------------------------------------
We didn't actively advertise these as not supporting remove but we never said
don't use it either. This is breaking for alpha-2 in the sense that instead of
removing the wrong element, these now throw as unsupported.
was (Author: paulk):
We didn't actively advertise these as not supporting remove but we never said
don't use it either. This is breaking in the sense that instead of removing the
wrong element, these now throw as unsupported.
> Lazy iterators that prefetch remove the wrong element in DefaultGroovyMethods
> -----------------------------------------------------------------------------
>
> Key: GROOVY-12219
> URL: https://issues.apache.org/jira/browse/GROOVY-12219
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
> Fix For: 6.0.0-beta-1
>
>
> h3. Summary
> Several lazy {{Iterator}} implementations in {{DefaultGroovyMethods}}
> *prefetch* the next element from their delegate (inside {{advance()}}, which
> {{next()}} calls before returning), but implement {{remove()}} by delegating
> to {{delegate.remove()}}. Because the delegate's cursor has already advanced
> past the just-yielded element, {{delegate.remove()}} removes the *wrong*
> element (the one prefetched ahead) rather than the element the caller last
> obtained from {{next()}}.
> This was found and fixed for {{TakeWhileIterator}} under GROOVY-12215 (its
> {{remove()}} now throws {{UnsupportedOperationException}}). The same pattern
> exists in other iterators in the file and should be audited and fixed
> consistently.
> h3. The pattern
> {code:java}
> public E next() {
> if (exhausted) throw new NoSuchElementException();
> E result = next; // element yielded to the caller
> advance(); // calls delegate.next() -> prefetches the NEXT
> element
> return result;
> }
> public void remove() {
> if (exhausted) throw new NoSuchElementException();
> delegate.remove(); // removes the prefetched-ahead element, not `result`
> }
> {code}
> h3. Affected iterators (private static classes in DefaultGroovyMethods)
> ||Iterator||Prefetches ahead?||{{remove()}}||Status||
> |{{TakeWhileIterator}}|yes ({{result = next; advance()}})|delegated
> (buggy)|*Fixed* in GROOVY-12215 — now throws
> {{UnsupportedOperationException}}|
> |{{ToUniqueIterator}}|yes ({{result = next; advance()}})|delegated|*Suspect*
> — same pattern; verify and fix|
> |{{DropRightIterator}}|yes (reads ahead into a {{discards}}
> buffer)|delegated|*Suspect* — reads ahead by N; verify and fix|
> |{{DropWhileIterator}}|no (no steady-state prefetch)|delegated|OK — removes
> the correct element|
> |{{CollectManyIterator}}, {{CollectManyFunctionIterator}},
> {{InjectAllIterator}}|n/a|throws {{UnsupportedOperationException}}|OK —
> already unsupported|
> h3. Reproduction (to confirm per iterator)
> {code:groovy}
> def source = new LinkedList([1, 1, 2, 3]) // mutable, supports
> Iterator.remove()
> def it = source.iterator().toUnique()
> assert it.next() == 1
> it.remove() // intends to remove the yielded 1
> // the delegate has already advanced past it, so a later element is removed
> instead
> {code}
> h3. Proposed fix
> Make {{remove()}} throw {{UnsupportedOperationException}} for the prefetching
> iterators ({{ToUniqueIterator}}, {{DropRightIterator}}), matching the fix
> applied to {{TakeWhileIterator}} and the existing behaviour of
> {{CollectManyIterator}}/{{InjectAllIterator}}. (Reworking them to avoid
> prefetching before {{remove()}} is possible but more invasive; making
> {{remove()}} unsupported is the simplest correct option and is consistent
> with the other lazy iterators.) Leave {{DropWhileIterator}} as-is.
> h3. Notes
> * No test currently exercises {{remove()}} on these iterators, so the change
> is low risk.
> * Turning a silently-wrong {{remove()}} into an explicit
> {{UnsupportedOperationException}} is strictly safer than the current
> behaviour.
> * Discovered while addressing a Copilot review comment on GROOVY-12215.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
