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

Don't throw on converting large Numbers to BigInts #132

Closed
waldemarhorwat opened this issue Mar 14, 2018 · 11 comments
Closed

Don't throw on converting large Numbers to BigInts #132

waldemarhorwat opened this issue Mar 14, 2018 · 11 comments

Comments

@waldemarhorwat
Copy link

Whereas it's good to throw RangeError when trying to convert NaN, ±∞, or a non-integral Number to a BigInt, throwing on integral Numbers ≥2^53 seems gratuitous. All such Numbers have well-defined mathematical values, and the conversion should just work.

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

I've raised this in the past, and was shot down over desires to make an operation between the two types be deterministic unrelated to their runtime value.

@jakobkummerow
Copy link
Collaborator

FWIW, the current behavior is asymmetric: BigInt -> Number conversions never throw, even when they lose precision.

d8> big = 2n ** 55n
36028797018963968n
d8> Number(big)
36028797018963970
d8> BigInt(Number(big))
(d8):1: RangeError: The number 36028797018963970 is not a safe integer and thus cannot be converted to a BigInt
BigInt(Number(2n ** 55n))
^

@ljharb:

desires to make an operation between the two types be deterministic unrelated to their runtime value

Can you please clarify or give a link? I'm confused because "be deterministic unrelated to their runtime value" seems to be at odds with "throw, or not, depending on value" ;-)

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

I don’t have a link offhand, but, specifically, what I recall is that it was OK to throw based on the types of the operands, but not based on the numeric values.

I would personally prefer that every coercion that preserved information was allowed and otherwise threw.

@jakobkummerow
Copy link
Collaborator

But throwing based on numeric values is precisely what's happening: BigInt(Number(2n ** 53n)) throws, BigInt(Number(2n ** 52n)) does not.

(I'm not saying that anything should be changed; I'm just having trouble seeing how the previous reasoning you remember applies here.)

@ljharb
Copy link
Member

ljharb commented Mar 14, 2018

Does that hold for comparison and math operators tho?

@jakobkummerow
Copy link
Collaborator

Yes; maybe that's what you remember. All binary operators throw if you mix types, regardless of values. All comparisons work without throwing (by comparing mathematical values).

For explicit conversions, you have different types by definition though. And the throwing depends on their values. Some cases are entirely uncontroversial: everyone seems to agree that BigInt(Infinity) should throw, BigInt(1) should not. The question that Waldemar raises is what should happen for non-fractional (i.e. integer-valued, albeit rounded to 64-bit-double precision) Numbers when they are explicitly converted to BigInt. A BigInt can represent the value, so there is no loss of precision in the conversion. (There is a loss of precision in the reverse direction, but that does not throw.)

@littledan
Copy link
Member

We arrived at the current semantics based on the rough consensus in #15 . Prior to that, it would just round and not throw. Where do you think we went wrong in the prior reasoning?

@sjrd
Copy link

sjrd commented Mar 14, 2018

AFAICT, in #15, it is not clear that a consensus was actually reached about large values of numbers, i.e., whole numbers whose absolute value is greater than 2**53. The consensus was that rounding or loss of precision should be forbidden, but for those values neither happens.

Not supporting direct conversions from large numbers to BigInt is somewhat problematic in practice. For example, here is what it takes to convert a number larger than 2**53 to a 64-bit BigInt:

function numberTo64BitBigInt(x) {
  const lo = x | 0;
  const rawHi = (x / 4294967296.0) | 0; // 2^32
  const hi = (x < 0 && lo != 0) ? (rawHi - 1) | 0 : rawHi;
  return (BigInt(hi) << 32n) | BigInt(lo >>> 0);
}

(taken from https://github.com/scala-js/scala-js/pull/3286/files#diff-be6a030f9e59fe0b8a579a377fdfae73R510), and the proof that it is correct is non-trivial. Extending that to an arbitrary number of bits is even trickier.

(the above algorithm also works for numbers with a fractional part, truncating towards 0, as required by the Scala/Java double-to-long conversion; something easier to understand can probably be found for whole numbers only)

@littledan
Copy link
Member

We discussed this issue in the March 2018 TC39 meeting. The consensus reached was that we should continue throwing on non-integer Numbers, but not throw on Numbers which are "unsafe" integers.

littledan added a commit that referenced this issue Apr 6, 2018
Previously, the BigInt constructor used IsSafeInteger to check if a Number
is safe to cast to a BigInt without losing information. This check is
unnecessarily conservative--it rules out large integer-valued Numbers.
This patch switches to just check that the Number is an integer value.

This patch specifies the resolution of the discussion in the March
2018 TC39 meeting.

Closes #132
littledan added a commit that referenced this issue Apr 6, 2018
Previously, the BigInt constructor used IsSafeInteger to check if a Number
is safe to cast to a BigInt without losing information. This check is
unnecessarily conservative--it rules out large integer-valued Numbers.
This patch switches to just check that the Number is an integer value.

This patch specifies the resolution of the discussion in the March
2018 TC39 meeting.

Closes #132
littledan added a commit that referenced this issue Apr 12, 2018
Previously, the BigInt constructor used IsSafeInteger to check if a Number
is safe to cast to a BigInt without losing information. This check is
unnecessarily conservative--it rules out large integer-valued Numbers.
This patch switches to just check that the Number is an integer value.

This patch specifies the resolution of the discussion in the March
2018 TC39 meeting.

Closes #132
@shaunlebron
Copy link

Chrome 67 seems to have backpedaled on this behavior. BigInt(2**1023) isn't permitted anymore.

@jakobkummerow
Copy link
Collaborator

There's no backpedaling. Chrome 67 has the previously spec'ed behavior. Chrome 68+ has the updated behavior, in accordance with the change made here.

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

6 participants