Skip to content
This repository was archived by the owner on Jun 16, 2020. It is now read-only.

Allow passing non-bigint values to I64 parameters #12

Closed
gahaas opened this issue Nov 30, 2018 · 16 comments
Closed

Allow passing non-bigint values to I64 parameters #12

gahaas opened this issue Nov 30, 2018 · 16 comments

Comments

@gahaas
Copy link
Contributor

gahaas commented Nov 30, 2018

At the moment, ToWebAssemblyValue(value) calls ToBigInt64(value) if the expected type is I64. ToBigInt64 throws a TypeError if value is a Number.

This behavior looks unexpected and inconsistent to me, because for all other types, ToWebAssemblyValue works differently.

I'm not convinced that we have to couple wasm.I64 so tightly to js.BigInt. BigInt is needed to allow wasm.I64 to be passed precisely to JavaScript, and also to big numbers precisely from JavaScript to WebAssembly. However, this does not mean that it should not be possible to pass normal numbers to WebAssembly as I64 parameters.

One scenario that comes to my mind is the following: if I want to pass an object to WebAssembly as an I32 parameter, I just have to add the valueOf function to the object to return a number. For an I64 parameter, however, I would have to add a toString function and avoid the valueOf function to avoid a TypeError. [edit] The valueOf function could also return a BigInt, but then it cannot be used for I32 parameters anymore.

@xtuc
Copy link
Collaborator

xtuc commented Nov 30, 2018

this does not mean that it should not be possible to pass normal numbers to WebAssembly as I64 parameters

In JavaScript a normal number is a float64 which allow up to 2**53 bits of value, which can't be used to represent properly a i64.

In WebAssembly there's no implicit type conversion, I would expect the exported/imported functions to behave the same.

I guess the ability to pass a float in ToBigInt64 has already been discussed in the JavaScript proposal, cc @littledan. I don't' want to close this PR to early but I feel like this is not the correct place to talk about that.

@gahaas
Copy link
Contributor Author

gahaas commented Nov 30, 2018

I accept that there are reasons why implicit conversions from numbers to BigInt are not the best solution in JavaScript in general. In this particular example, however, I think an implicit conversion are appropriate. We can also do that in the spec here by applying the BigInt constructor on the incoming value, i.e. use ToBigInt64(BigInt(|value|)) instead of ToBigInt64(|value|).

@titzer
Copy link

titzer commented Nov 30, 2018

I agree with @gahaas in that passing a JavaScript number into a WebAssembly function expecting an i64 would be a very common pitfall, but also doesn't have the same ergonomic problems of JS numbers and BigInt in general. E.g. We could have semantics where accepting integer-valued doubles in the range -2^53 to 2^53 is OK, and non-integral and larger values throw (though details TBD).

@xtuc
Copy link
Collaborator

xtuc commented Nov 30, 2018

I don't have a strong opinion about that. The interactions in JavaScript between BigInt and Number are mostly disallowed (example 1n + 1 throws). So I wouldn't expect both types to be compatible with int64.

Also in many cases the BigInt to i64 conversion is fast (on 64 bits just takes the first digit), while the conversion from float will probably be fast too it introduce a new bounds check.


What should be the type of a I64 return? If we follow the same login: if x > 2^53 ? BigInt(x) : x. It has the same issues I mentionned above IMO.

@titzer
Copy link

titzer commented Nov 30, 2018

I think it's fine to always return an I64.

@gahaas
Copy link
Contributor Author

gahaas commented Nov 30, 2018

As mentioned in #12 (comment), we could just implicitly convert a parameter to BigInt before passing it as an I64 parameter. The return value is always BigInt, because that's the only primitive type which can represent wasm.I64 bit-precisely.

@annevk
Copy link
Member

annevk commented Nov 30, 2018

FWIW, I think this kind of API decision ideally has TC39 sign-off. Coercing Numbers to BigInts in some places and not others will catch developers by surprise.

@mathiasbynens
Copy link

mathiasbynens commented Dec 3, 2018

Implicitly coercing Numbers into BigInts is a footgun. We shouldn't make it easier to make this mistake.

Developers can pass BigInt(number) instead of just number if this is what they really want (but that is probably very rarely the case; if you're really operating on int64-like values, you'd use BigInts from the start, not Numbers).

@mathiasbynens
Copy link

@titzer

We could have semantics where accepting integer-valued doubles in the range -2^53 to 2^53 is OK

Even if a given Number is within the safe range, it might have been the result of a calculation that crossed the safe boundary at some point, at which point the damage has potentially already been done.

Number.MAX_SAFE_INTEGER
// --> 9007199254740991
Number.MAX_SAFE_INTEGER + 2 - 2;
// --> 9007199254740990
// Within the safe range, but not the number you wanted!

For this reason, I argue that throwing for all Number values is better than throwing selectively.

@littledan
Copy link
Collaborator

Agree with @mathiasbynens. In TC39, we are trying to make a consistent call here, that using a Number in a place where BigInts are expected throws. This is hoped to encourage a pattern where developers make consistent use of types, to avoid overflow errors. We recently made a decision to affirm that this error exists in the case of 64-bit TypedArrays, which is basically the same case as this issue.

@titzer
Copy link

titzer commented Dec 4, 2018

@littledan @mathiasbynens I understand your reasoning, and I don't disagree that there are pitfalls to rampant implicit conversion of numbers to BigInt.

However, in this case this reduces the ergonomics of the Wasm surface, and I am not convinced this surface is the place to debug type errors in JavaScript programs. After all, arguments to Wasm functions already have an implicit ToNumber() applied to them today. If we enforce BigInt here, it seems much more likely that JavaScript developers will hit this trying to pass a number to an i64-accepting WASM function, get a type error, be confused and frustrated, and then, without really understanding why, insert a BigInt conversion blindly according to recommendations. IMO the chance that this will cause them to police the rest of their program for a deeper understanding seems low. It also seems likely that toolchains producing Wasm (e.g. Emscripten @kripken ) will offer an option to insert the conversion into JS glue code automatically, in which case everyone loses.

Requiring BigInt is also much less efficient, forcing every i64 argument to create garbage. (i32 arguments do not create garbage). It seems better to me that if a program wants to enforce i64 arguments must be BigInts and not numbers, they do this with (perhaps autogenerated) glue code in JS. Otherwise we are building in inefficiencies, which is very much against Wasm's performance goals.

@annevk
Copy link
Member

annevk commented Dec 4, 2018

ToNumber() is used by all JavaScript functions as well, so that cannot be an argument. The garbage argument is novel however.

Perhaps a reasonable compromise for now is to throw and then revisit (with TC39 folks included) if it indeed turns out to be prohibitive and cannot be optimized away? (As in, it's always easy to remove an exception, not to add one.)

@littledan
Copy link
Collaborator

I'm happy to consider relaxing the restrictions if this turns out to be un-optimizable. But, I would prefer if programmers had to do something explicitly when doing an operation which risks losing precision, to getting it implicitly.

General TAG design guidance thread: w3ctag/design-principles#115

@xtuc xtuc mentioned this issue Dec 20, 2018
8 tasks
@jakobkummerow
Copy link
Contributor

I am generally in favor of clearly separating between Numbers and BigInts in JavaScript in order to avoid hard-to-detect bugs due to precision loss. However I'm not convinced that that reasoning applies here. May I humbly present the following observations for your consideration:

(1) A value 42 has 6 significant bits. If it can be implicitly zero-extended to 32 bits to initialize an i32 value, why shouldn't it also be implicitly zero-extended to 64 bits for an i64?

(2) A value 42.2 can't be represented in an integer type. If it can be implicitly truncated to 42 to initialize an i32, why shouldn't it also be implicitly truncated to initialize an i64?

(3) We're talking about the JS ↔ Wasm boundary here, not about pure JavaScript. Crossing language boundaries typically involves some amount of mapping and conversions; that is fine.
(3a) Consequently, to me, this isn't a case of "implicitly converting Numbers to BigInt" (which we avoid for good reason), but one of converting JavaScript values to Wasm values.
(3b) In this light, "Number → i32", "Number → i64", "BigInt → i32", "BigInt → i64" are all conversions that potentially truncate the value in various ways, so it's hard to argue that some of those truncations are worse than others.
(3c) @mathiasbynens' Number.MAX_SAFE_INTEGER + 2 - 2 example would clearly be a bug in the JavaScript code (assuming that the precision loss was unintentional/unexpected), but is unrelated to then afterwards sending the result over the JS → Wasm boundary, so (echoing @titzer's comment) I'm not convinced that it is that boundary's job to try to guard against such a bug. In particular, it seems hard to explain why passing this presumed-buggy value to an i32-typed Wasm value is fine, but passing it to an i64-typed value throws.

(4) Wasm modules are typically compiled from C/C++. In C/C++, developers often choose int64 types on APIs when they're concerned that the int32 range might not quite be enough for some use cases.
(4a) In the interest of convenience and abstraction, it seems reasonable to let calling JavaScript code decide what types it needs for the values it works with: maybe the JavaScript code is perfectly happy with int32-ranged values stored as Numbers, and oblivious to the fact that the Wasm library it calls could handle int64-ranged values too.
(4b) In particular, consider the case where a C++ library gets upgraded from int32 values to int64 in a new version, and is recompiled to Wasm. Going from "I'll take an integer here, up to 32 bits long" to "..., up to 64 bits long" should intuitively be a backwards-compatible change, but with a strict "i64 needs BigInt" requirement, it means that all JavaScript code calling that library must be updated as well, just to keep working as it did before (as it'll pass the same values that previously fit into i32-typed values).

To be clear, lacking specific experience with all this, I don't feel very strongly about the ultimate outcome of this debate, I just wanted to point out that I don't find the argument presented so far particularly convincing. For returning Wasm i64 values to JavaScript, BigInts are clearly the right choice. For sending values from JavaScript to Wasm, some amount of conversion must happen anyways (as usual when crossing language boundaries) and happens already (one can initialize an i32 value from 1 as well as 1.5 or "1.5" or {toString: function() { return "42.2"}}), so the restriction that a string "42.2" can be used to initialize an i32 but not an i64 seems rather arbitrary.

@littledan
Copy link
Collaborator

For this upgrade scenario: Today, you usually have to write some JavaScript glue code to access C++. In the future, we may have something higher level with host/WebIDL bindings. I think it'd be good to have a way to declare something as an "int53" and accept Number, for this sort of scenario.

But I'd like to start out conservative, and consider relaxing restrictions over time. We discussed the analogous restriction recently in TC39 when it came to TypedArrays, and decided to be strict about BigInts. See this summary. The TAG guidance linked above is informed by this; the hope is that JavaScript can remain consistent across the whole platform.

If this argument is unconvincing, let's discuss it in an upcoming WebAssembly CG call to get a strong conclusion.

@Ms2ger
Copy link
Collaborator

Ms2ger commented May 12, 2020

Closing no-change following the TAG design principles and consistency with the rest of the platform. Authors are expected to use bigint consistently when they're planning to use an i64 API.

If this turns out to be problematic in practice, we can revisit; it's generally much easier to make an API stop throwing exceptions than the other way around.

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

8 participants