1426690 - Add support for defining ES6 modules via JSAPI

Status: NEW

(Core :: JavaScript Engine, enhancement, P3)

Description

[Till Schneidereit [:till]]

Right now, ES6 modules can only be created by parsing source code with a module target. We'd like to also have the ability to create them via JSAPI, exporting native functions and properties.

This is a bit annoying, because we'll want to be able to export some slightly complicated things.

The simplest case is just exporting a list of native functions. That could look something like this:

extern JS_PUBLIC_API(JSObject*)
JS::InitModule(JSContext* cx, const JSFunctionSpec* fs);

It doesn't cover default exports, but one could imagine adding support for that using an additional optional JSFunctionSpec*:

extern JS_PUBLIC_API(JSObject*)
JS::InitModule(JSContext* cx, const JSFunctionSpec* fs, const JSFunctionSpec* default = nullptr);

This isn't enough still: we'll want to be able to export plain values of all kinds. Crucially, we'll want to be able to export classes, presumably created using JS_InitClass. We could just support exporting values and be done with it using something (vaguely) like this:

extern JS_PUBLIC_API(JSObject*)
JS::InitModule(JSContext* cx, JS::StringVector names, JS::AutoValueVector values, HandleValue default = UndefinedHandleValue);

However, now we lose the convenience of easily exporting a bunch of native functions, which seems very nice to have. Maybe the best thing to do is add a helper for converting and appending a JSFunctionSpec* to a pair of JS::StringVector / JS::AutoValueVector and document that as the way to export lists of functions?


Implementation-wise the biggest issue is initializing module export tables explicitly; right now, their initialization is fairly deeply intertwined with module parsing.

Comment 1

[Nicolas B. Pierron [:nbp]]

I think this API would be useful for encoding and decoding modules with XDR as well.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Taking this.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Always expose Reflect.Loader in the JS shell

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Allow registering custom modules in the JS shell

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a new JSAPI for creating native modules

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c67aee5c03

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a new JSAPI for creating native modules

Fix introduction of possibly uninitialized lines and columns when there is no
token stream. Thanks asan!

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ca30648d94e

Comment 9

[Jon Coppeard (:jonco)]

Comment on attachment 8949593 [details] [diff] [review]
Part 0: Always expose Reflect.Loader in the JS shell

Review of attachment 8949593 [details] [diff] [review]:
-----------------------------------------------------------------

It occurred to me after we talked that you can probably just make your test case a module and this will be defined for you.  In a jit-test you write:

// |jit-test| module

at the start to get this.

Otherwise this is fine.

Comment 10

[Jon Coppeard (:jonco)]

Comment on attachment 8949594 [details] [diff] [review]
Part 1: Allow registering custom modules in the JS shell

Review of attachment 8949594 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 8949599 [details] [diff] [review]
Part 2: Add a new JSAPI for creating native modules

Review of attachment 8949599 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this.

At some point we might want to split the current ModuleObject into a base class and then separate subclasses for source text modules and native modules.  That's prbobably overkill for now, but it would have saved you from creating dummy scopes and environment objects here.

::: js/src/builtin/ModuleObject.cpp
@@ +1481,5 @@
>  ModuleBuilder::appendExportEntry(HandleAtom exportName, HandleAtom localName, ParseNode* node)
>  {
>      uint32_t line = 0;
>      uint32_t column = 0;
> +    if (node) {

Here and below, I think it should be the case that if node is non-null then tokenStream_ is non-null too.  We should probably assume that and assert it, rather than testing tokenStream_ every time.

::: js/src/jsapi.cpp
@@ +5019,5 @@
> +    JS::MutableHandle<JS::GCVector<JS::NativeModuleExportEntry>> exports,
> +    JS::HandleValue value)
> +{
> +    RootedString name(cx, cx->names().default_);
> +    return exports.append(JS::NativeModuleExportEntry(name, value));

You could encode the default export differently since it's legal to have an export named 'default' (e.g. use nullptr).  If you do it this way you should disallow 'default' in Add(), not just assert.

@@ +5043,5 @@
> +        return nullptr;
> +
> +    for (const auto& exp : exports) {
> +        assertSameCompartment(cx, exp.name);
> +        assertSameCompartment(cx, exp.value);

assertSameCompartment() will take more that two arguments.

@@ +5081,5 @@
> +    PodCopy(bindings->names, names.begin(), names.length());
> +    bindings->varStart = 0;
> +    bindings->letStart = 0;
> +    bindings->constStart = 0;
> +    bindings->length = names.length();

The comment in Scope.h describes the ModuleScope::Data arrangement like this:

        // imports - [0, varStart)
        //    vars - [varStart, letStart)
        //    lets - [letStart, constStart)
        //  consts - [constStart, length)

So assuming these exports are meant to look like vars, I think you want varStart to be zero but all the others set to names.length().

@@ +5115,5 @@
> +        if (!JS_DefineElement(cx, exportsArr, i, name, JSPROP_ENUMERATE))
> +            return nullptr;
> +    }
> +
> +    RootedModuleNamespaceObject ns(cx, ModuleObject::createNamespace(cx, module, exportsArr));

You don't need to do create a module namespace object here.  This should happen automatically when someone imports the module namespace.

You should add tests for this though.

::: js/src/jsapi.h
@@ +4320,5 @@
> + */
> +struct NativeModuleExportEntry
> +{
> +    JS::Heap<JSString*> name;
> +    JS::Heap<JS::Value> value;

These don't need to be Heap<> if they are always used rooted on the stack.  Please add a MOZ_RAII annotation to the class though if this is the case.

::: js/src/shell/js.cpp
@@ +6783,5 @@
> +        return false;
> +
> +    for (auto id : ids) {
> +        if (!JSID_IS_STRING(id))
> +            continue;

We should probably report an error here.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jon Coppeard (:jonco) from comment #11)
> Comment on attachment 8949599 [details] [diff] [review]
> Part 2: Add a new JSAPI for creating native modules
> 
> Review of attachment 8949599 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for doing this.

Thanks for the quick review!

> At some point we might want to split the current ModuleObject into a base
> class and then separate subclasses for source text modules and native
> modules.  That's prbobably overkill for now, but it would have saved you
> from creating dummy scopes and environment objects here.

Yeah, either that or a `mozilla::Variant` based internal `ModuleObject::Impl` private slot. I was originally going to go down this route, but wasn't sure exactly how to pick apart the current implementation.

> ::: js/src/builtin/ModuleObject.cpp
> @@ +1481,5 @@
> >  ModuleBuilder::appendExportEntry(HandleAtom exportName, HandleAtom localName, ParseNode* node)
> >  {
> >      uint32_t line = 0;
> >      uint32_t column = 0;
> > +    if (node) {
> 
> Here and below, I think it should be the case that if node is non-null then
> tokenStream_ is non-null too.  We should probably assume that and assert it,
> rather than testing tokenStream_ every time.

SGTM.

> 
> ::: js/src/jsapi.cpp
> @@ +5019,5 @@
> > +    JS::MutableHandle<JS::GCVector<JS::NativeModuleExportEntry>> exports,
> > +    JS::HandleValue value)
> > +{
> > +    RootedString name(cx, cx->names().default_);
> > +    return exports.append(JS::NativeModuleExportEntry(name, value));
> 
> You could encode the default export differently since it's legal to have an
> export named 'default' (e.g. use nullptr).  If you do it this way you should
> disallow 'default' in Add(), not just assert.

I thought about this, and decided it ultimately wasn't worth the ceremony. Happy to do it if you feel more strongly, though!

> @@ +5081,5 @@
> > +    PodCopy(bindings->names, names.begin(), names.length());
> > +    bindings->varStart = 0;
> > +    bindings->letStart = 0;
> > +    bindings->constStart = 0;
> > +    bindings->length = names.length();
> 
> The comment in Scope.h describes the ModuleScope::Data arrangement like this:
> 
>         // imports - [0, varStart)
>         //    vars - [varStart, letStart)
>         //    lets - [letStart, constStart)
>         //  consts - [constStart, length)
> 
> So assuming these exports are meant to look like vars, I think you want
> varStart to be zero but all the others set to names.length().

I actually meant to encode them as `const`, and this code definitely warrants a comment in retrospect.

I was thinking that, since the bindings will never change, `const` makes sense. Additionally, in some future world, if we ever optimize `const` bindings so that they don't go through the indirection table, native modules could take advantage of that.

If I am missing some reason why `var` is better, then please enlighten me!

> @@ +5115,5 @@
> > +        if (!JS_DefineElement(cx, exportsArr, i, name, JSPROP_ENUMERATE))
> > +            return nullptr;
> > +    }
> > +
> > +    RootedModuleNamespaceObject ns(cx, ModuleObject::createNamespace(cx, module, exportsArr));
> 
> You don't need to do create a module namespace object here.  This should
> happen automatically when someone imports the module namespace.
> 
> You should add tests for this though.

How would I test this?

> ::: js/src/jsapi.h
> @@ +4320,5 @@
> > + */
> > +struct NativeModuleExportEntry
> > +{
> > +    JS::Heap<JSString*> name;
> > +    JS::Heap<JS::Value> value;
> 
> These don't need to be Heap<> if they are always used rooted on the stack. 
> Please add a MOZ_RAII annotation to the class though if this is the case.

I suppose in practice that these will always be stack rooted, but the setup now is fully general and will work either way.

`MOZ_RAII` is a synonym for `MOZ_STACK_CLASS` -- will the fact that this type ends up in the heap-allocated elements buffer of a `Vector` cause problems?

> ::: js/src/shell/js.cpp
> @@ +6783,5 @@
> > +        return false;
> > +
> > +    for (auto id : ids) {
> > +        if (!JSID_IS_STRING(id))
> > +            continue;
> 
> We should probably report an error here.

SGTM.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Allow registering custom modules in the JS shell

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a new JSAPI for creating native modules

Switched to using `// |jit-test| module` in the test. This avoids the old part 0
patch, which required changing a couple other non262 tests, and was ultimately
unnecessary.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8949875 [details] [diff] [review]
Part 1: Allow registering custom modules in the JS shell

Carrying over r+

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f2458c56ee

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a new JSAPI for creating native modules

Actually fix header includes ordering.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

And yet another try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba4f79b4e68

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #18)
> And yet another try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba4f79b4e68

Looks like a couple things are still broken:

* non-unified builds: I'm just missing a couple of imports in the new jsapi-test file.

* the new jit-test is failing on windows, because of the different path separator.

* SM(rust) / bindgen is falling over. I think that I need to make `JS::NativeModuleExportEntry` opaque since `JS::Heap` is blacklisted.

    error[E0412]: cannot find type `Heap` in module `root::JS`
        --> /builds/worker/workspace/build/src/js/rust/target/debug/build/js-065e4190bdb07bb8/out/jsapi_debug.rs:4193:33
         |
    4193 |             pub name: root::JS::Heap<*mut root::JSString>,
         |                                 ^^^^ not found in `root::JS`

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Here is my latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50cd7470dcd8&group_state=expanded

(I'll attach the latest version of the patch in a moment.)

I can't seem to get this to work with Windows paths. Do you have any suggestions, jonco?

What do you think of adding some sentinel prefix character to modules that are manually registered and aren't supposed to exist on the file system? Maybe `@`? So the native module would be named `@my-native-module`. Then, ModuleLoader.js would check for this, and avoid the whole path normalization and resolution steps for these names.

Alternatively, if you know of a better way to fix the path stuff on Windows, I'm all ears.

Thanks!

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add a new JSAPI for creating native modules

Comment 22

[Jon Coppeard (:jonco)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #20)
In general let's make this work as much as possible like the HTML module loader.

So, I think we want module specifiers to be '/'-based paths on all OSs and do the backslash substitution and prepending of loadPath in fetch() rather than normalize() and resolve().

I'm not exactly sure what's failing here but I think that should fix it (it should mean fetch() never gets called for this case).  I'm not that strongly against adding a special prefix if we really need to though.

Comment 23

[Wesley W. Garland]

I have a use-case for this, FWIW.

We are embedding SpiderMonkey in Python (pythonmonkey.io), and auto-emitting wrappers (etc) between Python and JS code. We'd like to be able to import Python modules as ESMs (we are currently doing CommonJS .py modules). I can pass JSAPI a bunch of JSObject*, JSFunction*, etc -- would be nice if we could just say, "Hey! Here is a map of jsid->jsval (or whatever), please treat it as dynamically-loaded module ABC".