Fix unified number format

1. Move the reading of Notation before calling
   SetNumberFormatDigitOptions()
   ( sync after https://github.com/tc39/proposal-unified-intl-numberformat/pull/37)
2. Sync SetNumberFormatDigitOptions to the spec.
3. Consider the case that while RoundingType is "compact-rounding"
   do not set the precision.
4. correct the tests accordingly.
5. Fix the rounding of notation: "compact" and put regression cases
   into test/intl/regress-9408.js

Bug: v8:9408
Change-Id: I78d66601fe21b1a74a50047b2abe6a2838a58b8c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1681599
Commit-Queue: Frank Tang <ftang@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62450}

src/objects/intl-objects.cc

@@ -1186,40 +1186,56 @@ Maybe<int> Intl::GetNumberOption(Isolate* isolate, Handle<JSReceiver> options,
 
 Maybe<Intl::NumberFormatDigitOptions> Intl::SetNumberFormatDigitOptions(
     Isolate* isolate, Handle<JSReceiver> options, int mnfd_default,
-    int mxfd_default) {
+    int mxfd_default, bool notation_is_compact) {
   Factory* factory = isolate->factory();
   Intl::NumberFormatDigitOptions digit_options;
 
   // 5. Let mnid be ? GetNumberOption(options, "minimumIntegerDigits,", 1, 21,
   // 1).
-  int mnid;
+  int mnid = 1;
   if (!Intl::GetNumberOption(isolate, options,
                              factory->minimumIntegerDigits_string(), 1, 21, 1)
            .To(&mnid)) {
     return Nothing<NumberFormatDigitOptions>();
   }
 
-  // 6. Let mnfd be ? GetNumberOption(options, "minimumFractionDigits", 0, 20,
-  // mnfdDefault).
-  int mnfd;
-  if (!Intl::GetNumberOption(isolate, options,
-                             factory->minimumFractionDigits_string(), 0, 20,
-                             mnfd_default)
-           .To(&mnfd)) {
-    return Nothing<NumberFormatDigitOptions>();
-  }
+  int mnfd = 0;
+  int mxfd = 0;
+  Handle<Object> mnfd_obj;
+  Handle<Object> mxfd_obj;
+  if (FLAG_harmony_intl_numberformat_unified) {
+    // 6. Let mnfd be ? Get(options, "minimumFractionDigits").
+    Handle<String> mnfd_str = factory->minimumFractionDigits_string();
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate, mnfd_obj, JSReceiver::GetProperty(isolate, options, mnfd_str),
+        Nothing<NumberFormatDigitOptions>());
+
+    // 8. Let mnfd be ? Get(options, "maximumFractionDigits").
+    Handle<String> mxfd_str = factory->maximumFractionDigits_string();
+    ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+        isolate, mxfd_obj, JSReceiver::GetProperty(isolate, options, mxfd_str),
+        Nothing<NumberFormatDigitOptions>());
+  } else {
+    // 6. Let mnfd be ? GetNumberOption(options, "minimumFractionDigits", 0, 20,
+    // mnfdDefault).
+    if (!Intl::GetNumberOption(isolate, options,
+                               factory->minimumFractionDigits_string(), 0, 20,
+                               mnfd_default)
+             .To(&mnfd)) {
+      return Nothing<NumberFormatDigitOptions>();
+    }
 
-  // 7. Let mxfdActualDefault be max( mnfd, mxfdDefault ).
-  int mxfd_actual_default = std::max(mnfd, mxfd_default);
+    // 7. Let mxfdActualDefault be max( mnfd, mxfdDefault ).
+    int mxfd_actual_default = std::max(mnfd, mxfd_default);
 
-  // 8. Let mxfd be ? GetNumberOption(options,
-  // "maximumFractionDigits", mnfd, 20, mxfdActualDefault).
-  int mxfd;
-  if (!Intl::GetNumberOption(isolate, options,
-                             factory->maximumFractionDigits_string(), mnfd, 20,
-                             mxfd_actual_default)
-           .To(&mxfd)) {
-    return Nothing<NumberFormatDigitOptions>();
+    // 8. Let mxfd be ? GetNumberOption(options,
+    // "maximumFractionDigits", mnfd, 20, mxfdActualDefault).
+    if (!Intl::GetNumberOption(isolate, options,
+                               factory->maximumFractionDigits_string(), mnfd,
+                               20, mxfd_actual_default)
+             .To(&mxfd)) {
+      return Nothing<NumberFormatDigitOptions>();
+    }
   }
 
   // 9.  Let mnsd be ? Get(options, "minimumSignificantDigits").
@@ -1268,8 +1284,50 @@ Maybe<Intl::NumberFormatDigitOptions> Intl::SetNumberFormatDigitOptions(
   } else {
     digit_options.minimum_significant_digits = 0;
     digit_options.maximum_significant_digits = 0;
-  }
 
+    if (FLAG_harmony_intl_numberformat_unified) {
+      // 15. Else If mnfd is not undefined or mxfd is not undefined, then
+      if (!mnfd_obj->IsUndefined(isolate) || !mxfd_obj->IsUndefined(isolate)) {
+        // 15. b. Let mnfd be ? DefaultNumberOption(mnfd, 0, 20, mnfdDefault).
+        Handle<String> mnfd_str = factory->minimumFractionDigits_string();
+        if (!DefaultNumberOption(isolate, mnfd_obj, 0, 20, mnfd_default,
+                                 mnfd_str)
+                 .To(&mnfd)) {
+          return Nothing<NumberFormatDigitOptions>();
+        }
+
+        // 15. c. Let mxfdActualDefault be max( mnfd, mxfdDefault ).
+        int mxfd_actual_default = std::max(mnfd, mxfd_default);
+
+        // 15. d. Let mxfd be ? DefaultNumberOption(mxfd, mnfd, 20,
+        // mxfdActualDefault).
+        Handle<String> mxfd_str = factory->maximumFractionDigits_string();
+        if (!DefaultNumberOption(isolate, mxfd_obj, mnfd, 20,
+                                 mxfd_actual_default, mxfd_str)
+                 .To(&mxfd)) {
+          return Nothing<NumberFormatDigitOptions>();
+        }
+        // 15. e. Set intlObj.[[MinimumFractionDigits]] to mnfd.
+        digit_options.minimum_fraction_digits = mnfd;
+
+        // 15. f. Set intlObj.[[MaximumFractionDigits]] to mxfd.
+        digit_options.maximum_fraction_digits = mxfd;
+        // Else If intlObj.[[Notation]] is "compact", then
+      } else if (notation_is_compact) {
+        // a. Set intlObj.[[RoundingType]] to "compact-rounding".
+        // Set minimum_significant_digits to -1 to represent roundingtype is
+        // "compact-rounding".
+        digit_options.minimum_significant_digits = -1;
+        // 17. Else,
+      } else {
+        // 17. b. Set intlObj.[[MinimumFractionDigits]] to mnfdDefault.
+        digit_options.minimum_fraction_digits = mnfd_default;
+
+        // 17. c. Set intlObj.[[MaximumFractionDigits]] to mxfdDefault.
+        digit_options.maximum_fraction_digits = mxfd_default;
+      }
+    }
+  }
   return Just(digit_options);
 }
 

src/objects/intl-objects.h

@@ -185,7 +185,8 @@ class Intl {
   };
   V8_WARN_UNUSED_RESULT static Maybe<NumberFormatDigitOptions>
   SetNumberFormatDigitOptions(Isolate* isolate, Handle<JSReceiver> options,
-                              int mnfd_default, int mxfd_default);
+                              int mnfd_default, int mxfd_default,
+                              bool notation_is_compact);
 
   static icu::Locale CreateICULocale(const std::string& bcp47_locale);
 

src/objects/js-number-format.cc

@@ -164,7 +164,9 @@ icu::number::Notation ToICUNotation(Notation notation,
       return icu::number::Notation::scientific();
     case Notation::ENGINEERING:
       return icu::number::Notation::engineering();
+    // 29. If notation is "compact", then
     case Notation::COMPACT:
+      // 29. a. Set numberFormat.[[CompactDisplay]] to compactDisplay.
       if (compact_display == CompactDisplay::SHORT) {
         return icu::number::Notation::compactShort();
       }
@@ -639,6 +641,18 @@ icu::number::LocalizedNumberFormatter
 JSNumberFormat::SetDigitOptionsToFormatter(
     const icu::number::LocalizedNumberFormatter& icu_number_formatter,
     const Intl::NumberFormatDigitOptions& digit_options) {
+  icu::number::LocalizedNumberFormatter result = icu_number_formatter;
+  if (digit_options.minimum_integer_digits > 1) {
+    result = result.integerWidth(icu::number::IntegerWidth::zeroFillTo(
+        digit_options.minimum_integer_digits));
+  }
+  if (FLAG_harmony_intl_numberformat_unified) {
+    // Value -1 of minimum_significant_digits represent the roundingtype is
+    // "compact-rounding".
+    if (digit_options.minimum_significant_digits < 0) {
+      return result;
+    }
+  }
   icu::number::Precision precision =
       (digit_options.minimum_significant_digits > 0)
           ? icu::number::Precision::minMaxSignificantDigits(
@@ -648,13 +662,7 @@ JSNumberFormat::SetDigitOptionsToFormatter(
                 digit_options.minimum_fraction_digits,
                 digit_options.maximum_fraction_digits);
 
-  icu::number::LocalizedNumberFormatter result =
-      icu_number_formatter.precision(precision);
-  if (digit_options.minimum_integer_digits > 1) {
-    result = result.integerWidth(icu::number::IntegerWidth::zeroFillTo(
-        digit_options.minimum_integer_digits));
-  }
-  return result;
+  return result.precision(precision);
 }
 
 // static
@@ -1122,15 +1130,15 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
     }
   }
 
-  // 20. If style is "currency", then
+  // 23. If style is "currency", then
   int mnfd_default, mxfd_default;
   if (style == Style::CURRENCY) {
     //  a. Let mnfdDefault be cDigits.
     //  b. Let mxfdDefault be cDigits.
     mnfd_default = c_digits;
     mxfd_default = c_digits;
+    // 24. Else,
   } else {
-    // 21. Else,
     // a. Let mnfdDefault be 0.
     mnfd_default = 0;
     // b. If style is "percent", then
@@ -1143,19 +1151,11 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
       mxfd_default = 3;
     }
   }
-  // 22. Perform ? SetNumberFormatDigitOptions(numberFormat, options,
-  // mnfdDefault, mxfdDefault).
-  Maybe<Intl::NumberFormatDigitOptions> maybe_digit_options =
-      Intl::SetNumberFormatDigitOptions(isolate, options, mnfd_default,
-                                        mxfd_default);
-  MAYBE_RETURN(maybe_digit_options, Handle<JSNumberFormat>());
-  Intl::NumberFormatDigitOptions digit_options = maybe_digit_options.FromJust();
-  icu_number_formatter = JSNumberFormat::SetDigitOptionsToFormatter(
-      icu_number_formatter, digit_options);
 
+  Notation notation = Notation::STANDARD;
   if (FLAG_harmony_intl_numberformat_unified) {
-    // Let notation be ? GetOption(options, "notation", "string", « "standard",
-    // "scientific",  "engineering", "compact" », "standard").
+    // 25. Let notation be ? GetOption(options, "notation", "string", «
+    // "standard", "scientific",  "engineering", "compact" », "standard").
     Maybe<Notation> maybe_notation = Intl::GetStringOption<Notation>(
         isolate, options, "notation", service,
         {"standard", "scientific", "engineering", "compact"},
@@ -1163,10 +1163,23 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
          Notation::COMPACT},
         Notation::STANDARD);
     MAYBE_RETURN(maybe_notation, MaybeHandle<JSNumberFormat>());
-    Notation notation = maybe_notation.FromJust();
+    notation = maybe_notation.FromJust();
+  }
+
+  // 27. Perform ? SetNumberFormatDigitOptions(numberFormat, options,
+  // mnfdDefault, mxfdDefault).
+  Maybe<Intl::NumberFormatDigitOptions> maybe_digit_options =
+      Intl::SetNumberFormatDigitOptions(isolate, options, mnfd_default,
+                                        mxfd_default,
+                                        notation == Notation::COMPACT);
+  MAYBE_RETURN(maybe_digit_options, Handle<JSNumberFormat>());
+  Intl::NumberFormatDigitOptions digit_options = maybe_digit_options.FromJust();
+  icu_number_formatter = JSNumberFormat::SetDigitOptionsToFormatter(
+      icu_number_formatter, digit_options);
 
-    // Let compactDisplay be ? GetOption(options, "compactDisplay", "string", «
-    // "short", "long" »,  "short").
+  if (FLAG_harmony_intl_numberformat_unified) {
+    // 28. Let compactDisplay be ? GetOption(options, "compactDisplay",
+    // "string", « "short", "long" »,  "short").
     Maybe<CompactDisplay> maybe_compact_display =
         Intl::GetStringOption<CompactDisplay>(
             isolate, options, "compactDisplay", service, {"short", "long"},
@@ -1175,6 +1188,7 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
     MAYBE_RETURN(maybe_compact_display, MaybeHandle<JSNumberFormat>());
     CompactDisplay compact_display = maybe_compact_display.FromJust();
 
+    // 26. Set numberFormat.[[Notation]] to notation.
     // The default notation in ICU is Simple, which mapped from STANDARD
     // so we can skip setting it.
     if (notation != Notation::STANDARD) {
@@ -1182,20 +1196,20 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
           ToICUNotation(notation, compact_display));
     }
   }
-  // 23. Let useGrouping be ? GetOption(options, "useGrouping", "boolean",
+  // 30. Let useGrouping be ? GetOption(options, "useGrouping", "boolean",
   // undefined, true).
   bool use_grouping = true;
   Maybe<bool> found_use_grouping = Intl::GetBoolOption(
       isolate, options, "useGrouping", service, &use_grouping);
   MAYBE_RETURN(found_use_grouping, MaybeHandle<JSNumberFormat>());
-  // 24. Set numberFormat.[[UseGrouping]] to useGrouping.
+  // 31. Set numberFormat.[[UseGrouping]] to useGrouping.
   if (!use_grouping) {
     icu_number_formatter = icu_number_formatter.grouping(
         UNumberGroupingStrategy::UNUM_GROUPING_OFF);
   }
 
   if (FLAG_harmony_intl_numberformat_unified) {
-    // Let signDisplay be ? GetOption(options, "signDisplay", "string", «
+    // 32. Let signDisplay be ? GetOption(options, "signDisplay", "string", «
     // "auto", "never", "always",  "except-zero" », "auto").
     Maybe<SignDisplay> maybe_sign_display = Intl::GetStringOption<SignDisplay>(
         isolate, options, "signDisplay", service,
@@ -1206,6 +1220,7 @@ MaybeHandle<JSNumberFormat> JSNumberFormat::New(Isolate* isolate,
     MAYBE_RETURN(maybe_sign_display, MaybeHandle<JSNumberFormat>());
     SignDisplay sign_display = maybe_sign_display.FromJust();
 
+    // 33. Set numberFormat.[[SignDisplay]] to signDisplay.
     // The default sign in ICU is UNUM_SIGN_AUTO which is mapped from
     // SignDisplay::AUTO and CurrencySign::STANDARD so we can skip setting
     // under that values for optimization.

src/objects/js-plural-rules.cc

@@ -143,7 +143,7 @@ MaybeHandle<JSPluralRules> JSPluralRules::New(Isolate* isolate, Handle<Map> map,
 
   // 9. Perform ? SetNumberFormatDigitOptions(pluralRules, options, 0, 3).
   Maybe<Intl::NumberFormatDigitOptions> maybe_digit_options =
-      Intl::SetNumberFormatDigitOptions(isolate, options, 0, 3);
+      Intl::SetNumberFormatDigitOptions(isolate, options, 0, 3, false);
   MAYBE_RETURN(maybe_digit_options, MaybeHandle<JSPluralRules>());
   Intl::NumberFormatDigitOptions digit_options = maybe_digit_options.FromJust();
   icu_number_formatter = JSNumberFormat::SetDigitOptionsToFormatter(

test/intl/number-format/unified/constructor-order.js

@@ -34,26 +34,26 @@ new Intl.NumberFormat(['en-US'], {
   get unitDisplay() {
     assertEquals(6, getCount++);
   },
-  // End of new options
-  get minimumIntegerDigits() {
+  get notation() {
     assertEquals(7, getCount++);
   },
-  get minimumFractionDigits() {
+  // End of new options
+  get minimumIntegerDigits() {
     assertEquals(8, getCount++);
   },
-  get maximumFractionDigits() {
+  get minimumFractionDigits() {
     assertEquals(9, getCount++);
   },
-  get minimumSignificantDigits() {
+  get maximumFractionDigits() {
     assertEquals(10, getCount++);
   },
-  get maximumSignificantDigits() {
+  get minimumSignificantDigits() {
     assertEquals(11, getCount++);
   },
-  // Begin of new options
-  get notation() {
+  get maximumSignificantDigits() {
     assertEquals(12, getCount++);
   },
+  // Begin of new options
   get compactDisplay() {
     assertEquals(13, getCount++);
   },

test/intl/number-format/unified/notation.js

@@ -19,9 +19,9 @@ const testData = [
     ["standard", undefined, "987,654,321"],
     ["scientific", undefined, "9.877E8"],
     ["engineering", undefined, "987.654E6"],
-    ["compact", undefined, "987.654M"],
-    ["compact", "short", "987.654M"],
-    ["compact", "long", "987.654 million"],
+    ["compact", undefined, "988M"],
+    ["compact", "short", "988M"],
+    ["compact", "long", "988 million"],
 ];
 
 for (const [notation, compactDisplay, expect1] of testData) {
@@ -40,50 +40,50 @@ for (const [notation, compactDisplay, expect1] of testData) {
 // Test Germany which has different decimal marks.
 let notation = "compact";
 nf = new Intl.NumberFormat("de", {notation, compactDisplay: "short"});
-assertEquals("987,654 Mio.", nf.format(987654321));
-assertEquals("98,765 Mio.", nf.format(98765432));
+assertEquals("988 Mio.", nf.format(987654321));
+assertEquals("99 Mio.", nf.format(98765432));
 assertEquals("98.765", nf.format(98765));
 assertEquals("9876", nf.format(9876));
 nf = new Intl.NumberFormat("de", {notation, compactDisplay: "long"});
-assertEquals("987,654 Millionen", nf.format(987654321));
-assertEquals("98,765 Millionen", nf.format(98765432));
-assertEquals("98,765 Tausend", nf.format(98765));
-assertEquals("9,876 Tausend", nf.format(9876));
+assertEquals("988 Millionen", nf.format(987654321));
+assertEquals("99 Millionen", nf.format(98765432));
+assertEquals("99 Tausend", nf.format(98765));
+assertEquals("9,9 Tausend", nf.format(9876));
 
 // Test Chinese, Japanese and Korean, which group by 4 digits.
 nf = new Intl.NumberFormat("zh-TW", {notation, compactDisplay: "short"});
-assertEquals("9.877億", nf.format(987654321));
-assertEquals("9876.543萬", nf.format(98765432));
-assertEquals("9.877萬", nf.format(98765));
+assertEquals("9.9億", nf.format(987654321));
+assertEquals("9877萬", nf.format(98765432));
+assertEquals("9.9萬", nf.format(98765));
 assertEquals("9876", nf.format(9876));
 nf = new Intl.NumberFormat("zh-TW", {notation, compactDisplay: "long"});
-assertEquals("9.877億", nf.format(987654321));
-assertEquals("9876.543萬", nf.format(98765432));
-assertEquals("9.877萬", nf.format(98765));
+assertEquals("9.9億", nf.format(987654321));
+assertEquals("9877萬", nf.format(98765432));
+assertEquals("9.9萬", nf.format(98765));
 assertEquals("9876", nf.format(9876));
 
 // Test Japanese with compact.
 nf = new Intl.NumberFormat("ja", {notation, compactDisplay: "short"});
-assertEquals("9.877億", nf.format(987654321));
-assertEquals("9876.543万", nf.format(98765432));
-assertEquals("9.877万", nf.format(98765));
+assertEquals("9.9億", nf.format(987654321));
+assertEquals("9877万", nf.format(98765432));
+assertEquals("9.9万", nf.format(98765));
 assertEquals("9876", nf.format(9876));
 nf = new Intl.NumberFormat("ja", {notation, compactDisplay: "long"});
-assertEquals("9.877億", nf.format(987654321));
-assertEquals("9876.543万", nf.format(98765432));
-assertEquals("9.877万", nf.format(98765));
+assertEquals("9.9億", nf.format(987654321));
+assertEquals("9877万", nf.format(98765432));
+assertEquals("9.9万", nf.format(98765));
 assertEquals("9876", nf.format(9876));
 
 // Test Korean with compact.
 nf = new Intl.NumberFormat("ko", {notation, compactDisplay: "short"});
-assertEquals("9.877억", nf.format(987654321));
-assertEquals("9876.543만", nf.format(98765432));
-assertEquals("9.877만", nf.format(98765));
-assertEquals("9.876천", nf.format(9876));
+assertEquals("9.9억", nf.format(987654321));
+assertEquals("9877만", nf.format(98765432));
+assertEquals("9.9만", nf.format(98765));
+assertEquals("9.9천", nf.format(9876));
 assertEquals("987", nf.format(987));
 nf = new Intl.NumberFormat("ko", {notation, compactDisplay: "long"});
-assertEquals("9.877억", nf.format(987654321));
-assertEquals("9876.543만", nf.format(98765432));
-assertEquals("9.877만", nf.format(98765));
-assertEquals("9.876천", nf.format(9876));
+assertEquals("9.9억", nf.format(987654321));
+assertEquals("9877만", nf.format(98765432));
+assertEquals("9.9만", nf.format(98765));
+assertEquals("9.9천", nf.format(9876));
 assertEquals("987", nf.format(987));

test/intl/regress-9408.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --harmony-intl-numberformat-unified
+// Test precision of compact-rounding
+
+let compact = {notation: "compact"};
+assertEquals("1.2K", (1234).toLocaleString("en", compact));
+assertEquals("12K", (12345).toLocaleString("en", compact));
+assertEquals("123K", (123456).toLocaleString("en", compact));
+assertEquals("1.2M", (1234567).toLocaleString("en", compact));
+
+assertEquals("54K", (54321).toLocaleString("en", compact));
+
+let compact_no_rounding = {notation: "compact", minimumFractionDigits: 0};
+assertEquals("1.234K", (1234).toLocaleString("en", compact_no_rounding));
+assertEquals("12.345K", (12345).toLocaleString("en", compact_no_rounding));
+assertEquals("123.456K", (123456).toLocaleString("en", compact_no_rounding));
+assertEquals("1.235M", (1234567).toLocaleString("en", compact_no_rounding));
+
+assertEquals("54.321K", (54321).toLocaleString("en", compact_no_rounding));
+
+let fmt = new Intl.NumberFormat("en", compact_no_rounding);
+assertEquals("1", fmt.format(1));
+assertEquals("1K", fmt.format(1000));
+assertEquals("1.234K", fmt.format(1234));
+assertEquals("1.25K", fmt.format(1250));