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

LibJS+LibWeb: Implement WebAssembly Native Errors #15248

Closed

Conversation

networkException
Copy link
Member

This pull request adds the CompileError, RuntimeError and LinkError native errors and exposes them on the WebAssembly object.

Initially I tried defining the errors outside LibJS, this however is non trivial as both we and the spec rely (?) on intrinsics knowing about them.

This patch adds the WebAssembly CompileError, RuntimeError and LinkError
who are defined to be native errors like TypeError for example and
exposes them on the WebAssembly object.
Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

There's no reason whatsoever for these errors to live in LibJS. In fact, if anything that doesn't have a spec on tc39.es needs to exist in LibJS, that's an architectural issue we need to solve. (console excluded, and there's one HTML spec data member somewhere and that's already annoying)

Here's the spec you'll want to look at and implement in LibWeb: https://webassembly.github.io/spec/js-api/#error-objects

@networkException
Copy link
Member Author

Ah I wasn't aware of the strict spec isolation (but it makes a whole lot of sense obviously). Sadly I don't really know where to begin with changing the architecture to support non LibJS native errors.

@alimpfard
Copy link
Member

alimpfard commented Sep 21, 2022

You can look at how the various DOMExceptions are currently implemented, these basically follow the same concept.

looks like they're just PlatformObjects now: https://github.com/SerenityOS/serenity/blob/master/Userland/Libraries/LibWeb/DOM/DOMException.h

@linusg
Copy link
Member

linusg commented Sep 21, 2022

These are different though and explicitly not platform objects, only things defined via WebIDL are.

They implement the NativeError object structure from ES, as described in the spec I linked. You might be able to use the existing DECLARE_NATIVE_ERROR macros for this.

@networkException
Copy link
Member Author

Right, I tried using DECLARE_NATIVE_ERROR{,_CONSTRUCTOR,_PROTOTYPE} initially.

However implementing ConstructorName::construct requires ordinary_create_from_constructor which isn't really designed to take host specific prototypes:

  • The spec defines "Assert: intrinsicDefaultProto is this specification's name of an intrinsic object" as a step and

  • we require a member of Intrinsics to be passed.

@stale
Copy link

stale bot commented Oct 14, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Oct 14, 2022
@linusg linusg added invalid This doesn't seem right ⛔️ pr-is-blocked PR is blocked by something outside of the author's control, protected from stalebot and removed stale invalid This doesn't seem right labels Oct 14, 2022
@stale
Copy link

stale bot commented Nov 5, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Nov 5, 2022
@ADKaster ADKaster removed the stale label Nov 6, 2022
@stale
Copy link

stale bot commented Nov 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Nov 27, 2022
@linusg linusg removed the stale label Nov 27, 2022
@AtkinsSJ AtkinsSJ added the ⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased label Dec 6, 2022
@AtkinsSJ
Copy link
Member

AtkinsSJ commented Dec 6, 2022

This has conflicts.

@stale
Copy link

stale bot commented Dec 27, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Dec 27, 2022
@stale
Copy link

stale bot commented Jan 3, 2023

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

@stale stale bot closed this Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ pr-has-conflicts PR has merge conflicts and needs to be rebased ⛔️ pr-is-blocked PR is blocked by something outside of the author's control, protected from stalebot stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants