Paul King created GROOVY-12219:
----------------------------------
Summary: 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
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)
