Bug 184312 – [INTL] Implement Intl.PluralRules

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

[Andy VanWagoner]

[INTL] Implement Intl.PluralRules

[Andy VanWagoner]

Created attachment 337206 [details]
Patch

[Andy VanWagoner]

I've been getting a compile error: `Member access into incomplete type 'JSC::JSGlobalObject'`. I expect EWS will also get this error as it tries to build the patch. This error is blocking running real tests on the code right now, so there are likely additional problems to discover (beyond the obviously skeleton test expectation).

[Andy VanWagoner]

Simply adding the files seems trigger this error, particularly adding them to Sources.txt. I can completely empty the IntlPluralRules files of all content and still get this error. :(

[JF Bastien]

I think you should move the implementation of:
  static BigIntObject* create(VM& vm, JSGlobalObject* globalObject, JSBigInt* bigInt)
to BigIntObject.cpp, and in that file #include "JSGlobalObject.h"

[Andy VanWagoner]

(In reply to JF Bastien from comment #4)
> I think you should move the implementation of:
>   static BigIntObject* create(VM& vm, JSGlobalObject* globalObject,
> JSBigInt* bigInt)
> to BigIntObject.cpp, and in that file #include "JSGlobalObject.h"

That fixed the compile error. Thanks! I'll spend some time making sure the implementation and tests are in good shape before pushing up another patch.

[Andy VanWagoner]

Created attachment 337647 [details]
Patch

[EWS Watchlist]

Comment on attachment 337647 [details]
Patch

Attachment 337647 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7278696

New failing tests:
js/intl-pluralrules.html

[EWS Watchlist]

Created attachment 337680 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Andy VanWagoner]

Created attachment 337705 [details]
Requires ICU v59+ to be accurate

[Andy VanWagoner]

Comment on attachment 337705 [details]
Requires ICU v59+ to be accurate

This patch should provide a great working implementation of Intl.PluralRules if compiled with ICU v59 or higher.

With older ICU, we can expect the resolvedOptions().pluralCategories to always return the full list of keywords instead of only the applicable ones, and minimumFractionDigits to generate trailing 0s that will get lost again when converting to a double.

Due to the way the spec is written, `new Intl.PluralRules('en').select('1.0')` will always return `'one'` because the string is parsed to number (losing the trailing 0), and then reformatted (which reliably omits the trailing 0). Both Gecko and V8 appear to have the same behavior.

If we are unable to update ICU to v59 or higher, there are a few other options to consider:

1. Can we use the C++ api? The PluralRules class has getKeywords() since v4.0. iirc there is a reason we are only using the C apis.

2. We can try to read and parse the plural rules at runtime, emulating what the ICU apis do. I'm not a fan of trying to replicate this logic at runtime due to the complexity and difficulty in validating its correctness.

3. Write a script to generate code for each selection algorithm at build time. I have done this before for a JavaScript library[1]. In this case we can generate either C++ code or builtin JavaScript.

[1] https://github.com/format-message/format-message/blob/master/packages/format-message-interpret/plurals.js

[Andy VanWagoner]

Created attachment 337711 [details]
Rebased

[JF Bastien]

Check out what Source/JavaScriptCore/runtime/IntlDateTimeFormat.h does for JSC_ICU_HAS_UFIELDPOSITER. I think we want something similar: the feature is available if the ICU version supports it.

I'm not familiar with what it takes to update the ICU version. I think it's based on the platform, so out of our control?

Otherwise do we really need to support this if ICU is older? I'm not sure we want to do all we can to support this feature if ICU doesn't have it yet. It's a problem that'll resolve itself over time.

On C versus C++ API, I'm guessing we use C because it has a stable ABI and C++ doesn't. I don't know this for a fact.

[Andy VanWagoner]

Created attachment 337725 [details]
Without misleading pluralCategories

[Andy VanWagoner]

Comment on attachment 337725 [details]
Without misleading pluralCategories

This patch simply doesn't set the pluralCategories at all if they can't be accurate. select() matches the results for V8, and with ICU 59 matches SpiderMonkey. With ICU 59 the pluralCategories matches both other implementations.

[Andy VanWagoner]

Created attachment 337726 [details]
Rebased

[EWS Watchlist]

Comment on attachment 337726 [details]
Rebased

Attachment 337726 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7286959

New failing tests:
stress/ftl-get-by-id-getter-exception-interesting-live-state.js.ftl-eager-no-cjit

[JF Bastien]

Comment on attachment 337726 [details]
Rebased

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

A few comments, looks good otherwise.

I think you also need a runtime flag for this? https://lists.webkit.org/pipermail/webkit-dev/2018-April/029943.html

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:132
> +        m_type = UPLURAL_TYPE_ORDINAL;

I know you initialize to CARDINAL, but I'd rather at least debug assert here that typeString == "cardinal" && m_type == CARDINAL.

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:200
> +    }

You can still proceed here, even if m_initializedPluralRules is still false?

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:208
> +    if (m_minimumSignificantDigits) {

It kinda feels like these members should be a std::optional instead, which you then test the same way but access through .value() ? That way you don't just use 0 as a placeholder for "nothing", and you know it'll crash if you have a logic error (so the assert below isn't required because std::optional has your back).

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:222
> +    ASSERT(U_SUCCESS(status));

This can't fail through user input?

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:231
> +    RETURN_IF_EXCEPTION(scope, { });

I think you need this for every push? So the uenum_close needs to be a unique_ptr as well.

[Andy VanWagoner]

Comment on attachment 337726 [details]
Rebased

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

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:132
>> +        m_type = UPLURAL_TYPE_ORDINAL;
> 
> I know you initialize to CARDINAL, but I'd rather at least debug assert here that typeString == "cardinal" && m_type == CARDINAL.

Will use a ternary instead.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:200
>> +    }
> 
> You can still proceed here, even if m_initializedPluralRules is still false?

Honestly, I don't think it should be possible to get here without m_initializedPluralRules == true, so I'll throw instead.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:208
>> +    if (m_minimumSignificantDigits) {
> 
> It kinda feels like these members should be a std::optional instead, which you then test the same way but access through .value() ? That way you don't just use 0 as a placeholder for "nothing", and you know it'll crash if you have a logic error (so the assert below isn't required because std::optional has your back).

Yeah, that sounds better. I had just copied the logic that NumberFormat used. Will update.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:222
>> +    ASSERT(U_SUCCESS(status));
> 
> This can't fail through user input?

I don't believe it can fail from user input. Especially if I throw above if this PluralRules was not initialized properly.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:231
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> I think you need this for every push? So the uenum_close needs to be a unique_ptr as well.

K, will do.

[Andy VanWagoner]

Created attachment 338132 [details]
Patch

[Andy VanWagoner]

Created attachment 338137 [details]
Patch

[JF Bastien]

Comment on attachment 338137 [details]
Patch

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

> Source/JavaScriptCore/runtime/IntlObject.cpp:118
> +    putDirectWithoutTransition(vm, vm.propertyNames->PluralRules, pluralRulesConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum));

You need to do this conditionally under a runtime flag: https://lists.webkit.org/pipermail/webkit-dev/2018-April/029943.html

> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:167
> +        m_maximumSignificantDigits = std::optional<unsigned>(maximumSignificantDigits);

Why cast here? You can just assign an unsigned to an optional<unsigned>.

[Andy VanWagoner]

Comment on attachment 338137 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlObject.cpp:118
>> +    putDirectWithoutTransition(vm, vm.propertyNames->PluralRules, pluralRulesConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> You need to do this conditionally under a runtime flag: https://lists.webkit.org/pipermail/webkit-dev/2018-April/029943.html

k, I'll add that.

>> Source/JavaScriptCore/runtime/IntlPluralRules.cpp:167
>> +        m_maximumSignificantDigits = std::optional<unsigned>(maximumSignificantDigits);
> 
> Why cast here? You can just assign an unsigned to an optional<unsigned>.

Because I didn't know that. :) Will fix.

[Andy VanWagoner]

I have been trying to figure out how to use runtime feature flags in jsc, and the closest I've found are Options. Is this how it's intended for runtime feature flags to work in jsc?

runtime/Options.h
    v(bool, useIntlPluralRules, false, Normal, "If true, we will enable Intl.PluralRules.") \

runtime/IntlObject.cpp
if (Options::useIntlPluralRules())
    putDirectWithoutTransition(vm, vm.propertyNames->PluralRules, pluralRulesConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum));

LayoutTests/js/script-tests/intl-pluralrules.js
//@ runDefault("--useIntlPluralRules=true")

This seems to work to disable the feature, but the tests are not running with the feature enabled. Is this the right path to be going down?

[JF Bastien]

(In reply to Andy VanWagoner from comment #23)
> I have been trying to figure out how to use runtime feature flags in jsc,
> and the closest I've found are Options. Is this how it's intended for
> runtime feature flags to work in jsc?
> 
> runtime/Options.h
>     v(bool, useIntlPluralRules, false, Normal, "If true, we will enable
> Intl.PluralRules.") \
> 
> runtime/IntlObject.cpp
> if (Options::useIntlPluralRules())
>     putDirectWithoutTransition(vm, vm.propertyNames->PluralRules,
> pluralRulesConstructor, static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> LayoutTests/js/script-tests/intl-pluralrules.js
> //@ runDefault("--useIntlPluralRules=true")
> 
> This seems to work to disable the feature, but the tests are not running
> with the feature enabled. Is this the right path to be going down?

You'll want to use a JSC option as you're doing, as well as a define as done here:
  https://trac.webkit.org/changeset/224787/webkit

[Andy VanWagoner]

Created attachment 338429 [details]
Behind feature flag

[EWS Watchlist]

Comment on attachment 338429 [details]
Behind feature flag

Attachment 338429 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7384090

New failing tests:
jsc-layout-tests.yaml/js/script-tests/intl-pluralrules.js.default

[JF Bastien]

Looks like jsc-layout-tests.yaml/js/script-tests/intl-pluralrules.js.default fails?

[Andy VanWagoner]

Created attachment 338541 [details]
Patch

[Andy VanWagoner]

Comment on attachment 338541 [details]
Patch

I removed the comment I had thought would have told the test runner this test relies on a feature flag.

[Andy VanWagoner]

(In reply to JF Bastien from comment #27)
> Looks like jsc-layout-tests.yaml/js/script-tests/intl-pluralrules.js.default
> fails?

Yeah, it was dying on the `description()` call. Is that test runner not defining description but instead trying to skip some number of initial lines?

[Andy VanWagoner]

Created attachment 338585 [details]
Patch

[Andy VanWagoner]

Comment on attachment 338585 [details]
Patch

Doesn't try to create a 0 length array. Should make VS happier.

[JF Bastien]

./runtime/RegExpPrototype.cpp:511:12: error: use of undeclared identifier 'advanceStringUnicode'
    return advanceStringUnicode(str, strSize, index);
           ^

[Andy VanWagoner]

(In reply to JF Bastien from comment #33)
> ./runtime/RegExpPrototype.cpp:511:12: error: use of undeclared identifier
> 'advanceStringUnicode'
>     return advanceStringUnicode(str, strSize, index);
>            ^

:( I don't comprehend how removing an array declaration in IntlPluralRules.cpp suddenly causes advanceStringUnicode to be undeclared in RegExpPrototype.cpp, but I guess I need to unbreak that now too.

[Andy VanWagoner]

Created attachment 338593 [details]
Patch

[JF Bastien]

Comment on attachment 338593 [details]
Patch

Overall this looks good, but it looks like Windows build is sad?

[Andy VanWagoner]

(In reply to JF Bastien from comment #36)
> Comment on attachment 338593 [details]
> Patch
> 
> Overall this looks good, but it looks like Windows build is sad?

Windows does seem to be having trouble. I'm not sure why wincairo can't apply the patch, and the win errors doesn't look related to what I've changed. I went to rebase, but I'm already on the latest.

[Andy VanWagoner]

Created attachment 338654 [details]
rebased

[Andy VanWagoner]

Comment on attachment 338654 [details]
rebased

Windows failure looks like it doesn't respect the #pragma once in MacroAssemblerX86Common.h. iOS Simulator has some flaky tests. Neither appear to me to be related to this change.

[JF Bastien]

Comment on attachment 338654 [details]
rebased

r=me thanks for implementing this!

[WebKit Commit Bot]

Comment on attachment 338654 [details]
rebased

Clearing flags on attachment: 338654

Committed r231047: <https://trac.webkit.org/changeset/231047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39760186>

[Ryan Haddad]

(In reply to Andy VanWagoner from comment #39)
> Comment on attachment 338654 [details]
> rebased
> 
> Windows failure looks like it doesn't respect the #pragma once in
> MacroAssemblerX86Common.h.

The Windows build is failing now that this change has landed, so it does seem that it is somehow related:

https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/9044

[Andy VanWagoner]

(In reply to Ryan Haddad from comment #44)
> (In reply to Andy VanWagoner from comment #39)
> > Comment on attachment 338654 [details]
> > rebased
> > 
> > Windows failure looks like it doesn't respect the #pragma once in
> > MacroAssemblerX86Common.h.
> 
> The Windows build is failing now that this change has landed, so it does
> seem that it is somehow related:
> 
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 9044

I suspect that the way unified sources batches files together makes adding or removing source files change what might incidentally be included together. Otherwise, I have no explanation as to how this change could have impacted MacroAssemblerX86Common.h.

I can dig in to see why "error C2061: syntax error: identifier 'BaseIndex'", but it will probably take a few days for me to find the time.

[Keith Miller]

(In reply to Andy VanWagoner from comment #45)
> (In reply to Ryan Haddad from comment #44)
> > (In reply to Andy VanWagoner from comment #39)
> > > Comment on attachment 338654 [details]
> > > rebased
> > > 
> > > Windows failure looks like it doesn't respect the #pragma once in
> > > MacroAssemblerX86Common.h.
> > 
> > The Windows build is failing now that this change has landed, so it does
> > seem that it is somehow related:
> > 
> > https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> > 9044
> 
> I suspect that the way unified sources batches files together makes adding
> or removing source files change what might incidentally be included
> together. Otherwise, I have no explanation as to how this change could have
> impacted MacroAssemblerX86Common.h.
> 
> I can dig in to see why "error C2061: syntax error: identifier 'BaseIndex'",
> but it will probably take a few days for me to find the time.

Yeah, that's totally possible. Looking a the build log my guess is that MacroAssemblerX86Common.h isn't including MacroAssembler.h... I'll try a fix on the bots and see if that fixes things.

[Mark Lam]

FYI, no one should #include MacroAssemblerX86Common.h directly.  They should always #include MacroAssembler.h which will indirectly #include MacroAssemblerX86Common.h.

[Mark Lam]

I see an issue: Options.cpp is #include'ing MacroAssemblerX86.h directly.  I don't think it's related to this patch.

[Mark Lam]

(In reply to Mark Lam from comment #48)
> I see an issue: Options.cpp is #include'ing MacroAssemblerX86.h directly.  I
> don't think it's related to this patch.

I've landed a Windows build fix for the TARGET_ASSEMBLER issue in r231077: <http://trac.webkit.org/r231077>.