[JSC] BigInt constructor should accept larger integers than safe-integers

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

This is spec change introduced in https://github.com/tc39/proposal-bigint/pull/138.
We should change our BigInt constructor,

1. Accept larger integer
2. Add JSBigInt::createFrom(..., double) which properly creates JSBigInt with larger integers. Currently we are casting double to int64_t and this was safe because double is safe-integer. But after accepting larger integers, this cast produces incorrect value.

[Yusuke Suzuki]

Created attachment 397890 [details]
Patch

[Yusuke Suzuki]

Created attachment 397891 [details]
Patch

[Yusuke Suzuki]

Created attachment 397916 [details]
Patch

[Darin Adler]

Comment on attachment 397916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397916&action=review

> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:130
> +        if (std::abs(number) <= maxSafeInteger())
> +            return JSValue::encode(JSBigInt::makeHeapBigIntOrBigInt32(vm, static_cast<int64_t>(primitive.asDouble())));
> +        return JSValue::encode(JSBigInt::makeHeapBigIntOrBigInt32(vm, primitive.asDouble()));

Unclear why the maxSafeInteger special case here is valuable, with a conversion to int64_t, but that same special case is not a good optimization inside the makeHeapBigIntOrBigInt32 overload that takes a double. I suggest leaving the maxSafeInteger check out here entirely, and either include it, or do not, inside the makeHeapBigIntOrBigInt32 function.

Unless I am missing a reason why it’s more helpful here.

> Source/JavaScriptCore/runtime/JSBigInt.cpp:221
> +    const int32_t mantissaTopBit = doubleMantissaSize - 1; // 0-indexed.
> +    // 0-indexed position of most significant bit in the most significant digit.
> +    int32_t msdTopBit = exponent % digitBits;

Unclear to me why we use const in one place and not the other. I would just omit it in both places, but no big deal.

> Source/JavaScriptCore/runtime/JSBigInt.h:112
> +        auto ptr = JSBigInt::createFrom(vm, value);
> +        return JSValue(static_cast<JSCell*>(ptr));

Why is this two lines? Why is the static_cast needed? Why is the explicit conversion to JSValue needed?

> Source/JavaScriptCore/runtime/MathCommon.h:51
> +inline bool isInteger(double value)

Can this be constexpr? I guess not, because std::isfinite and std::trunc are not available in constexpr versions.

Slightly dangerous to have a function named isInteger that takes a double. You could call it on a float and get poor performance. Overloading for float would make that danger go away. Or using a template. Or something else that would turn off conversions. I wish there was a way to say "really gotta be a double" for the argument and disable conversions.

> Source/JavaScriptCore/runtime/MathCommon.h:53
> +    return std::isfinite(value) && trunc(value) == value;

Strange to mix std::isfinite with trunc. I think we should use std:: on both or neither.

> Source/JavaScriptCore/runtime/MathCommon.h:58
> +    return isInteger(value) && std::abs(value) <= maxSafeInteger();

Seems unnecessary to assume that std::abs(minSafeInteger()) == maxSafeInteger(). We could static_assert it, if this is really an important optimization, or compare against both min and max.

> Source/JavaScriptCore/runtime/NumberConstructor.cpp:150
>      return JSValue::encode(jsBoolean(isInteger));

Local variable should not be named isInteger. That’s actively incorrect, since it will be false for integers that are not safe! It should be named isSafeInteger, or result, or something that is not inaccurate.

> Source/JavaScriptCore/runtime/NumberConstructor.h:56
>              return true;
>          if (!value.isDouble())
>              return false;
> -
> -        double number = value.asDouble();
> -        return std::isfinite(number) && trunc(number) == number;
> +        return isInteger(value.asDouble());

I like boolean expressions:

    return value.isInt32() || (value.isDouble() && isInteger(value.asDouble()));

Look how pretty that is!

> JSTests/ChangeLog:25
> [phone]  Yusuke Suzuki  <[e-mail]>
> +
> +        [JSC] BigInt constructor should accept larger integers than safe-integers
> +        https://bugs.webkit.org/show_bug.cgi?id=210755
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * stress/large-number-to-bigint.js: Added.
> +        (shouldBe):
> +        (shouldThrow):
> +        * test262/config.yaml:
> +

Double change log.

> JSTests/stress/large-number-to-bigint.js:42
> +shouldBe(BigInt(Number.MAX_SAFE_INTEGER), 9007199254740991n);
> +shouldBe(BigInt(Number.MIN_SAFE_INTEGER), -9007199254740991n);

How about testing the ones that are just past the safe integers?

[Darin Adler]

Comment on attachment 397916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397916&action=review

>> Source/JavaScriptCore/runtime/MathCommon.h:58
>> +    return isInteger(value) && std::abs(value) <= maxSafeInteger();
> 
> Seems unnecessary to assume that std::abs(minSafeInteger()) == maxSafeInteger(). We could static_assert it, if this is really an important optimization, or compare against both min and max.

A little bit sad; the std::isfinite check isn’t needed, because infinite values won’t be in range, and NaN values will return false too. But I think the compiler won’t optimize it out.

[Yusuke Suzuki]

Comment on attachment 397916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397916&action=review

Thanks!

>> Source/JavaScriptCore/runtime/BigIntConstructor.cpp:130
>> +        return JSValue::encode(JSBigInt::makeHeapBigIntOrBigInt32(vm, primitive.asDouble()));
> 
> Unclear why the maxSafeInteger special case here is valuable, with a conversion to int64_t, but that same special case is not a good optimization inside the makeHeapBigIntOrBigInt32 overload that takes a double. I suggest leaving the maxSafeInteger check out here entirely, and either include it, or do not, inside the makeHeapBigIntOrBigInt32 function.
> 
> Unless I am missing a reason why it’s more helpful here.

This is optimization because JSBigInt::createFrom(VM&, int64_t) is more efficient than JSBigInt::createFrom(VM&, double).
I moved this optimization to JSBigInt::makeHeapBigIntOrBigInt32.

>> Source/JavaScriptCore/runtime/JSBigInt.cpp:221
>> +    int32_t msdTopBit = exponent % digitBits;
> 
> Unclear to me why we use const in one place and not the other. I would just omit it in both places, but no big deal.

Removed `const`.

>> Source/JavaScriptCore/runtime/JSBigInt.h:112
>> +        return JSValue(static_cast<JSCell*>(ptr));
> 
> Why is this two lines? Why is the static_cast needed? Why is the explicit conversion to JSValue needed?

Changed both `makeHeapBigIntOrBigInt32(VM&, int64_t)` and `makeHeapBigIntOrBigInt32(VM&, double)`.

>> Source/JavaScriptCore/runtime/MathCommon.h:51
>> +inline bool isInteger(double value)
> 
> Can this be constexpr? I guess not, because std::isfinite and std::trunc are not available in constexpr versions.
> 
> Slightly dangerous to have a function named isInteger that takes a double. You could call it on a float and get poor performance. Overloading for float would make that danger go away. Or using a template. Or something else that would turn off conversions. I wish there was a way to say "really gotta be a double" for the argument and disable conversions.

Yeah, unfortunately, we cannot use constexpr because `std::trunc` and `std::isfinite` are not constexpr function :(
https://stackoverflow.com/questions/17347935/constexpr-math-functions

Adding float version sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/MathCommon.h:53
>> +    return std::isfinite(value) && trunc(value) == value;
> 
> Strange to mix std::isfinite with trunc. I think we should use std:: on both or neither.

Attached std:: to trunc.

>>> Source/JavaScriptCore/runtime/MathCommon.h:58
>>> +    return isInteger(value) && std::abs(value) <= maxSafeInteger();
>> 
>> Seems unnecessary to assume that std::abs(minSafeInteger()) == maxSafeInteger(). We could static_assert it, if this is really an important optimization, or compare against both min and max.
> 
> A little bit sad; the std::isfinite check isn’t needed, because infinite values won’t be in range, and NaN values will return false too. But I think the compiler won’t optimize it out.

We could just rewrite this code with `std::trunc(value) == value && std::abs(value) <= maxSafeInteger();`. Maybe, this is better since std::isfinite will remain. Changed.

>> Source/JavaScriptCore/runtime/NumberConstructor.cpp:150
>>      return JSValue::encode(jsBoolean(isInteger));
> 
> Local variable should not be named isInteger. That’s actively incorrect, since it will be false for integers that are not safe! It should be named isSafeInteger, or result, or something that is not inaccurate.

Changed. Each if statement now returns value directly.

>> Source/JavaScriptCore/runtime/NumberConstructor.h:56
>> +        return isInteger(value.asDouble());
> 
> I like boolean expressions:
> 
>     return value.isInt32() || (value.isDouble() && isInteger(value.asDouble()));
> 
> Look how pretty that is!

Changed!

>> JSTests/ChangeLog:25
>> +
> 
> Double change log.

Oops, fixed.

>> JSTests/stress/large-number-to-bigint.js:42
>> +shouldBe(BigInt(Number.MIN_SAFE_INTEGER), -9007199254740991n);
> 
> How about testing the ones that are just past the safe integers?

Sounds good! Added.

[Yusuke Suzuki]

Committed r260863: <https://trac.webkit.org/changeset/260863>

[Radar WebKit Bug Importer]

<rdar://problem/62571797>

[Saam Barati]

Comment on attachment 397916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397916&action=review

> Source/JavaScriptCore/runtime/JSBigInt.cpp:199
> +    int32_t exponent = rawExponent - 0x3ff;

can we assert exponent >= 0?

> Source/JavaScriptCore/runtime/JSBigInt.cpp:200
> +    int32_t digits = exponent / digitBits + 1;

nit: I'd call this "numberOfDigits"

> Source/JavaScriptCore/runtime/JSBigInt.cpp:249
> +                mantissa = 0;

can we assert "remainingMantissaBits < 0"?