Assortment of Intl fixes and cleanup

* Properly name Intl.PluralRules functions
* Address issues brought up in the May 2018 and June 2018 Intl call
* Address previously uncaught issue where we failed fast unintentionally
when handling the sign part in ICU > 61.
* Always use null-prototyped property descriptors to avoid Object.prototype.get
tainting.

Author: jackhorton

lib/Runtime/Library/InJavascript/Intl.js

@@ -136,7 +136,11 @@
 
         keys: platform.builtInJavascriptObjectEntryKeys,
         hasOwnProperty(o, prop) { return callInstanceFunc(platform.builtInJavascriptObjectEntryHasOwnProperty, o, prop); },
-        defineProperty: platform.builtInJavascriptObjectEntryDefineProperty,
+        // If we don't set the descriptor's prototype to null, defining properties with `value`s can fail of Object.prototype.get is defined
+        defineProperty(o, prop, desc) {
+            _.setPrototypeOf(desc, null);
+            platform.builtInJavascriptObjectEntryDefineProperty(o, prop, desc);
+        },
         isExtensible: platform.builtInJavascriptObjectEntryIsExtensible,
         create(proto = null) { return platform.builtInJavascriptObjectCreate(proto); },
         setPrototypeOf(target, proto = null) { return platform.builtInSetPrototype(target, proto); },
@@ -846,19 +850,17 @@
             ? BestFitSupportedLocales(isAvailableLocale, requestedLocales)
             : LookupSupportedLocales(isAvailableLocale, requestedLocales);
 
-        // make sure property descriptor is null-prototyped to avoid tainting of Object.prototype.{get|set}
-        const descriptor = _.setPrototypeOf({ configurable: false, writable: false });
         for (let i = 0; i < supportedLocales.length; i++) {
-            _.defineProperty(supportedLocales, Internal.ToString(i), descriptor);
+            _.defineProperty(supportedLocales, Internal.ToString(i), { configurable: false, writable: false });
         }
 
         // test262 supportedLocalesOf-returned-array-elements-are-frozen.js:
         // Property length of object returned by SupportedLocales should not be writable
-        _.defineProperty(supportedLocales, "length", _.setPrototypeOf({
+        _.defineProperty(supportedLocales, "length", {
             writable: false,
             configurable: false,
             enumerable: false,
-        }));
+        });
 
         return supportedLocales;
     };
@@ -2015,7 +2017,7 @@
         };
 
         // params are explicitly `= undefined` to make PluralRules.length === 0
-        const PluralRules = function (locales = undefined, options = undefined) {
+        const PluralRules = function PluralRules(locales = undefined, options = undefined) {
             if (new.target === undefined) {
                 platform.raiseNeedObjectOfType("Intl.PluralRules", "PluralRules");
             }
@@ -2058,7 +2060,7 @@
         });
 
         // ECMA 402: #sec-intl.pluralrules.prototype.select
-        const select = function (value) {
+        const select = function select(value) {
             const pr = platform.getHiddenObject(this);
             if (!pr || !pr.initializedPluralRules) {
                 platform.raiseNeedObjectOfType("Intl.PluralRules.prototype.select", "PluralRules");
@@ -2075,31 +2077,29 @@
             writable: true,
         });
 
-        const resolvedOptions = function () {
+        const resolvedOptions = function resolvedOptions() {
             const pr = platform.getHiddenObject(this);
             if (!pr || !pr.initializedPluralRules) {
                 platform.raiseNeedObjectOfType("Intl.PluralRules.prototype.select", "PluralRules");
             }
 
-            const options = {};
-            _.forEach([
+            return createResolvedOptions([
                 "locale",
                 "type",
                 "minimumIntegerDigits",
                 "minimumFractionDigits",
                 "maximumFractionDigits",
                 "minimumSignificantDigits",
-                "maximumSignificantDigits"
-            ], (prop) => {
-                if (pr[prop] !== undefined) {
-                    options[prop] = pr[prop];
+                "maximumSignificantDigits",
+                "pluralCategories"
+            ], pr, (prop, resolved) => {
+                if (prop === "pluralCategories") {
+                    // https://github.com/tc39/ecma402/issues/224: create a copy of the pluralCategories array
+                    resolved.pluralCategories = _.slice(pr.pluralCategories, 0);
+                } else {
+                    resolved[prop] = pr[prop];
                 }
             });
-
-            // https://github.com/tc39/ecma402/issues/224: create a copy of the pluralCategories array
-            options.pluralCategories = _.slice(pr.pluralCategories, 0);
-
-            return options;
         };
         tagPublicFunction("Intl.PluralRules.prototype.resolvedOptions", resolvedOptions);
         _.defineProperty(PluralRules.prototype, "resolvedOptions", {
@@ -2114,10 +2114,10 @@
 
     // Initialize Intl properties only if needed
     if (InitType === "Intl") {
-        ObjectDefineProperty(Intl, "Collator",              { value: Collator,              writable: true, enumerable: false, configurable: true });
-        ObjectDefineProperty(Intl, "NumberFormat",          { value: NumberFormat,          writable: true, enumerable: false, configurable: true });
-        ObjectDefineProperty(Intl, "DateTimeFormat",        { value: DateTimeFormat,        writable: true, enumerable: false, configurable: true });
-        ObjectDefineProperty(Intl, "PluralRules",           { value: PluralRules,           writable: true, enumerable: false, configurable: true });
+        _.defineProperty(Intl, "Collator",              { value: Collator,              writable: true, enumerable: false, configurable: true });
+        _.defineProperty(Intl, "NumberFormat",          { value: NumberFormat,          writable: true, enumerable: false, configurable: true });
+        _.defineProperty(Intl, "DateTimeFormat",        { value: DateTimeFormat,        writable: true, enumerable: false, configurable: true });
+        _.defineProperty(Intl, "PluralRules",           { value: PluralRules,           writable: true, enumerable: false, configurable: true });
     }
 
     }

lib/Runtime/Library/IntlEngineInterfaceExtensionObject.cpp

@@ -1457,6 +1457,25 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
 #endif
     }
 
+#ifdef INTL_ICU
+    // This is used by both NumberFormat and PluralRules
+    static void SetUNumberFormatDigitOptions(UNumberFormat *fmt, DynamicObject *state)
+    {
+        if (JavascriptOperators::HasProperty(state, PropertyIds::minimumSignificantDigits))
+        {
+            unum_setAttribute(fmt, UNUM_SIGNIFICANT_DIGITS_USED, true);
+            unum_setAttribute(fmt, UNUM_MIN_SIGNIFICANT_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumSignificantDigits));
+            unum_setAttribute(fmt, UNUM_MAX_SIGNIFICANT_DIGITS, AssertIntegerProperty(state, PropertyIds::maximumSignificantDigits));
+        }
+        else
+        {
+            unum_setAttribute(fmt, UNUM_MIN_INTEGER_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumIntegerDigits));
+            unum_setAttribute(fmt, UNUM_MIN_FRACTION_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumFractionDigits));
+            unum_setAttribute(fmt, UNUM_MAX_FRACTION_DIGITS, AssertIntegerProperty(state, PropertyIds::maximumFractionDigits));
+        }
+    }
+#endif
+
     Var IntlEngineInterfaceExtensionObject::EntryIntl_CacheNumberFormat(RecyclableObject * function, CallInfo callInfo, ...)
     {
         EngineInterfaceObject_CommonFunctionProlog(function, callInfo);
@@ -1515,18 +1534,7 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
 
         unum_setAttribute(*fmt, UNUM_ROUNDING_MODE, UNUM_ROUND_HALFUP);
 
-        if (JavascriptOperators::HasProperty(state, PropertyIds::minimumSignificantDigits))
-        {
-            unum_setAttribute(*fmt, UNUM_SIGNIFICANT_DIGITS_USED, true);
-            unum_setAttribute(*fmt, UNUM_MIN_SIGNIFICANT_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumSignificantDigits));
-            unum_setAttribute(*fmt, UNUM_MAX_SIGNIFICANT_DIGITS, AssertIntegerProperty(state, PropertyIds::maximumSignificantDigits));
-        }
-        else
-        {
-            unum_setAttribute(*fmt, UNUM_MIN_INTEGER_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumIntegerDigits));
-            unum_setAttribute(*fmt, UNUM_MIN_FRACTION_DIGITS, AssertIntegerProperty(state, PropertyIds::minimumFractionDigits));
-            unum_setAttribute(*fmt, UNUM_MAX_FRACTION_DIGITS, AssertIntegerProperty(state, PropertyIds::maximumFractionDigits));
-        }
+        SetUNumberFormatDigitOptions(*fmt, state);
 
         if (currency != nullptr)
         {
@@ -2256,8 +2264,10 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
             // TODO(jahorto): Determine if this would ever be returned and what it would map to
             case UNUM_PERMILL_FIELD: AssertOrFailFastMsg(false, "Unexpected permill field");
 
-            case UNUM_SIGN_FIELD: num < 0 ? library->GetIntlMinusSignPartString() : library->GetIntlPlusSignPartString();
-            default: AssertOrFailFastMsg(false, "Unexpected unknown part"); return nullptr;
+            case UNUM_SIGN_FIELD: return num < 0 ? library->GetIntlMinusSignPartString() : library->GetIntlPlusSignPartString();
+
+            // At the ECMA-402 TC39 call for May 2017, it was decided that we should treat unmapped parts as type: "unknown"
+            default: return library->GetIntlUnknownPartString();
             }
         }
 
@@ -2418,7 +2428,7 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
         {
             return unum_formatDouble(*fmt, num, buf, bufLen, nullptr, status);
         }, scriptContext->GetRecycler(), &formatted, &formattedLen);
-        JavascriptOperators::InitProperty(part, PropertyIds::type, library->GetIntlLiteralPartString());
+        JavascriptOperators::InitProperty(part, PropertyIds::type, library->GetIntlUnknownPartString());
         JavascriptOperators::InitProperty(part, PropertyIds::value, JavascriptString::NewWithBuffer(formatted, formattedLen, scriptContext));
 
         ret->SetItem(0, part, PropertyOperationFlags::PropertyOperation_None);
@@ -2754,7 +2764,6 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
                 typeString = library->GetIntlLiteralPartString(); break;
 #endif
             default:
-                AssertMsg(false, "Unmapped UDateFormatField");
                 typeString = library->GetIntlUnknownPartString(); break;
             }
 
@@ -3046,11 +3055,35 @@ DEFINE_ISXLOCALEAVAILABLE(PR, uloc)
 
         FinalizableUPluralRules *pr = GetOrCreatePluralRulesCache(state, scriptContext);
 
+        // ICU has an internal API, uplrules_selectWithFormat, that is equivalent to uplrules_select but will respect digit options of the passed UNumberFormat.
+        // Since its an internal API, we can't use it -- however, we can work around it by creating a UNumberFormat with provided digit options,
+        // formatting the requested number to a string, and then converting the string back to a double which we can pass to uplrules_select.
+        // This workaround was suggested during the May 2018 ECMA-402 discussion.
+        // TODO(jahorto): investigate caching this UNumberFormat on the state as well. This is currently not possible because we are using InternalProperyIds::HiddenObject
+        // for all ICU object caching, but once we move to better names for the cache property IDs, we can cache both the UNumberFormat as well as the UPluralRules.
+        char localeID[ULOC_FULLNAME_CAPACITY] = { 0 };
+        LangtagToLocaleID(AssertStringProperty(state, PropertyIds::locale), localeID);
+        UErrorCode status = U_ZERO_ERROR;
+        FinalizableUNumberFormat *fmt = FinalizableUNumberFormat::New(scriptContext->GetRecycler(), unum_open(UNUM_DECIMAL, nullptr, 0, localeID, nullptr, &status));
+
+        SetUNumberFormatDigitOptions(*fmt, state);
+
+        char16 *formattedN = nullptr;
+        int formattedNLength = 0;
+        EnsureBuffer([&](UChar *buf, int bufLen, UErrorCode *status)
+        {
+            return unum_formatDouble(*fmt, n, buf, bufLen, nullptr, status);
+        }, scriptContext->GetRecycler(), &formattedN, &formattedNLength);
+
+        double nWithOptions = unum_parseDouble(*fmt, reinterpret_cast<UChar *>(formattedN), formattedNLength, nullptr, &status);
+        double roundtripDiff = n - nWithOptions;
+        ICU_ASSERT(status, roundtripDiff <= 1.0 && roundtripDiff >= -1.0);
+
         char16 *selected = nullptr;
         int selectedLength = 0;
         EnsureBuffer([&](UChar *buf, int bufLen, UErrorCode *status)
         {
-            return uplrules_select(*pr, n, buf, bufLen, status);
+            return uplrules_select(*pr, nWithOptions, buf, bufLen, status);
         }, scriptContext->GetRecycler(), &selected, &selectedLength);
 
         return JavascriptString::NewWithBuffer(selected, static_cast<charcount_t>(selectedLength), scriptContext);

test/Intl/NumberFormat.js

@@ -190,7 +190,7 @@ const tests = [
                     // real formatToParts support was only added with ICU 61
                     assert.areEqual(1, actualParts.length, `formatToParts(${n}) stub implementation should return only one part`);
                     const literal = actualParts[0];
-                    assert.areEqual("literal", literal.type, `formatToParts(${n}) stub implementation should return a literal part`);
+                    assert.areEqual("unknown", literal.type, `formatToParts(${n}) stub implementation should return an unknown part`);
                     assert.areEqual(nf.format(n), literal.value, `formatToParts(${n}) stub implementation should return one part whose value matches the fully formatted number`);
                     return;
                 }
@@ -207,6 +207,12 @@ const tests = [
                 { type: "group", value: "," },
                 { type: "integer", value: "000" }
             ]);
+            assertParts("en-US", undefined, -1000, [
+                { type: "minusSign", value: "-" },
+                { type: "integer" , value: "1" },
+                { type: "group", value: "," },
+                { type: "integer", value: "000" }
+            ]);
             assertParts("en-US", undefined, NaN, [{ type: "nan", value: "NaN" }]);
             assertParts("en-US", undefined, Infinity, [{ type: "infinity", value: "∞" }]);
             assertParts("en-US", undefined, 1000.3423, [

test/Intl/PluralRules.js

@@ -109,5 +109,42 @@ testRunner.runTests([
             opts1.pluralCategories[0] = "changed";
             assert.areNotEqual(opts1.pluralCategories[0], pr1.resolvedOptions().pluralCategories[0], "Changing the pluralCategories from one call to resolvedOptions should not impact future calls");
         }
-    }
+    },
+    {
+        name: "Number digit options",
+        body() {
+            function test(options, n, expected) {
+                const pr = new Intl.PluralRules("en", options);
+                assert.areEqual(expected, pr.select(n), `Incorrect result using n = ${n} and options = ${JSON.stringify(options)}`);
+            }
+
+            test(undefined, 1.0, "one");
+            test(undefined, 1.1, "other");
+            test(undefined, 1.001, "other");
+
+            test({ minimumFractionDigits: 1 }, 1.0, "one");
+            test({ minimumFractionDigits: 1 }, 1.1, "other");
+            test({ minimumFractionDigits: 1 }, 1.001, "other");
+
+            test({ maximumFractionDigits: 0 }, 1.0, "one");
+            test({ maximumFractionDigits: 0 }, 1.1, "one");
+            test({ maximumFractionDigits: 0 }, 1.001, "one");
+
+            test({ maximumFractionDigits: 1 }, 1.0, "one");
+            test({ maximumFractionDigits: 1 }, 1.1, "other");
+            test({ maximumFractionDigits: 1 }, 1.001, "one");
+
+            test({ minimumSignificantDigits: 2 }, 1.0, "one");
+            test({ minimumSignificantDigits: 2 }, 1.1, "other");
+            test({ minimumSignificantDigits: 2 }, 1.001, "other");
+
+            test({ maximumSignificantDigits: 2 }, 1.0, "one");
+            test({ maximumSignificantDigits: 2 }, 1.1, "other");
+            test({ maximumSignificantDigits: 2 }, 1.001, "one");
+
+            test({ maximumSignificantDigits: 1 }, 1.0, "one");
+            test({ maximumSignificantDigits: 1 }, 1.1, "one");
+            test({ maximumSignificantDigits: 1 }, 1.001, "one");
+        }
+    },
 ], { verbose: false });

test/Intl/common.js

@@ -7,6 +7,10 @@ WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
 
 const constructors = [Intl.Collator, Intl.NumberFormat, Intl.DateTimeFormat];
 
+if (WScript.Platform.INTL_LIBRARY === "icu") {
+    constructors.push(Intl.PluralRules);
+}
+
 testRunner.runTests([
     {
         name: "OSS-Fuzz #6657: stress uloc_forLanguageTag status code and parsed length on duplicate variant subtags",