Bug 209782

<rdar://problem/61079783>

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

Created attachment 396134 [details] Patch

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)

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?

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.

(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.

(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?

(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?

(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.

(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

(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.

Created attachment 396144 [details] Patch for landing

Committed r259919: <https://trac.webkit.org/changeset/259919> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396144 [details].