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

Editorial: have CreateIteratorFromClosure provide a new execution context to GeneratorStart #2398

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Apr 29, 2021

See more complete discussion in https://es.discourse.group/t/execution-context-suspension-and-resumption/756. I'll summarize:

GeneratorStart manipulates the code evaluation state of the running execution context, and also stores that context on the generator object. Prior to #2045, the sole caller of GeneratorStart was in EvaluateGeneratorBody (although it wasn't called that at the time). So, in particular, GeneratorStart was only invoked in step 7 of [[Call]] for ordinary functions, at which time the running execution context would have been one created specifically for the evaluation of the generator in step 3 of [[Call]] - that is, the execution state being manipulated and stored would have been created specifically for the generator. And it would be removed from the execution context stack immediately after the call to GeneratorStart (in step 8 of [[Call]]) (and so implicitly suspended), so it makes sense to be describing what should happen after resumption at this point.

But #2045 introduced a new callsite for GeneratorStart, CreateIteratorFromClosure. Here the execution context is not created for the "generator"; it's just whatever happens to be running at the time. This doesn't really make sense - you don't want to be manipulating the execution state of the caller of CreateIteratorFromClosure!

The fix, I believe, is to have CreateIteratorFromClosure create a push a new context before calling GeneratorStart, and pop it after. That way it can "belong" to the synthetic generator it's creating. That is what this PR does.


Note to self: this needs to address CreateAsyncIteratorFromClosure in the same way.

jmdyck
jmdyck previously requested changes Apr 29, 2021
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor Author

bakkot commented Apr 30, 2021

I suppose an alternative is to have GeneratorStart create an execution context itself, and never push it to the execution context stack. That has the advantage that it would not be manipulating the code evaluation state of the running execution context, but the disadvantage that there would then be two execution contexts created for a generator: one for evaluating the parameters, created by [[Call]], and one for evaluating the body, created by GeneratorStart.

@bakkot bakkot added the editor call to be discussed in the next editor call label Jun 5, 2021
@bakkot bakkot added spec bug and removed editor call to be discussed in the next editor call labels Jun 9, 2021
@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 4, 2021
@ljharb ljharb dismissed jmdyck’s stale review July 7, 2021 23:38

comments addressed

@ljharb ljharb merged commit d3e7c43 into master Jul 7, 2021
@ljharb ljharb deleted the generator-start branch July 7, 2021 23:45
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change ready to merge Editors believe this PR needs no further reviews, and is ready to land. spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants