[
https://issues.apache.org/jira/browse/GROOVY-12219?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100432#comment-18100432
]
ASF GitHub Bot commented on GROOVY-12219:
-----------------------------------------
codecov-commenter commented on PR #2753:
URL: https://github.com/apache/groovy/pull/2753#issuecomment-5131350954
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2753?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 69.9309%. Comparing base
([`4c9b312`](https://app.codecov.io/gh/apache/groovy/commit/4c9b31231fcca14f217a707cf43e866328d40b89?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`6100746`](https://app.codecov.io/gh/apache/groovy/commit/610074682a008855f853b261c9660ca30f882f7b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2753?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2753 +/- ##
==================================================
+ Coverage 69.9204% 69.9309% +0.0105%
Complexity 35314 35314
==================================================
Files 1555 1555
Lines 131192 131188 -4
Branches 24070 24068 -2
==================================================
+ Hits 91730 91741 +11
+ Misses 31162 31149 -13
+ Partials 8300 8298 -2
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2753?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[.../codehaus/groovy/runtime/DefaultGroovyMethods.java](https://app.codecov.io/gh/apache/groovy/pull/2753?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fruntime%2FDefaultGroovyMethods.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L3J1bnRpbWUvRGVmYXVsdEdyb292eU1ldGhvZHMuamF2YQ==)
| `75.1801% <100.0000%> (+0.1601%)` | :arrow_up: |
... and [8 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2753/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> 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
>
> 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)
