769871 - Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString per ECMA-402

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

This is one step towards a full implementation of the ECMAScript Internationalization API: Reimplement the three existing locale sensitive functions that the ECMAScript Internationalization spec redefines directly on top of ICU, using the default locale and default options per spec.

Comment 1

[Norbert Lindenberg]

The current implementations of these functions use different mechanisms to let software outside of SpiderMonkey influence their behavior, and we have to decide whether any of this has to be maintained in the future.

- For String.prototype.localeCompare, SpiderMonkey by default uses the dumb Unicode code unit comparison specified in ES5 11.8.5 (which is neither locale sensitive nor conforms to the specification of localeCompare in 15.5.4.9). Client software can provide better implementations via JS_SetLocaleCallbacks, and XPConnect does so to install implementations from the intl/ directory in the Mozilla tree. The implementations I've tested on Mac and Windows do not conform to ES5 15.5.4.9 either (they don't implement canonical equivalence).

- For Number.prototype.toLocaleString, SpiderMonkey has its own implementation, but obtains localized decimal separator, thousands separator, and grouping information from either the operating system or from environment variables. The latter mechanism is used by the Android GeckoAppShell.java classes. 

- For Date.prototype.toLocaleString, SpiderMonkey relies on the operating system's strftime function, and makes formatting based on strftime patterns available to applications through a non-standard method toLocaleFormat.


Proposal:

- Ignore localeCompare implementations provided by SpiderMonkey clients. No such implementation could conform to the ECMAScript Internationalization API specification because it doesn't have access to the new locales and options arguments. Such implementations will also no longer be necessary once SpiderMonkey relies on ICU to implement localeCompare. The field in the JSLocaleCallbacks struct remains in order to provide minimal binary compatibility with existing clients.

- Ignore decimal separator, thousands separator, and grouping information provided via environment variables. While such information could theoretically be merged into ICU output, it won't improve the results because the provider doesn't have access to the locales argument and ICU has all necessary information.

- Maintain Date.prototype.toLocaleFormat with its current functionality for compatibility with existing applications. However, this function will not benefit from the improvements provided by the ECMAScript Internationalization API (such as control over the language used). Date.prototype.toLocaleString will rely on ICU rather than strftime.

It may make sense to give SpiderMonkey containers a way to control the default locale used by locale sensitive functions; this is currently being discussed in bug 769872.

Comment 2

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

First functional version. Doesn't cache any ICU objects yet, which it likely will have to do to achieve reasonable performance. Uses file names intl.h and intl.cpp to avoid collisions with the jsintl.h and jsintl.cpp being added via bug 769872 - in the end, code will be merged into jsintl. Tested only in js shell.

Comment 3

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

Comment 4

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

Comment 5

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

Comment 6

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

Comment 7

[Norbert Lindenberg]

Comment on attachment 699997 [details] [diff] [review]
Reimplement String.localeCompare, Number.toLocaleString, Date.toLocaleString using ICU (WIP)

The upcoming attachments use a different design from the previous ones: They implement the locale sensitive functions in String, Number, and Date as self-hosted JavaScript on top of Intl.Collator, Intl.NumberFormat, and Intl.DateTimeFormat.

Comment 8

[Norbert Lindenberg]

Attachment: Reimplement String.localeCompare per ECMA-402.

Comment 9

[Norbert Lindenberg]

Attachment: Reimplement Number.toLocaleString per ECMA-402.

Comment 10

[Norbert Lindenberg]

Attachment: Reimplement Date.toLocaleString per ECMA-402.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 726761 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

Review of attachment 726761 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.cpp
@@ +448,5 @@
> +JS_ALWAYS_INLINE bool
> +num_nop(JSContext *cx, CallArgs args)
> +{
> +    JS_ASSERT(IsNumber(args.thisv()));
> +    return true;

It'll eventually be an error to return successfully from a JSNative without setting a return value, so add this, please:

args.rval().setUndefined();

@@ +457,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    // CallNonGenericMethod will handle proxies correctly and throw exceptions
> +    // in the right circumstances, but will report date_CheckThisDate as the

I imagine this date*Date bit is copypasta, please fix.  :-)

@@ +916,5 @@
>      JS_FN(js_valueOf_str,        js_num_valueOf,        0, 0),
>      JS_FN("toFixed",             num_toFixed,           1, 0),
>      JS_FN("toExponential",       num_toExponential,     1, 0),
>      JS_FN("toPrecision",         num_toPrecision,       1, 0),
> +// ??? must be last - functions listed after it aren't available in self-hosted code

Given the mixing we have in jsarray.cpp array_methods, I don't think this is necessary.  What's the behavior you see that causes you to think it is?

::: js/src/jsnum.h
@@ +152,5 @@
> +/**
> + * Checks that the this value provided meets the requirements for
> + * "this Number object" in ES5.1, 15.7.4, and throws a TypeError if not.
> + *
> + * Usage: callFunction(date_CheckThisNumber, this)

This could just be date_CheckThisNumber(this), right?  Don't see how callFunction is necessary for this case.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 726765 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

Review of attachment 726765 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Date.js
@@ +16,5 @@
> + * Spec: ECMAScript Internationalization API Specification, 13.3.1.
> + */
> +function Date_toLocaleString() {
> +    // Steps 1-2.
> +    callFunction(date_CheckThisDate, this);

date_checkThisDate(this);

Make the same change to the previous patches, in the self-hosted methods, where I previously only noted this for the header comment.  And to all the methods here, not just this one.

::: js/src/jsdate.cpp
@@ +1395,5 @@
> +JS_ALWAYS_INLINE bool
> +date_nop(JSContext *cx, CallArgs args)
> +{
> +    JS_ASSERT(IsDate(args.thisv()));
> +    return true;

More args.rval().setUndefined();.

@@ +3059,5 @@
>      JS_FN(js_toSource_str,       date_toSource,           0,0),
>  #endif
>      JS_FN(js_toString_str,       date_toString,           0,0),
>      JS_FN(js_valueOf_str,        date_valueOf,            0,0),
> +// ??? must be last - functions listed after it aren't available in self-hosted code

Still confused about this.

::: js/src/jsdate.h
@@ +68,5 @@
> +/**
> + * Checks that the this value provided meets the requirements for
> + * "this Date object" in ES5.1, 15.9.5, and throws a TypeError if not.
> + *
> + * Usage: callFunction(date_CheckThisDate, this)

Same date_CheckThisDate(this) again.

Comment 13

[Norbert Lindenberg]

Attachment: Reimplement Number.toLocaleString per ECMA-402.

Updated per comment 11. Carrying r+jwalden.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)

> @@ +916,5 @@
> >      JS_FN(js_valueOf_str,        js_num_valueOf,        0, 0),
> >      JS_FN("toFixed",             num_toFixed,           1, 0),
> >      JS_FN("toExponential",       num_toExponential,     1, 0),
> >      JS_FN("toPrecision",         num_toPrecision,       1, 0),
> > +// ??? must be last - functions listed after it aren't available in self-hosted code
> 
> Given the mixing we have in jsarray.cpp array_methods, I don't think this is
> necessary.  What's the behavior you see that causes you to think it is?

For String, my self-hosted code couldn't find split anymore - it looked like the function didn't exist. For Number, I don't remember exactly, but it may have been valueOf where it'd find only the one in Object.prototype. I'll file a ticket when this code has landed and is more available for testing.

The only Array methods that show up after the self-hosted entries in the method table are filter and iterator, neither of which are used in self-hosted code.

> > + * Usage: callFunction(date_CheckThisNumber, this)
> 
> This could just be date_CheckThisNumber(this), right?  Don't see how
> callFunction is necessary for this case.

date_CheckThisNumber(this) would be equivalent to callFunction(date_CheckThisNumber, undefined, this). CallNonGenericMethod expects the this value as args.thisv().

Comment 14

[Norbert Lindenberg]

Attachment: Reimplement Date.toLocaleString per ECMA-402.

Updated per comment 12. Carrying r+jwalden.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)

Same replies to same comments as above.

Comment 15

[Norbert Lindenberg]

(In reply to Norbert Lindenberg from comment #13)

> > > +// ??? must be last - functions listed after it aren't available in self-hosted code
> > 
> > Given the mixing we have in jsarray.cpp array_methods, I don't think this is
> > necessary.  What's the behavior you see that causes you to think it is?
> 
> For String, my self-hosted code couldn't find split anymore - it looked like
> the function didn't exist. For Number, I don't remember exactly, but it may
> have been valueOf where it'd find only the one in Object.prototype. I'll
> file a ticket when this code has landed and is more available for testing.
> 
> The only Array methods that show up after the self-hosted entries in the
> method table are filter and iterator, neither of which are used in
> self-hosted code.

My previous comment provided me with the idea for a simple test case, so there's now bug 853075.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 726759 [details] [diff] [review]
Reimplement String.localeCompare per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/71b015ae13c3

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 727091 [details] [diff] [review]
Reimplement Number.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bebe0562ab3

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 727093 [details] [diff] [review]
Reimplement Date.toLocaleString per ECMA-402.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d550a01e0ab2

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Norbert Lindenberg from comment #13)
> > > + * Usage: callFunction(date_CheckThisNumber, this)
> > 
> > This could just be date_CheckThisNumber(this), right?  Don't see how
> > callFunction is necessary for this case.
> 
> date_CheckThisNumber(this) would be equivalent to
> callFunction(date_CheckThisNumber, undefined, this). CallNonGenericMethod
> expects the this value as args.thisv().

Oh, right, I'm dumb.

Looking at this closer, it happens we get this non-generic test performed when we call Date.prototype.valueOf, actually.  So technically the helper, and the call to it, are unnecessary.  It was very tempting to just remove it...but probably that's enough change to want an actual patch and review.  I'll do that and post them here for you to test out, in a sec.

(In reply to Norbert Lindenberg from comment #15)
> My previous comment provided me with the idea for a simple test case, so
> there's now bug 853075.

Excellent!  Thank you for this.  I tweaked the comments on these to mention this bug, too.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Remove x_CheckThisX methods, as explained before

Comment 21

[Jeff Walden [:Waldo]]

Also pretend I removed the methods' additions from SelfHosting.cpp, turns out that's necessary to compile.  :-)

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71b015ae13c3
https://hg.mozilla.org/mozilla-central/rev/5bebe0562ab3
https://hg.mozilla.org/mozilla-central/rev/d550a01e0ab2

Comment 23

[Norbert Lindenberg]

Comment on attachment 728003 [details] [diff] [review]
Remove x_CheckThisX methods, as explained before

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #20)
> Created attachment 728003 [details] [diff] [review]
> Remove x_CheckThisX methods, as explained before

Most excellent! After pretending of course that the entries are removed from SelfHosting.cpp and those picky compilers are happy :-)

Related Test262 tests all pass.

Comment 24

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/195a2038433d

Comment 25

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/195a2038433d

Comment 26

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Also, please update the following docs:
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/localeCompare
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number/toLocaleString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleString

Comment 27

[Norbert Lindenberg]

While the code changes for this bug made it into m-c in time for Firefox 22, they are disabled, and the build system changes necessary to actually enable them are still under review (bug 724533). I've updated the target release to reflect this.

I moved the section from the Firefox 22 compatibility document to that for Firefox 23, and added more information.

I also updated the reference pages - would you like to review them?
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/String/localeCompare
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Number/toLocaleString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString

Comment 28

[Florian Scholz (Open Web Docs)]

Added a note to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/23#JavaScript as well.
Marking this as complete, looks good. Thanks Norbert!

Comment 29

[Norbert Lindenberg]

Sorry for the confusion, but the internationalization API and thus the changes in this bug have remained disabled for all Firefox releases at least up to 26. They're now enabled in nightly, so if all goes well they may get into Firefox 27. I've removed the note from the Firefox 23 release notes, to be added to the notes for the release in which the API actually ships. The bug that really determines whether the API is in or not is bug 853301.

Comment 30

[Florian Scholz (Open Web Docs)]

Going to mark this as complete as the reference docs for String.localeCompare, Number.toLocaleString, Date.toLocaleString are in place (see comment 27). I will use bug 853301 to update the dev doc release notes and compat tables as that bug is actually about enabling the Internationalization API and has less confusing version info.