Normative: avoid triggering throw in corner case in async generators

[bakkot]

Fixes #2813.

The idea is that if you are doing yield* m on a manual async iterator m (i.e., one which is not an async generator), it is possible for the inner iterator to yield { next: promise, done: false }. And if that promise rejects, it is treated as if someone had called .throw on the outer generator (the one doing yield*). That's inconsistent with the way the spec usually consumes iterators and is almost certainly a bug caused by the factoring of AsyncGeneratorYield. (Async generators await their result before yielding it, so it doesn't come up without a manually implemented iterator.)

There's only really three consistent options, that I can see.

1. treat this as a valid thing for a manual async iterator to do which happens to cause the outer generator to throw, in which case the inner iterator should be cleaned up before the exception propagates (which is what this PR does)
2. treat this as a protocol violation by the inner iterator, in which case neither .throw or .return should be called on the inner iterator (as if calling .next had thrown)
3. don't await the result from the inner iterator at all (which would mean it's possible for an async generator to yield a Promise without first unwrapping it, which is maybe surprising, but on the other hand is more consistent with treating yield* as being a completely transparent delegation to the inner iterator). This option would also reduce the number of microtask ticks entailed by async generators doing yield* in general. I've opened an alternative PR in Normative: avoid mostly-redundant await in async yield* #2819 which implements this option.

This PR takes the first option.

I don't really like the second option, because for await does not treat an async iterator yielding a promise as being a protocol violation - see this snippet.

I actually kind of prefer the third option, but it has broader implications than this PR does, and so is more likely to break stuff. On the other hand, it also would improve the performance of async generators, which is nice.

---

As mentioned in the issue, this does not match any existing engine, but engines are inconsistent here, so the change in this PR is probably web compatible.

[michaelficarra]

I like the sound of option 3. Can you go into more detail about the effects of such a change?

[bakkot]

Edit: moved reply to above comment to #2819.

[bergus]

I don't really like the second option, because for await does not treat an async iterator yielding a promise as being a protocol violation [and assigns the promise to the iteration variable]

That is the reason why I don't like the first option - if for await ignores promises as the value of an IteratorResult, then why should yield* wait for them? for await will happily continue the iteration and not clean up the iterator, so yield* shouldn't either.
But I don't like the second option either - not cleaning up the inner iterator, but then throwing an exception from the generator (after which the inner iterator won't be advanced any more) seems wrong.

I can even see a fourth option: Don't await the result from an inner iterator in yield* at all, but do await the result .value in for await. Then, if this is a rejected promise, have it clean up the looped iterator, which in turn will make yield* clean up the inner iterator. So basically yield* working like in the third option, but having every for await (const x of asyncIterable) { … } work like for await (const _x of asyncIterable) { const x = await _x; … }.

That said, of all these I do prefer option 3. (I mean, I'd be even happier if not even yield did implicitly await promises at all, but I doubt we could make that change)

[bakkot]

That is the reason why I don't like the first option - if for await ignores promises as the value of an IteratorResult, then why should yield* wait for them?

The story I'd tell, if we stick what that behavior, is that it's not looping part which is awaiting, but rather the yielding part, matching non-star yield does.

I can even see a fourth option: Don't await the result from an inner iterator in yield* at all, but do await the result .value in for await

That option was discussed and rejected during the original design of async iterators - it's option 2 in these slides; see also discussion in the notes and the issue tracker.

I'd definitely prefer to avoid changing the behavior yield or for await, at this point; I'm only proposing changing the much rarer case of yield*.

[bakkot]

Closing as we got consensus for the alternative #2819. We may need to revive this if that PR turns out not to be web compatible, though.