Skip to content
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

Normative: Fix extending null #1321

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Oct 9, 2018

Right now if you extend null it isn't a problem until [[Construct]] which would expect derived to already have this 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 keeps super() === this.

Fixes #1036

@ljharb
Copy link
Member

ljharb commented Oct 9, 2018

See #781 and #543

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text needs consensus This needs committee consensus before it can be eligible to be merged. labels Oct 9, 2018
@devsnek
Copy link
Member Author

devsnek commented Oct 9, 2018

after reading through all this history, it seems the solution needs to additionally include making super() a no-op?

@devsnek devsnek force-pushed the normative/extending-null branch 3 times, most recently from 862d902 to d941b5c Compare October 9, 2018 16:37
@devsnek devsnek changed the title Normative: Don't set ConstructorKind to derived if the class heritage is null. Oct 9, 2018
@littledan
Copy link
Member

Maybe, though I can imagine arguments against that as well... I am not sure what solution would be acceptable to all objections raised.

@devsnek
Copy link
Member Author

devsnek commented Oct 10, 2018

personally i would expect super() in an extends null to throw, but from what i read from the other issues and meeting notes, people wanted it to not throw because its usually enforced with syntax.

@devsnek devsnek force-pushed the normative/extending-null branch 2 times, most recently from 6fc9a49 to 2e6764b Compare October 31, 2018 03:47
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@bakkot
Copy link
Contributor

bakkot commented Nov 15, 2018

See also #699 and #755 and the notes.

To evaluate this, it would be helpful to describe what happens in a variety of cases. For example:

  • new class extends null {}
  • new class extends null { constructor() { } }
  • new class extends null { constructor() { super(); } }
  • class Base {}; class Derived extends Base {}; Object.setPrototypeOf(Derived, null); new Derived;
  • class Base {}; class Derived extends Base { constructor() { } }; Object.setPrototypeOf(Derived, null); new Derived;
  • class Base {}; class Derived extends Base { constructor() { super(); } }; Object.setPrototypeOf(Derived, null); new Derived;
  • class Base {}; class Derived extends null {}; Object.setPrototypeOf(Derived, Base); new Derived;
  • class Base {}; class Derived extends null { constructor() { } }; Object.setPrototypeOf(Derived, Base); new Derived;
  • class Base {}; class Derived extends null { constructor() { super(); } }; Object.setPrototypeOf(Derived, Base); new Derived;
  • function Base() { this.x = 1; } Base.prototype = null; new class extends Base {}
  • function Base() { this.x = 1; } Base.prototype = null; class Derived extends null {}; Object.setPrototypeOf(Derived, Base); new Derived;
@devsnek
Copy link
Member Author

devsnek commented Nov 15, 2018

@bakkot thanks for putting that together.

all of these except case 10 are passing, which i'm looking into now.

print('1');
{
  const x = new class extends null {}();
  if (x.hasOwnProperty) {
    throw new Error('1 should not inherit');
  }
}

print('2');
{
  const x = new class extends null {
    constructor() {}
  }();
  if (x.hasOwnProperty) {
    throw new Error('2 should not inherit');
  }
}

print('3');
{
  const x = new class extends null {
    constructor() { super(); }
  }();
  if (x.hasOwnProperty) {
    throw new Error('3 should not inherit');
  }
}

print('4');
{
  try {
    class Base {}
    class Derived extends Base {}
    Object.setPrototypeOf(Derived, null);
    new Derived();
    throw new Error('4 should have failed at super() in the default derived constructor');
  } catch {}
}

print('5');
{
  try {
    class Base {}
    class Derived extends Base { constructor() {} }
    Object.setPrototypeOf(Derived, null);
    new Derived();
    throw new Error('5 should have failed at thisER.GetThisBinding()');
  } catch {}
}

print('6');
{
  try {
    class Base {}
    class Derived extends Base {
      constructor() { super(); }
    }
    Object.setPrototypeOf(Derived, null);
    new Derived();
    throw new Error('6 should fail, null is not a constructor');
  } catch {}
}

print('7');
{
  class Base {}
  class Derived extends null {}
  Object.setPrototypeOf(Derived, Base);
  const x = new Derived();
  if (x instanceof Base) {
    throw new Error('7 should have base constructor kind, locked prototype');
  }
}

print('8');
{
  class Base {}
  class Derived extends null { constructor() { } }
  Object.setPrototypeOf(Derived, Base);
  const x = new Derived();
  if (x instanceof Base) {
    throw new Error('8 should have base constructor kind, locked prototype');
  }
}

print('9');
{
  class Base {}
  class Derived extends null { constructor() { super(); } }
  Object.setPrototypeOf(Derived, Base);
  const x = new Derived();
  if (x instanceof Base) {
    throw new Error('9 should have base constructor kind, locked prototype');
  }
}

print('10');
{
  function Base() { this.x = 1; }
  Base.prototype = null;
  class Derived extends Base {};
  const x = new Derived();
  if (x.x !== 1) {
    throw new Error('10 should work after spec change');
  }
}

print('11');
{
  function Base() { this.x = 1; }
  Base.prototype = null;
  class Derived extends null {}
  Object.setPrototypeOf(Derived, Base);
  const x = new Derived();
  if (Object.getPrototypeOf(Object.getPrototypeOf(x)) === Base || x.x === 1) {
    throw new Error('11 should have base constructor kind, locked prototype');
  }
}
@bakkot
Copy link
Contributor

bakkot commented Nov 15, 2018

I expect the reason #10 is not behaving as you expect is because this patch uses protoParent instead of superclass. See #755.

@devsnek
Copy link
Member Author

devsnek commented Nov 15, 2018

alright case 10 is passing 👌

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2018

@devsnek

If _protoParent_ is *null* and _constructorParent_ is %FunctionPrototype%, let _trueNullParent_ be *true*, otherwise let _trueNullParent_ be *false*.
If |ClassHeritage_opt| is present and _trueNullParent_ is *false*, set _F_.[[ConstructorKind]] to `"derived".

This has the effect that

Function.prototype.prototype = null;
(class extends Function.prototype {})

will result in a class which is considered a "base" class, which is probably not desirable. Why not just switch on superclass?

@devsnek
Copy link
Member Author

devsnek commented Nov 16, 2018

oh crap i was confusing Function.prototype and Function.prototype.prototype 😆 thanks for the pointer

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2018

So, given all the above, let me check if my understanding is correct / try to summarize:

Whether a class is considered "base" or "derived" is fixed at the time it is defined. "base" classes are those which lack a ClassHeritage and those whose ClassHeritage evaluates to the value null; all others are "derived".

There are roughly four distinct kinds of classes:

  1. classes which lack a ClassHeritage (these are "base")
  2. classes which have a ClassHeritage which evaluates to a non-null value and whose prototype has not been changed to null (these are "derived")
  3. classes which have ClassHeritage which evaluates to null, regardless of whether the class's prototype has been changed (these are "base")
  4. classes which have a ClassHeritage which evaluates to a non-null value and whose prototype has been changed to null (these are "derived")

1 and 2 are not changed, but I'll summarize anyway.

For 1, the constructor is syntactically prevented from calling super(). The default constructor does not call it. This is true even if the class's prototype is changed to a non-null value (which is more relevant than you might at first think, since in 2 super() may be called inside of eval). The class is always instantiable and (assuming no return-override) always results in a new object whose prototype is the class's .prototype property. Changing the class's prototype has no effects on instances.

For 2, the constructor is required to call super. The default constructor calls super(...args). If the class's prototype is changed to some other constructor, the super call will invoke that constructor and result in the return value, but will (assuming no return-override) still result in an object whose prototype is the original class's .prototype property. Changing the class's prototype (to some other constructor) has the effect of changing what super() returns and therefore what the constructor returns.

For 3, the constructor may or may not call super at its discretion, and may call it multiple times. The default constructor calls super(..args), which is observable because it performs an observable access to Array.prototype[Symbol.iterator]. Calling super() has no effects even if the class's prototype has been changed to some constructor, but its arguments are evaluated and the call returns this. this is bound for the lifetime of the class's constructor, even before the first call to super(). The class is always instantiable and (assuming no return-override) always results in a new object whose prototype is the class's .prototype property. Changing the class's prototype has no effects on instances.

For 4, the class is not instantiable (assuming no return-override): super() cannot be called in the constructor (because the super constructor is not a constructor), but this is not bound and, because super() cannot be called, cannot be bound. The default constructor calls super(...args) and hence throws, without an observable access to Array.prototype[Symbol.iterator] (cf #1351).

Do I have that right?

@devsnek
Copy link
Member Author

devsnek commented Nov 16, 2018

@bakkot looks good

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2018

Great. I think 3 is pretty weird - is there a reason to make the behavior of super() in that case "do nothing" rather than "throw a TypeError"? Given the other cases, that makes the most sense to me. (There might well be a good reason; I don't have the whole history here paged in.)

Edit: throwing super() requires some shenanigans to make the default constructor not throw, I suppose, which might be enough reason to avoid it on its own.

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 in ClassDefinitionEvaluation could just be "If ClassHeritage_opt is present _and superclass is not null, then" and then the default constructor for 3 would not attempt to invoke super().

@devsnek
Copy link
Member Author

devsnek commented Nov 16, 2018

@bakkot the reason to allow super() was to keep the distinction that syntax controls when super is allowed. there was also some other stuff about like class factories or something, but i don't remember which issue it was brought up in.

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2018

"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.

@devsnek
Copy link
Member Author

devsnek commented Nov 16, 2018

would it be breaking to remove the early error forbidding super in classes without heritage? we could remove the special case in SuperCall for null, prototypes of base classes being set wouldn't be ignored, etc.

@bakkot
Copy link
Contributor

bakkot commented Nov 16, 2018

would it be breaking to remove the early error forbidding super in classes without heritage?

No, but it would make me sad.

@moztcampbell
Copy link

When ClassBody_opt is not present, this proposed spec text does not appear to define superclass, yet in the synthesized default constructor we close-over and compare this uninitialized value to null. Is superclass expected to be the abstract empty and distinct from null?

@mhofman
Copy link
Member

mhofman commented Oct 22, 2021

I am really concerned about allowing the dynamic extend scenario as that would be a breaking change, and IMO making a static extends null allowable without allowing extends thingThatEvaluatesToNull is not right. At this point I believe that if we want to explicitly allow bare base classes that do not inherit from Object.prototype, it should be done through a different syntax.

@erights suggested extends void for this case, which was also suggested in #1036 (comment)

class ExNihilo extends void could be transpiled into the following approximation*:

function Null() {
  return Object.create(new.target.prototype);
}
Null.prototype = null;

class ExNihilo extends Null {}

A super usage in extends void classes would be a syntax error, the same as it's not allowed in other base classes.

class extends thingThatEvaluatesToUndefined would continue to not be allowed.

[*] It would only be an approximation since the transpilation would result in Object.getPrototypeOf(ExNihilo) === Null instead of Function.prototype.

@devsnek
Copy link
Member Author

devsnek commented Oct 22, 2021

can you expand on how extends thingThatEvaluatesToNull is causing breakage for you?

@ljharb
Copy link
Member

ljharb commented Oct 23, 2021

How is having extends void but not extends (null) any different than having extends null and not extends (null)?

No matter what we do here, we shouldn't distinguish null from something that evaluates to null.

@bakkot
Copy link
Contributor

bakkot commented Oct 23, 2021

How is having extends void but not extends (null) any different than having extends null and not extends (null)?

void is not an expression and so doesn't evaluate to anything.

@mhofman
Copy link
Member

mhofman commented Oct 23, 2021

I think I misunderstood @bathos's example.

Regardless I am still uncomfortable changing the behavior of extends null to dynamically allow the construction of bare instances.

A class that doesn't want to inherit from Object.prototype should be considered a base class, and syntactically disallow any super usage.

@bathos
Copy link
Contributor

bathos commented Oct 23, 2021

Regardless I am still uncomfortable changing the behavior of extends null to dynamically allow the construction of bare instances.

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
Copy link
Member

mhofman commented Oct 23, 2021

How is having extends void but not extends (null) any different than having extends null and not extends (null)?

No matter what we do here, we shouldn't distinguish null from something that evaluates to null.

"having" seem a little unclear.

extends void is currently a syntax error. I propose we allow it. However extends undefined would stay a runtime class declaration error.

extends null and extends (null) both are currently construction-time errors (unless the prototypes are explicitly overridden later, or the constructor leverages the return override, i.e the few in the wild use cases mentioned above). I agree we should not treat them differently and argue that at this point we should keep the current default behavior of throwing at construction time.

@bathos
Copy link
Contributor

bathos commented Oct 23, 2021

extends null and extends (null) both are currently construction-time errors (unless the prototypes are explicitly overridden later in the few in the wild use cases mentioned above).

Just for the sake of making sure this is well-understood, what is a construction time error currently is super() where no super constructor exists. Either setting the super constructor after class declaration or return override are currently valid ways to employ extends null. The former would stop working entirely with this change. The latter would continue working but with subtly different behavior.

AFAICT, if super() were changed to behave as Object.create(new.target.prototype) when the prototype of the function is null, extends null would then work more broadly without breaking any existing code. That is not what's being proposed. I'm guessing it must have been discussed at some point but I couldn't find it so am not sure why it was not the path taken.

@ljharb
Copy link
Member

ljharb commented Oct 23, 2021

@bakkot yes but conceptually they’re the same thing.

@ljharb
Copy link
Member

ljharb commented Oct 23, 2021

@mhofman all base classes per the current spec inherit from Object.prototype, so im not sure why that would be a requirement.

@mhofman
Copy link
Member

mhofman commented Oct 23, 2021

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 Object.prototype. extends void does that in an explicit way. Only a base class can declare it should be bare, and I argue this should be expressed syntactically.

@ljharb
Copy link
Member

ljharb commented Oct 23, 2021

A class can extend any expression, and i think that should be preserved, even if the expression evaluates to null.

@devsnek
Copy link
Member Author

devsnek commented Oct 23, 2021

@mhofman

extends null and extends (null) both are currently construction-time errors (unless the prototypes are explicitly overridden later, or the constructor leverages the return override, i.e the few in the wild use cases mentioned above). I agree we should not treat them differently and argue that at this point we should keep the current default behavior of throwing at construction time.

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.

@mhofman
Copy link
Member

mhofman commented Oct 23, 2021

A class can extend any expression, and i think that should be preserved, even if the expression evaluates to null.

I think we agree here.

To be pedantic, a class can extend any expression that evaluates to an object or null. In particular it's a runtime error if the extend expression evaluates to undefined. That's why I believe class extends void wouldn't be a breaking change, as it's purely new syntax.

@ljharb
Copy link
Member

ljharb commented Oct 23, 2021

@mhofman sure, but that's a technical loophole. Providing a way to make a class with a null prototype that doesn't work with both extends, and an expression, wouldn't really be fixing anything - the problem to fix is that the intuitive thing doesn't work.

@moztcampbell
Copy link

Can someone clarify if the desired behaviour of class D extends null {} is for instances of D to have a null prototype or a prototype of Object.create(null)? For example, is (new D) instanceof D true or false?

@bakkot
Copy link
Contributor

bakkot commented Oct 24, 2021

Can someone clarify if the desired behaviour of class D extends null {} is for instances of D to have a null prototype or a prototype of Object.create(null)? For example, is (new D) instanceof D true or false?

The latter. Methods defined on the class should still be accessible from instances, it's just that you won't get methods from Object.prototype.

@devsnek
Copy link
Member Author

devsnek commented Oct 24, 2021

and this pr doesn't actually change anything about that, it just fixes the errors during construction.

@jridgewell
Copy link
Member

Is it possible to just special case Function.prototype when constructing? Eg, if the super class is Function.prototype (which extends null does), we return a normal object without trying to call [[Construct]].

@bathos
Copy link
Contributor

bathos commented Oct 28, 2021

@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 new function() {}, where SuperCall is syntactically forbidden and the function is always [[base]], right? Are there cases where the Function.prototype approach would have consequences apart from “something might have thrown before but now doesn’t” outside of the scenario of interest?

@jridgewell
Copy link
Member

Since that approach would seemingly be a modification to SuperCall steps, it doesn’t appear this would interfere with new function() {}, where SuperCall is syntactically forbidden and the function is always [[base]], right?

Correct.

Are there cases where the Function.prototype approach would have consequences apart from “something might have thrown before but now doesn’t” outside of the scenario of interest?

Not that I know of.


Essentially, this entire PR becomes:

SuperCall : super Arguments
1. …
5. If IsConstructor(_func_) is true, let result be ? Construct(_func_, _argList_, _newTarget_).
6. Else if func is %Function.Prototype%, let result be ? OrdinaryObjectCreate(? newTarget.[[GetPrototypeOf]]()).
7. Else, throw a *TypeError* exception
…

I think.

@bathos
Copy link
Contributor

bathos commented Oct 28, 2021

@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 the Object constructor is what prevents Object from doing its normal thing (which would amount to a return override) when it’s being called from super(). Although different in form, special casing for Function.prototype in SuperCall itself is similar in effect: both are about preventing “default” intrinsics from creating surprising behavior at super() calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text