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: Factor out AO RunSuspendedContext() #2664

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Feb 13, 2022

This doesn't address the semantics of Suspend/Resume, just factors out a common chunk of code.

This PR is split into 6 commits where it's easier to confirm the correctness. They're probably fine to be squashed before landing.

spec.html Outdated
1. Push _asyncContext_ onto the execution context stack; _asyncContext_ is now the running execution context.
1. <emu-meta effects="user-code">Resume the suspended evaluation of _asyncContext_</emu-meta> using NormalCompletion(_value_) as the result of the operation that suspended it.
1. Assert: When we reach this step, _asyncContext_ has already been removed from the execution context stack and _prevContext_ is the currently running execution context.
1. Perform Completion(RunSuspendedContext(_asyncContext_, NormalCompletion(_value_))).
Copy link
Member

Choose a reason for hiding this comment

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

@jmdyck @bakkot Does ecmarkup complain if you remove these Completion(...)s? If so, we should allow them to be omitted when used with Perform since they're not actually entering the algorithm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes it does, e.g.: error: RunSuspendedContext returns a Completion Record, but is not consumed as if it does (typecheck) at spec.html:46018:24

@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Nov 21, 2022
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Dec 7, 2022
@jmdyck jmdyck force-pushed the RunSuspendedContext branch 2 times, most recently from ed3013c to 4cfe666 Compare January 14, 2023 14:29
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 14, 2023

This PR's RunSuspendedContext (on the 'next' side) is the complementary operation to RunCallerContext (on the 'yield' side), which I proposed in PR #2665, but which got separated out of that PR by editor request. So I'm expecting that this PR will be rejected. If that's the case, hearing it sooner would mean less maintenance work for me.

... because ExecuteModule does it before calling AsyncBlockStart,
so it seems like AsyncFunctionStart would have to as well.
(The semantics of "Suspend" are fuzzy, but this seems innoccuous to me.)
From: immediately before calls to AsyncBlockStart()
To: just inside AsyncBlockStart()
This gets things ready for the refactoring
that the next commit will do.
It's mostly just moving certain steps earlier or later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants