1722802 - Add JSAPI functions to create JS modules w/ custom exports

Status: UNCONFIRMED

(Core :: JavaScript Engine, enhancement, P2)

Description

[fitzgen]

We should add two new JSAPI functions:

/**
 * Create a new, unlinked, empty module record.
 */
extern JS_PUBLIC_API JSObject* CreateEmptyModule(JSContext* cx);

/**
 * Add an export to an unlinked module.
 */
extern JS_PUBLIC_API bool AddModuleExport(JSContext* cx,
                                          Handle<JSObject*> moduleRecord,
                                          Handle<JSString*> name,
                                          Handle<Value> value);

As a SpiderMonkey embedder, one would be able to use these functions to expose synthetic "native" modules to user JS that are made up of, for example, a collection of JSNative functions rather than defined by JS source text.

Comment 1

[fitzgen]

Attachment: WIP implementation

Jon, if I remember correctly, you are familiar with the modules implementation. Could you take a look at this, give me some feedback, and point me in the right direction? See the TODO comment specifically. Thanks!

Comment 2

[Jon Coppeard (:jonco)]

(In reply to fitzgen from comment #1)
This is a good idea, and it's been asked for before (it might even have been by you).

I think ideally we'd take a list of exports because there should not be a way to add exports to a module that already exists (this could break things if the module was already linked for example).

It would also be good to create the module object required rather than compile an empty script and change it afterwards. There is probably an expectation that module always have a script which would need to change though. One part is that you'll need a way to create an environment object like ModuleEnvironmentObject::create does, but for your exports rather than based on the script.

I'll try and take a proper look in the next couple of days.

Comment 3

[fitzgen]

(In reply to Jon Coppeard (:jonco) from comment #2)

I think ideally we'd take a list of exports because there should not be a way to add exports to a module that already exists (this could break things if the module was already linked for example).

Are you imagining something like this?

struct ModuleExport {
  Heap<JSString*> name;
  Heap<Value> value;

  void trace(JSTracer* tracer);
};

/**
 * Create a new, unlinked module record with the given exports.
 *
 * This can be used, for example, to create a "native" module that exports a
 * bunch of |JSNative| functions, and for which there is no associated JS source
 * text.
 */
extern JS_PUBLIC_API JSObject* CreateModule(
    JSContext* cx, Handle<GCVector<ModuleExport>> exports);

Comment 4

[Ted Campbell [:tcampbell]]

We could probably re-use the IdValuePairVector. I think that is the API we want.

Will need new helpers to construct the Shape, EnvironmentObject, and ModuleObject. The shape is probably made with something similar to js::CreateEnvironmentShape. The various flags, classes come from ModuleEnvironmentObject. Probably makes sense to support ModuleObject not needing a top-level JSScript at all.

Comment 5

[fitzgen]

(In reply to Ted Campbell [:tcampbell] from comment #4)

We could probably re-use the IdValuePairVector. I think that is the API we want.

I forget, is js/src/ds accessible to embedders or is it SM internal only?

Comment 6

[Ted Campbell [:tcampbell]]

No, we'd need to move those contents to js/public if it is useful

Comment 7

[fitzgen]

Thanks for the tips! Digging in.

Comment 8

[fitzgen]

Attachment: Bug 1722802 - Part 0: Expose `IdValuePair` to JSAPI embedders; r=tcampbell

Comment 9

[fitzgen]

Attachment: Bug 1722802 - Part 1: Add the `JS::GetModuleExport` JSAPI function; r=jonco

Depends on D121439

Comment 10

[fitzgen]

Attachment: Bug 1722802 - Part 2: WIP Add the `JS::CreateModule` JSAPI function; r=jonco

Depends on D121440

Comment 11

[fitzgen]

Attachment: Bug 1722802 - Part 3: Add a JSAPI test for creating modules; r=jonco

Depends on D121441

Comment 12

[fitzgen]

Attachment: Bug 1722802 - Part 0: Expose `IdValuePair` to JSAPI embedders; r=tcampbell

Comment 13

[fitzgen]

Attachment: Bug 1722802 - Part 1: Add the `JS::GetModuleExport` JSAPI function; r=jonco

Depends on D121443

Comment 14

[fitzgen]

Attachment: Bug 1722802 - Part 2: Add the `JS::CreateModule` JSAPI function; r=tcampbell

Depends on D121444

Comment 15

[fitzgen]

Attachment: Bug 1722802 - Part 3: Add a JSAPI test for creating modules; r=jonco

Depends on D121445

Comment 16

[fitzgen]

(Apologies for the phabricator noise, the CLI failed the first time I ran it, but I guess it still managed to upload some patches unbeknownst to me. First time using phabricator! Not used to this shiny new stuff, always liked splinter :-p)

Comment 17

[fitzgen]

Okay so I think I have this almost working.

As @tcampbell suggested in chat, I am stubbing out a CompilationStencil for the module and manually adding bindings for the given exports to the stencil. Then I use js::frontend::InstantiateModuleStencil to create the module object. If I understand correctly, this will result in a module object with the correct environment (and namespace? or is that added later, during linking?) that I need.

What I don't know how to do is to stuff the export values into the module's environment object at the right locations, now that I have an environment object with the right shape and slots. Would appreciate some help with this.

Also, I'm getting some assertion failures related to the [Shared]ImmutableScriptData that I am stubbing out for this stencil. Specifically, something ends up trying to index into an empty mozilla::Span and I'm not sure why it is trying to do that or how to get it to stop:

Assertion failure: idx < storage_.size(), at /home/nick/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Span.h:713
[New Thread 1523583.1523585]
[New Thread 1523583.1523586]

Thread 1 received signal SIGSEGV, Segmentation fault.
0x000055ca526a16d3 in mozilla::Span<JS::GCCellPtr const, 18446744073709551615ul>::operator[] (this=0x7ffcaccfaf10, idx=0)
    at /home/nick/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Span.h:713
713	    MOZ_RELEASE_ASSERT(idx < storage_.size());
(rr) bt
#0  0x000055ca526a16d3 in mozilla::Span<JS::GCCellPtr const, 18446744073709551615ul>::operator[] (this=0x7ffcaccfaf10, idx=0)
    at /home/nick/mozilla-central/obj-x86_64-pc-linux-gnu/dist/include/mozilla/Span.h:713
#1  0x000055ca526e1af2 in JSScript::getScope (this=0x2c4b3c045060, index=...) at /home/nick/mozilla-central/js/src/vm/JSScript.h:2116
#2  0x000055ca528b43e8 in JSScript::outermostScope (this=0x2c4b3c045060) at /home/nick/mozilla-central/js/src/vm/JSScript.h:1976
#3  0x000055ca530cf7ba in InstantiateTopLevel (cx=0x7f5dc9e24000, input=..., stencil=..., gcOutput=...)
    at /home/nick/mozilla-central/js/src/frontend/Stencil.cpp:1176
#4  0x000055ca530cda31 in js::frontend::CompilationStencil::instantiateStencilAfterPreparation (cx=0x7f5dc9e24000, input=..., stencil=..., 
    gcOutput=...) at /home/nick/mozilla-central/js/src/frontend/Stencil.cpp:1426
#5  0x000055ca530cd4e3 in js::frontend::CompilationStencil::instantiateStencils (cx=0x7f5dc9e24000, input=..., stencil=..., gcOutput=...)
    at /home/nick/mozilla-central/js/src/frontend/Stencil.cpp:1356
#6  0x000055ca5300ae9f in js::frontend::InstantiateStencils (cx=0x7f5dc9e24000, input=..., stencil=..., gcOutput=...)
    at /home/nick/mozilla-central/js/src/frontend/BytecodeCompiler.cpp:388
#7  0x000055ca52ae7cf7 in JS::CreateModule (cx=0x7f5dc9e24000, exports=...) at /home/nick/mozilla-central/js/src/vm/Modules.cpp:372
#8  0x000055ca524746ff in cls_testCreateModule::run (this=0x55ca53d2c430 <cls_testCreateModule_instance>, global=...)
    at /home/nick/mozilla-central/js/src/jsapi-tests/testCreateModule.cpp:57
#9  0x000055ca52459bfb in main (argc=2, argv=0x7ffcaccfc448) at /home/nick/mozilla-central/js/src/jsapi-tests/tests.cpp:168
(rr) 

I would appreciate some help figuring out how to solve this issue as well.

This is all part of the "part 2" patch.

I also ended up adding a new JSAPI for grabbing exports off of a module object (part 1), which was necessary so that I could write the JSAPI test (part 3).

I'll also probably end up adding a new shell builtin to reflect JS::CreateModule so I can write integration tests to check that these modules can be put into a module registry, returned for a module resolve hook, and then linked into a an actual module graph. Not going to bother with this until I have the existing JSAPI test passing though.

Comment 18

[fitzgen]

Attachment: Bug 1722802 - Part 4: Add a JSAPI test for getting module exports; r=jonco

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:sdetar, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 20

[Wesley W. Garland]

What's the status on this bug? It basically describes what my team needs for PythonMonkey's ESM<-Python milestone. I could put some manpower on this, either Q3 or Q4 this year if it has a good chance of landing? :tcampbell, :jonco, does it seem likely you'll have r? bandwidth?

:fitzgen, you cool if we pick this up?

Is it related at all to the work in bug 1426690?

Comment 21

[Jon Coppeard (:jonco)]

I think the status of this bug is that it is abandoned. The work sounds like a duplicate of 1426690 but also duplicates of some of the work in bug 1670176 to create synthetic modules.

Wesley, the idea here is that you would call these APIs in your module loader to create a module to return, right?