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: Model execution context as a record #2246

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

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Dec 8, 2020

Resolves the first comment in issue #1742

I believe this PR is complete in the sense that it would leave the spec in a consistent state. However, there are various further changes you might want, so it's currently a Draft PR.

  • I introduced the term ExecutionContext Record, but left some occurrences of execution context. You might prefer to get rid of execution context entirely.

  • I changed the caption text of the three Fields tables, but not their id attributes.

  • The status quo creates the context and then separately sets each of its components. I kept that format, but you might prefer the use of a record "literal" to define it all in one step.
    [Later: @bakkot prefers it as-is.]

  • Where the status quo refers to the running execution context's SomethingOrOther, I introduced a step Let _runningContext_ be the running execution context and then referred to _runningContext_.[[SomethingOrOther]]. You might prefer to introduce a compact way to say "the running execution context". (About the only precedent for this is the use of NewTarget in algorithms.)
    [Later: @syg suggests "the [[SomethingOrOther]] field of the running execution context"]
    [Even later: settle on "the running execution context's [[SomethingOrOther]]"]

  • When we define Additional Fields elsewhere in the spec (for Environment Records and Module Records), they're aligned with a (quasi) subtype hierarchy. This sort of works for ExecutionContext Record and ECMAScript code ExecutionContext Record, but not for Generator ExecutionContext Record: it's difficult to see the latter as a subtype. An ExecutionContext Record that represents the evaluation of a generator object is created as an ECMAScript code ExecutionContext Record (not a Generator ExecutionContext Record) and then later (after it's already been made the running execution context), it has a [[Generator]] field attached to it.

  • The execution context stack could conceivably be modeled by giving each ExecutionContext Record something like a [[CallerContext]] field .


Downstream effects:
The HTML spec has some references to an execution context's "Realm component", which would be changed to its "[[Realm]] field" after this PR.

@bakkot
Copy link
Contributor

bakkot commented Dec 8, 2020

Nice! I'll give this a more thorough review soon, but while I'm thinking of it:

The status quo creates the context and then separately sets each of its components. I kept that format, but you might prefer the use of a record "literal" to define it all in one step.

For now, I think we should stick with the existing format. I would like to address #2095 eventually, at which point I think it might make sense to switch to the literal syntax. But we'd need to find a way to express the CodeEvaluationState field clearly; it doesn't really have a "value" the same way other record fields would.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 8, 2020

For now, I think we should stick with the existing format. I would like to address #2095 eventually, at which point I think it might make sense to switch to the literal syntax.

Yup.

But we'd need to find a way to express the CodeEvaluationState field clearly; it doesn't really have a "value" the same way other record fields would.

Yeah, I maybe should have raised that as another bullet. I mean, it's no less clear than in the status quo, but it does stick out more when we model things this way. The points where code evaluation state / [[CodeEvaluationState]] gets set are nowhere near the points where the context is created. This raises the question of what its status is between those points, and/or whether we should give it some nominal value at the point where the record is created.

@syg
Copy link
Contributor

syg commented Dec 11, 2020

Where the status quo refers to the running execution context's SomethingOrOther, I introduced a step Let runningContext be the running execution context and then referred to runningContext.[[SomethingOrOther]]. You might prefer to introduce a compact way to say "the running execution context". (About the only precedent for this is the use of NewTarget in algorithms.)

In the memory model, the following step is often repeated:

  1. Let execution be the [[CandidateExecution]] field of the surrounding agent's Agent Record.

Analogously here, how do you feel about

  1. Let something be the [[Something]] field of the running execution context.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 16, 2021

(force-pushed to resolve merge conflicts)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 28, 2021

(force-pushed to rebase to master)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 29, 2021

[...] how do you feel about

1. Let _something_ be the [[Something]] field of the running execution context.

I guess I'm okay with it.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 29, 2021

(force-pushed to use the phrase "the [[Something]] field of the running execution context")

@michaelficarra
Copy link
Member

@jmdyck This doesn't resolve #1742 on its own. There's also #2287, #2288, and the "ordered pair" in the Pattern Semantics section that I mentioned. Please remove (or rephrase) your reference to #1742 so we don't accidentally close it by merging this PR.

@bakkot
Copy link
Contributor

bakkot commented Feb 3, 2021

@michaelficarra You should probably rename #1742 to make it clear it's tracking more stuff than its title currently claims.

@michaelficarra
Copy link
Member

@bakkot Okay, changed it to match the project card.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 5, 2021

force-pushed to:

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 21, 2021

(force-pushed to resolve merge conflicts)

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 13, 2021

force-pushed to:

  • rebase to master, resolve merge conflicts
  • handle the new PrivateEnvironment component of ES Code Execution Contexts.

@ljharb ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21
@jmdyck jmdyck marked this pull request as ready for review July 9, 2021 04:15
@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 17, 2021

(force-pushed to rebase to master + resolve merge conflicts from #2408)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 1, 2023

(force-pushed to resolve merge conflicts)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 11, 2024

(force-pushed to resolve merge conflicts)

@safinaskar
Copy link

@jmdyck , thanks a lot! As well as I understand you continuously (and absolutely thanklessly) rebased this PR for 4 years. Thanks a lot for this job! It would be great if this PR finally will be merged (same for #2288 )

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch 2 times, most recently from 32fb60e to 9e0477e Compare August 14, 2024 15:02
@michaelficarra
Copy link
Member

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch 2 times, most recently from 000d66a to a83b586 Compare October 25, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants