Increase coverage of iterator helpers on abrupt completion - #5099
Open
saghul wants to merge 8 commits into
Open
Conversation
bakkot
reviewed
Jul 31, 2026
bakkot
left a comment
bakkot
left a comment
Member
There was a problem hiding this comment.
Gave it a quick skim, mostly looks good except for the same comments from the first file on every file.
Contributor
Author
|
Thanks for the feedback! |
An abrupt completion from IteratorStepValue is propagated as is: the iterator record is marked done, but the underlying iterator is not closed. Only an abrupt completion from the callback goes through IfAbruptCloseIterator. The existing next-method-throws.js, next-method-returns-non-object.js, next-method-returns-throwing-done.js and next-method-returns-throwing-value.js tests only check which exception surfaces, which cannot tell a spurious IteratorClose apart: the close happens with an exception pending, so IteratorClose swallows anything the extra return lookup or call throws. These tests count the calls to the underlying iterator's return method instead. flatMap gets a second test for an abrupt completion from stepping the inner iterator, which does close the underlying iterator, but not the inner one.
Same coverage as the previous commit, for the methods that consume the iterator eagerly: every, some, find, forEach, reduce and toArray. An abrupt completion from stepping the iterator does not close it; only an abrupt completion from the callback does. reduce gets a second test for the TypeError it throws when it is called without an initial value on an empty iterator: the iterator has been exhausted by then, so it is not closed either.
The closure of an iterator helper is the body of a generator, so an abrupt completion from it sets [[GeneratorState]] to completed: a subsequent next() returns an undefined, done result without stepping the underlying iterator again, and a subsequent return() does not forward to the underlying iterator. Existing tests stop at the first exception, so an implementation that leaves the helper suspended (and steps the underlying iterator again on the next call to next()) passes all of them.
The iterator chunking proposal builds on the same abstract operations, so chunks and windows behave like the other iterator helpers when stepping the underlying iterator throws: the iterator is not closed, and the helper is completed.
An abrupt completion of the Yield in the closure of Iterator.concat runs
IteratorClose on the iterator record of the iterable being consumed. The
existing tests only exercise an iterator whose return method exists and
returns an object, and never look at what return() evaluated to, so they
miss every other branch of IteratorClose:
- an iterator without a return method (or with a null one) is left
alone, and return() still evaluates to an undefined, done result
- a return property that is not callable throws a TypeError
- a return method that throws propagates that exception
- a return method that returns a non-object throws a TypeError
- what the return method returns is not what return() evaluates to:
the result is a fresh iterator result object, and the argument of
return() is not observable in it
In each of the failing cases the iterator helper is completed all the
same, which the tests check with a subsequent next() and return().
The closure of Iterator.concat is the body of a generator too, so an abrupt completion from stepping the iterator of one of the iterables completes it: a subsequent next() returns an undefined, done result without stepping that iterator again, and the iterables that follow are never opened.
Iterator.concat builds its result with CreateIteratorFromClosure and %IteratorHelperPrototype%, so it shares its prototype, and with it its next and return methods and its @@toStringTag, with the iterator helpers returned by %Iterator.prototype%.map and friends. result-is-iterator.js says as much in its description, but only asserts that the result is an instance of Iterator, which any other prototype inheriting from %Iterator.prototype% satisfies as well.
The only IteratorClose in the closure of Iterator.concat is the one performed for an abrupt completion of the Yield. An abrupt completion from IteratorStepValue is propagated as is, so a throwing next method, a throwing done or value getter, and a next method returning a non-object all leave the iterator alone. Note that V8 fails this test: it closes the iterator in all four cases, unlike its own iterator helpers, which do not. JavaScriptCore does not close it.
saghul
force-pushed
the
iterator-helpers-abrupt-completion
branch
from
August 3, 2026 14:36
8364a66 to
7bffd76
Compare
Contributor
Author
|
Updated! |
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
3 participants
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.
Disclaimer: this PR was made with AI assiatance while working on iterator helpers on QuickJS-NG.