[js-api] Subclassing of WebAssembly classes

RReverser commented 5 years ago

In ES6 / ES2015, a lot of work and attention went into making sure that native classes can be subclasses in the same way as user-provided ones, to the point that it's now commonly expected for all native APIs (whether they come from ECMAScript, DOM or other standards).

Currently, WebAssembly.Module, WebAssembly.Instance and others break this expectation by always returning instance of the built-in class.

Arguably, this might not be a frequent usecase, but I had at least one case where I wanted to subclass built-in API and got hit by this. As a workaround, I had to resort to Object.setPrototypeOf hack to get instance of a correct class: https://github.com/GoogleChromeLabs/asyncify/blob/9e4a95d5cf324be3cdb6120c497b0e3ab31a0072/asyncify.mjs#L172

Either way, even if this is not a common occurence, it seems always worth returning instance of a class that constructor was invoked with, at least for consistency with other native APIs.

Open question: what to do with static APIs like WebAssembly.compile{Streaming} / WebAssembly.instantiate{Streaming}?

Unfortunately, these don't live on corresponding classes, so they can't guess which target class user wanted.

There are few options:

Add their duplicates to the corresponding classes as WebAssembly.Module.compile{Streaming} / WebAssembly.Instance.instantiate{Streaming}, so that they would always have access to specific context. Not pretty, but would integrate more naturally with subclassing.
Pass target class(es) in an extra param (I believe there were discussions about adding it for some other runtime options).
For now, don't worry about it, and just add basic subclassing in constructor, while for static APIs libraries will have to continue using setPrototypeOf for wrapping resulting instances.

binji commented 4 years ago

Seems like a relatively minor change to the JS API, may be worth bringing up at a future CG meeting.

Ms2ger commented 4 years ago

Currently, WebAssembly.Module, WebAssembly.Instance and others break this expectation by always returning instance of the built-in class.

Not from the constructors, they don't. See the steps in https://heycam.github.io/webidl/#create-an-interface-object and https://heycam.github.io/webidl/#internally-create-a-new-object-implementing-the-interface in particular. I wouldn't be surprised if there were implementation bugs, of course. I'll make a note to add tests at some point.

Open question: what to do with static APIs like WebAssembly.compile{Streaming} / WebAssembly.instantiate{Streaming}?

Unfortunately, these don't live on corresponding classes, so they can't guess which target class user wanted.

There are few options:

1. Add their duplicates to the corresponding classes as `WebAssembly.Module.compile{Streaming}` / `WebAssembly.Instance.instantiate{Streaming}`, so that they would always have access to specific context. Not pretty, but would integrate more naturally with subclassing.

2. Pass target class(es) in an extra param (I believe there were discussions about adding it for some other runtime options).

3. For now, don't worry about it, and just add basic subclassing in constructor, while for static APIs libraries will have to continue using `setPrototypeOf` for wrapping resulting instances.

I don't have a strong opinion on this yet.

RReverser commented 4 years ago

Huh, I wanted to say that for some reason no engine implements this subclassing and thought that these WebIDL steps must be explicitly linked from WebAssembly JS API spec, but just checked on macOS, and it seems JSC gets this right.

As such, it appears that this is indeed implementation bugs in SpiderMonkey and V8 which I'll report now.

RReverser commented 4 years ago

Submitted.

Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=1070695 Firefox issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1629998

RReverser commented 4 years ago

@backes I don't suppose we need a separate issue to add tests; would you want to take care of a PR to add relevant spec tests and reference this issue?