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

class extends null with implicit constructor still broken #1036

Open
allenwb opened this Issue Nov 21, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@allenwb
Member

allenwb commented Nov 21, 2017

#781 reverted class extends null {} to the ES2015 semantics and says:

Future work will be done to re-enable the use of a null
extends clause,

but there does not appear to be an issue for that "future work".

The original problem in ES2015 is that new class extends null {} throws because the default constructor that is inserted does a super() call to %FunctionPrototype% which is not a constructor.

The guards in ES2015 to prevent inserting that super() call were wrong and other subsequent attempts to correct that did things that cause other problems (see #781).

I believe there is actually a simple spec. fix for this problem (referencing the ES2015 spec so things don't change out from under. Step 10.a currently is:

a. If ClassHeritageopt is present, then
the fix is:
a. If constructorParent is not the intrinsic object %FunctionPrototype%, then

Previous "fixes" tried to condition the implicit constructor choice on a null superclass value and/or other more global changes to the construction process. The new fix is a localized change that fixes the actual bug: generating a 'super()call to %FunctionPrototype% which we know will fail because it is not a constructor. Also note that the above change is safe because:class extends Function.prototype ()` will independently throw.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Nov 22, 2017

Member

In addition to the 10.a change, step 15 should be changed from

  1. If ClassHeritageopt is present, set F.[[ConstructorKind]] to "derived".

to

  1. If constructorParent is not the intrinsic object %FunctionPrototype%, set F.[[ConstructorKind]] to "derived".

Basically after step 10, the appropriate test to determine whether we are defining a base class is whether constructorParent is %FunctionPrototype%.

Member

allenwb commented Nov 22, 2017

In addition to the 10.a change, step 15 should be changed from

  1. If ClassHeritageopt is present, set F.[[ConstructorKind]] to "derived".

to

  1. If constructorParent is not the intrinsic object %FunctionPrototype%, set F.[[ConstructorKind]] to "derived".

Basically after step 10, the appropriate test to determine whether we are defining a base class is whether constructorParent is %FunctionPrototype%.

@bakkot bakkot referenced this issue Nov 27, 2017

Merged

do es-shims API #3

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Dec 21, 2017

It's not just the default constructor that throws; an explicit construct also throws, at least as implemented in v8, JSC and SpiderMonkey:

new (class extends null {}) // throws because of implicit constructor
new (class extends null { constructor() {} }) // throws because of missing super call
new (class extends null { constructor() { super() } }) // throws because of super call

Kovensky commented Dec 21, 2017

It's not just the default constructor that throws; an explicit construct also throws, at least as implemented in v8, JSC and SpiderMonkey:

new (class extends null {}) // throws because of implicit constructor
new (class extends null { constructor() {} }) // throws because of missing super call
new (class extends null { constructor() { super() } }) // throws because of super call
@saschanaz

This comment has been minimized.

Show comment
Hide comment
@saschanaz

saschanaz Dec 21, 2017

ChakraCore in MSEdge build 17063 allows the empty constructor() {} thing, not sure it's spec-compliant.

saschanaz commented Dec 21, 2017

ChakraCore in MSEdge build 17063 allows the empty constructor() {} thing, not sure it's spec-compliant.

@claudepache

This comment has been minimized.

Show comment
Hide comment
@claudepache

claudepache Dec 21, 2017

Contributor

@Kovensky: It is possible to define a working constructor to a class extending null, provided that that constructor returns an explicit object, e.g.:

class C extends null { constructor() { return Object.create(new.target.prototype) } }
Contributor

claudepache commented Dec 21, 2017

@Kovensky: It is possible to define a working constructor to a class extending null, provided that that constructor returns an explicit object, e.g.:

class C extends null { constructor() { return Object.create(new.target.prototype) } }
@claudepache

This comment has been minimized.

Show comment
Hide comment
@claudepache

claudepache Dec 22, 2017

Contributor

@allenwb Unless I missed something (which is possible), your proposed fix is observably equivalent to the last reverted one in #781?

Contributor

claudepache commented Dec 22, 2017

@allenwb Unless I missed something (which is possible), your proposed fix is observably equivalent to the last reverted one in #781?

@saschanaz

This comment has been minimized.

Show comment
Hide comment
@saschanaz

saschanaz Dec 22, 2017

Can we make super() be no-op if ClassHeritage is null/undefined?

saschanaz commented Dec 22, 2017

Can we make super() be no-op if ClassHeritage is null/undefined?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 22, 2017

Member

When @ajklein presented on the reversal of this change in #781, I thought one of the concerns we discussed for that sort of path was that this would be the first time we'd make it based on runtime values and not syntax whether something is a base class or subclass.

Member

littledan commented Dec 22, 2017

When @ajklein presented on the reversal of this change in #781, I thought one of the concerns we discussed for that sort of path was that this would be the first time we'd make it based on runtime values and not syntax whether something is a base class or subclass.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Jan 24, 2018

Member

@claudepache Do you mean f3881fe ? You are probably right that they are observably equivalent. Probably because they both express my original intent.

The intent was that the values present at ClassDefinitionEvaluation determined whether a base class or a derived class was created and the form of default constructor that is emitted when one is needed. A class that doesn't have an extends clause or whose extends clause evaluated to null should be created as a Base class.
All other cases where the extends clause evaluates to a constructor function should be created as a Derived class.

It isn't relevant whether whether after the class is created somebody uses SetPrototypeOf to modify either the [[Prototype]] of the constructor or the [[Prototype]] or the prototype. This was agreed upon during ES6 development. It was concluded that it was hacker beware if they type to play proto games with constructors created using class definitions. Given that, I think

It was a bug ES2015 generated the wrong kind of default constructor and set extends null classes to derived. But it is by design that things are likely to break if you do proto hacking.

It isn't clear to me why f3881fe need to be reverted. Or my not to use the fix I have above. As either gives the original intended behavior which of buggy in ES2015.

It is kind of embarrassing that this feature has been broken for so long. It should really be fixed. I guess I think that this is the type of thing that the editor should be driving to resolution.

Member

allenwb commented Jan 24, 2018

@claudepache Do you mean f3881fe ? You are probably right that they are observably equivalent. Probably because they both express my original intent.

The intent was that the values present at ClassDefinitionEvaluation determined whether a base class or a derived class was created and the form of default constructor that is emitted when one is needed. A class that doesn't have an extends clause or whose extends clause evaluated to null should be created as a Base class.
All other cases where the extends clause evaluates to a constructor function should be created as a Derived class.

It isn't relevant whether whether after the class is created somebody uses SetPrototypeOf to modify either the [[Prototype]] of the constructor or the [[Prototype]] or the prototype. This was agreed upon during ES6 development. It was concluded that it was hacker beware if they type to play proto games with constructors created using class definitions. Given that, I think

It was a bug ES2015 generated the wrong kind of default constructor and set extends null classes to derived. But it is by design that things are likely to break if you do proto hacking.

It isn't clear to me why f3881fe need to be reverted. Or my not to use the fix I have above. As either gives the original intended behavior which of buggy in ES2015.

It is kind of embarrassing that this feature has been broken for so long. It should really be fixed. I guess I think that this is the type of thing that the editor should be driving to resolution.

@saschanaz

This comment has been minimized.

Show comment
Hide comment
@saschanaz

saschanaz Jan 24, 2018

this would be the first time we'd make it based on runtime values and not syntax whether something is a base class or subclass.

Allowing class extends void {} may be a syntactic way, although I'm not sure whether there is enough interest to add a new syntax for this feature.

saschanaz commented Jan 24, 2018

this would be the first time we'd make it based on runtime values and not syntax whether something is a base class or subclass.

Allowing class extends void {} may be a syntactic way, although I'm not sure whether there is enough interest to add a new syntax for this feature.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Mar 5, 2018

Contributor

@littledan Is this issue a blocker for the public/private fields proposal or is it acceptable if classes extending null won't support public/private fields?

Contributor

anba commented Mar 5, 2018

@littledan Is this issue a blocker for the public/private fields proposal or is it acceptable if classes extending null won't support public/private fields?

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Mar 5, 2018

Member

@anba You can get around this limitation with the following idiom:

class X {
  field;
}

X.__proto__ = null;
X.prototype.__proto__ = null;

The constructor will not call super(), and everything just works like it should. I think this workaround is cleaner than using Object.create manually in the constructor or anything like that that you'd have to do to get around the brokenness of classes that extend null.

For that reason, I'd say that this issue isn't a blocker. Still, I wouldn't mind seeing it resolved.

Member

littledan commented Mar 5, 2018

@anba You can get around this limitation with the following idiom:

class X {
  field;
}

X.__proto__ = null;
X.prototype.__proto__ = null;

The constructor will not call super(), and everything just works like it should. I think this workaround is cleaner than using Object.create manually in the constructor or anything like that that you'd have to do to get around the brokenness of classes that extend null.

For that reason, I'd say that this issue isn't a blocker. Still, I wouldn't mind seeing it resolved.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Mar 5, 2018

Contributor

@littledan I'd 👍 your response, but I just can't because... __proto__! 😉

Contributor

anba commented Mar 5, 2018

@littledan I'd 👍 your response, but I just can't because... __proto__! 😉

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