New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Compatibility Hazard] ECMA 402 2nd Edition Changed the [[Call]] Behavior of Intl Constructors #57

Closed
ericf opened this Issue Nov 24, 2015 · 59 comments

Comments

Projects
None yet
@ericf
Member

ericf commented Nov 24, 2015

The 1st Edition of ECMA 402 specified the [[Call]] behavior for Intl constructors; e.g. Intl.DateTimeFormat.call(this [, locales [, options]]) to return the this context object that was passed-in. In the 2nd Edition, the [[Call]] behavior no longer states that the passed-in context object should be retuned. This change is a potential compatibility hazard.

As the developer and maintainer of the popular FormatJS i18n libraries, I've begun receiving issues from developers testing Chrome Canary (49) that their dates and numbers were failing for format, causing an Error to be thrown.

The high-level framework integration libs that are part of FormatJS — react-intl, ember-intl, handlebars-intl — are used by many web apps, including many of Yahoo's web apps. All these libraries use the underlying intl-format-cache which memoizes the Intl constructors because they are expensive to create. The memoization technique essentially does the following:

function constructIntlInstance(IntlConstructor) {
    return function () {
        var args = Array.prototype.slice.call(arguments);
        var instance = Object.create(IntlConstructor.prototype);
        IntlConstructor.apply(instance, args);
        return instance;
    };
}

Note: That this code depends on the following invariant:

var instance = Object.create(IntlConstructor.prototype);
instance === IntlConstructor.call(instance); // true

It is dependent on the the Intl constructors being .call()-able and the returning the context object passed-in. This .call() behavior for the Intl constructors is supported in all ECMA 402 1st Edition implementations.

After receiving an issue report about this code causing an Error to be thrown Chrome Canary (49), I dug in and found this recent V8 change which updates V8's implementation to match ECMA 402 2nd Edition, thus removing the code that returns the passed-in context object when the Intl constructors are .call()-ed.

Today, I've released intl-format-cache@2.0.5 which changes the memoization implementation to make sure the [[Construct]] behavior always happens by invoking the Intl constructors with new. Essentially doing the following:

function constructIntlInstance(IntlConstructor) {
    return (...args) => new IntlConstructor(...args);
}

In ES5 an equivalent would be:

function constructIntlInstance(IntlConstructor) {
    return function () {
        var args = Array.prototype.slice.call(arguments);
        return new (Function.prototype.bind.apply(IntlConstructor, [null].concat(args)))();
    };
}

While this issue is now "fixed" in intl-format-cache, developers must upgrade their dependencies and re-deploy their apps. I will help to communicate this change, but I'm worried that removing the 1st Edition [[Call]] behavior will break many apps/sites 😞

How should we move forward to prevent end-users from having broken experiences?

Edited based on @rwaldron's feedback.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Nov 24, 2015

Member

Thanks for the notice here. I've reverted the patch, so in a day or two, Chrome Canary should have this "bug" fixed. Even if it was a change from violating the spec to following it, if it breaks web compatibility, then it is a bug.

Ideally, the place to start would be in improving the specification and test262 tests. New specification versions should not mandate web-incompatible changes. I'd like to update V8 to meet ECMA-402 down to all the details that test262 tests for, but only if it won't break the web. Now that ECMA-402 is in HTML and can take pull requests, it seems like a great time for fixups like this.

Member

littledan commented Nov 24, 2015

Thanks for the notice here. I've reverted the patch, so in a day or two, Chrome Canary should have this "bug" fixed. Even if it was a change from violating the spec to following it, if it breaks web compatibility, then it is a bug.

Ideally, the place to start would be in improving the specification and test262 tests. New specification versions should not mandate web-incompatible changes. I'd like to update V8 to meet ECMA-402 down to all the details that test262 tests for, but only if it won't break the web. Now that ECMA-402 is in HTML and can take pull requests, it seems like a great time for fixups like this.

kisg pushed a commit to paul99/v8mips that referenced this issue Nov 24, 2015

Revert of [Intl] create new instances when new.target is undefined (p…
…atchset #2 id:20001 of https://codereview.chromium.org/1440593003/ )

Reason for revert:
This breaks backwards compatibility by disallowing call. Web application authors have noticed the breakage. tc39/ecma402#57

Original issue's description:
> [Intl] create new instances when new.target is undefined
>
> BUG=v8:4360
> LOG=N
> R=littledan@chromium.org
>
> Committed: https://crrev.com/fa9c39eeadd8e692af03b024fe2fdcf94ad0da6b
> Cr-Commit-Position: refs/heads/master@{#31971}

TBR=caitpotter88@gmail.com
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4360

Review URL: https://codereview.chromium.org/1473493003

Cr-Commit-Position: refs/heads/master@{#32189}

@ericf ericf changed the title from [Compatibility Hazard] ECMA 402 2nd Edition Removed [[Call]] from Intl Constructors to [Compatibility Hazard] ECMA 402 2nd Edition the [[Call]] Behavior of Intl Constructors Nov 24, 2015

@ericf ericf changed the title from [Compatibility Hazard] ECMA 402 2nd Edition the [[Call]] Behavior of Intl Constructors to [Compatibility Hazard] ECMA 402 2nd Edition Changed the [[Call]] Behavior of Intl Constructors Nov 24, 2015

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Nov 24, 2015

Contributor

@carady WDYT? Should we restore this behavior:

returning the context object passed-in.

For 3rd ed.?

Contributor

rwaldron commented Nov 24, 2015

@carady WDYT? Should we restore this behavior:

returning the context object passed-in.

For 3rd ed.?

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Nov 24, 2015

Contributor

For background: removing that was intentionally, as it was thought to be a bug in the 1st Ed. spec.

Contributor

rwaldron commented Nov 24, 2015

For background: removing that was intentionally, as it was thought to be a bug in the 1st Ed. spec.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 24, 2015

thanks @littledan that puts the pressure off, as we @rwaldron / @caridy / @ericf come up with the resolution.

stefanpenner commented Nov 24, 2015

thanks @littledan that puts the pressure off, as we @rwaldron / @caridy / @ericf come up with the resolution.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Nov 24, 2015

Member

This change was made to bring the object instantiation model used within ECMA 402 in alignment with with that used by ECMAScript 2015. In particular WRT how subclassable built-ins are supported.

In V1, the assumption was that an arbitrary object could be dynamically initialized as an Intl constructor instance (or even initialized multiple times as an instance of multiple Intl consrtrucors). This is likely incompatible with implementations that use internal slots.

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with
existing self hosted implementations.

Member

allenwb commented Nov 24, 2015

This change was made to bring the object instantiation model used within ECMA 402 in alignment with with that used by ECMAScript 2015. In particular WRT how subclassable built-ins are supported.

In V1, the assumption was that an arbitrary object could be dynamically initialized as an Intl constructor instance (or even initialized multiple times as an instance of multiple Intl consrtrucors). This is likely incompatible with implementations that use internal slots.

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with
existing self hosted implementations.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Nov 24, 2015

Member

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with existing self hosted implementations.

Thanks @allenwb! I will continue on the front of ridding the Web of the code that relies on the v1 spec'd behavior as much as I can.

Member

ericf commented Nov 24, 2015

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with existing self hosted implementations.

Thanks @allenwb! I will continue on the front of ridding the Web of the code that relies on the v1 spec'd behavior as much as I can.

@caridy caridy added the bug label Nov 24, 2015

@caridy caridy added this to the 3rd Edition milestone Nov 24, 2015

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Nov 24, 2015

Member

It might be hard to catch all users. For example, @bterlson is API stability for the Intl object needed for Windows apps that are written in ECMAScript?

The note of implementation concern seems a bit odd here. Seems like implementations are getting by already, while also progressing to do subclassable builtins. I think it's safe to say that the V1 spec is implementable. It might not be the prettiest or most consistent, but this is the web platform, and we often end up needing to put up with inconsistency. Are you saying it'd be difficult to implement in ECMAScript with the new class syntax, or that it'd be difficult to implement in ECMAScript at all?

Member

littledan commented Nov 24, 2015

It might be hard to catch all users. For example, @bterlson is API stability for the Intl object needed for Windows apps that are written in ECMAScript?

The note of implementation concern seems a bit odd here. Seems like implementations are getting by already, while also progressing to do subclassable builtins. I think it's safe to say that the V1 spec is implementable. It might not be the prettiest or most consistent, but this is the web platform, and we often end up needing to put up with inconsistency. Are you saying it'd be difficult to implement in ECMAScript with the new class syntax, or that it'd be difficult to implement in ECMAScript at all?

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Nov 24, 2015

Contributor

@allenwb

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with existing self hosted implementations.

This was my plan as well, but I definitely appreciate your eyes on this. I'd really rather not lose the correction if possible.

@littledan

It might be hard to catch all users. For example, @bterlson is API stability for the Intl object needed for Windows apps that are written in ECMAScript?

The API didn't "change". A weird bug was fixed.

Are you saying it'd be difficult to implement in ECMAScript with the new class syntax, or that it'd be difficult to implement in ECMAScript at all?

This has nothing to do with implementing the feature in terms of classes or functions. It has to do with a mistake that was made in the first edition and also the desire to align Intl constructors with ES6 semantics.

1st Edition:

var o = Object.create(Intl.Collator.prototype);
var c = Intl.Collator.call(o);
c === o; // true

...Which does not match ES6 semantics for constructors that have construct and call semantics, here are some examples:

var o = Object.create(Error.prototype);
var e = Error.call(o);
o === e; // false

// Or...

var o = Object.create(String.prototype);
var s = String.call(o);
o === s; // false

So @allenwb and I agreed that following Error's specification was the smartest route, so I made the change it was reviewed by many people and 👍 . The problem here is that @ericf's library code was written to rely on that bug, because now...

2nd Edition:

var o = Object.create(Intl.Collator.prototype);
var c = Intl.Collator.call(o)
o === c; // false
Contributor

rwaldron commented Nov 24, 2015

@allenwb

I'll take a look to see if there is a better fix that meets the ES6 requirements and remains compatible with existing self hosted implementations.

This was my plan as well, but I definitely appreciate your eyes on this. I'd really rather not lose the correction if possible.

@littledan

It might be hard to catch all users. For example, @bterlson is API stability for the Intl object needed for Windows apps that are written in ECMAScript?

The API didn't "change". A weird bug was fixed.

Are you saying it'd be difficult to implement in ECMAScript with the new class syntax, or that it'd be difficult to implement in ECMAScript at all?

This has nothing to do with implementing the feature in terms of classes or functions. It has to do with a mistake that was made in the first edition and also the desire to align Intl constructors with ES6 semantics.

1st Edition:

var o = Object.create(Intl.Collator.prototype);
var c = Intl.Collator.call(o);
c === o; // true

...Which does not match ES6 semantics for constructors that have construct and call semantics, here are some examples:

var o = Object.create(Error.prototype);
var e = Error.call(o);
o === e; // false

// Or...

var o = Object.create(String.prototype);
var s = String.call(o);
o === s; // false

So @allenwb and I agreed that following Error's specification was the smartest route, so I made the change it was reviewed by many people and 👍 . The problem here is that @ericf's library code was written to rely on that bug, because now...

2nd Edition:

var o = Object.create(Intl.Collator.prototype);
var c = Intl.Collator.call(o)
o === c; // false
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Nov 24, 2015

Unfortunately, fixing the bug (and I agree, it is a weird/buggy behaviour) does change behaviour that web developers are (I wasn't expecting this) relying on, so :( It probably means adding a new wrinkle/inconsistency to the language ._.

caitp commented Nov 24, 2015

Unfortunately, fixing the bug (and I agree, it is a weird/buggy behaviour) does change behaviour that web developers are (I wasn't expecting this) relying on, so :( It probably means adding a new wrinkle/inconsistency to the language ._.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Nov 24, 2015

Collaborator

I think it's safe to say that the V1 spec is implementable.

Except it's not even fully implemented in SpiderMonkey (https://bugzilla.mozilla.org/show_bug.cgi?id=899361) or V8 (non-standard "resolved" and "boundXXX" properties, missing [[initializedIntlObject]] locking, ...).

Collaborator

anba commented Nov 24, 2015

I think it's safe to say that the V1 spec is implementable.

Except it's not even fully implemented in SpiderMonkey (https://bugzilla.mozilla.org/show_bug.cgi?id=899361) or V8 (non-standard "resolved" and "boundXXX" properties, missing [[initializedIntlObject]] locking, ...).

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Nov 24, 2015

Member

You caught me--we still have bugs. At least this aspect of it seems implementable. Are there implementations which don't act like this?

Member

littledan commented Nov 24, 2015

You caught me--we still have bugs. At least this aspect of it seems implementable. Are there implementations which don't act like this?

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Nov 24, 2015

Collaborator

Not sure about Edge, but JSC only implements the 2nd edition semantics.

Collaborator

anba commented Nov 24, 2015

Not sure about Edge, but JSC only implements the 2nd edition semantics.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Nov 24, 2015

Collaborator

Apropos bugs, you may want to improve the intl-object type check in addBoundMethod (src/js/i18n.js) to avoid illegal access errors.

Collaborator

anba commented Nov 24, 2015

Apropos bugs, you may want to improve the intl-object type check in addBoundMethod (src/js/i18n.js) to avoid illegal access errors.

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Nov 24, 2015

Yeah, I was about to jump in and state that JSC uses the new semantics. The v1 spec allows arbitrary objects to become format objects, which at least to me seemed very difficult to implement.

vanwagonet commented Nov 24, 2015

Yeah, I was about to jump in and state that JSC uses the new semantics. The v1 spec allows arbitrary objects to become format objects, which at least to me seemed very difficult to implement.

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Nov 24, 2015

The problematic pattern seen in the wild here though is not using an "arbitrary" object, but one created from the Intl.Collator.prototype. As a consumer of the API, I agree that that should work.

vanwagonet commented Nov 24, 2015

The problematic pattern seen in the wild here though is not using an "arbitrary" object, but one created from the Intl.Collator.prototype. As a consumer of the API, I agree that that should work.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Nov 24, 2015

Contributor

The problematic pattern seen in the wild here though is not using an "arbitrary" object, but one created from the Intl.Collator.prototype. As a consumer of the API, I agree that that should work.

No other built-in object behaves that way.

Contributor

rwaldron commented Nov 24, 2015

The problematic pattern seen in the wild here though is not using an "arbitrary" object, but one created from the Intl.Collator.prototype. As a consumer of the API, I agree that that should work.

No other built-in object behaves that way.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Nov 24, 2015

Member

@thetalecrafter Has JSC shipped a version of Safari with the new semantics?

Member

littledan commented Nov 24, 2015

@thetalecrafter Has JSC shipped a version of Safari with the new semantics?

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Nov 25, 2015

The Intl implementation is still in progress in JSC. Safari has not shipped a version with it enabled yet. WebKit has it enabled by default, but I assume ports are all disabling it for releases until it is complete.

vanwagonet commented Nov 25, 2015

The Intl implementation is still in progress in JSC. Safari has not shipped a version with it enabled yet. WebKit has it enabled by default, but I assume ports are all disabling it for releases until it is complete.

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Nov 25, 2015

@rwaldron It may not be how built-ins work, but it is how most user constructors work, so I don't think it is an unreasonable expectation. That said, I like the v2 semantics, and would prefer them if we can avoid compat issues.

vanwagonet commented Nov 25, 2015

@rwaldron It may not be how built-ins work, but it is how most user constructors work, so I don't think it is an unreasonable expectation. That said, I like the v2 semantics, and would prefer them if we can avoid compat issues.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Nov 25, 2015

Member

It's pretty clear given https://bugzilla.mozilla.org/show_bug.cgi?id=899361 that the V1 behavior being able to initialize any preexisting objects has not been inoperablely implemented across all major browsers.

@ericf can you verify. Does the intl-format-cache (prior to your recent change) actually work on FF or versions of IE that support ECMA-402 Edition 1?

Member

allenwb commented Nov 25, 2015

It's pretty clear given https://bugzilla.mozilla.org/show_bug.cgi?id=899361 that the V1 behavior being able to initialize any preexisting objects has not been inoperablely implemented across all major browsers.

@ericf can you verify. Does the intl-format-cache (prior to your recent change) actually work on FF or versions of IE that support ECMA-402 Edition 1?

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Nov 25, 2015

Member

@allenwb Yes intl-format-cache has worked in the latest versions of IE 11, Firefox, Chrome, and with the Inlt.js polyfill since September 18, 2014. This test passes in all of the them:

var o = Object.create(Intl.NumberFormat.prototype);
var n = Intl.NumberFormat.call(o);
n === o; // true

Chrome Canary (49) is the first browser release where the above test started to fail when it was previously working in that browser.

Member

ericf commented Nov 25, 2015

@allenwb Yes intl-format-cache has worked in the latest versions of IE 11, Firefox, Chrome, and with the Inlt.js polyfill since September 18, 2014. This test passes in all of the them:

var o = Object.create(Intl.NumberFormat.prototype);
var n = Intl.NumberFormat.call(o);
n === o; // true

Chrome Canary (49) is the first browser release where the above test started to fail when it was previously working in that browser.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Nov 25, 2015

Member

@ericf
But does o/n actually work as a NumberFormat instance after such a call?

If it does, it isn't clear how that reconciles with the Mozilla bug.

Member

allenwb commented Nov 25, 2015

@ericf
But does o/n actually work as a NumberFormat instance after such a call?

If it does, it isn't clear how that reconciles with the Mozilla bug.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Nov 25, 2015

Collaborator

The Firefox bug is only observable when multiple globals are present.

var iframe = document.createElement("iframe");
document.body.appendChild(iframe);
var otherGlobal = iframe.contentWindow;

var o = Object.create(Intl.NumberFormat.prototype);
var n = Intl.NumberFormat.call(o, "en");

// Throws TypeError in Firefox
otherGlobal.Intl.NumberFormat.prototype.resolvedOptions.call(n);
Collaborator

anba commented Nov 25, 2015

The Firefox bug is only observable when multiple globals are present.

var iframe = document.createElement("iframe");
document.body.appendChild(iframe);
var otherGlobal = iframe.contentWindow;

var o = Object.create(Intl.NumberFormat.prototype);
var n = Intl.NumberFormat.call(o, "en");

// Throws TypeError in Firefox
otherGlobal.Intl.NumberFormat.prototype.resolvedOptions.call(n);
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Nov 25, 2015

Member

@ericf If Safari ships an Intl object which doesn't work like the others do with respect to this property, will it break existing websites which fail to upgrade to your new version (vs the current feature test and fallback to the polyfill)?

Member

littledan commented Nov 25, 2015

@ericf If Safari ships an Intl object which doesn't work like the others do with respect to this property, will it break existing websites which fail to upgrade to your new version (vs the current feature test and fallback to the polyfill)?

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Feb 21, 2016

At https://esdiscuss.org/notes/2016-01/2016-01-28.md I wrote

MM: what you do, is the call behavior of Intl.Collator if its sees an object that inherits from Collator.prototype that is not a collator, it creates a new Collator and uses a unique Symbol to hang that newCollator off of that object. All the builtin Collator methods then checks whether their alleged Collator argument is a non-Collator with a property named by that Symbol. If so, it looks up the Collator state on the value of that property, i.e., it forwards all state lookup to the value of that property. A hack to deal with an emergency. No security issue here, so no problem that the Symbol can be read and used for other purposes. No accidental collisions, all the primitives must then detect the collator.

So, going just from what I say above and not reviewing the previous notes above, here's an untested first shot at a self-hosted solution:

const KLUDGE = Symbol();
function IsCollator(specimen) {
  // test if it is a builtin exotic Collator instance
}
Intl.Collator.call = function(self, ...args) {
  if (self instanceof this && !IsCollator(self)) {
    const genuine = new this(...args);
    self[KLUDGE] = genuine;
    return self;
  } else {
    // not quite transparent, so this whole kludge must be 
    // removed once it is no longer needed
    return this.apply(self, args);
  }
};

const BUILTIN_METHODS_NEEDING_PATCHING = [
  // names of builtin Collator.prototype methods that currently
  // require a genuine builtin exotic Collator instance.
  // Do not include the name "constructor".
];

for (const name of BUILTIN_METHODS_NEEDING_PATCHING) {
  const original = Collator.prototype[name];
  Collator.prototype[name] = function(...args) {
    return original.apply(this[KLUDGE] || this, args);
  };
}

The generic monkey patching here will probably work for most of these builtin methods, but it depends on the semantics of each. A builtin method that makes the identity of its this observable, say, by returning it, must return the this it was given rather than this[KLUDGE]. If one of these builtin methods accesses own properties of its this, those property accesses should again be on the this it was given.

Once something works, please let me know how close I came ;).

erights commented Feb 21, 2016

At https://esdiscuss.org/notes/2016-01/2016-01-28.md I wrote

MM: what you do, is the call behavior of Intl.Collator if its sees an object that inherits from Collator.prototype that is not a collator, it creates a new Collator and uses a unique Symbol to hang that newCollator off of that object. All the builtin Collator methods then checks whether their alleged Collator argument is a non-Collator with a property named by that Symbol. If so, it looks up the Collator state on the value of that property, i.e., it forwards all state lookup to the value of that property. A hack to deal with an emergency. No security issue here, so no problem that the Symbol can be read and used for other purposes. No accidental collisions, all the primitives must then detect the collator.

So, going just from what I say above and not reviewing the previous notes above, here's an untested first shot at a self-hosted solution:

const KLUDGE = Symbol();
function IsCollator(specimen) {
  // test if it is a builtin exotic Collator instance
}
Intl.Collator.call = function(self, ...args) {
  if (self instanceof this && !IsCollator(self)) {
    const genuine = new this(...args);
    self[KLUDGE] = genuine;
    return self;
  } else {
    // not quite transparent, so this whole kludge must be 
    // removed once it is no longer needed
    return this.apply(self, args);
  }
};

const BUILTIN_METHODS_NEEDING_PATCHING = [
  // names of builtin Collator.prototype methods that currently
  // require a genuine builtin exotic Collator instance.
  // Do not include the name "constructor".
];

for (const name of BUILTIN_METHODS_NEEDING_PATCHING) {
  const original = Collator.prototype[name];
  Collator.prototype[name] = function(...args) {
    return original.apply(this[KLUDGE] || this, args);
  };
}

The generic monkey patching here will probably work for most of these builtin methods, but it depends on the semantics of each. A builtin method that makes the identity of its this observable, say, by returning it, must return the this it was given rather than this[KLUDGE]. If one of these builtin methods accesses own properties of its this, those property accesses should again be on the this it was given.

Once something works, please let me know how close I came ;).

@erights

This comment has been minimized.

Show comment
Hide comment
@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Feb 21, 2016

Member

For a delayed status update on my end: based on other web-breaking changes that ended up being shippable later when libraries were fixed (e.g. an issue with es6shim), I'm now suspecting this might not be so bad. I was thinking of trying out shipping it in three weeks or so for Chrome Canary 51 and seeing if anyone complains. Last time I tried to ship it, react-intl broke with the change, but now it has been fixed for months. If I do get complaints of site breakage I'll implement Mark's fix and write it up in spec text to be clear for other implementors.

Member

littledan commented Feb 21, 2016

For a delayed status update on my end: based on other web-breaking changes that ended up being shippable later when libraries were fixed (e.g. an issue with es6shim), I'm now suspecting this might not be so bad. I was thinking of trying out shipping it in three weeks or so for Chrome Canary 51 and seeing if anyone complains. Last time I tried to ship it, react-intl broke with the change, but now it has been fixed for months. If I do get complaints of site breakage I'll implement Mark's fix and write it up in spec text to be clear for other implementors.

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Feb 21, 2016

Thanks @erights! I'll give that a try.

vanwagonet commented Feb 21, 2016

Thanks @erights! I'll give that a try.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Feb 29, 2016

Collaborator

@thetalecrafter any update on this?

Collaborator

caridy commented Feb 29, 2016

@thetalecrafter any update on this?

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Feb 29, 2016

I created a patch for the JSC issue that works around the problem by hanging a proper NumberFormat/DateTimeFormat object off of a private name property. It isn't implemented in js, like @erights's gist, but is the same idea.

vanwagonet commented Feb 29, 2016

I created a patch for the JSC issue that works around the problem by hanging a proper NumberFormat/DateTimeFormat object off of a private name property. It isn't implemented in js, like @erights's gist, but is the same idea.

@vanwagonet

This comment has been minimized.

Show comment
Hide comment
@vanwagonet

vanwagonet Feb 29, 2016

Also worth noting: I did not apply the workaround to Collator objects, since they are not available in the Intl.js polyfill and are not created by the intl-format-cache library.

vanwagonet commented Feb 29, 2016

Also worth noting: I did not apply the workaround to Collator objects, since they are not available in the Intl.js polyfill and are not created by the intl-format-cache library.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Mar 1, 2016

Collaborator

cool, thanks @thetalecrafter. @littledan will attempt to do the same in v8, and we can probably add an annex to 402 to reflect that.

Collaborator

caridy commented Mar 1, 2016

cool, thanks @thetalecrafter. @littledan will attempt to do the same in v8, and we can probably add an annex to 402 to reflect that.

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Mar 1, 2016

If we expect this to be a temporary fix that is reversed after, at most, a few months, an annex would be overkill. Just point all interested parties at this thread in the meantime, until it is no longer needed.

erights commented Mar 1, 2016

If we expect this to be a temporary fix that is reversed after, at most, a few months, an annex would be overkill. Just point all interested parties at this thread in the meantime, until it is no longer needed.

@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Mar 1, 2016

Collaborator

@erights that's even easier :)

I will keep this open for the time being until @ericf, @bterlson and @littledan report back.

Collaborator

caridy commented Mar 1, 2016

@erights that's even easier :)

I will keep this open for the time being until @ericf, @bterlson and @littledan report back.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 2, 2016

Member

A note: My expectation that libraries were updated quickly was based on a misunderstanding of a particular bug report's resolution. Turns out the bug was still there, and I needed to work around a similar issue. We'll see how long this lasts. I'm working on implementing this and writing up a description; sorry for the confusion coming from me on this part.

Member

littledan commented Mar 2, 2016

A note: My expectation that libraries were updated quickly was based on a misunderstanding of a particular bug report's resolution. Turns out the bug was still there, and I needed to work around a similar issue. We'll see how long this lasts. I'm working on implementing this and writing up a description; sorry for the confusion coming from me on this part.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 9, 2016

Member

OK, looking at the spec and @erights 's proposal (instanceof seems like a sound thing to test for as far as I can tell), I have a couple slight modifications to propose:

  • Rather than first reading the symbol for the real object, and then defaulting to the object, what if we first do the type check (checking for the right internal slot), then if that fails, read the symbol and do the type check on that, throwing if that fails, and continuing with that object if successful? This would eliminate the extra property access in the common case, and get rid of a not-so-useful-but-still-observable place to override things.
  • Let's tie this in with bound functions. All the legacy functions are bound, so to be conservative, maybe all bound functions should get this treatment. But new proposed functions don't need it. In V8's implementation, there's a central piece of machinery for creating bound functions. The spec (as well as SpiderMonkey) seems to reimplement bound functions all over the place. I'm thinking of tying this in with a little spec refactoring to make an internal algorithm for handling bound functions, which would also handle the type checking/symbol lookup changes.

Thoughts?

Member

littledan commented Mar 9, 2016

OK, looking at the spec and @erights 's proposal (instanceof seems like a sound thing to test for as far as I can tell), I have a couple slight modifications to propose:

  • Rather than first reading the symbol for the real object, and then defaulting to the object, what if we first do the type check (checking for the right internal slot), then if that fails, read the symbol and do the type check on that, throwing if that fails, and continuing with that object if successful? This would eliminate the extra property access in the common case, and get rid of a not-so-useful-but-still-observable place to override things.
  • Let's tie this in with bound functions. All the legacy functions are bound, so to be conservative, maybe all bound functions should get this treatment. But new proposed functions don't need it. In V8's implementation, there's a central piece of machinery for creating bound functions. The spec (as well as SpiderMonkey) seems to reimplement bound functions all over the place. I'm thinking of tying this in with a little spec refactoring to make an internal algorithm for handling bound functions, which would also handle the type checking/symbol lookup changes.

Thoughts?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 18, 2016

Member

Any feedback on this proposal would be appreciated. Thanks.

Member

littledan commented Mar 18, 2016

Any feedback on this proposal would be appreciated. Thanks.

@erights

This comment has been minimized.

Show comment
Hide comment
@erights

erights Mar 18, 2016

Oh sorry. After our chat where you clarified some of my confusions, this looks good to me.

erights commented Mar 18, 2016

Oh sorry. After our chat where you clarified some of my confusions, this looks good to me.

littledan added a commit to littledan/ecma402 that referenced this issue Mar 25, 2016

ECMA-402 v1 legacy constructor semantics compromise
This patch addresses #57 by allowing certain legacy constructor
patterns to coexist with the new guarantees in ECMA-402 v2, which
allows for a pattern where all internal slots exist from the
beginning of the object's lifetime. The compromise is based on
storing a "real" object inside of a symbol-named property to allow
for object initialization in cases of the
Intl.<constructor>.call(Object.create(Intl.<constructor>) pattern.
Legacy methods have to forward their calls to this "real" object.

This patch specifies the change for Intl.NumberFormat, but an
analogous change would also be needed for Intl.DateTimeFormat and
Intl.Collator. This version is being sent out for review for feedback
from users and implementors. A sample implementation in V8 can
be found at
https://codereview.chromium.org/1828543007

littledan added a commit to littledan/ecma402 that referenced this issue Mar 25, 2016

ECMA-402 v1 legacy constructor semantics compromise
This patch addresses #57 by allowing certain legacy constructor
patterns to coexist with the new guarantees in ECMA-402 v2, which
allows for a pattern where all internal slots exist from the
beginning of the object's lifetime. The compromise is based on
storing a "real" object inside of a symbol-named property to allow
for object initialization in cases of the
Intl.<constructor>.call(Object.create(Intl.<constructor>) pattern.
Legacy methods have to forward their calls to this "real" object.

This patch specifies the change for Intl.NumberFormat, but an
analogous change would also be needed for Intl.DateTimeFormat and
Intl.Collator. This version is being sent out for review for feedback
from users and implementors. A sample implementation in V8 can
be found at
https://codereview.chromium.org/1828543007

littledan added a commit to littledan/ecma402 that referenced this issue Mar 28, 2016

ECMA-402 v1 legacy constructor semantics compromise for NumberFormat
This patch addresses #57 by allowing certain legacy constructor
patterns to coexist with the new guarantees in ECMA-402 v2, which
allows for a pattern where all internal slots exist from the
beginning of the object's lifetime. The compromise is based on
storing a "real" object inside of a symbol-named property to allow
for object initialization in cases of the
Intl.<constructor>.call(Object.create(Intl.<constructor>) pattern.
Legacy methods have to forward their calls to this "real" object.

This patch specifies the change for Intl.NumberFormat, and a follow-on
patch makes the same change for Intl.DateTimeFormat.

This patch, together with changes for Intl.DateTimeFormat,
has been demonstrated to fix old versions of Intl.js on a deployed
website.

A sample implementation in V8 can be found at
https://codereview.chromium.org/1828543007
@jungshik

This comment has been minimized.

Show comment
Hide comment
@jungshik

jungshik Dec 19, 2016

@thetalecrafter wrote on Feb 29 regarding his change to JSC:

Also worth noting: I did not apply the workaround to Collator objects, since they are not available
in the Intl.js polyfill and are not created by the intl-format-cache library.

Does this mean that Intl.Collator can be left alone (no change in v3 spec is necessary, but v2 spec is ok) in terms of web compat? Besides, @ericf talked about intl-format-cache from whose name I guess it does not deal with Collator. Am I right? Is there any other widely used library that relies on the v1 behavior of Intl.Collator? If not, is it a bad idea to keep the v2 behavior for Intl.Collator while adding a v1-compatibility change in v3 for Intl.{DateTime,Number}Format?

jungshik commented Dec 19, 2016

@thetalecrafter wrote on Feb 29 regarding his change to JSC:

Also worth noting: I did not apply the workaround to Collator objects, since they are not available
in the Intl.js polyfill and are not created by the intl-format-cache library.

Does this mean that Intl.Collator can be left alone (no change in v3 spec is necessary, but v2 spec is ok) in terms of web compat? Besides, @ericf talked about intl-format-cache from whose name I guess it does not deal with Collator. Am I right? Is there any other widely used library that relies on the v1 behavior of Intl.Collator? If not, is it a bad idea to keep the v2 behavior for Intl.Collator while adding a v1-compatibility change in v3 for Intl.{DateTime,Number}Format?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 21, 2016

Member

@jungshik Yes, not applying to Collator was the plan we agreed on in TC39 when this was discussed IIRC.

Member

littledan commented Dec 21, 2016

@jungshik Yes, not applying to Collator was the plan we agreed on in TC39 when this was discussed IIRC.

littledan added a commit to littledan/ecma402 that referenced this issue Dec 28, 2016

ECMA-402 v1 legacy constructor semantics compromise for NumberFormat
This patch addresses #57 by allowing certain legacy constructor
patterns to coexist with the new guarantees in ECMA-402 v2, which
allows for a pattern where all internal slots exist from the
beginning of the object's lifetime. The compromise is based on
storing a "real" object inside of a symbol-named property to allow
for object initialization in cases of the
Intl.<constructor>.call(Object.create(Intl.<constructor>) pattern.
Legacy methods have to forward their calls to this "real" object.

This patch specifies the change for Intl.NumberFormat, and a follow-on
patch makes the same change for Intl.DateTimeFormat.

This patch, together with changes for Intl.DateTimeFormat,
has been demonstrated to fix old versions of Intl.js on a deployed
website.

A sample implementation in V8 can be found at
https://codereview.chromium.org/1828543007

littledan added a commit to littledan/ecma402 that referenced this issue Jan 3, 2017

ECMA-402 v1 legacy constructor semantics compromise for NumberFormat
This patch addresses #57 by allowing certain legacy constructor
patterns to coexist with the new guarantees in ECMA-402 v2, which
allows for a pattern where all internal slots exist from the
beginning of the object's lifetime. The compromise is based on
storing a "real" object inside of a symbol-named property to allow
for object initialization in cases of the
Intl.<constructor>.call(Object.create(Intl.<constructor>) pattern.
Legacy methods have to forward their calls to this "real" object.

This patch specifies the change for Intl.NumberFormat, and a follow-on
patch makes the same change for Intl.DateTimeFormat.

This patch, together with changes for Intl.DateTimeFormat,
has been demonstrated to fix old versions of Intl.js on a deployed
website.

A sample implementation in V8 can be found at
https://codereview.chromium.org/1828543007
@caridy

This comment has been minimized.

Show comment
Hide comment
@caridy

caridy Aug 10, 2017

Collaborator

This has landed in 4rd edition.

Collaborator

caridy commented Aug 10, 2017

This has landed in 4rd edition.

@caridy caridy closed this Aug 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment