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: phrase tail calls as discarding resources rather than popping execution context stack #2495

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Aug 23, 2021

In #2409, #2400, #2429, #2413, and other issues, we've been trying to clarify how execution contexts work within the spec. I think I have a good handle on how to clarify all of the sources of confusion with some editorial tweaks, with the exception of PrepareForTailCall.

PrepareForTailCall is particularly strange. Unlike everywhere else in the spec (as of #2413), it pops the execution context stack without immediately resuming the execution context which is now at the top of the stack, leaving the spec's abstract machine in an odd state where the "running execution context" (defined to be the top of the stack) is not actually running in the sense of "not suspended".

I talked to @allenwb about the intended interpretation of PrepareForTailCall (thanks Allen!). He explained that the actual goal of the AO is just to require engines discard the resources associated with the current call - popping the execution context stack and discarding the former top-of-stack is intended to signify that. But manipulating the execution context stack has other consequences with regards to the transfer of control within the specification's abstract machine, and I don't have a good way to make that coherent for PrepareForTailCall as currently phrased.

Instead, this PR replaces PrepareForTailCall's pop of the execution context stack with an explicit instruction to discard the resources associated with the running execution context, without actually removing it from the execution context stack in the specification's abstract machine.

Unfortunately, this has observable consequences for the realm of the TypeError which results when tail-calling a revoked proxy (although no other observable consequences, that I can tell), which is why this is Normative. Concretely, in

let F = evalInNewRealm(`
  (function F() {
    let { proxy, revoke } = Proxy.revocable(() => {}, {});
    revoke();
    return proxy();
  })
`);

try {
  F();
} catch (e) {
  console.log(e instanceof TypeError);
}

prior to this PR the above snippet would return true (because the TypeError is created in the realm of the code which invokes F, since F's execution context is discarded from the stack before the TypeError is thrown), and with this PR it would return false (because F's execution context is still on top of the stack at the time the TypeError is thrown).

Of the engines I tested, only JavaScriptCore and XS implemented tail calls. I couldn't figure out how to observe the realm for error objects in XS, and in JSC the error is always created in the realm of the Proxy object itself, which is wrong both before and after this PR. So I don't think there's any web-reality issues with this PR.

This PR is an alternative to #2430.

@bakkot bakkot force-pushed the prepare-for-tail-call-phrasing branch from 9d7d735 to 9478f2b Compare August 23, 2021 03:19
@devsnek
Copy link
Member

devsnek commented Aug 23, 2021

this feels more confusing in the context of communicating to someone how to implement this language.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 23, 2021

this feels more confusing in the context of communicating to someone how to implement this language.

Hm. I very, very strongly disagree.

@bakkot bakkot added the editor call to be discussed in the next editor call label Aug 23, 2021
@jmdyck
Copy link
Collaborator

jmdyck commented Aug 23, 2021

this feels more confusing in the context of communicating to someone how to implement this language.

More confusing than the status quo?

@devsnek
Copy link
Member

devsnek commented Aug 23, 2021

More confusing than the status quo?

yes, i don't find the idea that the spec requires an execution context to perform logic to be very useful. this also came up with the duplicated execution context when async functions are called.

@bakkot
Copy link
Contributor Author

bakkot commented Aug 23, 2021

i don't find the idea that the spec requires an execution context to perform logic to be very useful

This PR doesn't really do anything to entrench that idea. Rather, it's attempting to address the thing where changing the top-of-stack execution context is sometimes used as a transfer of control, but in an incoherent way - see the removed note about "the above call will not return here", which implies that PrepareForTailCall in its current state affects the control of the VM in some unspecified and difficult-to-reason-about way, and also see the various notes about transfers of control in generators.

The goal of this PR is to make the transfers of control consistent with the execution stack manipulation, by removing this one execution stack manipulation that is said to cause a transfer of control (eventually) in a different (and unspecified) way than all others.

@bakkot bakkot removed the editor call to be discussed in the next editor call label Aug 25, 2021
@ljharb ljharb added the normative change Affects behavior required to correctly evaluate some ECMAScript source text label Aug 31, 2021
@bakkot bakkot added the has consensus This has committee consensus. label Aug 31, 2021
@bakkot
Copy link
Contributor Author

bakkot commented Aug 31, 2021

I got consensus for the normative change implied by this PR at TC39 today.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

This LGTM but I'd like to discuss the "Discard all resources associated with the current execution context" phrasing at the next editor call before merging. I think we can choose a phrasing that better reflects the committee's desired constraints for PTCs.

@bakkot bakkot added the editor call to be discussed in the next editor call label Sep 2, 2021
jugglinmike added a commit to tc39/test262 that referenced this pull request Sep 3, 2021
Normative: phrase tail calls as discarding resources rather than popping
execution context stack
tc39/ecma262#2495
@jugglinmike
Copy link
Contributor

Test under review here: tc39/test262#3181

@jmdyck
Copy link
Collaborator

jmdyck commented Sep 4, 2021

I got consensus for the normative change implied by this PR at TC39 today.

Am I right in thinking that #2430 doesn't involve this normative change, and so is effectively dead?

@bakkot
Copy link
Contributor Author

bakkot commented Sep 4, 2021

Almost certainly, yes. I'll confirm with the other editors at our next call what approach we want to take.

rwaldron pushed a commit to tc39/test262 that referenced this pull request Sep 7, 2021
Normative: phrase tail calls as discarding resources rather than popping
execution context stack
tc39/ecma262#2495
@michaelficarra michaelficarra added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Sep 8, 2021
@bakkot
Copy link
Contributor Author

bakkot commented Sep 8, 2021

@jmdyck Yes, we're going to go with this approach rather than #2430. Thanks for writing it up, though; it and our conversations elsewhere have been very helpful in thinking through this, particularly your very detailed PR message.

@ljharb ljharb force-pushed the prepare-for-tail-call-phrasing branch from 9478f2b to a7054ce Compare September 9, 2021 04:55
@ljharb ljharb merged commit a7054ce into master Sep 9, 2021
@ljharb ljharb deleted the prepare-for-tail-call-phrasing branch September 9, 2021 04:59
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
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.

None yet

7 participants