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: avoid mostly-redundant await in async yield* #2819

Merged
merged 1 commit into from Dec 10, 2022

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Jul 7, 2022

This is an alternative to #2818. See that PR for more context - this implements option 3 of the options listed there. Fixes #2813.

The main effect of this PR would be that if you had

let done = false;
let inner = {
  [Symbol.asyncIterator]: () => ({
    next() {
      if (done) {
        return Promise.resolve({ done: true });
      }
      done = true;
      return Promise.resolve({ done: false, value: Promise.resolve(0) });
    },
  }),
};

async function* outer() {
  yield* inner;
}

(async () => {
  for await (let x of outer()) {
    console.log(x);
  }
})().catch(e => {
  console.log('threw', e);
});

then you would see printed Promise.resolve(0) instead of, as currently, 0. In other words, it would behave as if you'd written for await (let x of inner) instead of of outer().

The other effect is a reduction in the number of promise ticks.

No difference (except in timing) would be observable without manually implementing Symbol.asyncIterator because async generators (and the async-from-sync wrapper, see step 6) already await values before yielding or returning them.

@bakkot bakkot changed the title Normative: avoid mostly-redundant yield in async yield* Normative: avoid mostly-redundant await in async yield* Jul 7, 2022
@bakkot bakkot force-pushed the async-generator-yield-await-2 branch from e1b848b to 4978c21 Compare July 7, 2022 19:43
@michaelficarra michaelficarra added needs consensus This needs committee consensus before it can be eligible to be merged. 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 Jul 7, 2022
@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 Jul 27, 2022
@bakkot
Copy link
Contributor Author

bakkot commented Jul 28, 2022

Tests: tc39/test262#3619

@bakkot bakkot 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 Jul 28, 2022
@raulsebastianmihaila
Copy link

My understanding was that yield* inner in async generators was intended to work as

for await (let x of inner) {
    yield x;
}

and I understand this changes that. I think this is a footgun. Imagine you had the for-await version because you had a bit of logic before yielding and then you removed that logic. One might think that you would be able to simplify the code by replacing the for-await with yield*, which was working before, but I understand this will not work anymore. You would need to know how inner was implemented which is not a good idea.

@bakkot
Copy link
Contributor Author

bakkot commented Jul 28, 2022

My understanding was that yield* inner in async generators was intended to work as

No, they're different in other ways - yield* forwards the entire generator protocol (next, return, and throw, including arguments), whereas yield does not. So you already need to know the contract of the inner iterator to know if that's a semantics-preserving rewrite.

More importantly, though, this basically should not come up. Syntactic async generators, as well as the async-from-sync wrapper, will prevent you from having a Promise in the value slot at all. The only way you can observe a different value as a result of this change is by manually writing an async iterator which goes out of its way to put a Promise in the value field, which is a contract violation. Having all async generators using yield* pay a cost (the extra await) for well-behaved iterators just to handle that contract violation seems silly, and as far as I can tell was not particularly intended.

By a similar token, you might think that using for await (x of y) { ... } means you never need to await x. This is almost true, except that, just as in the above case, that assumption can fail with a manually implemented async iterator. Should for await then also unconditionally await the value field? We explicitly decided not to do that, with the rationale that it's the producer's responsibility not to do that, not the consumer. The same rationale applies here.


Also, even if you are just reasoning by analogy to other code, yield* is supposed to be deferring the entire protocol to another generator, not doing a transformation on part of it. Consuming (async function*(){ yield* inner })() should work exactly like consuming inner directly. Right now that principle is violated, and I think that's a more important analogy than the analogy to for await (x of y) yield.

@raulsebastianmihaila
Copy link

Nice explanation! I think it's unfortunate that we use the same keyword ('yield') for these 2 different ways of producing the resulting value.

spec.html Outdated Show resolved Hide resolved
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Aug 25, 2022
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Aug 25, 2022
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Aug 25, 2022
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Aug 25, 2022
JLHwung pushed a commit to babel/babel that referenced this pull request Sep 1, 2022
* Extract asyncGeneratorDelegate/AsyncGenerator/awaitAsyncGenerator (no changes)

* Remove one promise tick in yield* (tc39/ecma262#2819)

* [babel 8] Don't pass the second parameter to asyncGeneratorDelegate
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 12, 2022
Spec PR: tc39/ecma262#2819

The PR moves the `Await` operation of out `AsyncGeneratorYield` and instead
executes it before calling `AsyncGeneratorYield` from `Yield`.

`BytecodeEmitter::emitYieldStar()` has an inlined implementation of the
`AsyncGeneratorYield` operation. Removing the call to `emitAwaitInInnermostScope()`
is equivalent to removing the `Await` operation in `AsyncGeneratorYield`.

And in `BytecodeEmitter::emitYield()` we only need to remove the comment
explaining why we call `emitAwaitInInnermostScope()`, because executing `Await`
is now part of the normal evaluation of the `Yield` expression and no longer
happens in `AsyncGeneratorYield`.

Differential Revision: https://phabricator.services.mozilla.com/D156933
@bakkot bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed needs implementations labels Dec 4, 2022
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 ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yield * in an async iterator should not call .throw when the inner iterator produces a rejected promise
5 participants