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

introducing the wrapped module namespace exotic objects #370

Closed
wants to merge 3 commits into from

Conversation

caridy
Copy link
Collaborator

@caridy caridy commented Jun 20, 2022

Description

This PR introduces the concept of a wrapped module namespace exotic object.

Goals

  1. To provide a more ergonomic way to interact with the ShadowRealm instance.
  2. To resolve importValue: How to import a module with no exports? #364 (missing capability)
  3. To resolve Inspecting errors thrown from ShadowRealm #353 (ergonomic of module resolution errors)

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 implements ShadowRealm.import() method that is easier to understand, and similar to the dynamic import() 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:

const wns1 = await myShadowRealm.import('/something');
const wns2 = await myShadowRealm.import('/something');
wns1 === wns2; // yields false

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:

const wns = await myShadowRealm.import('/something');
typeof wns.foo === 'function'; // yields true
wns.foo === wns.foo; // yields false it creates a new wrapped function for `foo` every time

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:

const importer = myShadowRealm.evaluate(`(specifier, name, callback) => {
    const ns = await import(specifier);
    callback(ns[name]);
}`);
importer('/something', 'foo', (value) => {
    value; // will be different every time you call this import with these arguments
});

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 accessing ns 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. :

const importer = myShadowRealm.evaluate(`(specifier, callback) => callback(await import(specifier))`);
importer('/something', (wns) => {
    wns.foo();
});

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.

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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@erights @mhofman this is important. If an iframe gets a reference to a shadow realm from another realm, and invokes its sr.import() method, the produced promise, and the resolved wrapped module namespace exotic object should belong to the incubator realm rather than the iframe's realm.

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]].
Copy link
Collaborator Author

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.

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??

Copy link
Collaborator Author

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

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>
Copy link
Collaborator Author

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>
Copy link
Collaborator Author

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

Copy link
Member

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.

@mhofman
Copy link
Member

mhofman commented Jun 20, 2022

  1. Should wrapped module namespace exotic objects be cached by the host?

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.

  1. Should the wrapped module namespace exotic object cache the wrapped value associated to a binding?

Definitely not. As you mention, a userland implementation of this would similarly recreate the value for every Get

  1. Should Module Namespace Exotic objects be wrapped automatically when crossing the callable boundary?

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.

@mhofman
Copy link
Member

mhofman commented Jun 20, 2022

Do we have spec rendering for PRs on this repo?

Comment on lines +107 to +108
1. If Type(_P_) is Symbol, then
1. Return ! OrdinaryGet(_O_, _P_, _Receiver_).
Copy link
Member

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?

Copy link
Collaborator Author

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">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

</emu-clause>
</emu-clause>

<emu-clause id="sec-finishshadowrealmimport" type="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.

</emu-alg>
</emu-clause>

<emu-clause id="sec-hostimportvaluemoduledynamically" type="host-defined 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.

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*).
Copy link
Member

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 (?/!).

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

@leobalter
Copy link
Member

Do we have spec rendering for PRs on this repo?

@mhofman while this repo doesn't have a auto renderer for PRs, you can do this locally by fetching this branch and running the npm install && npm run build on it. The files will be at the dist/ folder.

spec.html Outdated Show resolved Hide resolved
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]]).
Copy link
Collaborator Author

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_).
Copy link
Member

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>
Copy link
Member

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~

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?

Copy link
Collaborator Author

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.

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]].

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??

@caridy caridy requested review from syg and littledan June 23, 2022 04:13
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]].
Copy link
Member

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.

@caridy caridy marked this pull request as draft July 1, 2022 03:48
@caridy
Copy link
Collaborator Author

caridy commented Jul 1, 2022

@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.

@caridy
Copy link
Collaborator Author

caridy commented Jan 27, 2023

Closing this to avoid confusion.

@caridy caridy closed this Jan 27, 2023
@ljharb ljharb deleted the caridy/wrapped-module-namespace-exotic-objects branch January 27, 2023 17:14
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.

importValue: How to import a module with no exports? Inspecting errors thrown from ShadowRealm
7 participants