Implement ECMA-402 v1 legacy constructor semantics compromise

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 wontfix, firefox54 fixed)

Details

(URL)

Attachments

(9 attachments, 8 obsolete attachments)

19.63 KB, patch
anba
: review+
Details | Diff | Splinter Review
28.11 KB, patch
anba
: review+
Details | Diff | Splinter Review
11.66 KB, patch
anba
: review+
Details | Diff | Splinter Review
18.28 KB, patch
anba
: review+
Details | Diff | Splinter Review
8.28 KB, patch
anba
: review+
Details | Diff | Splinter Review
42.76 KB, patch
anba
: review+
Details | Diff | Splinter Review
40.50 KB, patch
anba
: review+
Details | Diff | Splinter Review
39.45 KB, patch
anba
: review+
Details | Diff | Splinter Review
846 bytes, patch
anba
: review+
Details | Diff | Splinter Review
(Assignee)

Comment 1

2 years ago
Posted patch bug1328386-part1.patch (obsolete) — Splinter Review
This is only code cleanup to fix a few things I've noticed while working on the actual changes.

- Intl.cpp: Removed boilerplate code when calling self-hosted functions.
- Intl.cpp: Removed a misleading assertion in CreatePluralRulesPrototype (createBlankPrototype() creates a fresh object, so none of the internal can be set already; didn't notice this when reviewing the change).
- Intl.js: Added a direct call to the maybeInternalProperties() helper in getInternals() (and reordered the type checks in alphabetical order).
- Intl.js: Added step comments to Intl_getCanonicalLocales and renamed the variables to match the spec (well, more or less - I've expanded "ll" to the more expressive "localeList").
- Intl.js: Reindented and added comments to Intl_getCalendarInfo.
- And fixed a few code style issues (e.g. adding additional blank lines per the current code style in Intl.{cpp,h,js}...).
Attachment #8824547 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

2 years ago
Posted patch bug1328386-part2.patch (obsolete) — Splinter Review
This is needed for a later patch in this series.

I've copied the style to create NativeObject subclasses from builtin/MapObject.h, I hope that's the correct way to do this nowadays. 

(There are a few lines exceeding 99 chars, this will be fixed in a later patch.)
Attachment #8824550 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

2 years ago
Posted patch bug1328386-part3.patch (obsolete) — Splinter Review
It's not possible to initialize arbitrary objects as Intl.PluralRules, and Intl.PluralRules won't use the legacy constructor semantics. Add more assertions that PluralRules methods can only be called with proper PluralRules instances and add tests for calling Intl.PluralRules as a function.
Attachment #8824552 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

2 years ago
Posted patch bug1328386-part4.patch (obsolete) — Splinter Review
Similar to part 3, but this time for Intl.Collator.
Attachment #8824553 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

2 years ago
Posted patch bug1328386-part5.patch (obsolete) — Splinter Review
Add Intl.[[FallbackSymbol]] as a global variable to the self-hosted environment. This change is needed for part 6 and part 7.
Attachment #8824558 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

2 years ago
Posted patch bug1328386-part6.patch (obsolete) — Splinter Review
Implement the legacy constructor semantics for Intl.NumberFormat.

There are still a few bugs in the proposed spec change, I've added FIXME notes to mark them. I'll update this patch as soon as the spec PR has been fixed.
Attachment #8824559 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 7

2 years ago
Posted patch bug1328386-part7.patch (obsolete) — Splinter Review
Similar to part 6, but this time for Intl.DateTimeFormat.
Attachment #8824562 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

2 years ago
Posted patch bug1328386-part8.patch (obsolete) — Splinter Review
And now we can finally remove the internalsMap WeakMap from Intl.js and instead store the internals objects in internal slots.
Attachment #8824564 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

2 years ago
Blocks: 1331474
Comment on attachment 8824547 [details] [diff] [review]
bug1328386-part1.patch

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

::: js/src/builtin/Intl.js
@@ +3255,5 @@
>  }
>  
> +/**
> + * This function is a custom method designed after Intl API, but currently
> + * not part of the spec or spec proposal.

"""
This function is a custom function in the style of the standard Intl.* functions, that isn't part of any spec or proposal yet.
"""

@@ +3295,5 @@
> +    localeOpt.localeMatcher = "best fit";
> +
> +    // 5. Let r be ResolveLocale(%DateTimeFormat%.[[availableLocales]],
> +    //      requestedLocales, localeOpt,
> +    //      %DateTimeFormat%.[[relevantExtensionKeys]], localeData).

Align the trailing lines, please.

@@ +3364,2 @@
>      const localeOpt = new Record();
>      // 6. Set localeOpt.[[localeMatcher]] to "best fit".

Add a blank line before this, or put the two comments adjacent with the two lines of code immediately after, don't care which.
Attachment #8824547 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8824550 [details] [diff] [review]
bug1328386-part2.patch

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

::: js/src/builtin/Intl.cpp
@@ +1019,5 @@
>              if (!proto)
>                  return false;
>          }
>  
> +        obj = NewObjectWithGivenProto<CollatorObject>(cx, proto);

Maybe assign to an |auto*| here, then assign that to |obj| as final step in the block, so you don't need the as-cast to set the reserved slot.

@@ +1066,5 @@
>      // brokenness in object allocation code.  For the moment, hack around it by
>      // explicitly guarding against the possibility of the reserved slot not
>      // containing a private.  See bug 949220.
> +    CollatorObject* collator = &obj->as<CollatorObject>();
> +    const Value& slot = collator->getReservedSlot(CollatorObject::UCollatorSlot);

I'd probably just have a one-liner like before.  And same for the others.

::: js/src/builtin/Intl.h
@@ +186,5 @@
> +{
> +  public:
> +    static const Class class_;
> +
> +    enum { UCollatorSlot, SlotCount };

Make these static constexpr uint32_t, for clearer typing and consistency with how we specify slot numbers/limits everywhere else.  Same for the other classes.
Attachment #8824550 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8824552 [details] [diff] [review]
bug1328386-part3.patch

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

::: js/src/builtin/Intl.js
@@ +3199,2 @@
>      // Step 2.
> +    if (!(IsObject(pluralRules) && IsPluralRules(pluralRules)))

Would prefer  |if (!IsObject || !IsPluralRules)|, myself (and later).  Fewer harder-to-read parentheses nestings.

::: js/src/tests/Intl/PluralRules/call.js
@@ +41,5 @@
> +        ])),
> +    ];
> +}
> +
> +// Invoking [[Call]] for Intl.PluralRules always returns a new PluralRules instance.

Huh, I'd have expected them to only be constructible like Uint8Array et al. are.
Attachment #8824552 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8824553 [details] [diff] [review]
bug1328386-part4.patch

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

::: js/src/builtin/Intl.cpp
@@ +993,4 @@
>      }
>  
> +    Rooted<CollatorObject*> collator(cx);
> +    collator = NewObjectWithGivenProto<CollatorObject>(cx, proto);

These don't fit in one line in 99ch?

@@ +1023,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 2);
>      MOZ_ASSERT(!args.isConstructing());
>      // intl_Collator is an intrinsic for self-hosted JavaScript, so it cannot
>      // be used with "new", but it still has to be treated as a constructor.

Probably can just remove this comment, now that calling Collator acts as if you constructed it.

::: js/src/builtin/Intl.js
@@ +1690,5 @@
>   * Spec: ECMAScript Internationalization API Specification, 10.3.2.
>   */
>  function Intl_Collator_compare_get() {
>      // Check "this Collator object" per introduction of section 10.3.
> +    if (!(IsObject(this) && IsCollator(this)))

Same |if (!IsObject || !IsCollator)| thing.  (And for any subsequent patches with the similar idea, and elsewhere in this/the other patches.)
Attachment #8824553 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8824558 [details] [diff] [review]
bug1328386-part5.patch

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

::: js/src/builtin/Intl.js
@@ +1210,5 @@
> + */
> +function intlFallbackSymbol() {
> +    var fallbackSymbol = intlFallbackSymbolHolder.value;
> +    if (!fallbackSymbol)
> +        intlFallbackSymbolHolder.value = fallbackSymbol = std_Symbol();

Hmm.  Is there a reason this symbol doesn't have a description in the spec?  Seems like it wouldn't hurt for the spec to give it one.

This probably could stand to be a well-defined symbol (albeit without a corresponding Symbol.* property).

::: js/src/builtin/Utilities.js
@@ +42,5 @@
>  //
>  // Do not create an alias to a self-hosted builtin, otherwise it will be cloned
>  // twice.
>  //
> +// Symbol is a bare constructor without properties or methods.

Bare function?  Or can this be constructed?  (There was that comment in the other patch about not new-ing intrinsics, the background of which I forget now.)
Attachment #8824558 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> ::: js/src/builtin/Intl.h
> @@ +186,5 @@
> > +{
> > +  public:
> > +    static const Class class_;
> > +
> > +    enum { UCollatorSlot, SlotCount };
> 
> Make these static constexpr uint32_t, for clearer typing and consistency
> with how we specify slot numbers/limits everywhere else.  Same for the other
> classes.

I thought using enums was still the way to go, because that's currently used in ModuleObject.h, MapObject.h, and Promise.{h,cpp}.
(Assignee)

Comment 15

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> ::: js/src/tests/Intl/PluralRules/call.js
> @@ +41,5 @@
> > +        ])),
> > +    ];
> > +}
> > +
> > +// Invoking [[Call]] for Intl.PluralRules always returns a new PluralRules instance.
> 
> Huh, I'd have expected them to only be constructible like Uint8Array et al.
> are.

Maybe :gandalf knows why Intl.PluralRules is allowed to be called as a function.
Flags: needinfo?(gandalf)
Comment on attachment 8824559 [details] [diff] [review]
bug1328386-part6.patch

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

My eyes are glazing over reading these tests.  :-\

::: js/src/tests/Intl/NumberFormat/call.js
@@ +39,5 @@
> +        ])),
> +    ];
> +}
> +
> +const intlFallbackSymbol = Object.getOwnPropertySymbols(Intl.NumberFormat.call(Object.create(Intl.NumberFormat.prototype)))[0];

lol
Attachment #8824559 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 17

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> ::: js/src/builtin/Intl.js
> @@ +1210,5 @@
> > + */
> > +function intlFallbackSymbol() {
> > +    var fallbackSymbol = intlFallbackSymbolHolder.value;
> > +    if (!fallbackSymbol)
> > +        intlFallbackSymbolHolder.value = fallbackSymbol = std_Symbol();
> 
> Hmm.  Is there a reason this symbol doesn't have a description in the spec? 
> Seems like it wouldn't hurt for the spec to give it one.

We could probably request to change https://github.com/tc39/ecma402/pull/84 to add a description.

> 
> This probably could stand to be a well-defined symbol (albeit without a
> corresponding Symbol.* property).

Well-defined symbols are shared across all Realms, but [[FallbackSymbol]] is a new symbol per each Realm. (But I don't know why littledan wants to have this per-Realm instead of shared across all Realms.)
I don't think there is any good reason. We probably copied tests from previous tests. Feel free to fix it.
Flags: needinfo?(gandalf)
Attachment #8824562 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8824564 [details] [diff] [review]
bug1328386-part8.patch

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

::: js/src/builtin/Intl.h
@@ +190,5 @@
>  
> +    enum { InternalsSlot, UCollatorSlot, SlotCount };
> +
> +    static_assert(InternalsSlot == INTL_INTERNALS_OBJECT_SLOT,
> +                  "InternalsSlot must match self-hosting define for internals object slot.");

Assertion messages are usually treated as sentence fragments, so no period.  And for the others.

::: js/src/builtin/Intl.js
@@ +1223,5 @@
>      assert(IsObject(obj), "Non-object passed to initializeIntlObject");
> +    assert((type === "Collator" && IsCollator(obj))
> +           || (type === "DateTimeFormat" && IsDateTimeFormat(obj))
> +           || (type === "NumberFormat" && IsNumberFormat(obj))
> +           || (type === "PluralRules" && IsPluralRules(obj)),

|| at the end of the various lines.
Attachment #8824564 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 20

2 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #13)
> ::: js/src/builtin/Utilities.js
> @@ +42,5 @@
> >  //
> >  // Do not create an alias to a self-hosted builtin, otherwise it will be cloned
> >  // twice.
> >  //
> > +// Symbol is a bare constructor without properties or methods.
> 
> Bare function?  Or can this be constructed?  

Yes and no. It's a constructor which always throws a TypeError when called with `new`. 

> (There was that comment in the
> other patch about not new-ing intrinsics, the background of which I forget
> now.)

intl_DateTimeFormat is exported to the self-hosting global as a function, whereas std_Symbol, just like std_WeakMap, is exported as a constructor without properties. If you prefer I can change this to use JS_FN for exporting Symbol to the self-hosting global.
(Assignee)

Comment 21

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824547 - Attachment is obsolete: true
Attachment #8829508 - Flags: review+
(Assignee)

Comment 22

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824550 - Attachment is obsolete: true
Attachment #8829509 - Flags: review+
(Assignee)

Comment 23

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824552 - Attachment is obsolete: true
Attachment #8829510 - Flags: review+
(Assignee)

Comment 24

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824553 - Attachment is obsolete: true
Attachment #8829511 - Flags: review+
(Assignee)

Comment 25

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824558 - Attachment is obsolete: true
Attachment #8829512 - Flags: review+
(Assignee)

Comment 26

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824559 - Attachment is obsolete: true
Attachment #8829513 - Flags: review+
(Assignee)

Comment 27

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824562 - Attachment is obsolete: true
Attachment #8829514 - Flags: review+
(Assignee)

Comment 28

2 years ago
Rebased and applied review comments. Carrying r+ from Waldo.
Attachment #8824564 - Attachment is obsolete: true
Attachment #8829515 - Flags: review+
(In reply to André Bargull from comment #20)
> If you prefer I can change this to use JS_FN for
> exporting Symbol to the self-hosting global.

Nah, it's fine.
Filed bug 1333621 for the PluralRules ctor without new bug.
Blocks: 1333621
(In reply to André Bargull from comment #29)
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=83ad8b9e09f3739283a72a801d975fb85a81fd0a

I see a lot of SM failures in this run.
Keywords: checkin-needed
(Assignee)

Comment 33

2 years ago
The cgc failures look like bug 1131425.

@jandem Are you okay with relaxing the error message for "js/src/jit-test/tests/ion/bug913749.js" to also accept "undefined has no properties"?
Flags: needinfo?(jdemooij)
(In reply to André Bargull from comment #33)
> @jandem Are you okay with relaxing the error message for
> "js/src/jit-test/tests/ion/bug913749.js" to also accept "undefined has no
> properties"?

I'd like to understand why we get the different error message here (we run this test with different JIT flags so it should not be interpreter vs Baseline vs Ion).

I'll take a look in a bit.
(In reply to Jan de Mooij [:jandem] from comment #34)
> I'd like to understand why we get the different error message here (we run
> this test with different JIT flags so it should not be interpreter vs
> Baseline vs Ion).
> 
> I'll take a look in a bit.

OK I can reproduce this with JS_GC_ZEAL=14,25

The problem is that we run in Ion and the expression decompiler bails if frameIter.isIon(). I'm not sure why this requires CGC, but it probably has something to do with discarding type information or JIT code at the right time.

Looking back at bug 913749, the exact error message doesn't matter much, so rs=me to change it to accept the other one too.
Flags: needinfo?(jdemooij)
(Assignee)

Comment 36

2 years ago
Attachment #8830386 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 37

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/862bc2b2553e
Part 1: Remove boilerplate code and add comments in Intl code. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/36ad679ec7db
Part 2: Add a NativeObject subclass for each Intl object. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/16d0d9e90e25
Part 3: Ensure PluralRules methods are always called with actual PluralRules instances. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1415ba91dd2
Part 4: No longer allow to initialize arbitrary objects as Intl.Collator instances per ECMA-402, 2nd edition. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/985a0c1baa89
Part 5: Add Intl.[[FallbackSymbol]] to support ECMA402, 4th edition legacy constructor semantics. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/91080dad0ff8
Part 6: Implement legacy constructor semantics for Intl.NumberFormat per ECMA-402, 4th edition. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/60adc4589abc
Part 7: Implement legacy constructor semantics for Intl.DateTimeFormat per ECMA-402, 4th edition. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/e15e0f265264
Part 8: Store internals object for Intl objects in internal slot instead of using a WeakMap. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35fe2367a4d
Part 9: Relax error message in jit-test file so the test doesn't fail when the decompiler bails out. rs=jandem
Keywords: checkin-needed
Does this need to be considered for backport?
Flags: needinfo?(andrebargull)
Blocks: 1314354
No longer blocks: 1331474
(Assignee)

Comment 40

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Does this need to be considered for backport?

No, this one shouldn't be backported, because there are still some open issues in the spec for this change.
Flags: needinfo?(andrebargull)
You need to log in before you can comment on or make changes to this bug.