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

Reconsider usage of bigint in generated JS code #4246

Open
RunDevelopment opened this issue Nov 3, 2024 · 6 comments
Open

Reconsider usage of bigint in generated JS code #4246

RunDevelopment opened this issue Nov 3, 2024 · 6 comments
Labels

Comments

@RunDevelopment
Copy link
Contributor

Summary

#3164 set the precedent that WBG should not emit bigint literals in generated JS code, and I wanted to ask whether this could be reconsidered and #3164 be reverted.

I'm asking for this, because (1) this is a blocker for #4222 in its current form, and (2) #3164 didn't actually solve anything.

What #3164 tried to achieve

The motivation behind #3164 was to solve a specific esbuild error. Here's the error:

[vite:esbuild-transpile] Transform failed with 1 error:
assets/index.baa0f021.js:431:63: ERROR: Big integer literals are not available in the configured target environment ("chrome87", "edge88", "es2020", "firefox78", "safari13" + 2 overrides)

Big integer literals are not available in the configured target environment ("chrome87", "edge88", "es2020", "firefox78", "safari13" + 2 overrides)
429|          const v = getObject(arg1);
430|          const ret = typeof(v) === 'bigint' ? v : undefined;
431|          getBigInt64Memory0()[arg0 / 8 + 1] = isLikeNone(ret) ? 0n : ret;
   |                                                                 ^
432|          getInt32Memory0()[arg0 / 4 + 0] = !isLikeNone(ret);
433|      };

This error means that esbuild wasn't able to convert the generated JS code of WBG into JS code supported by all configured target environments. In particular, safari 13 doesn't support bigint. So esbuild correctly errors out, as it cannot fulfill the request to support bigints (specifically bigint literals) in safari 13.

The "solution" #3164 implemented was to replace the bigint literal 0n with BigInt(0), so the constant is created at runtime instead. BigInt(0) is simple JS and so the transpiler doesn't need to do anything and so won't error.

However, the approach taken in #3164 does nothing to fix the compatibility issue. Even BigInt(0) isn't supported in safari 13. The example function in the issue will still not work in safari 13. The only thing that changed is that this compatibility problem is no longer detected by esbuild. #3164 did not fix the underlying issue, it hid it. Even the other code example given in #3164 doesn't work on safari 13, it's just that esbuild didn't detect that and so didn't emit an error.

There's also no point in emitting BigInt(value) for older browsers, because it will always fail at runtime or (if polyfilled) be unusable. As the author of esbuild explains in this issue, polyfilling bigint is pretty much impossible. And even if you somehow hack together a spotty polyfill, it wouldn't help. The WebAssembly JS API uses bigint for i64, and a polyfill wouldn't be compatible with that.

So all #3164 did was to make esbuild misrepresent which target environments are actually supported.

IMO, users that encounter this error should either:

  1. Suppress the error.
  2. Don't use anything in their wasm API that requires bigint.
  3. Accept that they cannot support certain older browsers and adjust their target environment accordingly.

If users still insist on not using bigint literals, then I think they should use a transpiler for that. 64-bit integer types require bigint, and I don't think it's WBG's job to hide this fact.

Why this blocks support for u128 (#4222)

To support u128 with the ABI implemented in #4222 (pass one u128 as two u64), we need to perform bit-shifts on bigints. This operation is impossible to polyfill, so #4222 requires that bigint be supported.

However, #3164 sets the precedent that bigint doesn't need to be supported. The only ABI for u128 that would be compatible with a hypothetical bigint polyfill is the same ABI as js_sys::BitInt: a string of decimal digits. For performance reasons, I would like to avoid this.

ES version policy

Similar to a MSRV, it would probably be a good idea to have a similar policy for ES versions. Right now, I'm not sure which features I am allowed to use in generated JS code.

We extensively use ES2015 features, so I would assume that the minimum supported ES version is at least ES2015. However, we also use bigints which are ES2020, but we also apparently don't target ES2020 because of #3164. So what ES version do we target?

@daxpedda
Copy link
Collaborator

daxpedda commented Nov 8, 2024

I think the desire was to make an application only fail at runtime so if the usage of BigInt is avoided, during runtime, the rest of the application can run just fine. This is probably a quite questionable approach, but it has already been done this way.

Do we actually need BigInt literal support? Can't we continue to use constructors? Eventually we can move on to literals with the next breaking change, unless we find good reasoning for the argument above.

ES version policy

Similar to a MSRV, it would probably be a good idea to have a similar policy for ES versions. Right now, I'm not sure which features I am allowed to use in generated JS code.

We extensively use ES2015 features, so I would assume that the minimum supported ES version is at least ES2015. However, we also use bigints which are ES2020, but we also apparently don't target ES2020 because of #3164. So what ES version do we target?

I agree that a policy around this should be established. Unfortunately I have no idea where we stand or what we should do about this. A more informed opinion from somebody actually knowledgeable about JS/TS and the ecosystem would be much appreciated.

@RunDevelopment
Copy link
Contributor Author

I think the desire was to make an application only fail at runtime so if the usage of BigInt is avoided, during runtime, the rest of the application can run just fine.

That might be the case. I did some digging to find how how Safari 13 even supports the WebAssembly JS API when it doesn't support bigint. The answer is that an old version of the JS API spec (wayback) just doesn't support i64. If you try calling a WASM function that uses i64 anyway, you get a runtime error. The release notes of Safari 14.1 (the version that added bigint) also mention something in that regard.

Note that this only affects the JS API of WebAssembly. You can use i64 internally just fine and even import/export functions that use it. You just can't call any function that use i64.

This is probably a quite questionable approach, but it has already been done this way.

On a related note, this also means that changing the WASM ABI of any type to use i64/u64 is a breaking change. Good thing we went with f64 in #4183...

I agree that a policy around this should be established. Unfortunately I have no idea where we stand or what we should do about this.

Maybe we could start by documenting which browser versions are supported? The browser support page doesn't say. Going back to safari, they added wasm support in Safari 11 (released Sep 2017), but thanks to sign-ext being a default feature, WBG mostly just supports Safari >=14.1 (released Apr 2021). Recently, Rust enabled multivalue and reference-types by default too, and reference-types are supported started Safari 15 (released Sep 2021).

Luckily, there's this handy page, so we "just" have to pick out what's relevant to WBG. Knowing exactly what browsers we currently support would probably also be good to know to inform such a policy.

@danielhjacobs
Copy link

reference-types are supported started Safari 15

Not trying to hijack this issue, but worth noting despite claimed support Safari 15 has issues with reference-types, at least as it relates to using the wgpu backend of Ruffle, which results in this error message:

RuntimeError: Out of bounds table access (evaluating 'b._dyn_core__ops__function__FnMut__A____Output___R_as_wasm_bindgen__closure__WasmClosure___describe__invoke__h281f2f93d3d6cf28(n,e,t)')

@daxpedda
Copy link
Collaborator

daxpedda commented Nov 9, 2024

I think the desire was to make an application only fail at runtime so if the usage of BigInt is avoided, during runtime, the rest of the application can run just fine.

That might be the case. I did some digging to find how how Safari 13 even supports the WebAssembly JS API when it doesn't support bigint. The answer is that an old version of the JS API spec (wayback) just doesn't support i64. If you try calling a WASM function that uses i64 anyway, you get a runtime error. The release notes of Safari 14.1 (the version that added bigint) also mention something in that regard.

Note that this only affects the JS API of WebAssembly. You can use i64 internally just fine and even import/export functions that use it. You just can't call any function that use i64.

This isn't part of Wasm v1.0, its a proposal called "JS BigInt to Wasm i64 Integration". You can find it on the Wasm feature page as well.

This is probably a quite questionable approach, but it has already been done this way.

On a related note, this also means that changing the WASM ABI of any type to use i64/u64 is a breaking change. Good thing we went with f64 in #4183...

Uff, true!

I agree that a policy around this should be established. Unfortunately I have no idea where we stand or what we should do about this.

Maybe we could start by documenting which browser versions are supported? The browser support page doesn't say. Going back to safari, they added wasm support in Safari 11 (released Sep 2017), but thanks to sign-ext being a default feature, WBG mostly just supports Safari >=14.1 (released Apr 2021). Recently, Rust enabled multivalue and reference-types by default too, and reference-types are supported started Safari 15 (released Sep 2021).

Luckily, there's this handy page, so we "just" have to pick out what's relevant to WBG. Knowing exactly what browsers we currently support would probably also be good to know to inform such a policy.

We shouldn't confuse wasm-bindgen with Rust though, we do have an MSRV of v1.57 after all. Not to mention the new wasmv1-none target should alleviate most of these issues (when it comes out in Rust v1.84).

reference-types are supported started Safari 15

Not trying to hijack this issue, but worth noting despite claimed support Safari 15 has issues with reference-types, at least as it relates to using the wgpu backend of Ruffle, which results in this error message:

RuntimeError: Out of bounds table access (evaluating 'b._dyn_core__ops__function__FnMut__A____Output___R_as_wasm_bindgen__closure__WasmClosure___describe__invoke__h281f2f93d3d6cf28(n,e,t)')

Indeed, Safari's v15 implementation of the reference type proposal was faulty (don't remember the details), we should pretend that support was added in Safari v16.

@RunDevelopment
Copy link
Contributor Author

Okay, so taking this all together: For documentation, we need one table that explains which stable Rust versions with the target wasm32-unknown-unknown map to which features being enabled by default and another for the browser support of each (important) target feature. Did I get that right?

We might have to amend the Rust table in the future for other target and nightly tricks, but at least the target feature browser support table should be rather unchanging.

This isn't part of Wasm v1.0, its a proposal called "JS BigInt to Wasm i64 Integration". You can find it on the Wasm feature page as well.

I think proposals that are purely about the JS API raise an interesting issue regarding browser support. What #3164 established is essentially partial browser support.

From my understanding, things like target feature (e.g. simd128) are a lot more cut and dry. AFAIK, if you try to WebAssembly.compile a WASM file with unsupported instructions, compilation just fails, meaning that the JS fail fails to load properly. So if you use it, your whole package doesn't support the browser/runtime anymore. It's all or nothing.

The question is: should this "all or nothing" mantra for supporting certain browser versions apply to all WASM features, or should we support partial browser support?

On that note: are there other JS-API-focused WASM features for which partial support would also apply? I quickly scanned through the list of features, but couldn't find anything that sounded like it.

@daxpedda
Copy link
Collaborator

Okay, so taking this all together: For documentation, we need one table that explains which stable Rust versions with the target wasm32-unknown-unknown map to which features being enabled by default and another for the browser support of each (important) target feature. Did I get that right?

We might have to amend the Rust table in the future for other target and nightly tricks, but at least the target feature browser support table should be rather unchanging.

What should actually be required of us is to specify which browsers we support considering no target features are enabled and note that we don't control what target features are enabled by the Rust compiler and it does already enable some target features by default.

Anything more, including what you wrote, would be very appreciated, but isn't absolutely necessary.

The question is: should this "all or nothing" mantra for supporting certain browser versions apply to all WASM features, or should we support partial browser support?

I would like to see some substantial use-case for partial browser support, otherwise I remain unconvinced. But its already here, so we can only change it with the next breaking change.

There are some cases where this is required however, e.g. WBG does some TextDe/Encoder shenanigans to support Strings, but it has some built-in detection where it fails during runtime if TextDe/Encoder is not available. Mainly this is done to support Worklets. There might be more things like this in the future.

On that note: are there other JS-API-focused WASM features for which partial support would also apply? I quickly scanned through the list of features, but couldn't find anything that sounded like it.

I'm not aware of one.

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

No branches or pull requests

3 participants