Skip to content
This repository has been archived by the owner on Oct 10, 2019. It is now read-only.

Normative: Allow BigInt<->Number casts for TypedArray set #106

Merged
merged 3 commits into from
Jan 4, 2018

Conversation

littledan
Copy link
Member

@littledan littledan commented Dec 17, 2017

This patch explores a suggestion from @jakobkummerow in
#102 (comment)

The logic of this change is that TypedArrays are already explicit in
their types, and it would be redundant to force users to insert additional
casts. Further, TypedArrays without the existence of BigInt64Arrays
already generally allow for implicit conversions between element types,
e.g., when applying one TypedArray constructor with another TypedArray
as an argument; it could be argued that the requirement of BigInt64Arrays
to have an explicit mapping function introduces an inconsistency.

Namely, when writing into a TypedArray, there is a more flexible
cast operation used: If a BigInt is written into an existing TypedArray
type, it is implicitly cast to the appropriately sized type. Similarly,
a Number is implicitly cast to a 64-bit integer when writing it into
a BigInt64Array/BigUint64Array. Several TypedArray methods and functions
are updated to propagate the flexibility to them, including Atomics
and DataView functions.

This patch is not ready to merge until we have more discussion about
whether such a change is desirable.

@littledan
Copy link
Member Author

littledan commented Dec 18, 2017

I'm not sure whether this change would be an improvement (by improving flexibility and a form of consistency), make things worse (by implicitly losing more information and having a different form of consistency), or not matter one way or the other. I'd be interested in any of your feedback here.

@littledan
Copy link
Member Author

littledan commented Dec 18, 2017

I'm interested in getting broader feedback on this PR or sticking with the current specification, especially if you might see yourself using this feature. Some cases where the difference could come up:

let i32 = new Int32Array(500);
let i64 = BigInt64Array(i32);  // currently throws, with this patch would cast the values to 0s

cc @mikolalysenko @feross @kgryte @LeszekSwirski @jaro-sevcik @leobalter @apaprocki @rauschma @caiolima @cxielarko @bterlson

@caiolima
Copy link
Collaborator

I'm not an expert into TypedArray, however I think this flexibility is much better IMO.
For instance, if We want to copy an Int32Array into BigInt64Array, right now e need to iterate through each element of Int32Array, properly cast each element and insert into BigInt64Array. This seems painful and inconsistent since it's is possible to totally use previous @littledan's sample among all other typed arrays. I would not for this change in Spec.

@kgryte
Copy link

kgryte commented Dec 18, 2017

I agree with @caiolima that the ergonomics of having to iterate through each element in, e.g., an Int32Array to create a BigInt64Array is less than optimal. So supporting an API which allows creating a BigInt64Array from another typed array flavor seems to make sense to me.

@littledan
Copy link
Member Author

littledan commented Dec 18, 2017

Note that this patch changes more than the additional ability for TypedArray constructors--it makes casts between BigInts and Numbers implicit when it comes to setting elements of TypedArrays in general. For example, it allows:

let arr = new Uint8Array(1);
arr[0] = 1n;
arr[0] // => 1

However, other things which expect a Number throw, such as Math.sin, when you pass a BigInt. These TypedArray semantics are based on having a consistent policy that you "force" casts when writing into a TypedArray. I believe it's simpler to be "all or nothing" here, rather than forcing casts only for a few particular methods or constructor paths.

@kgryte
Copy link

kgryte commented Dec 18, 2017

Yes, that also makes sense to me. I understand we have concerns about losing information, but, as pointed out in the issue thread in the OP, we have already come to expect (implicitly or explicitly) that this is how typed arrays work. So the typed array consistency proposed here seems sensible.

@littledan
Copy link
Member Author

On Twitter, @bmeck expressed some hesitation about this change, preferring the current semantics.

@littledan
Copy link
Member Author

@cxielarko suggested that, if we do ahead with this sort of change, we should also consider removing the "IsSafeInteger" check for writes into TypedArrays and instead doing the "double truncation" behavior (shaving off both the too-significant and too-insigificant) behavior of other integer TypedArrays. This seems reasonable to me; what do others here think?

@jakobkummerow
Copy link
Collaborator

Sounds reasonable to me, yes. Maybe the check should disappear from NumberToBigInt entirely? What's wrong with var big = BigInt(1e100)?

@ljharb
Copy link
Member

ljharb commented Dec 30, 2017

What’s wrong with it is that it could be silently wrong :-/ I’m less concerned about it for typed arrays, but in the standard usage it could lead to otherwise easily avoidable bugs.

@littledan
Copy link
Member Author

@jakobkummerow The throwing semantics were adopted based on discussion in #15 .

@bmeck
Copy link
Member

bmeck commented Dec 30, 2017 via email

@caiolima
Copy link
Collaborator

Sort answer: I'm ok into "double truncation".

BigInt64TypedArray main use case is to have a 64-bit TypedArray support, right? If I'm not confused, BigInt are going to be the main way to manipulate 64-bits, and in the case of TypedArray, it is a tool to carry such values. Thus, IMO, makes more sense being more flexible into BigInt casting rules into writes, in favor of keeping the implementation consistent with other TypedArrays.

@littledan
Copy link
Member Author

@bmeck This loss would be just like what you write a Number into an existing integer TypedArray--you lose both the higher digits and anything after the decimal place. Example:

let x = new Uint8Array(1)
x[0] = 256.1
console.log(x[0]);  // 0

It occurs to me, with @cxielarko 's feedback, that the current patch without "double truncation" would lead to TypeErrors potentially in the middle of a bunch of TypedArray operations. It seems like, if a goal is to avoid throwing in the middle of operations, we would to either do this stronger form of rounding or go back to the original version.

I understand we have concerns about losing information, but, as pointed out in the issue thread in the OP, we have already come to expect (implicitly or explicitly) that this is how typed arrays work.

It's not just TypedArrays that do this casting and losing information implicitly--all of JS works this way. One reason TypedArrays are special is that they have certain operations, like the constructor and set, which convert between TypedArray types, and can be used to convert between any two of them. It's for this reason that we'd be loosening the type guards and rounding just for TypedArrays and not the rest of JS. (It would be pretty complicated to just loosen the type guards for those particular operations, and not the rest of TypedArrays, so I'd prefer to not go that specifically.) Does this point seem reasonable to everybody?

@bmeck
Copy link
Member

bmeck commented Jan 1, 2018

@littledan Yes, I am not opposed to typed arrays being special case lossy since that already is the existing behavior even if I do not think it good behavior. My comment is against completely removing error throwing behavior when changing between types from the comment above.

@littledan littledan force-pushed the moar-casts branch 2 times, most recently from 26dfc75 to 8573b78 Compare January 2, 2018 11:50
@littledan
Copy link
Member Author

OK, I've made a change to this PR to switch to the weaker cast version we discussed in this thread, which should never throw on Numbers. Does this seem good to merge?

This patch explores a suggestion from @jakobkummerow in
#102 (comment)

The logic of this change is that TypedArrays are already explicit in
their types, and it would be redundant to force users to insert additional
casts.

Namely, when writing into a TypedArray, there is a more flexible
cast operation used: If a BigInt is written into an existing TypedArray
type, it is implicitly cast to the appropriately sized type. Similarly,
a Number is implicitly cast to a 64-bit integer when writing it into
a BigInt64Array/BigUint64Array. Several TypedArray methods and functions
are updated to propagate the flexibility to them, including Atomics
and DataView functions.

This patch is not ready to merge until we have more discussion about
whether such a change is desirable.
Thanks for @cxielarko for pointing this issue out, and @bmeck,
@caiolima, @ljharb and @jakobkummerow for helpful discussion about
this idea.
@littledan
Copy link
Member Author

OK, I'm not hearing any objections, so I'll merge this patch and review it at the coming TC39 meeting. @thejoshwolfe will be updating the test262 tests for BigInt to ensure it has proper coverage.

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

Successfully merging this pull request may close these issues.

6 participants