Rename anyref to externref to match proposal change

[jayphelps]

Note: this is a breaking change, but only for those who have opted-in to the reference-types experimental feature. It and anyref usage are also undocumented AFAIK.

The reference type proposal recently had some changes, including the removal of all subtyping of reference types for now, as well as renaming anyref to externref so that a future type that actually supports subtyping (a real anyref) could be added back later--maybe.

I wanted to open this PR as a starting point for the work, but also because I believe some dialog needs to be had because this change impacts more than just renaming thing for AS and there is not yet a PR in binaryen to change those things. For example, now ref.null requires an immediate to know whether it's a null for func or null for extern but this is not yet done in Binaryen.

I thought it might be important to discuss this ASAP since the proposal ship seems to be sailing to stage 4 soon.

• type checking assumes subtyping exists in several places e.g. truthy
• makeZero(), for initialization assumes subtyping
• tests assume subtyping.

Proposal update: WebAssembly/reference-types#87
Binaryen update: WebAssembly/binaryen#2900
⯈
⯈
⯈

• I've read the contributing guidelines

[jayphelps]

@torch2424 not sure of the process to add things to the agenda. Can this get added to one of the upcoming meetings?

[torch2424]

@jayphelps Oops! Just saw this, it slipped through my github notifications.

I saw you commented on the weekly meeting, that's totally how you add something to the agenda! Looking forward to discussing this then! 😄 🎉

[jayphelps]

Thanks!

[dcodeIO]

As discussed at the last WG meeting, I decided to go ahead and raise some concerns here: WebAssembly/reference-types#61 (comment)

Turns out that I misunderstood the implications there. With externref being the new anyref, just not callable, I don't see hard issues with the change. One can still call it, but would do it through a JS helper, which seems fine to get reference-types out the door in the first place. Sorry for the noise, not operating at a 100% currently.

[MaxGraey]

@jayphelps could we move forward and finish started changes in this PR? Currently we can't update to latest binaryen without this changes)

[jayphelps]

The delay was because Binaryen had not yet adapted the rest of the changes to the proposal, which I believe impact AS too. We could update just the name, anyref to externref, but we would then be relying on part of the old version of r proposal and part of the new version intermixed, I think. Maybe that’s not a big deal, for now? After all, it’s pretty unclear if anyone else at all is using anyref in AS right now and if they are, I don’t think they’ve been vocal. Thoughts?

I haven’t checked Binaryen yet to see if someone else had updated the rest of the proposal—I did the initial changes, but I don’t have cycles to prioritize updating Binaryen for the rest, right now at least.

[dcodeIO]

Given that we are blocked on this, and that reftypes is an experimental feature anyway, just renaming for now to reflect the state in Binaryen and looking into everything else later sounds reasonable to me.

[jayphelps]

Okiedoke, I’ll move forward. Looks like we might have some test issues, looking into it.

…

On Jul 11, 2020, at 8:49 PM, Daniel Wirtz ***@***.***> wrote:  Given that we are blocked on this, and that reftypes is an experimental feature anyway, just renaming for now to reflect the state in Binaryen and looking into everything else later sounds reasonable to me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[dcodeIO]

Testing with https://nodejs.org/download/v8-canary/v15.0.0-v8-canary202006192e28363093/ (latest with Windows binaries), the experimental wasm flags appear to be

  --experimental-wasm-eh (enable prototype exception handling opcodes for wasm)
        type: bool  default: false
  --experimental-wasm-simd (enable prototype SIMD opcodes for wasm)
        type: bool  default: false
  --experimental-wasm-return-call (enable prototype return call opcodes for wasm)
        type: bool  default: false
  --experimental-wasm-compilation-hints (enable prototype compilation hints section for wasm)
        type: bool  default: false
  --experimental-wasm-gc (enable prototype garbage collection for wasm)
        type: bool  default: false
  --experimental-wasm-typed-funcref (enable prototype typed function references for wasm)
        type: bool  default: false
  --experimental-wasm-reftypes (enable prototype reference type opcodes for wasm)
        type: bool  default: false
  --experimental-wasm-bigint (enable prototype JS BigInt support for wasm)
        type: bool  default: false
  --experimental-wasm-mv (enable prototype multi-value support for wasm)
        type: bool  default: false
  --experimental-wasm-threads (enable prototype thread opcodes for wasm)
        type: bool  default: false
  --experimental-wasm-type-reflection (enable prototype wasm type reflection in JS for wasm)
        type: bool  default: false
  --experimental-wasm-bulk-memory (enable prototype bulk memory opcodes for wasm)
        type: bool  default: true

The error seen in the CI log is

Error: unrecognized flag --experimental-wasm-externref
Try --help for options
- compile untouched
  SUCCESS (154.718 ms)

- compare fixture
  SUCCESS (0.939 ms)

- compile optimized
  SUCCESS (368.300 ms)

- instantiate untouched
  [call preInstantiate]
  ---
  CompileError: WebAssembly.Module(): invalid local type @+16

so I guess that'd have to be --experimental-wasm-reftypes in tests/features.json instead?

[jayphelps]

How did you get it to change the node V8 version? I changed the test.yml file but it didn’t seem to change which version the test ran with, according to the logs.

[dcodeIO]

Hmm, the change to test.yml looks correct to me. Perhaps CI got confused somehow and installed v14 from master's test.yml, or there is something wrong with that exact version (maybe try v15.0.0-v8-canary202006192e28363093). But also just guessing.

[jayphelps]

Damn. I didn’t think about the fact that, while Binaryen and AS both just renamed it to externref, V8 actually implemented all the other changes around removing subtyping/null stuff. That means as-is this PR can’t be merged.

Options I can think of:

• Remove anyref/externref support entirely for now
• Temporarily remove the ability to support null for variables of type externref
• Remove the test code that relies on null, meaning the PR will pass but if someone actually uses null for externref’s in V8 it will fail validation because it’s technically malformed.
• Someone updates Binaryen for the other ref types changes, then updates AS
• We wait on this PR and updating Binaryen for other reasons won’t be possible.

I’m inclined to say if updating Binaryen now is needed for other reasons, anyref support should just be removed for now. It was experimental, and this PR would have been a breaking change anyway.

Thoughts?

[MaxGraey]

Personally I'm ok with:

Remove the test code that relies on null, meaning the PR will pass but if someone actually uses null for externref’s in V8 it will fail validation because it’s technically malformed.

but with extra FIXME comments

cc @dcodeIO

[dcodeIO]

Commenting out the conflicting tests with a TODO sounds good to me. Perhaps we should also comment out codegen related to ref.null and ref.is_null, and make these Not implemented error for now so there are at least no validation failure surprises included in emitted binaries until we know more?

[jayphelps]

Ok I changed all null ref stuff to be no longer implemented, and commented out the related tests. All green, ready for final review I believe.

[dcodeIO]

Thanks for looking into this! :)

[github-actions]

🎉 This PR is included in version 0.14.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀