- Notifications
You must be signed in to change notification settings - Fork1.3k
Normative: Fix extending null#1321
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
after reading through all this history, it seems the solution needs to additionally include making super() a no-op? |
862d902
tod941b5c
CompareMaybe, though I can imagine arguments against that as well... I am not sure what solution would be acceptable to all objections raised. |
personally i would expect |
6fc9a49
to2e6764b
CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
2e6764b
to6477326
Comparebakkot commentedNov 15, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
See also#699 and#755 and thenotes. To evaluate this, it would be helpful to describe what happens in a variety of cases. For example:
|
devsnek commentedNov 15, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@bakkot thanks for putting that together. all of these except case 10 are passing, which i'm looking into now. print('1');{constx=newclassextendsnull{}();if(x.hasOwnProperty){thrownewError('1 should not inherit');}}print('2');{constx=newclassextendsnull{constructor(){}}();if(x.hasOwnProperty){thrownewError('2 should not inherit');}}print('3');{constx=newclassextendsnull{constructor(){super();}}();if(x.hasOwnProperty){thrownewError('3 should not inherit');}}print('4');{try{classBase{}classDerivedextendsBase{}Object.setPrototypeOf(Derived,null);newDerived();thrownewError('4 should have failed at super() in the default derived constructor');}catch{}}print('5');{try{classBase{}classDerivedextendsBase{constructor(){}}Object.setPrototypeOf(Derived,null);newDerived();thrownewError('5 should have failed at thisER.GetThisBinding()');}catch{}}print('6');{try{classBase{}classDerivedextendsBase{constructor(){super();}}Object.setPrototypeOf(Derived,null);newDerived();thrownewError('6 should fail, null is not a constructor');}catch{}}print('7');{classBase{}classDerivedextendsnull{}Object.setPrototypeOf(Derived,Base);constx=newDerived();if(xinstanceofBase){thrownewError('7 should have base constructor kind, locked prototype');}}print('8');{classBase{}classDerivedextendsnull{constructor(){}}Object.setPrototypeOf(Derived,Base);constx=newDerived();if(xinstanceofBase){thrownewError('8 should have base constructor kind, locked prototype');}}print('9');{classBase{}classDerivedextendsnull{constructor(){super();}}Object.setPrototypeOf(Derived,Base);constx=newDerived();if(xinstanceofBase){thrownewError('9 should have base constructor kind, locked prototype');}}print('10');{functionBase(){this.x=1;}Base.prototype=null;classDerivedextendsBase{};constx=newDerived();if(x.x!==1){thrownewError('10 should work after spec change');}}print('11');{functionBase(){this.x=1;}Base.prototype=null;classDerivedextendsnull{}Object.setPrototypeOf(Derived,Base);constx=newDerived();if(Object.getPrototypeOf(Object.getPrototypeOf(x))===Base||x.x===1){thrownewError('11 should have base constructor kind, locked prototype');}} |
6477326
to1d04de0
Comparealright case 10 is passing 👌 |
This has the effect that Function.prototype.prototype=null;(classextendsFunction.prototype{}) will result in a class which is considered a |
oh crap i was confusing Function.prototype and Function.prototype.prototype 😆 thanks for the pointer |
1d04de0
to47a65e4
Comparebakkot commentedNov 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So, given all the above, let me check if my understanding is correct / try to summarize: Whether a class is considered There are roughly four distinct kinds of classes:
1 and 2 are not changed, but I'll summarize anyway. For 1, the For 2, the For 3, the For 4, the class is not instantiable (assuming no return-override): Do I have that right? |
devsnek commentedNov 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@bakkot looks good |
bakkot commentedNov 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Great. I think 3 is pretty weird - is there a reason to make the behavior of Edit: throwing Edit2: actually, it's pretty easy to avoid the above problem, because the decision about what to make the default constructor happens after evaluating the heritage. Step 10a inClassDefinitionEvaluation could just be "If ClassHeritage_opt is present _andsuperclass is notnull, then" and then the default constructor for 3 would not attempt to invoke |
@bakkot the reason to allow |
"Allowed" is a funny term. Making it legal but throwing is still "allowed" in some sense. I think it's less confusing to have it legal syntactically but forbidden at runtime rather than also legal at runtime but not invoke the class's constructor, given the behavior in 2. |
would it be breaking to remove the early error forbidding super in classes without heritage? we could remove the special case in SuperCall for |
No, but it would make me sad. |
moztcampbell commentedOct 22, 2021
When |
I am really concerned about allowing the dynamic extend scenario as that would be a breaking change, and IMO making a static @erights suggested
functionNull(){returnObject.create(new.target.prototype);}Null.prototype=null;classExNihiloextendsNull{} A
[*] It would only be an approximation since the transpilation would result in |
can you expand on how |
How is having No matter what we do here, we shouldn't distinguish |
|
I think I misunderstood@bathos's example. Regardless I am still uncomfortable changing the behavior of A class that doesn't want to inherit from |
bathos commentedOct 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This is how it works currently, unless I'm misunderstanding? The last example I gave is of code that has worked for the last six years. The changes proposed here would make that currently-valid code throw rather than enable it to work. |
mhofman commentedOct 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
"having" seem a little unclear.
|
bathos commentedOct 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Just for the sake of making sure this is well-understood, what is a construction time error currently is AFAICT, if super() were changed to behave as Object.create(new.target.prototype) when the prototype of the function is null, |
@bakkot yes but conceptually they’re the same thing. |
@mhofman all base classes per the current spec inherit from Object.prototype, so im not sure why that would be a requirement. |
What would be a requirement? Sorry I lost track. From what I understand, a use case is to enable bare classes that don't inherit from |
A class can extend any expression, and i think that should be preserved, even if the expression evaluates to null. |
That's fair, though I want to point out that's not currently the consensus of the committee. I don't know exactly how the process goes here but I suspect you'll want to bring this up in plenary. |
I think we agree here. To be pedantic, a class can extend any expression that evaluates to an object or |
@mhofman sure, but that's a technical loophole. Providing a way to make a class with a null prototype thatdoesn't work with both |
moztcampbell commentedOct 24, 2021
Can someone clarify if the desired behaviour of |
bakkot commentedOct 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The latter. Methods defined on the class should still be accessible from instances, it's just that you won't get methods from |
and this pr doesn't actually change anything about that, it just fixes the errors during construction. |
Is it possible to just special case |
bathos commentedOct 28, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jridgewell that’s what I was wondering about too, though I’d mistakenly been thinking “is null” instead of “is Function.prototype” and now I can see why there might be more resistance to that path. Even so I’m in favor of it since it would not break or alter existing extends null usage (I think). Since that approach would seemingly be a modification to SuperCall steps, it doesn’t appear this would interfere with |
Correct.
Not that I know of. Essentially, this entire PR becomes:
I think. |
@jridgewell Thinking about this more I realized the other “default” class — Object, rather than Function — already has a kind of special casing related to derived class construction. The “active function” condition in theObject constructor is what prevents Object from doing its normal thing (which would amount to a return override) when it’s being called from |
Uh oh!
There was an error while loading.Please reload this page.
Right now if you extend null it isn't a problem until [[Construct]] which would expect
derived
to already havethis
bound which isn't going to be the case because there is never a super call. We can just bypass this directly by keeping the ConstructorKind set to base.This PR also allows
super()
and keepssuper() === this
.Fixes#1036