Skip to content

Intl.NumberFormat changed behavior (possibly after ICU 58 -> 59 bump) #15223

Closed
#20826
Closed
Intl.NumberFormat changed behavior (possibly after ICU 58 -> 59 bump)#15223
#20826
Assignees
srl295TimothyGu
Labels
confirmed-bugIssues and PRs for confirmed bugs.i18n-apiIssues and PRs related to Node.js internationalization support.v8 engineIssues and PRs related to the V8 dependency.

Description

@sclinede
sclinede
opened on Sep 6, 2017
  • Version:
    v8.4.0
  • Platform:
    Linux 8a6b069d5b19 4.10.0-22-generic Simple project messaging. #24~16.04.1-Ubuntu SMP Tue May 23 17:03:51 UTC 2017 x86_64 GNU/Linux
  • Subsystem:
    Ubuntu

We're using Jest on Travis for testing and discovered that after Node upgraded from v7.9.0 to v8.4.0 some tests failed. That's okay I thought, but. Finally, I found this odd behavior:

$ node -v
v7.9.0
$ node
> Intl.NumberFormat('en', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9'
> Intl.NumberFormat('ru', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9' 

----

$ node -v
v8.4.0
$ node
> Intl.NumberFormat('en', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9'
> Intl.NumberFormat('ru', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'US$ 9'

Maybe I missed some breaking changes, so I'll appreciate any help with that.
Otherwise, I think it's a regression and we should reflect on the proper solution.

Activity

  1. vsemozhetbyt
    added
    i18n-apiIssues and PRs related to Node.js internationalization support.
    on Sep 6, 2017

  2. TimothyGu commented on Sep 6, 2017

    @TimothyGu
    TimothyGu
    on Sep 6, 2017
    Member

    Just wondering, do you have full-icu installed?

  3. sclinede commented on Sep 6, 2017

    @sclinede
    sclinede
    on Sep 6, 2017
    Author

    Nope! Only the small-icu that we have already packed.

    Also, this bug is not reproducible on Mac by default.
    On homebrew node is installed with system-icu option (proof).

  4. sclinede commented on Sep 7, 2017

    @sclinede
    sclinede
    on Sep 7, 2017
    Author

    Further investigation showed that the fallback logic for unknown locale changed:

    $ node -v
v7.9.0
$ node
> Intl.NumberFormat('xx', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9'
> Intl.NumberFormat(['xx', 'en'], {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9' 
> Intl.NumberFormat('xx', {style: 'currency', currency: 'USD', currencyDisplay: 'symbol'}).resolvedOptions()    
{ locale: 'en-US',                                      
  numberingSystem: 'latn',                              
  style: 'currency',                                    
  useGrouping: true,                                    
  minimumIntegerDigits: 1,                              
  minimumFractionDigits: 2,                             
  maximumFractionDigits: 2,                             
  currency: 'USD',                                      
  currencyDisplay: 'symbol' }

----

$ node -v
v8.4.0
$ node
> Intl.NumberFormat('xx', {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'US$ 9'
> Intl.NumberFormat(['xx', 'en'], {style: 'currency', currency: 'USD', minimumFractionDigits: 0}).format(9);            
'$9' 
> Intl.NumberFormat('xx', {style: 'currency', currency: 'USD', currencyDisplay: 'symbol'}).resolvedOptions()    
{ locale: 'und',            
  numberingSystem: 'latn',  
  style: 'currency',        
  useGrouping: true,        
  minimumIntegerDigits: 1,  
  minimumFractionDigits: 2, 
  maximumFractionDigits: 2, 
  currency: 'USD',          
  currencyDisplay: 'symbol' }

  5. sclinede commented on Sep 7, 2017

    @sclinede
    sclinede
    on Sep 7, 2017
    Author

    And we don't have any test for that. We just verify that in case of unknown locale we return defaults here.

  6. TimothyGu commented on Sep 7, 2017

    @TimothyGu
    TimothyGu
    on Sep 7, 2017
    Member

    Interestingly, %GetDefaultICULocale() returns en-US so that test would fail on Node.js v8.x...

    @nodejs/intl Please weigh in.

  7. TimothyGu commented on Sep 7, 2017

    @TimothyGu
    TimothyGu
    on Sep 7, 2017
    Member

    So it looks like the test case is hitting this piece of code in V8...

    node/deps/v8/src/objects/intl-objects.cc

    Lines 456 to 459 in 56a0577

    // This would never happen, since we got the locale from ICU.
    JSObject::SetProperty(resolved, factory->NewStringFromStaticChars("locale"),
    factory->NewStringFromStaticChars("und"), SLOPPY)
    .Assert();

    the comment makes it look like there is a legitimate bug somewhere. Digging deeper...

  8. bnoordhuis commented on Sep 7, 2017

    @bnoordhuis
    bnoordhuis
    on Sep 7, 2017
    Member

    v8/v8@2b5a36d from July might or might not help with this. It's not in our tree yet.

  9. TimothyGu commented on Sep 7, 2017

    @TimothyGu
    TimothyGu
    on Sep 7, 2017
    Member

    @bnoordhuis Unfortunately that commit does not fix this. I think I got to the bottom of this, however. The key lies in the following lines:

    node/deps/v8/src/objects/intl-objects.cc

    Lines 707 to 720 in 56a0577

    // Convert BCP47 into ICU locale format.
    UErrorCode status = U_ZERO_ERROR;
    icu::Locale icu_locale;
    char icu_result[ULOC_FULLNAME_CAPACITY];
    int icu_length = 0;
    v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
    if (bcp47_locale.length() != 0) {
    uloc_forLanguageTag(*bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY,
    &icu_length, &status);
    if (U_FAILURE(status) || icu_length == 0) {
    return NULL;
    }
    icu_locale = icu::Locale(icu_result);
    }

    The docs for uloc_forLanguageTag says:

    int32_t uloc_forLanguageTag(const char* langtag,
                            char* localeID,
                            int32_t localeIDCapacity,
                            int32_t* parsedLength,
                            UErrorCode* err);

    parsedLength: if not NULL, successfully parsed length for the input language tag is set

    Returns: the length of the locale ID.

    So the returned value should actually be used for the icu_length parameter and checked, instead of parsedLength.

    For the test case in the OP, bcp47_locale contains "und" as normalized by other layers, so icu_length would always be 3. This piece of code would then read icu_result which is uninitializd. This might explain a bit of non-deterministic behaviors between different ICU versions and different machines (e.g. why @bnoordhuis's patch fixes the tests for him).

    If that were fixed, we should also use the default ICU locale instead of returning NULL, to truly fix the tests.

    All of this is included in the following patch, which fixes this bug and also factors out the common BCP 47 conversion code for me:

    From 588bb87e12c9867eca7141b93fff4329a38e5650 Mon Sep 17 00:00:00 2001
From: Timothy Gu <timothygu99@gmail.com>
Date: Thu, 7 Sep 2017 22:08:41 +0800
Subject: [PATCH] Fix BCP47-to-ICU conversion

---
 deps/v8/src/objects/intl-objects.cc | 77 ++++++++++---------------------------
 1 file changed, 21 insertions(+), 56 deletions(-)

diff --git a/deps/v8/src/objects/intl-objects.cc b/deps/v8/src/objects/intl-objects.cc
index fd6546b390..7a56821a19 100644
--- a/deps/v8/src/objects/intl-objects.cc
+++ b/deps/v8/src/objects/intl-objects.cc
@@ -698,26 +698,30 @@ void SetResolvedBreakIteratorSettings(Isolate* isolate,
         .Assert();
   }
 }
+
+// Convert BCP47 into ICU locale format.
+icu::Locale ConvertFromBCP47(Handle<String> locale) {
+  v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
+  if (bcp47_locale.length() != 0) {
+    UErrorCode status = U_ZERO_ERROR;
+    char icu_result[ULOC_FULLNAME_CAPACITY];
+    int icu_length = uloc_forLanguageTag(*bcp47_locale, icu_result,
+                                         ULOC_FULLNAME_CAPACITY, nullptr,
+                                         &status);
+    if (!U_FAILURE(status) && icu_length != 0) {
+      return icu::Locale(icu_result);
+    }
+  }
+  return icu::Locale();
+}
+
 }  // namespace
 
 // static
 icu::SimpleDateFormat* DateFormat::InitializeDateTimeFormat(
     Isolate* isolate, Handle<String> locale, Handle<JSObject> options,
     Handle<JSObject> resolved) {
-  // Convert BCP47 into ICU locale format.
-  UErrorCode status = U_ZERO_ERROR;
-  icu::Locale icu_locale;
-  char icu_result[ULOC_FULLNAME_CAPACITY];
-  int icu_length = 0;
-  v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
-  if (bcp47_locale.length() != 0) {
-    uloc_forLanguageTag(*bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY,
-                        &icu_length, &status);
-    if (U_FAILURE(status) || icu_length == 0) {
-      return NULL;
-    }
-    icu_locale = icu::Locale(icu_result);
-  }
+  icu::Locale icu_locale = ConvertFromBCP47(locale);
 
   icu::SimpleDateFormat* date_format =
       CreateICUDateFormat(isolate, icu_locale, options);
@@ -753,20 +757,7 @@ void DateFormat::DeleteDateFormat(const v8::WeakCallbackInfo<void>& data) {
 icu::DecimalFormat* NumberFormat::InitializeNumberFormat(
     Isolate* isolate, Handle<String> locale, Handle<JSObject> options,
     Handle<JSObject> resolved) {
-  // Convert BCP47 into ICU locale format.
-  UErrorCode status = U_ZERO_ERROR;
-  icu::Locale icu_locale;
-  char icu_result[ULOC_FULLNAME_CAPACITY];
-  int icu_length = 0;
-  v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
-  if (bcp47_locale.length() != 0) {
-    uloc_forLanguageTag(*bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY,
-                        &icu_length, &status);
-    if (U_FAILURE(status) || icu_length == 0) {
-      return NULL;
-    }
-    icu_locale = icu::Locale(icu_result);
-  }
+  icu::Locale icu_locale = ConvertFromBCP47(locale);
 
   icu::DecimalFormat* number_format =
       CreateICUNumberFormat(isolate, icu_locale, options);
@@ -804,20 +795,7 @@ icu::Collator* Collator::InitializeCollator(Isolate* isolate,
                                             Handle<String> locale,
                                             Handle<JSObject> options,
                                             Handle<JSObject> resolved) {
-  // Convert BCP47 into ICU locale format.
-  UErrorCode status = U_ZERO_ERROR;
-  icu::Locale icu_locale;
-  char icu_result[ULOC_FULLNAME_CAPACITY];
-  int icu_length = 0;
-  v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
-  if (bcp47_locale.length() != 0) {
-    uloc_forLanguageTag(*bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY,
-                        &icu_length, &status);
-    if (U_FAILURE(status) || icu_length == 0) {
-      return NULL;
-    }
-    icu_locale = icu::Locale(icu_result);
-  }
+  icu::Locale icu_locale = ConvertFromBCP47(locale);
 
   icu::Collator* collator = CreateICUCollator(isolate, icu_locale, options);
   if (!collator) {
@@ -852,20 +830,7 @@ void Collator::DeleteCollator(const v8::WeakCallbackInfo<void>& data) {
 icu::BreakIterator* V8BreakIterator::InitializeBreakIterator(
     Isolate* isolate, Handle<String> locale, Handle<JSObject> options,
     Handle<JSObject> resolved) {
-  // Convert BCP47 into ICU locale format.
-  UErrorCode status = U_ZERO_ERROR;
-  icu::Locale icu_locale;
-  char icu_result[ULOC_FULLNAME_CAPACITY];
-  int icu_length = 0;
-  v8::String::Utf8Value bcp47_locale(v8::Utils::ToLocal(locale));
-  if (bcp47_locale.length() != 0) {
-    uloc_forLanguageTag(*bcp47_locale, icu_result, ULOC_FULLNAME_CAPACITY,
-                        &icu_length, &status);
-    if (U_FAILURE(status) || icu_length == 0) {
-      return NULL;
-    }
-    icu_locale = icu::Locale(icu_result);
-  }
+  icu::Locale icu_locale = ConvertFromBCP47(locale);
 
   icu::BreakIterator* break_iterator =
       CreateICUBreakIterator(isolate, icu_locale, options);
-- 
2.11.0

    Does this patch look okay to be submitted to V8?

    /cc @nodejs/v8 @addaleax

  10. TimothyGu
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    v8 engineIssues and PRs related to the V8 dependency.
    v8.x
    on Sep 7, 2017
  11. srl295
    self-assigned this
    on Sep 8, 2017

  12. srl295 commented on Sep 8, 2017

    @srl295
    srl295
    on Sep 8, 2017
    Member

    @sclinede I don't think this is a bug. The display for USD changed to US$ because there are a lot of currencies which use the dollarsign in the world.

  13. TimothyGu commented on Sep 8, 2017

    @TimothyGu
    TimothyGu
    on Sep 8, 2017
    Member

    @srl295 I think the problem lies in the fact that V8 isn't getting the default ICU locale, not that ICU changed its behaviors. The default locale did not change between 58 and 59, nor did the from/toLanguageTag functions.

  14. 25 remaining items

  15. targos
    added a commit that references this issue on May 31, 2018

    deps: cherry-pick 6989b3f6d7 from V8 upstream

    4b5b1a9
  16. MylesBorins
    added 2 commits that reference this issue on Jun 1, 2018

    deps: cherry-pick 6989b3f6d7 from V8 upstream

    a60ab57

    deps: cherry-pick 6989b3f6d7 from V8 upstream

    35d6661
  17. MylesBorins
    added a commit that references this issue on Aug 7, 2018

    deps: cherry-pick 6989b3f6d7 from V8 upstream

    bb1e8a6

  18. jay123anta commented on Aug 16, 2026

    @jay123anta
    jay123anta
    on Aug 16, 2026

    Late reproduction that reframes this: it wasn't Russian locale data. On the small-icu builds these Nodes shipped, the default fallback locale changed:

    7.10.1: 'ru-RU' resolves to en-US-u-va-posix  ->  $9.00
8.0.0:  'ru-RU' resolves to und               ->  US$ 9.00

    With real full-ICU data installed, output is identical on both (9,00 $). resolvedOptions().locale is the one-line diagnostic for this whole class - I've since automated it in https://github.com/jay123anta/intl-drift for anyone hitting similar drift on upgrades.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

confirmed-bugIssues and PRs for confirmed bugs.i18n-apiIssues and PRs related to Node.js internationalization support.v8 engineIssues and PRs related to the V8 dependency.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions