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 jmdyck force-pushed the ExecutionContext_Record branch from 85251e6 to 36f5402 Compare January 16, 2021 04:44
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 16, 2021

(force-pushed to resolve merge conflicts)

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 36f5402 to 1e28eeb Compare January 28, 2021 22:46
@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 jmdyck force-pushed the ExecutionContext_Record branch from 1e28eeb to 2b46387 Compare January 29, 2021 02:01
@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 jmdyck force-pushed the ExecutionContext_Record branch from 2b46387 to 2843c22 Compare February 5, 2021 01:31
@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 5, 2021

force-pushed to:

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 2843c22 to de3eb5a Compare April 21, 2021 03:52
@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 21, 2021

(force-pushed to resolve merge conflicts)

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from de3eb5a to 0e8d49f Compare April 21, 2021 14:32
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 0e8d49f to 8959226 Compare May 13, 2021 18:05
@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 force-pushed the ExecutionContext_Record branch from 9d9e403 to 9ae108b Compare July 9, 2021 04:13
@jmdyck jmdyck marked this pull request as ready for review July 9, 2021 04:15
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 9ae108b to 076766e Compare July 18, 2021 14:53
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 076766e to 847cc36 Compare August 17, 2021 03:57
@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 17, 2021

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

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 847cc36 to c02e1b4 Compare August 17, 2021 03:58
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 2fe8a99 to 850156d Compare November 19, 2022 04:38
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 850156d to 6e0ef3f Compare January 15, 2023 01:22
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 6e0ef3f to dd8bd1e Compare February 10, 2023 22:32
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from dd8bd1e to c5bfe97 Compare March 1, 2023 04:33
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from c5bfe97 to 9d52d45 Compare April 21, 2023 00:33
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 9d52d45 to d78e496 Compare August 17, 2023 22:56
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 2424321 to 0693eb6 Compare September 1, 2023 00:36
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 1, 2023

(force-pushed to resolve merge conflicts)

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 0693eb6 to 0cc704f Compare October 13, 2023 19:22
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 0cc704f to f882a94 Compare January 11, 2024 01:20
@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 11, 2024

(force-pushed to resolve merge conflicts)

@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from f882a94 to d489f26 Compare February 25, 2024 03:00
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from d489f26 to 792a368 Compare May 23, 2024 14:14
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 11c8690 to 9d229e1 Compare June 5, 2024 18:48
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from 9d229e1 to 4029119 Compare July 11, 2024 17:20
@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
@jmdyck jmdyck force-pushed the ExecutionContext_Record branch from a83b586 to 8e2bd10 Compare November 13, 2024 01:51
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