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][test] Refine DefaultValue for new reference types #522

Merged
merged 4 commits into from
Feb 16, 2024

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented Feb 15, 2024

This PR addresses issue #501 and adjust DefaultValue in the JS API so that it delegates to a embedder API function in the core spec, with a special case for externref to use undefined.

Note that for anyref and other non-externref types it will use null as the default value. For non-null types, it produces error and that is thrown as a TypeError for WebAssembly.Table.prototype.set and WebAssembly.Table.prototype.grow.

Rendered spec change for the core spec:

image

Rendered change for DefaultValue (there are changes to use sites too, not shown):

image

The PR also adds a new WPT test for the error cases specifically. Both Firefox and Chrome passed the test when I tried it out.

DefaultValue will return undefined for externref as before, but
for other nullable reference types will return null.

For non-nullable reference types, operations like grow and set
should error if the value to set is missing.

Also adds WPT tests for the error cases. This requires some
updates to wasm-module-builder.js as well.

Fixes issue WebAssembly#501
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Excellent, thanks!

document/core/appendix/embedding.rst Outdated Show resolved Hide resolved
document/core/appendix/embedding.rst Outdated Show resolved Hide resolved
@takikawa
Copy link
Contributor Author

Thanks for the suggestions! Should be updated now:

image
image

Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks!

document/core/appendix/embedding.rst Outdated Show resolved Hide resolved
@rossberg rossberg merged commit 322e948 into WebAssembly:main Feb 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants