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: InitializeHostDefinedRealm-related refactorings #3139

Merged
merged 2 commits into from
May 23, 2024

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Aug 2, 2023

In 9.6 InitializeHostDefinedRealm, there are two If-steps that allow the host to customize the global object and global *this* binding. If the host just wants the default (of either), that's handled by passing *undefined* to (the relevant parameter of) SetRealmGlobalObject, which takes care of setting the realm's [[GlobalObject]] and [[GlobalEnv]] appropriately.

This division of labor always struck me as a bit odd. E.g., why set _global_ to *undefined* and then have prose to explain the significance of doing so? Why not just set _global_ appropriately right there? That was the impetus for this PR. (My guess was that SetRealmGlobalObject was used by the HTML spec, but it isn't, and as far as I can tell it never was. Perhaps that was briefly someone's plan.)

This PR is split into 6 commits for ease of review. If approved, I'll do some squashing.

  • I start by moving InitializeHostDefinedRealm to a better location.

    It's odd for InitializeHostDefinedRealm (clearly a realm-centric operation) to be out on its own in 9.6, when there's a "Realms" section at 9.3.

    (When Realms were introduced in ES6, the relative location of InitializeHostDefinedRealm was somewhat different and made more sense, but things have changed since then.)

  • Line-break the two "If the host requires" steps for readability.

  • Inline SetRealmGlobalObject into InitializeHostDefinedRealm.

    (Originally, I only hoisted out the SetRealmGlobalObject steps that assigned 'meanings' to the *undefined* parameters, but that left so little in SetRealmGlobalObject that I didn't see any reason for it to continue to exist.)

  • Reorganize/simplify the result of the previous commit.

  • Tweak the return of SetDefaultGlobalBindings.

    In SetDefaultGlobalBindings, _global_ is an alias for _realmRec_.[[GlobalObject]]. In InitializeHostDefinedRealm, this is _realm_.[[GlobalObject]], which is just _global_. But SetDefaultGlobalBindings returns that object, which InitializeHostDefinedRealm then assigns to _globalObj_, meaning that we now have two aliases for the same object, which is unnecessarily confusing.

    So this commit tweaks SetDefaultGlobalBindings to return ~unused~, and tweaks InitializeHostDefinedRealm to refer to _global_ rather than introducing _globalObj_.

  • Inline CreateRealm into InitializeHostDefinedRealm.

    This wasn't part of my original plan, but I like it because it surfaces both the early and later settings of _realm_.[[GlobalObject]] and _realm_.[[GlobalEnv]]. This makes it really obvious that the early settings are transient. (See Editorial: More info in 'PromiseCapability Record' table #2380 (comment))

Checking the downstream specs, nobody references SetRealmGlobalObject, CreateRealm, or SetDefaultGlobalBindings.

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.

LGTM, though I think @bakkot may prefer to omit the second commit. In the past, he has been explicitly in support of these single-step definitions.

@bakkot
Copy link
Contributor

bakkot commented Aug 8, 2023

Have I? I don't have any recollection of that.

@michaelficarra
Copy link
Member

@bakkot Yes, in an editor call discussing #3043 you wanted to make sure we did not outright ban this form.

@bakkot
Copy link
Contributor

bakkot commented Aug 8, 2023

I support being able to use the form; that doesn't mean I'm in support of every particular instance. I'm neutral in this case.

@michaelficarra
Copy link
Member

I support being able to use the form

That's the thing I was trying to say.

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

+1 for these changes. I wonder if everything from If the host requires to Create any host-defined global object properties should be moved into a single host hook with the default implementation using these steps verbatim and the HTML spec doing its own thing - I'm happy to look into changing that in light of the discussion in #3274.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 16, 2024

I wonder if [steps] should be moved into a single host hook ...

I have thoughts, but maybe discussion of the proposed host hook would be better in #3274?

@linusg
Copy link
Member

linusg commented May 16, 2024

Yes, I don't mean to block this PR - IMO these changes are good as is while preserving the status quo of how the global object customizations are spec'd.

@michaelficarra michaelficarra requested a review from a team May 17, 2024 02:46
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label May 21, 2024
@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 May 22, 2024
@linusg
Copy link
Member

linusg commented May 23, 2024

Checking the downstream specs, nobody references SetRealmGlobalObject, CreateRealm, or SetDefaultGlobalBindings.

It's worth noting that https://tc39.es/proposal-shadowrealm/#sec-shadowrealm will need an update.

@jmdyck jmdyck force-pushed the InitializeHostDefinedRealm branch 2 times, most recently from 4b9eb20 to 6a5342c Compare May 23, 2024 20:14
…ostDefinedRealm (tc39#3139(

... and put the result in the "Realms" section.
In the status quo, the value returned by SetDefaultGlobalBindings
is always just the [[GlobalObject]] of the Realm Record passed in.

At the only invocation of SetDefaultGlobalBindings
(in InitializeHostDefinedRealm),
this is `_realm_.[[GlobalObject]]`, which is just `_global_`.
So use `_global_` instead of `_globalObj_`
(the alias for the value returned by SetDefaultGlobalBindings),
and tweak SetDefaultGlobalBindings to return ~unused~.
@ljharb ljharb force-pushed the InitializeHostDefinedRealm branch from 6a5342c to 052defa Compare May 23, 2024 21:56
@ljharb ljharb merged commit 052defa into tc39:main May 23, 2024
7 checks passed
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants