search

WebAssembly / gc

Branch of the spec repo scoped to discussion of GC integration in WebAssembly
https://webassembly.github.io/gc/
Other
982 stars 70 forks source link

[js-api] add tests for global imports with GC types #498

Open takikawa opened 8 months ago

takikawa commented 8 months ago

This PR adds tests for the update to the JS API made in https://github.com/WebAssembly/gc/pull/467 to enable global imports of various reftype values.

These tests pass in V8 and there's a pending patch for JSC. I'm looking into the status for Spidermonkey.

There is one open question with these tests, which is that all the error cases are assumed to throw a WebAssembly.LinkError. However, I believe the current spec actually mandates throwing TypeError instead. Which behavior would we prefer here?

(note: V8 & Spidermonkey currently disagree on the error type to throw)

takikawa commented 8 months ago

Update: I ran the tests in Firefox/SM as well and they all pass if I change WebAssembly.LinkError to TypeError.

eqrion commented 8 months ago

There is one open question with these tests, which is that all the error cases are assumed to throw a WebAssembly.LinkError. However, I believe the current spec actually mandates throwing TypeError instead. Which behavior would we prefer here?

(note: V8 & Spidermonkey currently disagree on the error type to throw)

The issue here (IIUC) is that 'read the imports' generally tries to throw LinkError, but it can delegate to ToWebAssemblyValue which can throw TypeError for GC types if match_valtype fails?

Digging a little deeper, it seems like we have a pattern in 'read the imports' of catching every case that could cause ToWebAssemblyValue to fail and eagerly throwing a LinkError. So e.g. we check for BigInt for i64 to ensure that ToWebAssemblyValue won't fail to convert. I think we could amend the spec to hoist the match_valtype check out into read the imports' and match that eager throwing of LinkError behavior.

rossberg commented 6 months ago

Sorry, I had somehow missed this PR before the holidays.

I agree that it looks like the original intention was that all failures in linking throw LinkError. Perhaps the most modular fix to the spec is to catch a TypeError from ToWebAssemblyValue and rethrow it as LinkError?