[[Construct]]: extends the check against non-undefined primitive #469

claudepache · 2016-03-10T17:24:06Z

When a non-undefined primitive is returned, throw a TypeError
exception, except in the legacy case when the constructor was defined
with the function keyword.

claudepache · 2016-03-10T17:29:18Z

See: https://esdiscuss.org/topic/primitive-values-from-class-constructors

With this change, the following code will throw a TypeError

new class { constructor() { return 42 } }

but the following code will continue to "work" as previously (because of BC constraints):

new function() { return 42 }

Is this change sensible?

When a non-undefined primitive is returned, throw a TypeError exception, except in the legacy case when the constructor was defined with the `function` keyword.

littledan · 2016-03-10T20:59:11Z

While these semantics would be sensible for a new language, I worry about the web compatibility impact on existing ECMAScript code. Does anyone have data on whether this pattern occurs in practice?

raulsebastianmihaila · 2016-03-12T04:34:41Z

@littledan: It should be irrelevant that this pattern occurs in practice, because there's currently no way to actually get the primitive value, because when called as a normal function (without new) the class constructor throws.

ljharb · 2016-03-12T06:38:35Z

Sure, but existing code that does new class { constructor() { return 42 } } which works fine will suddenly start throwing a TypeError, which makes it very relevant if it occurs in practice.

bergus · 2016-03-15T01:48:33Z

I don't think that pattern occurs often enough - on the web, anywhere - to be a concern. I admit I don't have hard numbers, but I really wouldn't worry to make such code throw. For consistency!

UltCombo · 2016-03-15T01:53:54Z

"For consistency" you would want class constructors to behave as close as possible to function in order to avoid refactoring hazards.

bergus · 2016-03-15T03:08:54Z

@UltCombo For integrity! then :-) class constructors already don't work like functions, as they throw without new.
And should you ever refactor function() { return 3; } to a class, it would only be a good thing to have it throw. Such code is a hazard, not the refactoring.

UltCombo · 2016-03-15T03:24:55Z

That's good reasoning indeed, I can agree with that.

Though, vendors may have to add telemetry in order to ensure that this change wouldn't break more than 0.001% of page views (the cutoff for Blink/V8, I believe). And if no one actually uses this pattern, implementors may not care enough to collect data and make this change anyway.

claudepache · 2016-03-15T08:13:38Z

Though, vendors may have to add telemetry in order to ensure that this change wouldn't break more than 0.001% of page views (the cutoff for Blink/V8, I believe).

Since Chrome and Firefox have shipped classes only in their latest stable version, I'm not particularly worried about that point. At least, that should dramatically reduce the intersection between broken pages and unmaintained pages. And for maintained pages, breaking is probably good, because it could reveal a hidden bug in the logic...

bterlson · 2016-03-25T18:42:17Z

I think this is probably a needs-consensus change. I can discuss briefly at the next meeting.

bterlson · 2016-05-23T15:10:30Z

Accepting this PR, thank you!

bterlson · 2016-05-23T15:25:04Z

huh, GH won't let me merge :( Will figure it out.

littledan · 2016-05-25T16:36:17Z

I know we came to consensus on this feature, but given that this is a semantics change, could we have an implementation try it out before it lands in the spec? Thinking about it more, I'm a little worried that it'll decrease performance of ordinary constructors by adding an extra check; if someone tried out implementing this and found it to be fine, then I would be less worried. This hasn't gone through the stage process and is not a bug fix but rather new semantics; maybe it doesn't need stages but some kind of implementation feedback would make me more comfortable.

bterlson · 2016-09-29T18:06:30Z

@littledan if you want to revisit this change now is the time. Otherwise, I'm going to pull this in :-P

bterlson · 2016-09-29T18:06:41Z

Once there's tests, that is.

ajklein · 2016-10-19T14:40:45Z

What I see in the notes from the meeting where this gained consensus is the following conclusion:

Yes on the PR, though we will have to take web compatibility feedback into account as implementations attempt this

The way I interpret this is that the committee agreed that they wanted to change the spec, but wanted to leave the door open for reverting in case this turned out to be non-web-compatible. Given the specific concern, it seems like it might make more sense to treat this as a "Stage 3" proposal, and waiting until it reaches "Stage 4" before merging. That would still allow it to get into test262, and signal to implementations that they should make this change. But it would avoid having a spec which contravenes all existing implementations.

Maybe we're just missing something from our process (or from the labels on these github issues) to make it easy to track state like this?

bterlson · 2016-10-22T15:33:38Z

@ajklein IMO: consensus on this issue is no different from anything else. Any consensus we give is always subject to reevaluation if it is not possible to implement for whatever reason. I think your interpretation makes sense in a world where the spec never leads implementations but that is not the way we have chosen to operate this document for better or worse. tl;dr this change should go in and implementations should complain if they can't implement it.

Happy to clear this up with actual process though. Feel free to make a proposal :)

ajklein · 2016-10-24T07:44:53Z

@bterlson I thought the staging process was all about making sure the spec doesn't lead implementations. Or are you saying that for "smaller" things (which this might fall into) you get the sense the committee is comfortable with the spec leading implementations?

bterlson · 2016-10-24T18:33:23Z

@ajklein yeah, for "smaller" things. Even some non-small things (eg. the LHS reference re-evaluation issue we all had until a few months ago). I think anything with a likelihood of compat risk should not go in until we validate it can, but I'm kind of making a judgement call on this one that it's just a matter of convincing runtimes to do the work and seems unlikely to break things. Putting it in the spec seems like a good way to do that. Again, though, with my Chakra hat on I see exactly where you're coming from :)

littledan · 2017-04-11T11:58:41Z

FWIW implementation behind a flag in V8 is in progress by @gsathya, and if anyone wants to write a test, you can use his great tests from that patch

This change mirrors the semantics for derived class constructors. This change doesn't affect non class constructors. This change could potentially break web compat. More details: tc39/ecma262#469 Bug=v8:5536 Change-Id: I519599949523733332d0b35e4f8d9ecb01cac495 Reviewed-on: https://chromium-review.googlesource.com/461225 Reviewed-by: Benedikt Meurer <bmeurer@chromium.org> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Reviewed-by: Adam Klein <adamk@chromium.org> Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org> Cr-Commit-Position: refs/heads/master@{#44594}

… primitives Port a7c4e77 Original Commit Message: This change mirrors the semantics for derived class constructors. This change doesn't affect non class constructors. This change could potentially break web compat. More details: tc39/ecma262#469 R=gsathya@chromium.org, joransiu@ca.ibm.com, bjaideep@ca.ibm.com, michael_dawson@ca.ibm.com BUG= LOG=N Review-Url: https://codereview.chromium.org/2815873003 Cr-Commit-Position: refs/heads/master@{#44627}

ljharb · 2017-04-15T22:49:32Z

@claudepache why is this closed?

claudepache · 2017-04-16T05:35:51Z

@ljharb That was an accident. Reopened.

ljharb · 2018-04-21T23:25:29Z

Per @gsathya, this is not web-compatible: https://www.chromestatus.com/metrics/feature/timeline/popularity/2054, so I think this needs to be closed.

bergus · 2018-04-22T15:42:02Z

Ouch, those are high numbers. High enough that it should be possible to find some examples of real-world code doing this. Does anyone know how to collect them?

ljharb · 2018-04-22T15:43:47Z

I’d also be curious to see “non null non undefined” numbers - it’s possible that that would paint a different picture.

raulsebastianmihaila · 2018-09-14T17:32:55Z

@ljharb was this closed by mistake?

ljharb · 2018-09-14T17:37:07Z

@raulsebastianmihaila nope. we can reopen it if someone provides different data, but lacking that, #469 (comment) is pretty much conclusive.

[[Construct]]: extends the check against non-undefined primitive

21cc21d

When a non-undefined primitive is returned, throw a TypeError exception, except in the legacy case when the constructor was defined with the `function` keyword.

claudepache force-pushed the construct-return-primitive branch from 99b5be4 to 21cc21d Compare March 10, 2016 18:18

bterlson added the needs consensus This needs committee consensus before it can be eligible to be merged. label Mar 25, 2016

bterlson closed this May 23, 2016

bterlson reopened this May 23, 2016

bterlson added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Sep 29, 2016

bterlson removed the needs consensus This needs committee consensus before it can be eligible to be merged. label Sep 29, 2016

claudepache closed this Apr 15, 2017

claudepache deleted the construct-return-primitive branch April 15, 2017 20:49

claudepache restored the construct-return-primitive branch April 16, 2017 05:33

claudepache reopened this Apr 16, 2017

ljharb closed this Aug 15, 2018

raulsebastianmihaila mentioned this pull request Jan 31, 2021

Inconsistent constructor return checks for base and derived classes #2291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[[Construct]]: extends the check against non-undefined primitive #469

[[Construct]]: extends the check against non-undefined primitive #469

claudepache commented Mar 10, 2016

claudepache commented Mar 10, 2016

littledan commented Mar 10, 2016

raulsebastianmihaila commented Mar 12, 2016

ljharb commented Mar 12, 2016

bergus commented Mar 15, 2016

UltCombo commented Mar 15, 2016

bergus commented Mar 15, 2016

UltCombo commented Mar 15, 2016

claudepache commented Mar 15, 2016

bterlson commented Mar 25, 2016

bterlson commented May 23, 2016

bterlson commented May 23, 2016

littledan commented May 25, 2016

bterlson commented Sep 29, 2016

bterlson commented Sep 29, 2016

ajklein commented Oct 19, 2016 •

edited

Loading

bterlson commented Oct 22, 2016 •

edited

Loading

ajklein commented Oct 24, 2016 •

edited

Loading

bterlson commented Oct 24, 2016

littledan commented Apr 11, 2017 •

edited

Loading

ljharb commented Apr 15, 2017

claudepache commented Apr 16, 2017

ljharb commented Apr 21, 2018

bergus commented Apr 22, 2018

ljharb commented Apr 22, 2018

raulsebastianmihaila commented Sep 14, 2018

ljharb commented Sep 14, 2018

[[Construct]]: extends the check against non-undefined primitive #469

[[Construct]]: extends the check against non-undefined primitive #469

Conversation

claudepache commented Mar 10, 2016

claudepache commented Mar 10, 2016

littledan commented Mar 10, 2016

raulsebastianmihaila commented Mar 12, 2016

ljharb commented Mar 12, 2016

bergus commented Mar 15, 2016

UltCombo commented Mar 15, 2016

bergus commented Mar 15, 2016

UltCombo commented Mar 15, 2016

claudepache commented Mar 15, 2016

bterlson commented Mar 25, 2016

bterlson commented May 23, 2016

bterlson commented May 23, 2016

littledan commented May 25, 2016

bterlson commented Sep 29, 2016

bterlson commented Sep 29, 2016

ajklein commented Oct 19, 2016 • edited Loading

bterlson commented Oct 22, 2016 • edited Loading

ajklein commented Oct 24, 2016 • edited Loading

bterlson commented Oct 24, 2016

littledan commented Apr 11, 2017 • edited Loading

ljharb commented Apr 15, 2017

claudepache commented Apr 16, 2017

ljharb commented Apr 21, 2018

bergus commented Apr 22, 2018

ljharb commented Apr 22, 2018

raulsebastianmihaila commented Sep 14, 2018

ljharb commented Sep 14, 2018

ajklein commented Oct 19, 2016 •

edited

Loading

bterlson commented Oct 22, 2016 •

edited

Loading

ajklein commented Oct 24, 2016 •

edited

Loading

littledan commented Apr 11, 2017 •

edited

Loading