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: make async iterators next/return/throw not pass undefined when value is absent #1776

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Nov 13, 2019

This is admittedly a very minor change.

const o = {
  next(...args) { console.log('args', args); return { done: true }; },
  [Symbol.iterator]() { return this }
};
for (const item of o) {}

logs 'args', []

(async () => {
  const o = {
    next(...args) { console.log('args', args); return { done: true }; },
    [Symbol.iterator]() { return this }
  };
  for await (const item of o) {}
})();

logs 'args', [undefined]. The intention of this change is to make async iteration match sync iteration from the perspective of a userland iterator next method.

This was an intentional decision made by the feature champion in tc39/proposal-async-iteration#113, but I don't recall it being discussed in committee. I find this inconsistency mildly troubling, and would like to correct it.

It has relevance as well for the "iterator helpers" proposal.

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. spec bug needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Nov 13, 2019
@devsnek

This comment has been minimized.

@ljharb ljharb added has consensus This has committee consensus. needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Dec 3, 2019
@ljharb
Copy link
Member Author

ljharb commented Dec 3, 2019

This PR achieved consensus at today's TC39. It will first require at least one browser implementation committing to implement it, and merged test262 tests, and it being web compatible before this PR can be merged.

ljharb added a commit to ljharb/ecma262 that referenced this pull request Dec 3, 2019
@ljharb
Copy link
Member Author

ljharb commented Dec 3, 2019

@codehag has confirmed that SpiderMonkey is OK making the change, although we'll still have to wait for at least one shipping implementation due to web compat concerns.

spec.html Outdated Show resolved Hide resolved
@ljharb
Copy link
Member Author

ljharb commented Mar 29, 2020

Tests PR: tc39/test262#2551

ljharb added a commit to ljharb/ecma262 that referenced this pull request Mar 29, 2020
@shvaikalesh
Copy link
Member

shvaikalesh commented Mar 29, 2020

I will submit a patch for JSC as soon as the tests are merged & synced.

EDIT Apr 30: r260915

ljharb added a commit to ljharb/ecma262 that referenced this pull request Apr 2, 2020
@ljharb ljharb added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Apr 6, 2020
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Apr 15, 2020
…e. r=yulia

We're using a shared implementation for the "next", "return", and "throw"
methods, so we only need to adjust a single line of code.

Spec PR: tc39/ecma262#1776

Depends on D70815

Differential Revision: https://phabricator.services.mozilla.com/D70816
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 15, 2020
…e. r=yulia

We're using a shared implementation for the "next", "return", and "throw"
methods, so we only need to adjust a single line of code.

Spec PR: tc39/ecma262#1776

Depends on D70815

Differential Revision: https://phabricator.services.mozilla.com/D70816

--HG--
extra : moz-landing-system : lando
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb
Copy link
Member Author

ljharb commented Jun 15, 2020

Given that Firefox ships this and the next version of Safari is likely to as well, this seems like sufficient web compat data to me.

I'm going to remove the "needs data" label and rebase it; does anyone think that we should wait longer before landing it (modulo approvals)?

ljharb added a commit to ljharb/ecma262 that referenced this pull request Jun 15, 2020
@ljharb ljharb removed the needs data This PR needs more information; such as web compatibility data, “web reality” (what all engines do)… label Jun 15, 2020
spec.html Outdated
@@ -38868,7 +38874,7 @@ <h1>%AsyncFromSyncIteratorPrototype%.return ( _value_ )</h1>
</emu-clause>

<emu-clause id="sec-%asyncfromsynciteratorprototype%.throw">
<h1>%AsyncFromSyncIteratorPrototype%.throw ( _value_ )</h1>
<h1>%AsyncFromSyncIteratorPrototype%.throw ( [ _value_ ] )</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a NOTE explaining that this optional argument is, in fact, always passed? (Since AsyncFromSyncIteratorPrototype objects are not user-observable and the only in-spec callsite, in yield *, always passes an argument.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but can you help me understand the value of noting that when the entire % AsyncFromSyncIteratorPrototype% object is already unobservable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it is unobservable, it's basically an abstract operation. By comparison, all abstract operations are unobservable, but it would still be surprising to define an abstract operation which took an optional parameter which was, in fact, always passed.

Here I don't really mind marking the parameter as optional and having logic for what to do if the argument is not passed (though now that I write this I guess I'd be happier still if we just asserted it were, but whatever), but since it's surprising, I think it would benefit from a NOTE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note added.

ljharb added a commit to ljharb/ecma262 that referenced this pull request Jun 16, 2020
backwardn pushed a commit to backwardn/v8 that referenced this pull request Jun 17, 2020
…e methods

tc39/ecma262#1776 is a normative change that
reached consensus in the November 2019 TC39. It changes
%AsyncFromSyncIteratorPrototype% methods to forward the absence of
arguments to the underlying sync iterator. This is observable via
`arguments.length` inside the underlying sync iterator.

For example, .next is changed to, roughly:

```
%AsyncFromSyncIteratorPrototype%.next = function(value) {
  let res;
  if (arguments.length < 1) {
     res = [[SyncIteratorRecord]].[[Iterator]].next();
  } else {
     res = [[SyncIteratorRecord]].[[Iterator]].next(value);
  }
  // ...
};
```

Bug: v8:10395
Change-Id: Ib8127d08cd78b8d502e6510241f3f13fbbaba5c7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2247041
Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68398}
@ljharb ljharb self-assigned this Jun 28, 2020
@ljharb ljharb requested a review from a team June 28, 2020 03:33
spec.html Outdated Show resolved Hide resolved
ljharb added a commit to ljharb/ecma262 that referenced this pull request Jun 28, 2020
@ljharb ljharb merged commit 18c134e into tc39:master Jun 29, 2020
@ljharb ljharb deleted the async-iterator-next branch June 29, 2020 23:30
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. has test262 tests 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.

None yet

5 participants