Closed Bug 1263040 Opened 8 years ago Closed 8 years ago

Add Intl.getCanonicalLocales

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

Latest ECMA 402 spec exposes CanonicalizeLocaleList as Intl.getCanonicalLocales - https://tc39.github.io/ecma402/#intl-object

It's a very useful feature for all language negotiation operations and it basically just requires us to expose an already existing operation.
Attached patch bug1263040.patch (obsolete) — Splinter Review
Jeff, can you review this for me?
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8739270 - Flags: review?(jwalden+bmo)
Comment on attachment 8739270 [details] [diff] [review]
bug1263040.patch

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

Please include a variety of tests in the next iteration of this patch.  Add tests to js/src/tests/Intl/, with test file names that attempt to describe what's being tested (e.g. getCanonicalLocales-with-duplicates.js for the first issue noted below).  Ideally one test per file.  (Although it'd probably be good to have one basic test that just tests a big random list of things and their effects, as well -- that could be one file.)  If you haven't done it, at this point you should take the time to learn how to build SpiderMonkey.  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation  And *definitely* you need to run tests (python tests/jstests.py objdir/dist/bin/js Intl/ to run all the Intl/ tests) to verify that 1) your patch works, and 2) that your tests work.

::: js/src/builtin/Intl.js
@@ +2888,5 @@
> +function Intl_getCanonicalLocales(locales) {
> +  let codes = CanonicalizeLocaleList(locales);
> +  let result = [];
> +
> +  let len = locales.length;

This should be |codes.length| -- you're running off the end of |codes| if you do it this way:

  var locales = Intl.getCanonicalLocales(["en-US", "en-US"]);
  assertEq(locales.length, 1); // would be 2 with this patch

This also would incorrectly access .length multiple times on the input:

  var count = 0;
  var locs = { get length() { if (count++ > 0) throw 42; return 0; } };
  var locales = Intl.getCanonicalLocales(locs); // shouldn't throw 42
  assertEq(locales.length, 0);

@@ +2892,5 @@
> +  let len = locales.length;
> +  let k = 0;
> +
> +  while (k < len) {
> +    result.push(codes[k]);

Either this doesn't build, or it'll be a runtime failure.

The problem with |obj.fun()| in self-hosted code like this is that it's often possible for an adversarial script to make |.fun| not be what the self-hosted code thinks it is.  So we *require* that for a call like that, either |callContentFunction| (if the function might be overridden) or |callFunction| (if the function can only be a self-hosted function) must be used, an entirely different syntactic form.

I can't remember whether failure to use one of these is a build error, a runtime error/assertion when the self-hosted code is parsed, or a logic error in that the the code could do the wrong thing at runtime:

  Array.prototype.push = () => { throw 42; };
  Intl.getCanonicalLocales(["en-US"]); // must not throw 42, might if push is used

Moreover, .push will *set* elements rather than defining them, so if this actually runs that far, this would be wrong as well:

  Object.defineProperty(Array.prototype, 0, { set() { throw 42; } });
  Intl.getCanonicalLocales(["en-US"]); // must not throw 42, might if push is used

But in any case, this code is definitely wrong.
Attachment #8739270 - Flags: review?(jwalden+bmo) → review-
On a more bikeshed point -- is it just me, or is "getCanonicalLocales" a bad name for the method?  When I read "get" here, I think of accessing some sort of internal state and exposing it.  I don't think of a function whose behavior is entirely prescribed by its input.  "canonicalizeLocaleList" seems like a better name to me, with the mini-bonus of being the name for the underlying spec operation already.  (Although it's a bit verbose.  "createLocaleList" or something would be mildly better still, but I'm fast running out of interest in finding the perfect name here.  :-) )
Attached patch patch, v2 (obsolete) — Splinter Review
Updated version of the patch with tests.


(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> If you
> haven't done it, at this point you should take the time to learn how to
> build SpiderMonkey. 
> https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/
> Build_Documentation  And *definitely* you need to run tests (python
> tests/jstests.py objdir/dist/bin/js Intl/ to run all the Intl/ tests) to
> verify that 1) your patch works, and 2) that your tests work.

I did, the previous patch compiles and runs all tests, I just didn't add any tests for it yet.

> @@ +2892,5 @@
> > +  let len = locales.length;
> > +  let k = 0;
> > +
> > +  while (k < len) {
> > +    result.push(codes[k]);
> 
> Either this doesn't build, or it'll be a runtime failure.

Neither. I was able to successfully run it without problems. I changed the way I define properties, but if it's supposed to be a build failure, it's not.

> Moreover, .push will *set* elements rather than defining them, so if this
> actually runs that far, this would be wrong as well:
> 
>   Object.defineProperty(Array.prototype, 0, { set() { throw 42; } });
>   Intl.getCanonicalLocales(["en-US"]); // must not throw 42, might if push
> is used

I was unable to make this test pass.

In this version of the patch I use `_DefineDataProperty` which is used elsewhere in the file, and it still throws here.

According to spec I should be using CreateArrayFromList[0] which uses CreateDataProperty[1] which uses something called "DefineOwnProperty" but `Object.defineProperty` doesn't work in that context (returns `TypeError: Object.defineProperty is not a function`).

What should I use here to make this test pass?

Also, I'll work with ECMA402 editor to add those tests to test262 and I'll bring up your feedback on the function name.

[0] https://tc39.github.io/ecma262/#sec-createarrayfromlist
[1] https://tc39.github.io/ecma262/#sec-createdataproperty
Attachment #8739270 - Attachment is obsolete: true
Attachment #8740009 - Flags: review?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> On a more bikeshed point -- is it just me, or is "getCanonicalLocales" a bad
> name for the method?

It's not perfect, agree, but it's not that bad either. We use some internal data and we may do more of that in the future. We're discussing extending the method with options argument and one of them would allow us to add extension keys to locale code, like this:

Intl.getCanonicalLocales(['en-us', 'pl'], {extensionKeys: true}); // ['en-US-u-ca-gregorian-u-nu-arab', 'pl-u-ca-gregorian-u-nu-arab']
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> I did, the previous patch compiles and runs all tests, I just didn't add any
> tests for it yet.

Are you doing a debug build? In an opt build this'll probably work; we have some debug asserts around method calls in self-hosted code.
Comment on attachment 8740009 [details] [diff] [review]
patch, v2

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

::: js/src/builtin/Intl.js
@@ +2892,5 @@
> +  let len = codes.length;
> +  let k = 0;
> +
> +  while (k < len) {
> +    _DefineDataProperty(result, k.toString(), codes[k]);

It's illegal to have |obj.prop(...)| anywhere in self-hosted code that |obj.prop| might be a user-controllable function -- at runtime in some cases, if not at compile time.  (And maybe it requires a debug build to hit, but that's very much by design.)

Here, behavior in a debug build is like so:

js> Number.prototype.toString = () => { throw 42 }
() => { throw 42 }
js> Intl.getCanonicalLocales(["en-US"])
Assertion failure: isObject(), at /home/jwalden/moz/clean/js/src/dbg/dist/include/js/Value.h:1281
Segmentation fault (core dumped)

And if you did that in an opt build, the result would wrongly be a thrown 42.

As it happens, you can use _DefineDataProperty directly with integers.  So just get rid of the |.toString()| here to deal (but add the test, because why not).

::: js/src/tests/Intl/getCanonicalLocales-overrided-arg-length.js
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Tests the getCanonicalLocales function for overridden argument's length.

s/overrided/overridden/ in the test file name, and in all the other test file names.

::: js/src/tests/Intl/getCanonicalLocales-overrided-push.js
@@ +5,5 @@
> +
> +// Tests the getCanonicalLocales function for overriden Array.push.
> +
> +Array.prototype.push = () => { throw 42; };
> +Intl.getCanonicalLocales(["en-US"]); // must not throw 42, might if push is used

var arr = Intl.get...;
assertShallowArray(arr, ["en-US"]);

May as well do minimal sanity-checking of the result while we're here -- also to avoid some hypothetical optimizer ever deciding nobody looks at the result of this method, therefore we can just not calculate it.

::: js/src/tests/Intl/getCanonicalLocales-overrided-set.js
@@ +5,5 @@
> +
> +// Tests the getCanonicalLocales function for overridden set().
> +
> +Object.defineProperty(Array.prototype, 0, { set() { throw 42; } });
> +Intl.getCanonicalLocales(["en-US"]); // must not throw 42, might if push is used

Minimally test the result here, too.

::: js/src/tests/Intl/getCanonicalLocales-weird-cases.js
@@ +15,5 @@
> +   "en-x-u-foo-a-bar",
> +   "en-a-bar-u-baz-x-u-foo",
> +  ];
> +
> +Intl.getCanonicalLocales(weirdCases);

Maybe instead of this,

  for (let weird of weirdCases)
    assertShallowArray(Intl.getCanonicalLocales(weird), [weird]);

::: js/src/tests/Intl/getCanonicalLocales-with-duplicates.js
@@ +9,5 @@
> +  Intl.getCanonicalLocales(
> +    ['ab-cd', 'ff', 'de-rt', 'ab-Cd']), ['ab-CD', 'ff', 'de-RT']);
> +
> +var locales = Intl.getCanonicalLocales(["en-US", "en-US"]);
> +assertEq(locales.length, 1);

Use assertShallowArray for this, too.

::: js/src/tests/Intl/getCanonicalLocales.js
@@ +8,5 @@
> +let gCL = Intl.getCanonicalLocales;
> +
> +assertDeepEq(gCL(), []);
> +
> +assertDeepEq(gCL(NaN), []);

Move this one and the gCL(1) one to the end, then add

Number.prototype[0] = "en-US";
Number.prototype.length = 1;
assertShallowArray(gCL(NaN), ["en-US"]);

as extra clarification.  (I double-checked semantics here because I was thinking NaN was excluded explicitly, so as not to have NaN -> "nan" and misconstrue a weird value as a real tag.  But really it gets handled normally, and normal handling means you box up NaN into a Number object, whose ToUint32(.length) is 0.  This test would exercise that oddity, making clearer to the reader what's happening.)

::: js/src/tests/Intl/shell.js
@@ +1,1 @@
> +if (typeof assertDeepEq === 'undefined') {

This full generality makes it hard, IMO, to know exactly what's being tested.  I'd prefer if, instead of assertDeepEq, you used this vastly-simpler function:

function assertShallowArray(actual, expected) {
  var len = actual.length;
  assertEq(len, expected.length, "mismatching array lengths");

  for (var i = 0; i < len; i++)
    assertEq(actual[i], expected[i], "mismatch at element " + i);
}
Attachment #8740009 - Flags: review?(jwalden+bmo) → review-
Attached patch patch v3Splinter Review
> As it happens, you can use _DefineDataProperty directly with integers.  So just get rid of the |.toString()| here to deal (but add the test, because why not).

Yeah! Got the assertion when switched to debug builds! Removed .toString and adding a test. Thanks!

Updated the patch. Still getting the exception thrown in the getCanonicalLocales-overriden-set.js test. Should I remove that test?
Attachment #8740009 - Attachment is obsolete: true
Attachment #8740196 - Flags: review?(jwalden+bmo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> Still getting the exception thrown in the
> getCanonicalLocales-overriden-set.js test. Should I remove that test?

Tentatively, I'm betting there's an actual bug somewhere in our implementation.  I'll investigate locally.
Oh, bah.  The Array.prototype[0] setter interferes with test harness goo triggered by |reportCompare|.  No bug in the patch, just a (take your pick) test deficiency or a test harness deficiency.  I incline to the latter view.  This patch fixes the instantaneous problem, but it'll take a full test run to verify (just in case anyone instead overrides Object.defineProperty, to produce similar bustage).  We'll have to do that before going with this fix:

diff --git a/js/src/tests/shell.js b/js/src/tests/shell.js
--- a/js/src/tests/shell.js
+++ b/js/src/tests/shell.js
@@ -73,17 +73,17 @@ function TestCase(n, d, e, a)
   this.name = n;
   this.description = d;
   this.expect = e;
   this.actual = a;
   this.passed = getTestCaseResult(e, a);
   this.reason = '';
   this.bugnumber = typeof(BUGNUMER) != 'undefined' ? BUGNUMBER : '';
   this.type = (typeof window == 'undefined' ? 'shell' : 'browser');
-  gTestcases[gTc++] = this;
+  Object.defineProperty(gTestcases, gTc++, { value: this, enumerable: true, configurable: true, writable: true });
 }
 
 gFailureExpected = false;
 
 TestCase.prototype.dump = function () {
   // let reftest handle error reporting, otherwise
   // output a summary line.
   if (typeof document != "object" ||
Comment on attachment 8740196 [details] [diff] [review]
patch v3

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

Feel free to fold the change in the previous comment into this patch and push to try -- if it passes, r=me to land, otherwise we'll need to try another approach there before this can land.

::: js/src/tests/Intl/getCanonicalLocales-with-duplicates.js
@@ +9,5 @@
> +  Intl.getCanonicalLocales(
> +    ['ab-cd', 'ff', 'de-rt', 'ab-Cd']), ['ab-CD', 'ff', 'de-RT']);
> +
> +var locales = Intl.getCanonicalLocales(["en-US", "en-US"]);
> +assertEq(locales.length, 1);

No need for the length-check if assertShallowArray will do it.

::: js/src/tests/Intl/getCanonicalLocales.js
@@ +5,5 @@
> +
> +// Tests the getCanonicalLocales function with a diverse set of arguments.
> +
> +let gCL = Intl.getCanonicalLocales;
> +

Also add

assertEq(Intl.getCanonicalLocales.length, 1);

::: js/src/tests/Intl/shell.js
@@ +1,2 @@
> +if (typeof assertShallowArray === 'undefined') {
> +  function assertShallowArray(actual, expected) {

if (typeof ...) {
  var assertShallowArray = function assertShallowArray(...
Attachment #8740196 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> -  gTestcases[gTc++] = this;
> +  Object.defineProperty(gTestcases, gTc++, { value: this, enumerable: true,
> configurable: true, writable: true });

So this didn't work - 7 regressions, but I went for this:

+  ({}).constructor.defineProperty(...)

and that works with no regressions.

I'll push to try now and I hope you'll be ok with using this solution :)
Jeff, I'll need your help debugging this. I have a running debug build of Firefox and I'm running `./mach reftest` but can't reproduce those errors and I'm not sure how to launch jsreftests.
Flags: needinfo?(gandalf) → needinfo?(jwalden+bmo)
Bah.  jstests *in the browser* historically had a restriction that they had to be at js/src/tests/*/*/*.js, denoting suite/subsuite/test.js.  Then I eased that to at least two *-directories, for test262 stuff.  And I *thought* I'd eased it to just one, everywhere.  But seems not.  Modified the browser harness to not require the extra nesting level, then relanded.
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/5177d7cdb97f
https://hg.mozilla.org/mozilla-central/rev/b6cd434fdaf5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.