Skip to content
This repository has been archived by the owner on Nov 3, 2021. It is now read-only.

[js-api] WebAssembly.Table.prototype.grow and a fill value #22

Closed
lars-t-hansen opened this issue Nov 30, 2018 · 11 comments
Closed

[js-api] WebAssembly.Table.prototype.grow and a fill value #22

lars-t-hansen opened this issue Nov 30, 2018 · 11 comments
Assignees

Comments

@lars-t-hansen
Copy link
Contributor

The envisioned table.grow instruction takes a required fill value as a second argument. We should similarly change WebAssembly.Table.prototype.grow to accept a fill value, indeed for non-nullable element types we must have a fill value. Here are some notes about that.

Backward compatibility concerns when the table is table-of-anyfunc:

  • the second argument must be optional
  • the default value of the optional second argument must be null
  • if the second argument is present but is not a function value that can be stored, then it must be ignored

For table-of-anyref it is probably most correct if the default value for the optional second argument is null, not undefined, since null is a value that is in wasm, unlike undefined. (This matters only because undefined is representable as an anyref value and is thus a candidate at least in principle.) FWIW, the table.grow instruction cannot talk about undefined values without getting them from the host, but it can synthesize null. A JS caller to WebAssembly.Table.prototype.grow() can pass an undefined value explicitly as the second argument to initialize new slots with that value.

Long-term it's inevitable that the behavior of W.T.p.grow depends on the table type anyway; for non-nullable element types there can be no default, for example. We could choose now to require the second argument also for anyref tables, we would just have to leave anyfunc tables as a special case.

@lars-t-hansen lars-t-hansen changed the title WebAssembly.Table.prototype.grow and an initializer argument WebAssembly.Table.prototype.grow and a fill value Nov 30, 2018
@lars-t-hansen
Copy link
Contributor Author

I guess accepting a second argument here for table-of-anyfunc is technically incompatible with existing code that does pass an exported wasm function as the second argument, since that argument would previously be ignored but now will not be.

@rossberg
Copy link
Member

Agreed with this proposal. For non-nullable table types leaving of the default should cause a TypeError, of course, but the simplest way to achieve that probably is to define null as the default for all reference types in the API. Then the later check will throw the error anyway.

The ES spec has added additional arguments to some of its functions in the past, so we should be okay doing the same.

@lukewagner
Copy link
Member

Just to confirm: do we all agree it's ok to break backwards-compatibility by changing the case where a non-exported-wasm-function JS value is passed as the second argument to Table.prototype.grow from silently ignoring to throwing?

I ask because @Ms2ger judiciously wrote a wpt for exactly this case that fails if we make this breaking change, but I expect (and we can verify in FF by immediately landing a throw-if-passing-any-second-argument patch ahead of full bulk-operations) that this breaking change won't actually break web content. If the assumption is correct, that means we'd change the wpt to test for the throwing behavior.

Make sense to everyone? [cc @Ms2ger @littledan ]

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 18, 2018

It makes sense to me to throw on any invalid values here. Please do update the test, and add one for the three-argument case :)

@lars-t-hansen lars-t-hansen changed the title WebAssembly.Table.prototype.grow and a fill value [js-api] WebAssembly.Table.prototype.grow and a fill value Nov 15, 2019
@rossberg
Copy link
Member

It's in the embedder interface, but the JS API still needs to add the init argument.

@littledan, would you be willing to look into this?

@wingo
Copy link
Contributor

wingo commented Jan 17, 2020

I'll take it if that works for y'all. Am out of the office next week but will pick it up the week after.

@rossberg
Copy link
Member

Thanks @wingo. By any chance, would you be willing to also look into #51 and #67? Those seem to be the other open issues around the JS API. Both of them should be fairly easy to fix.

@rossberg
Copy link
Member

rossberg commented Feb 8, 2020

@wingo, any update on this? Would be good to have this fixed before the meeting on Tuesday.

@wingo
Copy link
Contributor

wingo commented Feb 10, 2020

Hi, sorry for the delay; didn't realize how much travel would discombobulate me :P No update currently, but I will switch to this task to see if I can finish by tomorrow.

Ms2ger added a commit to Ms2ger/reference-types that referenced this issue Feb 11, 2020
Ms2ger added a commit to Ms2ger/reference-types that referenced this issue Feb 18, 2020
Ms2ger added a commit to Ms2ger/reference-types that referenced this issue Feb 18, 2020
@lars-t-hansen
Copy link
Contributor Author

@wingo, any update?

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 25, 2020

@lars-t-hansen see #79

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants