-
Notifications
You must be signed in to change notification settings - Fork 67
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
introducing the wrapped module namespace exotic objects #370
Conversation
1. Let _targetEnv_ be _targetModule_.[[Environment]]. | ||
1. If _targetEnv_ is ~empty~, throw a *ReferenceError* exception. | ||
1. Let _value_ be _targetEnv_.GetBindingValue(_binding_.[[BindingName]], *true*). | ||
1. NOTE: To avoid dynamic scoping get the realm to wrap the value from the wrapped module namespace exotic object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Suspend _evalContext_ and remove it from the execution context stack. | ||
1. Resume the context that is now on the top of the execution context stack as the running execution context. | ||
1. If _namespace_ is an abrupt completion, then | ||
1. Let _error_ be an Error object from _callerRealm_ equivalent to _namespace_.[[Value]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMPORTANT: I could not figure (from the latest 262 specs) when is this going to occur and what the value of _namespace_.[[Value]]
will be under those circumstances. In the old days, if there was an abrupt completion when getting the namespace, it's [[Value]]
was never set IIRC. I need some help here.
Ultimate, the goal here is to produce an error object in the _callerRealm_
that can have as much information as possible about the actual error if the error is related to the module graph resolution mechanisms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds to me like this might be a spec refactoring actually in that I don't believe GetModuleNamespace
can ever fail??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might very well be, but I'm not sure @guybedford
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be worth verifying again for certain, but I'm pretty much 90% sure that is the case.
|
||
<dd> | ||
<ul> | ||
<li>At some future time, the host environment must perform FinishShadowRealmImport(_callerRealm__, _specifier_, _promiseCapability_, _promise_), where _promise_ is a Promise rejected with an error object from _callerRealm_ representing the cause of failure.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@syg @legendecas is this enough information for implementers to produce such error from the _callerRealm_
rather than from the _evalRealm_
? Or should we be more specific in the spec on how to construct such error?
<ul> | ||
<li>At some future time, the host environment must perform FinishShadowRealmImport(_callerRealm__, _specifier_, _promiseCapability_, _promise_), where _promise_ is a Promise resolved with *undefined*.</li> | ||
|
||
<li>Any subsequent call to HostResolveImportedModule after FinishShadowRealmImport has completed, given the arguments *null* and _specifier_, must return a normal completion containing a module which has already been evaluated, i.e. whose Evaluate concrete method has already been called and returned a normal completion.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to remove this sentence all together from here since HostResolveImportedModule
is not used directly here. cc @legendecas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HostResolveImportedModule
is called in the closure created in FinishShadowRealmImport
. IIUC, this sentence still applies in that case.
I'd say no, especially if a module namespace object is made to implicitly cross the callable boundary (which has its own concerns detailed below). Regardless, we had the same argument about callables, and didn't see a valid reason to persist identity.
Definitely not. As you mention, a userland implementation of this would similarly recreate the value for every Get
My main concern on this is that it'd enable a way to brand check module namespaces, which the spec currently does not expose. However I believe the Compartment / Module Loader API will likely expose such a brand check too, and a membrane will likely be the one using the callable boundary anyway, allowing it to intercept module namespace objects and proxy them however needed. |
Do we have spec rendering for PRs on this repo? |
1. If Type(_P_) is Symbol, then | ||
1. Return ! OrdinaryGet(_O_, _P_, _Receiver_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the use case of getting a Symbol on a namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jack-Works this is the same as in https://tc39.es/ecma262/#sec-module-namespace-exotic-objects-get-p-receiver. Module Namespace Exotic Objects can allow interaction with symbols in general because symbols are not valid export names.
</emu-note> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-wrappedmodulenamespacecreate" type="abstract operation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is analogous to https://tc39.es/ecma262/#sec-modulenamespacecreate
</emu-clause> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-finishshadowrealmimport" type="abstract operation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is analogous to https://tc39.es/ecma262/#sec-finishdynamicimport
</emu-alg> | ||
</emu-clause> | ||
|
||
<emu-clause id="sec-hostimportvaluemoduledynamically" type="host-defined abstract operation"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is analogous to https://tc39.es/ecma262/#sec-hostimportmoduledynamically
1. Return ? GetModuleNamespace(_targetModule_). | ||
1. Let _targetEnv_ be _targetModule_.[[Environment]]. | ||
1. If _targetEnv_ is ~empty~, throw a *ReferenceError* exception. | ||
1. Let _value_ be _targetEnv_.GetBindingValue(_binding_.[[BindingName]], *true*). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetBindingValue
of a Module Environment Record returns a completion that needs to be unwrapped (?
/!
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@linusg can you elaborate more on your comment/suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, accessing a binding name value cannot throw, that's an invariant of the module system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit unwrapping of completion records was removed from ECMA-262 a while ago - and 9.1.1.5.1 GetBindingValue returns "either a normal completion containing an ECMAScript language value or a throw completion."
If you're sure it can't throw in this situation, this needs to be ... be ! _targetEnv_.GetBindingValue...
, otherwise ... be ? _targetEnv_.GetBindingValue...
- you can't pass a completion record to GetWrappedValue
.
@mhofman while this repo doesn't have a auto renderer for PRs, you can do this locally by fetching this branch and running the |
1. Assert: _targetModule_ is not *undefined*. | ||
1. If _binding_.[[BindingName]] is ~namespace~, then | ||
1. Let _namespace_ be ? GetModuleNamespace(_targetModule_). | ||
1. Return WrappedModuleNamespaceCreate(_realm_, _namespace_.[[Module]], _namespace_.[[Exports]]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thsi is important, this allows sr.import()
to access a binding name that holds another module namespace exotic object.
1. Let _callerRealm_ be the current Realm Record. | ||
1. Let _evalRealm_ be _O_.[[ShadowRealm]]. | ||
1. Let _evalContext_ be _O_.[[ExecutionContext]]. | ||
1. Return ? ShadowRealmImportValue(_specifierString_, _exportName_, _callerRealm_, _evalRealm_, _evalContext_). | ||
1. Perform HostShadowRealmImportModuleDynamically(_specifierString_, _promiseCapability_, _callerRealm_, _evalRealm_, _evalContext_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HostShadowRealmImportModuleDynamically
is been called when the execution context is the one in which ShadowRealm is created. Do we need to switch to the execution context corresponding to the ShadowRealm.[[ExecutionContext]]
like the steps defined in AO ShadowRealmImportValue
?
<ul> | ||
<li>At some future time, the host environment must perform FinishShadowRealmImport(_callerRealm__, _specifier_, _promiseCapability_, _promise_), where _promise_ is a Promise resolved with *undefined*.</li> | ||
|
||
<li>Any subsequent call to HostResolveImportedModule after FinishShadowRealmImport has completed, given the arguments *null* and _specifier_, must return a normal completion containing a module which has already been evaluated, i.e. whose Evaluate concrete method has already been called and returned a normal completion.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HostResolveImportedModule
is called in the closure created in FinishShadowRealmImport
. IIUC, this sentence still applies in that case.
_callerRealm_: a Realm Record, | ||
_evalRealm_: a Realm Record, | ||
_evalContext_: an execution context, | ||
): ~unused~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be a new host function, or could it be an internal function which calls HostImportModuleDynamically
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can certainly generalize HostImportModuleDynamically
to accept those 3 new arguments, and accommodate both. Seems doable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it makes sense for the proposal overall, that would sound preferable to me.
1. Suspend _evalContext_ and remove it from the execution context stack. | ||
1. Resume the context that is now on the top of the execution context stack as the running execution context. | ||
1. If _namespace_ is an abrupt completion, then | ||
1. Let _error_ be an Error object from _callerRealm_ equivalent to _namespace_.[[Value]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds to me like this might be a spec refactoring actually in that I don't believe GetModuleNamespace
can ever fail??
1. Let _callerRealm_ be the current Realm Record. | ||
1. Let _evalRealm_ be _O_.[[ShadowRealm]]. | ||
1. Let _evalContext_ be _O_.[[ExecutionContext]]. | ||
1. Return ? ShadowRealmImportValue(_specifierString_, _exportName_, _callerRealm_, _evalRealm_, _evalContext_). | ||
1. Perform HostShadowRealmImportModuleDynamically(_specifierString_, _promiseCapability_, _callerRealm_, _evalRealm_, _evalContext_). | ||
1. Return _promiseCapability_.[[Promise]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick: We should add an editorial note saying this method is designed to eventually house a parameter for import assertions.
@littledan convinced me to split this into two separate changes, one is to figure the importing errors, while the other one about the wrapped module namespace exotic objects can be defer to v2. |
Closing this to avoid confusion. |
Description
This PR introduces the concept of a wrapped module namespace exotic object.
Goals
Details
A wrapped module namespace exotic object is very similar to a module namespace exotic object, at least in shape and behavior. There are two main differences though:
a) a wrapped module namespace exotic object cannot be part of the module graph, this interface is merely there for programatic access to the exported values of a module, nothing else.
b) from a wrapped module namespace exotic object you can only access primitive values, or wrapped values as defined by the semantics of the callable boundary.
The wrapped module namespace exotic object does not hold a direct reference to the corresponding module namespace exotic object, it only wraps the module record and its exported names list.
ShadowRealm.importValue()
method becomes obsolete, and instead this PR implementsShadowRealm.import()
method that is easier to understand, and similar to the dynamicimport()
with the main difference that it doesn't provide dynamic scoping, meaning that the result of the invocation is not subject to the context of the caller.The wrapped module namespace exotic object is always bound to the incubator realm, in fact, it holds a back pointer to the incubator's Realm Record, which is similar to wrapped function exotic objects. This back pointer is used to create any wrapped values.
Open Questions
1. Should wrapped module namespace exotic objects be cached by the host?
As of right now, this PR doesn't cache the result of calling
.import()
on a shadow realm. Therefore:I don't believe caching is needed from the perspective that wrapped module namespace exotic objects are not participants of the module graph, so checking for its persistent is not going to be common. On the other hand, adding a cache is not super complicated to have another commonality with dynamic
import()
.2. Should the wrapped module namespace exotic object cache the wrapped value associated to a binding?
As of right now, this PR doesn't cache the result of the
[[Get]]
operation when the result is a wrapped value. Therefore:I remember some conversations with @littledan where he was concerned about this particular aspect. I would like to explain this different, with an example of what would happen if you implement your own protocol on top of the callable boundary:
Basically, these two mechanisms are analogous, and you can reasoning about them from that perspective. An alternative would be to cache the result of the
[[Get]]
operation at the wrapped module namespace exotic object with a weakmap-like mechanism where the key must be the callable value returned from the[[Module]]
, and the value must be the wrapped function exotic object.3. Should Module Namespace Exotic objects be wrapped automatically when crossing the callable boundary?
This PR only creates wrapped module namespace exotic objects when the
.import()
method of a ShadowRealm is called. There are two main reasons to generalize this for the callable boundary:a) when a module exports a namespace object, e.g.
export * as ns from "/something";
, in which case accessingns
of the wrapped module namespace exotic object will throw an error.b) the ergonomics of passing a module namespace exotic object are great, e.g. :
Note 1: Doing this auto-wrapping is very straight forward in terms of the spec text.
Note 2: A binding name exporting a module namespace exotic object is also going to be wrapped when accessed thru the wrapped module namespace exotic object. Doing so at the call boundary level for arguments and returned values seems like the right thing to do.