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

[js-api] Subclassing of WebAssembly classes #1107

Open
RReverser opened this issue Dec 3, 2019 · 11 comments
Open

[js-api] Subclassing of WebAssembly classes #1107

RReverser opened this issue Dec 3, 2019 · 11 comments

Comments

@RReverser
Copy link

@RReverser RReverser commented Dec 3, 2019

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:

  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.
@binji
Copy link
Member

@binji binji commented Jan 14, 2020

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

@Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Apr 14, 2020

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

@RReverser RReverser commented Apr 14, 2020

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

@RReverser RReverser commented Apr 15, 2020

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

@backes
Copy link
Member

@backes backes commented Apr 15, 2020

I don't know much about Web IDL, and what exactly to test here. @Ms2ger, do you want to take this?

@lars-t-hansen
Copy link

@lars-t-hansen lars-t-hansen commented Apr 16, 2020

There's a simple test case attached to the pending patch at https://bugzilla.mozilla.org/show_bug.cgi?id=1629998

@RReverser
Copy link
Author

@RReverser RReverser commented Apr 17, 2020

@lars-t-hansen That test looks simple yet robust enough. Would Mozilla want to upstream it to spec tests?

@lars-t-hansen
Copy link

@lars-t-hansen lars-t-hansen commented Apr 20, 2020

@RReverser, i'll look into it.

@eqrion
Copy link
Contributor

@eqrion eqrion commented May 5, 2020

@RReverser That test case was upstreamed to WPT [1]

[1] web-platform-tests/wpt#23402

@RReverser
Copy link
Author

@RReverser RReverser commented May 5, 2020

@eqrion Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.