WebAssembly / function-references

Proposal for Typed Function References
https://webassembly.github.io/function-references/
Other
101 stars 15 forks source link

Issues with the JavaScript spec tests #100

Closed jakobkummerow closed 1 year ago

jakobkummerow commented 1 year ago

I've run the current version of the JavaScript tests in this repo in V8, and encountered a couple of issues.

(1) The (generated?) function assert_return() has a bug in the "ref.null" case, where it treats actual as a scalar value, but it's actually an array (see other cases in the same switch for comparison). In other words, in the snippet:

case "ref.null":
        if (actual !== null) {
          throw new Error("Wasm null return value expected, got " + actual);
        };

both occurrences of actual should be actual[i]. This makes the following tests fail:

(2) There are a few tests that require global.get $non-imported-constant-global to be a "constant expression" that can be used to initialize other globals, but that's a change we made in the GC proposal, not here. (It's also not a big deal for any implementations that are planning to ship function-references and GC at the same time.) This affects the following tests:

(3) The test type-equivalence asserts that (type $t (func (result (ref $t)))) is an invalid type definition, i.e. a type cannot refer to itself. However, if I'm not mistaken, we have decided that standalone type definitions are interpreted as being in an implicit single-entry recgroup, which means that they can refer to themselves.

rossberg commented 1 year ago

Thanks for looking into this!

(1) That bug seems to have snuck in during some merge, fixed now.

(2) Good point, this should be rectified, either by moving the tests, or by moving the feature.

(3) That's working as intended: This proposal does not yet introduce recursive types (GC does), so can't allow that.

jakobkummerow commented 1 year ago

Re (3): Fair, but that does mean that the test will need changing (or deleting) once both funcref and GC are merged into the main spec.

rossberg commented 1 year ago

Yes, effectively, the downstream GC repo already does that.

rossberg commented 1 year ago

I removed the tests using non imported globals from this repo, they now only appear in the GC repo. Closing this issue.