Normative: Use OrdinaryHasInstance in normative optional steps

[shvaikalesh]

This PR reduces the surface area of legacy constructor semantics by using OrdinaryHasInstance instead of instanceof operator, which skips Symbol.hasInstance lookup/call and a few checks we don't need or already implement.

The change is safe because a) Symbol.hasInstance is rarely used and b) the code that depends on legacy constructor semantics precedes cross-browser Symbol.hasInstance implementation.

JSC already implements this change, while V8 and SpiderMonkey perform InstanceofOperator.

Intl.NumberFormat test case

const nf = new Intl.NumberFormat();

Object.defineProperty(Intl.NumberFormat, Symbol.hasInstance, {
    get() { throw new Error("Intl.NumberFormat[@@hasInstance] lookup"); }
});

nf.resolvedOptions(); // JSC: OK, V8 & SM: exception
nf.format; // JSC: OK, V8 & SM: exception
Intl.NumberFormat(); // JSC: OK, V8 & SM: exception

Intl.DateTimeFormat test case

const dtf = new Intl.DateTimeFormat();

Object.defineProperty(Intl.DateTimeFormat, Symbol.hasInstance, {
    get() { throw new Error("Intl.DateTimeFormat[@@hasInstance] lookup"); }
});

dtf.resolvedOptions(); // JSC: OK, V8 & SM: exception
dtf.format; // JSC: OK, V8 & SM: exception
Intl.DateTimeFormat(); // JSC: OK, V8 & SM: exception

cc @littledan @anba @caridy

EDIT: Updated test cases with resolvedOptions and format.

[FrankYFTang]

FYI- v8 bug filed https://bugs.chromium.org/p/v8/issues/detail?id=10981 for tracking

[FrankYFTang]

@shvaikalesh - could you update the jsc status on https://github.com/tc39/ecma402/wiki/Proposal-and-PR-Progress-Tracking about this? Thanks

[shvaikalesh]

@FrankYFTang I would love to, but I don't have editing rights. JSC implements OrdinaryHasInstance for quite a while.

EDIT: Updated status in 3e50241.

[FrankYFTang]

Your test case cover the changes to the constructor, but not UnwrapNumberFormat nor UnwrapDateTimeFormat.
The changes to UnwrapNumberFormat will change the observable behavior of
get Intl.NumberFormat.prototype.format
Intl.NumberFormat.prototype.resolvedOptions ( )
The changes to UnwrapDateTimeFormat will change the observable behavior of
get Intl.DateTimeFormat.prototype.format
Intl.DateTimeFormat.prototype.resolvedOptions ( )

Do you mind to show the test code for verifying these four functions here too?
Also, could you file an issue (or PR) in test262 about this?

[FrankYFTang]

@sffc "@FrankYFTang I would love to, but I don't have editing rights. " what is our current policy about wiki editing

[sffc]

@sffc "@FrankYFTang I would love to, but I don't have editing rights. " what is our current policy about wiki editing

I just invited @shvaikalesh as an outside collaborator on this repo, which gives wiki edit rights.

[FrankYFTang]

In theory, I support this PR, I think the risk this Normative change to break the web is very small. However, I would still want to see the test cases against the four functions I listed above first. Also, I think we need to understand better of what kind of code are depending on the current legacy way of handling this and see would this change break them. Could someone point out what kind of code would be break if we complete remove the legacy unwrap (I am not proposing to do that, I am just asking to see that kind of code and make sure this change won't break them)

[FrankYFTang]

also, in your sample test code
const nf = Intl.NumberFormat(); // JSC: OK, V8 & SM: exception
should be
const nf = new Intl.NumberFormat(); // JSC: OK, V8 & SM: exception

[FrankYFTang]

FYI - CL to implement this change in v8 - https://chromium-review.googlesource.com/c/v8/v8/+/2444092

[FrankYFTang]

I would like to learn more about the benefit of this PR. What is the key motivation for making the change now? How would this change benefit the current web developers or prevent problem for the future web developers?

[shvaikalesh]

Do you mind to show the test code for verifying these four functions here too?

Updated the #500 (comment).

Also, could you file an issue (or PR) in test262 about this?

I will make sure to submit a test262 PR if this reaches consensus.

Could someone point out what kind of code would be break if we complete remove the legacy unwrap ...

According to #57, legacy unwrap is necessary for FormatJS, which targets browsers that never had @@hasInstance (IE 11).
I've searched its code on GitHub: no usages of hasInstance.

const nf = Intl.NumberFormat(); // JSC: OK, V8 & SM: exception
should be
const nf = new Intl.NumberFormat(); // JSC: OK, V8 & SM: exception

🤔 Isn't instanceof currently invoked only if new.target === undefined (step 5 of Intl.NumberFormat), excluding [[Construct]]?

What is the key motivation for making the change now?

This incompatibility came to my attention during recent legacy Intl constructors update, which implemented the [[FallbackSymbol]] in WebKit.
It seems like a good time to either change the spec or align JSC with other runtimes.

How would this change benefit the current web developers or prevent problem for the future web developers?

As a web dev, I expect that setting @@hasInstance affects only instanceof operator, and not other parts of the spec.

[FrankYFTang]

All your response sounds reasonable to me.

[sffc]

2021-01-14: agreed to move forward with this PR, despite the risks to break ecosystem code. We feel that the simplicity, better hygiene, and performance improvement justify the risks.

Tracking issue for MDN: tc39/ecma402-mdn#19

Tracking issue for Test262: tc39/test262#2933

[sffc]

This achieved TC39 consensus today. @codehag followed up offline.

[FrankYFTang]

FYI- V8 changes landed in https://chromium-review.googlesource.com/c/v8/v8/+/2444092 and will be in chrome m90

[ryzokuken]

@sffc does this require any MDN documentation? The details that have been changed by this seem too internal to be reflected on MDN.

[FrankYFTang]

I agree w/ @ryzokuken 's previous comment. I don't think we need MDN change for this.