Iterator helper fixes - #1622
Merged
Merged
Conversation
saghul
commented
Jul 31, 2026
saghul
commented
Contributor
- Fix the throw paths of the iterator helpers
- Close the iterator properly in Iterator.concat().return()
- Make Iterator.concat() return an Iterator Helper
Two deviations, both verified against V8 and JavaScriptCore, which agree with
each other and with the spec:
- An abrupt completion did not finish the helper, because the shared |fail|
label left it->done at *pdone, which is false when the throw happened after a
successful step. The next next() therefore stepped the underlying iterator
again and threw again, where the spec sets the generator's [[GeneratorState]]
to completed and reports {value: undefined, done: true}. Affected map,
filter, flatMap, take, drop and Iterator.concat.
- A throw from stepping the iterator closed it. GetIteratorDirect and
IteratorStepValue propagate as is; the latter only marks the iterator record
done. Only a throw from the callback goes through IfAbruptCloseIterator. So
the iterator's return method was called when its next method threw, when the
done or value getter of the result threw, when the result was not an object,
and when reading .next threw. Affected all of the helpers as well as every,
some, find, forEach and reduce; reduce also closed the iterator when it threw
on an empty one, and flatMap closed the inner iterator when stepping it
threw. toArray was already correct.
test262 does not cover either: the tests stop at the first throwing next() and
only check the type of the exception, and the error from the extra return
lookup is swallowed because the iterator is closed with an exception pending.
js_iterator_concat_return() called the .return method of the iterator it was iterating by hand, which was wrong in three ways, all of them verified against V8 and JavaScriptCore: - It did not check whether the method is there, so closing a concatenation of plain arrays threw: Iterator.concat([1,2]) yields an array iterator, which has no .return method, and calling it raised "TypeError: not a function". - It did not check that the result of the call is an object. - It returned that result as is, so .return() evaluated to whatever the underlying iterator handed back, or to undefined when there was nothing to close, instead of to an iterator result object. JS_IteratorClose() does all of that, so use it, and return CreateIterResultObject(undefined, true) as %IteratorHelperPrototype%.return does. The iterator is now finished even when closing it throws, and .return is declared with a length of 0 to match the method it stands in for. test262 misses this: its only close test uses an iterator whose .return method exists and returns an object, and it never inspects the result of .return().
The spec creates the iterator with CreateIteratorFromClosure(closure, "Iterator Helper", %IteratorHelperPrototype%), so its prototype, its next and return methods and its toStringTag are the ones every other iterator helper gets. quickjs gave it a prototype of its own with a private copy of next and return and an "Iterator Concat" tag, which V8 and JavaScriptCore both disagree with: const it = Iterator.concat([1]); Object.getPrototypeOf(it) === Object.getPrototypeOf([].values().map(x => x)); Object.prototype.toString.call(it); // "[object Iterator Helper]" it.next.length; // 0 Point the class at %IteratorHelperPrototype% and dispatch on the class id in js_iterator_helper_next(), which now serves both kinds of helper: the ones with a JSIteratorHelperData and the concatenation with its JSIteratorConcatData. The concat state, the reentrancy guard and the closing behaviour are unchanged. test262 does not notice: concat/proto.js checks the prototype of the Iterator.concat function, not the prototype of what it returns.
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
1 participant
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.