Bug 209782 – [ECMA-402] Properly implement BigInt.prototype.toLocaleString

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Shane Carr]

V8 and SpiderMonkey support BigInt.prototype.toLocaleString as well as passing a BigInt into Intl.NumberFormat.prototype.format.  You cannot do this in WebKit; this will lead to broken web sites and/or reduced performance for WebKit users.

ICU4C provides bindings that accept arbitrary decimal strings.  Fixing this bug is a matter of updating JSC to call those functions.

BigInt umbrella bug: https://bugs.webkit.org/show_bug.cgi?id=179001

[Radar WebKit Bug Importer]

<rdar://problem/61079783>

[Ross Kirsling]

(Adjusting title slightly since we do have a 262-only version of this method.)

[Ross Kirsling]

Created attachment 396134 [details]
Patch

[Ross Kirsling]

Comment on attachment 396134 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:368
> +    Vector<UChar, 32> buffer(32);

This line is copied as-is from the Number path; we could change it, though I don't have a better suggestion.

Unlike the Number case, we do have a string length to work with, but:
1. The string length is generally not long enough, because we might be inserting commas
2. The string length could actually be way *too* long in certain cases
   (e.g., 1000000000n.toLocaleString('en', { notation: 'compact' }) is "1B", though we don't support that option yet)

[Darin Adler]

Comment on attachment 396134 [details]
Patch

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

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> +        return throwVMTypeError(globalObject, scope, "'this' value must be a BigInt or BigIntObject"_s);

Do we have test cases for this?

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:126
> +        return throwVMTypeError(globalObject, scope, "'this' value must be a BigInt or BigIntObject"_s);

I don’t see any test cases for this.

> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:128
> +    IntlNumberFormat* numberFormat = IntlNumberFormat::create(vm, globalObject->numberFormatStructure());

JavaScriptCore is a whole other world, but if I was writing this in elsewhere in WebKIt I would suggest auto rather  than writing the typename here.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:374
> +    auto string = value->toString(globalObject, 10);
> +    RETURN_IF_EXCEPTION(scope, { });
> +
> +    UErrorCode status = U_ZERO_ERROR;
> +    Vector<UChar, 32> buffer(32);
> +    auto length = unum_formatDecimal(m_numberFormat.get(), string.utf8().data(), string.length(), buffer.data(), buffer.size(), nullptr, &status);
> +    if (status == U_BUFFER_OVERFLOW_ERROR) {
> +        buffer.grow(length);
> +        status = U_ZERO_ERROR;
> +        unum_formatDecimal(m_numberFormat.get(), string.utf8().data(), string.length(), buffer.data(), length, nullptr, &status);
> +    }

Seems wasteful here to first make a WTF::String that is guaranteed to be all ASCII, then call UTF-8, allocating a WTF::CString, and even doing it a second time if the result is more than 32 characters.

It’s not typical to write WTF::String code that depends on LChar vs. UChar, but I think here we could use characters8() here because we are guaranteed that JSBigInt::toString returns a Latin-1 string.

Also, it’s a bad pattern to use string.utf8().data() with string.length(); if the string had any non-ASCII characters, then the length would be the length of the Latin-1, not the length of the UTF-8 (which is longer). Irrelevant, since we should not be using utf8().

If we cared about performance we could make a function on JSBigInt that puts the data into a vector with inline capacity instead of a string. Related to bug 18067.

> Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:376
> +        return throwException(globalObject, scope, createError(globalObject, "Failed to format a BigInt."_s));

I don’t see any test cases for this. Maybe because there is actually no real case where this could fail, and this is effectively dead code?

[Keith Miller]

Comment on attachment 396134 [details]
Patch

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

>> Source/JavaScriptCore/runtime/BigIntPrototype.cpp:128
>> +    IntlNumberFormat* numberFormat = IntlNumberFormat::create(vm, globalObject->numberFormatStructure());
> 
> JavaScriptCore is a whole other world, but if I was writing this in elsewhere in WebKIt I would suggest auto rather  than writing the typename here.

While that's somewhat true we try to follow WebKit's conventions! While I was originally against it, these days I think as long as the statement (or next one or two statements) contains the name of the class somewhere it's better to use auto. So I would also use auto here.

[Ross Kirsling]

(In reply to Darin Adler from comment #5)
> > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:376
> > +        return throwException(globalObject, scope, createError(globalObject, "Failed to format a BigInt."_s));
> 
> I don’t see any test cases for this. Maybe because there is actually no real
> case where this could fail, and this is effectively dead code?

This can only occur if ICU has an internal error. It appears that V8 and SM each have a special error message used across all such cases (of which there are a great many):
https://github.com/v8/v8/blob/master/src/common/message-template.h#L25
https://searchfox.org/mozilla-central/source/js/src/js.msg#536

Perhaps we should consolidate accordingly.

[Keith Miller]

(In reply to Ross Kirsling from comment #7)
> (In reply to Darin Adler from comment #5)
> > > Source/JavaScriptCore/runtime/IntlNumberFormat.cpp:376
> > > +        return throwException(globalObject, scope, createError(globalObject, "Failed to format a BigInt."_s));
> > 
> > I don’t see any test cases for this. Maybe because there is actually no real
> > case where this could fail, and this is effectively dead code?
> 
> This can only occur if ICU has an internal error. It appears that V8 and SM
> each have a special error message used across all such cases (of which there
> are a great many):
> https://github.com/v8/v8/blob/master/src/common/message-template.h#L25
> https://searchfox.org/mozilla-central/source/js/src/js.msg#536
> 
> Perhaps we should consolidate accordingly.

Seems like a good idea to me. Can you file a bug?

[Darin Adler]

(In reply to Ross Kirsling from comment #7)
> This can only occur if ICU has an internal error.

Sure I understood that.

It’s an interesting hypothetical. In general we don’t want to have code paths that we have never tested. Maybe we should just crash if ICU has an internal error? Not sure it's valuable to report it to the webpage. Do we have any reason to believe an internal ICU error is a real possibility?

[Ross Kirsling]

(In reply to Darin Adler from comment #9)
> (In reply to Ross Kirsling from comment #7)
> > This can only occur if ICU has an internal error.
> 
> Sure I understood that.
> 
> It’s an interesting hypothetical. In general we don’t want to have code
> paths that we have never tested. Maybe we should just crash if ICU has an
> internal error? Not sure it's valuable to report it to the webpage. Do we
> have any reason to believe an internal ICU error is a real possibility?

This particular case appears to have been hit in V8 such that changes were needed in ICU itself:
https://github.com/v8/v8/blob/master/src/objects/js-number-format.cc#L1264

It's a good question though. It's easy to say what others are doing but much harder to say definitively what the right thing to do is. We have at least 20 existing U_FAILURE checks in each of WTF, JSC, and WebCore, but it appears that just JSC is choosing to throw a JS error.

[Ross Kirsling]

(In reply to Keith Miller from comment #8)
> Seems like a good idea to me. Can you file a bug?

https://bugs.webkit.org/show_bug.cgi?id=210367

[Ross Kirsling]

(In reply to Darin Adler from comment #5)
> Comment on attachment 396134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396134&action=review
> 
> > Source/JavaScriptCore/runtime/BigIntPrototype.cpp:102
> > +        return throwVMTypeError(globalObject, scope, "'this' value must be a BigInt or BigIntObject"_s);
> 
> Do we have test cases for this?
> 
> > Source/JavaScriptCore/runtime/BigIntPrototype.cpp:126
> > +        return throwVMTypeError(globalObject, scope, "'this' value must be a BigInt or BigIntObject"_s);
> 
> I don’t see any test cases for this.


As for these two:
The former isn't new (I'm just inlining a function and throwVMTypeError encodes the result), but the latter is what that bank of shouldThrow cases is verifying.

[Ross Kirsling]

Created attachment 396144 [details]
Patch for landing

[EWS]

Committed r259919: <https://trac.webkit.org/changeset/259919>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396144 [details].