[
https://issues.apache.org/jira/browse/GROOVY-12219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100577#comment-18100577
]
ASF GitHub Bot commented on GROOVY-12219:
-----------------------------------------
paulk-asert merged PR #2753:
URL: https://github.com/apache/groovy/pull/2753
> 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
> Labels: breaking
>
> 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)
