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

adding step for implementation-defined globals as part of the creation of the realm #236

Closed
wants to merge 2 commits into from

Conversation

caridy
Copy link
Collaborator

@caridy caridy commented Mar 5, 2020

closes #22

@@ -74,6 +74,7 @@ location: https://rawgit.com/tc39/proposal-realms/master/index.html
1. Set _O_.[[Realm]] to _realmRec_.
1. Perform ? SetRealmGlobalObject(_realmRec_, *undefined*, *undefined*).
1. Perform ? SetDefaultGlobalBindings(_O_.[[Realm]]).
1. Create any implementation-defined global object properties on _globalObj_.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

maybe the common steps between this, and InitializeHostDefinedRealm, could be factored out into a shared abstract operation?

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, we discussed this extensibly today in a bigger context. @erights raised valid concerns about clause 18 (https://tc39.es/ecma262/#sec-global-object) last bullet point:

may have host defined properties in addition to the properties defined in this specification. This may include a property whose value is the global object itself.

That is part of SetDefaultGlobalBindings abstract operation (which iterates over every property of each property of the Global Object as defined by clause 18.

The two main concerns are:

  1. "implementation-defined properties" vs "host defined properties". that on itself is confusing.
  2. having that as part of the global object definition makes it hard for compartments, and other ways to create a global object in the future if we want to introduce user-defined properties via some hooks.

The recommendation at this point is to create a new abstract operation, and call that right after SetDefaultGlobalBindings(), and remove the last bullet of clause 18 entirely. This change has no impact on implementation, it is just spec refactor.

@ljharb @littledan can we do that on a separate PR for 262? or should we continue with that here?

Copy link
Member

Choose a reason for hiding this comment

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

imo anything that can hit 262 separate from a proposal, should.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ljharb what? Can you rephrase?

Copy link
Member

@ljharb ljharb Mar 6, 2020

Choose a reason for hiding this comment

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

"Any editorial refactorings that can land in 262 without consensus, and make nonzero sense without a proposal, should imo land prior to the proposal".

Copy link
Collaborator

Choose a reason for hiding this comment

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

good, thanks.

@littledan
Copy link
Member

I'm having trouble understanding this patch. I never interpreted that line to be "invoke Annex B" but rather "add host-defined globals like window, fetch, etc". I thought the Annex B properties were added the same way as non-Annex B properties.

My initial thought is, we should not have this line. The typical global object has a bunch of properties added by the host, but in Realms, we should not have these properties. Therefore, we shouldn't copy the logic from the initialization of the global object that adds these properties.

To close #22, I'd simply clarify, in the introduction to Annex B, that it can be included in Realms. In the introduction to Annex B, something like the following:

The ECMAScript language syntax and semantics defined in this annex are required when the ECMAScript host is a web browser. The content of this annex is normative but optional if the ECMAScript host is not a web browser. This annex applies equally inside and outside of Realms, that is, it is required in Realms in a browser environment, and normative optional in Realms if the host is not a web browser.

(I'd be happy with weaker wording as well, but this stronger wording is what seems to fit naturally with what's written there already. I'd also be fine with this addition being in a note, as it could be seen to be logically implied from existing text.)

@caridy
Copy link
Collaborator Author

caridy commented Mar 7, 2020

@littledan while I do agree about #22, and that paragraph for Annex B looks great to me, I do believe that the change in this PR is more about specifying when can the host make such determination in preparation for the compartment/evaluator to provide a hook for it in the future. As for the wording of that specific step, I don't feel attached to it, we can rewording it.

@littledan
Copy link
Member

I'd rather use host hooks to indicate where the host can add behavior, as we did in #230. Host hooks are nice because they give us an opportunity to may out invariants for the hook. For example, that documentation makes it clear that the host should not add properties. This PR does the opposite, specifically encouraging the host to add properties, which risks making Realms less interoperable.

At a high level: we shouldn't copy this language from the ordinary global object, since it has host-created properties that Realms are intended not to have!

@leobalter
Copy link
Member

Closing this in favor of the current spec text.

@leobalter leobalter closed this Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should globals from 262's Annex B be part of the intrinsics/stdlib?
5 participants