Move the awaiting (back) into yield

[domenic]

This is reverting some of ca6942b, per the May 2017 TC39 meeting agreement to make yield automatically await.

This also simplifies yield* to be more simply a for-await-of that yields; the only modifications it requires from the original yield* spec text are those needed to parallel for-await-of, and those needed to call AsyncGeneratorYield instead of GeneratorYield.

Fixes #93: both the original issue posted there, and the much larger issue it evolved into.

---

/cc @caitp @littledan @gskachkov @arai-a. Careful review appreciated of the whole spec after this change, to make sure I didn't miss anything... I've posted a preview of it here: https://dl.dropboxusercontent.com/u/20140634/yield-is-yield-await/index.html

[arai-a]

is there any reason that AsyncGeneratorYield does Await(value) after setting generator.[[AsyncGeneratorState]] to "suspendedYield", instead of before it?

actually it's a bit hard to implement in that exact order, and I'm wondering if those steps can be swapped.

[littledan]

@arai-a Swapping them makes sense to me; once swapped, the logic would be strictly "do the await, then do the yield", which is what I would expect. Would the semantics here allow for a little more parallelism, if next() is called before ? Await() is done? If so, that seems unintentional.

Editorial suggestion: if the Async Iterator Value Unwrap Functions are only called by the AsyncFromSyncIterators, maybe the name should reflect this, or at least it could be moved into that section.

Otherwise, I don't see any issues with this patch (apart from the extra microtask item issue that @caitp noted in the other thread).

[caitp]

given that yield is now implicitly yield await, should syntactic/explicit yield await be forbidden, or that explicit await ignored? It seems weird to tack on yet another extra microtask cycle

Otherwise, I don't see any issues with this patch (apart from the extra microtask item issue that @caitp noted in the other thread).

hmm, well AsyncGeneratorResolve used to create an extra task anyways, so I suppose it's not too different from before, apart from producing fatter code.

[littledan]

@caitp We don't prohibit tons of other things that would be arbitrarily slow. For example, in an async function or generator, you can sprinkle async roughly anywhere you want, and most of the time the only important effect of that will be that your code runs slightly slower. I'm not sure if it makes sense to make this the place where we put our foot down and start on restrictions--maybe that's better for external tools.

[caitp]

There may be an issue with AsyncGeneratorEnqueue and abrupt completions AsyncGeneratorResolve for "return" completions. They will no longer unwrap the values for abrupt return completions when the generator is in the "suspendStart" state (via AsyncGeneratorResumeNext) or for normal return statements (via AsyncGeneratorStart).

let asyncGenerator = gen();
asyncGenerator.return(promise);
// AsyncGeneratorResumeNext step 9.b.i:
//         state === "suspendStart"
//         completed === true
//    return ! AsyncGeneratorResolve(generator, completion.[[Value]] (/promise/), true)

// -> returned promise resolves to an Iterator Result object with the value being the
//      Promise passed to .return() rather than the unwrapped value

There are a few other ways to observe this:

var resolve, reject;
var p = new Promise(function(resolve_, reject_) {
	resolve = resolve_;
	reject = reject_;
});

async function* g() {
	return p;
}

var gen = g();
gen.next().then(
    x => print(x.value),  // [object Promise]
    x => print(x));

resolve("floof");

I think https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorstart needs to do unwrapping in step 5.g., and https://tc39.github.io/proposal-async-iteration/#sec-asyncgeneratorresumenext needs to do unwrapping in step 9.b.i., but can skip unwrapping in step 10.

[ljharb]

return await is in the same category as yield await; unfortunately I think it'll also end up being the sort of thing that a linter rule forbids instead of the language forbidding.

[domenic]

Updated to swap the steps and move/rename the value unwrap functions. Preview link also updated.

Agreed we should not prohibit yield await just like we don't prohibit await await.

@caitp's trickier issue at #102 (comment) I haven't been able to process yet, but will make a top priority tonight or tomorrow.

[domenic]

@caitp thanks for the detailed analysis and examples. After attempting to investigate other ways of tackling the problem just to be sure, I came to the conclusion your suggestions were exactly right, and pushed another commit to fix them. PTAL!

[caitp]

So, just to clarify:

The following code will unwrap the Promise, but unwrapping will not impact control flow at all. Thus, the finally block is evaluated immediately after the return statement is evaluated, and before any rejection handlers can fire.

That's fine, but is that the desired behaviour here? Maybe this actually needs an adjustment to https://tc39.github.io/ecma262/#sec-return-statement-runtime-semantics-evaluation instead?

async function* g() {
  try {
    return Promise.resolve().then(x => { throw new Error("floof"); });
  } finally {
    doImportantCleanupWork();
  }
}

---

Of course, using a syntactic await would work fix the control flow issue, so it's flexible and supports either case. Maybe that's a footgun, or maybe it's great.

[domenic]

Ugh, no, that wasn't the intention... And yeah, adjusting the return statement semantics was what I was thinking of as an alternative. But then, don't we also need this change, to deal with the gen.return(promise) case? Or maybe there are less invasive fixes for that case, e.g. just unwrapping in .return() before doing the enqueuing. What do you think?

[domenic]

So I have a patch for adding an Await() in return statements. That does feel pretty nice, basically saying that we make return <-> return await similar to yield <-> yield await.

But I'm not sure what to do about the gen.return(promise) issue; will keep thinking on it, and I welcome suggestions.

[caitp]

When the .return(promise) request is resumed, my understanding is:

• If generator state is "suspendedStart", control flow cannot be affected, so just do what it already does, more or less

• Otherwise, somehow we essentially evaluate return <value> when the generator is resumed anyways (I've asked @littledan which clause in the spec causes this, but I don't actually know), so I think it would just fall through to the changes to ReturnStatement evaluation, but it may need a change somewhere else instead.

[caitp]

e.g. just unwrapping in .return() before doing the enqueuing. What do you think?

I don't think you'd want to "unwrap before enqueuing", because I think that would mess with the predictable order of resumption.

gen.return(promise); // promise unwrapping begins
gen.next(1); // .next request enqueued and run immediately
resolvePromise(2); // promise is resolved
// microtasks run, generator enqueues the .return request and resumes for it.

this is confusing and likely bug-prone. Simpler and more predictable to do the await on resumption rather than before enqueuing.

[domenic]

The more I think about it, the more I am leaning toward not covering the .return() case (unless somehow it magically reduces to the return statement like you allude to, but I don't see that here...). The point of this PR is to move the unwrapping entirely to be "inside" the generator; we shouldn't mess with things inserted from the "outside".

I'll upload my tweaks in that direction.

[caitp]

I might suggest using the Async Iterator Value Unwrap rather than Await here, because the unwrapping at this point can never impact control flow inside the generator body. It is a bit wordier, but clearer maybe. (Same for the above change, depending on the decision made re: control flow)

[domenic]

Take n now ready, per some IRC discussions. People may enjoy this minor writeup I had to make to convince myself on the return-related changes: https://gist.github.com/domenic/9c743590125cb5b2fe2bcfb1e93c664a

Please take a look, and thanks so much @caitp for your patience and diligence <3

[arai-a]

If AwaitIfReturn is inserted into AsyncGeneratorResumeNext, will all functions that calls it become async?

for example, AsyncGenerator.prototype.return calls AsyncGeneratorEnqueue:
https://dl.dropboxusercontent.com/u/20140634/yield-is-yield-await/index.html#sec-asyncgenerator-prototype-return

11.4.1.3 AsyncGenerator.prototype.return ( value )
...
3. Return ! AsyncGeneratorEnqueue(generator, completion).

and AsyncGeneratorEnqueue calls AsyncGeneratorResumeNext:
https://dl.dropboxusercontent.com/u/20140634/yield-is-yield-await/index.html#sec-asyncgeneratorenqueue

11.4.3.6 AsyncGeneratorEnqueue ( generator, completion )
...
8. If state is not "executing", then
__a. Perform ! AsyncGeneratorResumeNext(generator).
9. Return promiseCapability.[[Promise]].

and AsyncGeneratorResumeNext stops at step 9.a.iii.
https://dl.dropboxusercontent.com/u/20140634/yield-is-yield-await/index.html#sec-asyncgeneratorresumenext

11.4.3.5 AsyncGeneratorResumeNext ( generator )
...
9. If completion is an abrupt completion, then
__a. If state is "suspendedStart", then
____...
____iii. Set completion to AwaitIfReturn(completion).
...

so, the question is, when 11.4.3.6 step 9 (Return promiseCapability.[[Promise]]. ) is executed?

[domenic]

Oh, yep, that's not working as intended. Thanks... taking today off for my birthday, but will fix first thing Monday.

[domenic]

(I should know better than to promise things like "first thing Monday"...) New version up. This required introducing two new special-purpose closures, which is a big deal in spec-ese, although hopefully not for implementations. (I filed tc39/ecma262#933 on making the spec-ese less annoying.)

PTAL!

[caitp]

I believe at this point, it's still possible to resolve an unwrapped promise.

async function* g() {
  yield 1;
  yield 2;
}

let gen = g();
gen.next().then(x => {
  gen.return(Promise.resolve(1)).then(stillAPromise => {
    // stillAPromise is not unwrapped at this point
  });
});

[caitp]

Oops, this comment isn't quite right. If you add a return statement between the 2 yields, though, and add an extra .next() call before the .return() call, I believe it is still not unwrapping the promise.

async function* g() {
  yield 1;
  return 2;
}

let gen = g();
gen.next().then(x => {
  gen.next().then(x => {
    gen.return(Promise.resolve(1)).then(stillAPromise => {
      // stillAPromise is not unwrapped at this point
    });
  });
});

[caitp]

I suppose the correct fix is to just move all of this block of changes to line 108, replacing the "AsyncGeneratorResolve" call.

[domenic]

Great catch, that makes perfect sense.

[arai-a]

with the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1352312 , I get the following (the expected result I think) from the first comment in #93 (comment)

fulfilled #1 #1
fulfilled #2 success#2
reject #3 Error: Some error
fulfilled #4 undefined

anything else to verify?

[domenic]

Here are some other scenarios to test:

• Clarification of execution of next() and throw() with await  #93 (comment)
• Move the awaiting (back) into yield #102 (comment)
• Move the awaiting (back) into yield #102 (comment)
• Move the awaiting (back) into yield #102 (comment)

plus the following:

async function* f() {
  yield Promise.resolve(1);
  yield Promise.reject(2);
  yield 3;
}

async function* g() {
  yield 1;
  throw 2;
  yield 3;
}

where both f() and g() should give, upon calling next():

• First a fulfilled promise for { value: 1, done: false }
• Then a rejected promise for 2
• Then (forever) fulfilled promises for { value: undefined, done: true }

[arai-a]

#93 (comment) hits an assertion failure.

Looks like, after the async generator gets closed, AsyncGeneratorResumeNext leaves the request in the queue, until the corresponding promise gets resolved.
and that results in handling the same request twice, with the following path:

1. AsyncGenerator.prototype.return calls AsyncGeneratorEnqueue

2. AsyncGeneratorEnqueue enqueues a newly created request (req-1)

3. AsyncGeneratorEnqueue calls AsyncGeneratorResumeNext, since state is "suspendedStart"

4. AsyncGeneratorResumeNext peeks a request (req-1) from the queue and handles it

5. AsyncGeneratorResumeNext sets the state to "completed"

6. AsyncGeneratorResumeNext awaits on the return value

7. AsyncGeneratorResumeNext returns without removing req-1

8. AsyncGenerator.prototype.throw calls AsyncGeneratorEnqueue

9. AsyncGeneratorEnqueue enqueues a newly created request (req-2)

10. AsyncGeneratorEnqueue calls AsyncGeneratorResumeNext, since state is "completed"

11. AsyncGeneratorResumeNext peeks a request (still req-1) from the queue and handles it

12. AsyncGeneratorResumeNext awaits on the return value

13. the promise for the return value gets resolved for step 6

14. AsyncGeneratorResolve removes a request (req-1) from the queue and resolves the promise

15. AsyncGeneratorResolve calls AsyncGeneratorResumeNext

16. AsyncGeneratorResumeNext peeks a request (now req-2) from the queue and handles it

17. AsyncGeneratorResumeNext calls AsyncGeneratorReject

18. AsyncGeneratorReject removes a request (req-2) from the queue and rejects the promise

19. AsyncGeneratorReject calls AsyncGeneratorResumeNext

20. AsyncGeneratorResumeNext exits since the queue is empty

21. the promise for the return value gets resolved for step 12

22. AsyncGeneratorResolve tries to remove a request from the queue, but the queue is empty and the assertion fails

[arai-a]

so, if we await on the return value there, we should remove the request from the queue and hold the request in the different place, associating the request to the promise (await).

[caitp]

@arai-a, I believe the thing that makes sense is: if an Await is in progress, abort AsyncGeneratorResumeNext().

I don't know if thats written in the proposal anywhere, but I've implemented it that way as it's the only thing that makes sense (to me).

[arai-a]

yeah, that change works and all testcases seem to work as expected

[domenic]

@caitp @arai-a if I am understanding the problem correctly, I think my most recent commit implements @caitp's fix. Let me know if you agree!

[arai-a]

yes, that's exactly same as what I did in above comment.