Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: iterator helpers close receiver on argument validation failure #3467

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Nov 3, 2024

The general rule when working with iterators / iterables is that once you get an iterator it is your responsibility to close it when you're done with it, unless it errors out or completes. We're consistent about this when we get the iterator from an iterable, and in some of the places where we get the iterator directly (e.g. in Set.prototype.isDisjointFrom step 5.c.ii.1.a), but as pointed out by @zloirock, iterator helpers don't close their receiver when one of their argument is invalid. Since iterator helpers are conceptually "take ownership" of their receiver (either by consuming it fully, or by closing the receiver when the iterator helper itself is closed), and since an argument failing validation is not an error of the underlying iterator, helpers should close their receiver in this case to be consistent with the general rule above.

Unfortunately IfAbruptCloseIterator and IteratorClose both take an Iterator Record, not the iterator object itself, and we don't construct the Iterator Record until after argument validation. So we have to make a "partial" Iterator Record for use by these methods. (It's still a well-typed Iterator Record.) This is a spec fiction to simplify the spec text and doesn't need to correspond to anything in an actual implementation.

@bakkot bakkot added the needs consensus This needs committee consensus before it can be eligible to be merged. label Nov 3, 2024
spec.html Show resolved Hide resolved
@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Nov 13, 2024
1. Let _numLimit_ be Completion(ToNumber(_limit_)).
1. IfAbruptCloseIterator(_numLimit_, _iterated_).
1. If _numLimit_ is *NaN*, then
1. Let _error_ be ThrowCompletion(a newly created *RangeError* object).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would inline these ThrowCompletions into the IteratorClose calls.

@bakkot bakkot added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Dec 2, 2024
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

As a follow up, as we discussed, would weakly prefer a IteratorCloseDirect instead of making these administrative Iterator Records just so we can close them.

@bakkot
Copy link
Contributor Author

bakkot commented Dec 15, 2024

would weakly prefer a IteratorCloseDirect instead of making these administrative Iterator Records just so we can close them.

Started doing this but it turns out we'd also need an IfAbruptCloseIteratorDirect, or some additional bookkeeping to avoid that, so I'm now less convinced this would be worth it.

sosukesuzuki added a commit to sosukesuzuki/WebKit that referenced this pull request Dec 15, 2024
…nvalid

https://bugs.webkit.org/show_bug.cgi?id=284709

Reviewed by NOBODY (OOPS!).

The normative change[1] that requires closing underlying iterators when
argument validation fails for iterator helpers reached consensus at the
TC39 meeting in December 2024[2].

This patch implements it.

[1]: tc39/ecma262#3467
[2]: https://github.com/tc39/agendas/blob/main/2024/12.md

* JSTests/stress/iterator-helpers-close-for-invalid-argument.js: Added.
(shouldThrow):
(shouldBe):
(throw.new.Error.let.closable.get next):
(throw.new.Error):
(shouldBe.let.closable.get next):
(shouldBe.OurError):
(let.closable.get next):
(shouldBe.get shouldBe):
(OurError):
(get shouldBe):
* Source/JavaScriptCore/builtins/JSIteratorPrototype.js:
(some.wrapper.iterator):
(some):
(every.wrapper.iterator):
(every):
(find.wrapper.iterator):
(find):
(reduce):
sosukesuzuki added a commit to sosukesuzuki/WebKit that referenced this pull request Dec 17, 2024
…nvalid

https://bugs.webkit.org/show_bug.cgi?id=284709

Reviewed by NOBODY (OOPS!).

The normative change[1] that requires closing underlying iterators when
argument validation fails for iterator helpers reached consensus at the
TC39 meeting in December 2024[2].

This patch implements it.

[1]: tc39/ecma262#3467
[2]: https://github.com/tc39/agendas/blob/main/2024/12.md

* JSTests/stress/iterator-helpers-close-for-invalid-argument.js: Added.
(shouldThrow):
(shouldBe):
(throw.new.Error.let.closable.get next):
(throw.new.Error):
(shouldBe.let.closable.get next):
(shouldBe.OurError):
(let.closable.get next):
(shouldBe.get shouldBe):
(OurError):
(get shouldBe):
* Source/JavaScriptCore/builtins/JSIteratorPrototype.js:
(some.wrapper.iterator):
(some):
(every.wrapper.iterator):
(every):
(find.wrapper.iterator):
(find):
(reduce):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants