ecma262 icon indicating copy to clipboard operation
ecma262 copied to clipboard

Normative: Fix extending null

Open devsnek opened this issue 6 years ago • 60 comments

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

devsnek avatar Oct 09 '18 15:10 devsnek

See #781 and #543

ljharb avatar Oct 09 '18 16:10 ljharb

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

devsnek avatar Oct 09 '18 16:10 devsnek

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

littledan avatar Oct 10 '18 03:10 littledan

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 avatar Oct 10 '18 13:10 devsnek

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;

bakkot avatar Nov 15 '18 21:11 bakkot

@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');
  }
}

devsnek avatar Nov 15 '18 23:11 devsnek

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

bakkot avatar Nov 15 '18 23:11 bakkot

alright case 10 is passing 👌

devsnek avatar Nov 15 '18 23:11 devsnek

@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?

bakkot avatar Nov 16 '18 00:11 bakkot

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

devsnek avatar Nov 16 '18 00:11 devsnek

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?

bakkot avatar Nov 16 '18 01:11 bakkot

@bakkot looks good

devsnek avatar Nov 16 '18 01:11 devsnek

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().

bakkot avatar Nov 16 '18 04:11 bakkot

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

devsnek avatar Nov 16 '18 05:11 devsnek

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

bakkot avatar Nov 16 '18 05:11 bakkot

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.

devsnek avatar Nov 16 '18 17:11 devsnek

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

No, but it would make me sad.

bakkot avatar Nov 16 '18 17:11 bakkot

In addition to making @bakkot and me sad, this sort of change would also have an effect on some implementation strategies, which can compile base class and subclass constructors differently, rather than making the base-ness determination at runtime. That's not necessarily a dealbreaker, just something to consider.

littledan avatar Nov 21 '19 13:11 littledan

which can compile base class and subclass constructors differently

Are these spec compliant strategies? I'm honestly curious about how this is done, so I'd appreciate if you have any example to share.


For the current discussion: I find it counter intuitive disallowing (early error) super in classes without inheritance but now allowing them on class extending null. It supports a false idea that null is an actual object. The early error prevents me from guessing if super refers to Function or Object, and we would need to have this well defined first. I don't think we want to prioritize this part.

I'd prefer if usage of super remains an early error for classes extending null.

[Edit: typo/missing word]

leobalter avatar Nov 21 '19 16:11 leobalter

@littledan @leobalter are there other solutions to extending null that seem more palpable?

bmeck avatar Nov 25 '19 14:11 bmeck

are there other solutions to extending null that seem more palpable?

I'm not against "extending null", my points are related to usage of super. I don't know of any immediate alternative for this.

leobalter avatar Nov 25 '19 15:11 leobalter

@bmeck I'm sorry, I'm not aware of a solution which meets all of the constraints that have been raised. @ajklein had a proposal about this, which seemed like it might be somehow practical, but which violated some other commitee constraints as well (notes).

littledan avatar Nov 25 '19 15:11 littledan

@leobalter

Are these spec compliant strategies? I'm honestly about how this is done, so I'd appreciate if you have any example to share.

I believe they are spec-compliant; I won't be able to dig up details right now, unfortunately. Maybe @syg or @efaust knows more details offhand.

littledan avatar Nov 25 '19 15:11 littledan

super(...args) is equivalent‑ish to:

this = Reflect.construct(Reflect.getPrototypeOf(F), [...args], new.target);

And in the case of class F extends null, Reflect.getPrototypeOf(F) === Function.prototype, which isn’t a constructor, so super(…) throws at runtime.


Note that with https://github.com/tc39/ecma262/pull/2216, the default constructor will no longer observably call the @@iterator method of %Array.prototype%.


Also note that:

class extends Function.prototype {}

already throws a TypeError during ClassDefinitionEvaluation because %Function.prototype% doesn’t have a [[Construct]] internal method.

ExE-Boss avatar Feb 21 '21 17:02 ExE-Boss

I put this on the agenda for next meeting. I'd like to come to a conclusion about what we want to do with extends null. I'll try to put together a presentation with all the various options so we can either move forward with this in some way or call off the effort.

devsnek avatar Feb 21 '21 18:02 devsnek

An alternative solution for null prototype objects could be to allow static prototype = null. Currently trying to define a static property called prototype is an early error so it would not conflict with existing code.

This might be kinda confusing though as this would be more like the prototype's prototype. Alternatively we could allow an explicit object that methods are attached to i.e.:

class Foo {
    static prototype = { __proto__: null };
}

Jamesernator avatar Oct 13 '21 07:10 Jamesernator

@devsnek is there an existing summary anywhere of the solutions which are considered viable? As someone who has used extends null before (with return override) I'd like to get a sense of whether changes made here would potentially affect that existing code. In particular, I'm hoping that such constructors will continue to behave as derived so that there is no observable Get introduced for new.target.prototype unless/until an explicit super(). I didn't dig in deep but at a glance it appears as though the specific changes in this PR do not preserve that behavior and thus would alter the semantics of our existing code.

AFAICT, only solutions which extend the behavior of a super call when the heritage is null - not construction proper, nor the derived/base status of the constructor - would be backwards compatible in this regard. The solution here would appear to only be backwards compatible if it had actually been impossible to extend null before, but it was not - it just only worked for return override cases. While I doubt many people took advantage of this, I can say at least one person did (and that the no-observable-get was actually significant to that usage*).

I'm able change the code that relies on this, but it seems somewhat unlikely that I was the only person during the last six years to make use of it.

NT = Object.defineProperty(class{}.bind(), "prototype", { get() { console.log("observed"); } });

Reflect.construct(class extends null {
  constructor() {
    console.log("derived");
    return {};
  }
}, [], NT);

// -> derived

Reflect.construct(class {
  constructor() {
    console.log("base");
    return {};
  }
}, [], NT);

// -> observed
// -> base
* Why

For high fidelity Web IDL implementations in JS, you need to assemble interface objects using return override for several reasons, some very subtle, and using null heritage was a natural choice for the inner stub being generated prior to determining the specific [[Prototype]]s of the constructor and constructor.prototype. A premature Get for new.target.prototype can change observable behavior; it has to be done sort of manually even for interfaces that don't inherit in Web IDL. I can alter the code in question to achieve this by other means but, maybe kinda ironically, literally the essential thing / the entire purpose of the code in question was that the constructor is derived, not base, while it appears this proposed solution is centered on changing such constructors to be base, not derived. If this change were made we'd literally have tests that would begin failing :)

bathos avatar Oct 13 '21 07:10 bathos

@Jamesernator aside from it being the wrong prototype (which you noted), it also forgets about the second prototype (Foo.__proto__)

@bathos I would be surprised if more than like 3 people on the planet use extends null, and they're all probably in this thread too. Also a search of sourcegraph reveals 0 hits that are not tests for js engines or js linters.

devsnek avatar Oct 13 '21 14:10 devsnek

Also a search of sourcegraph reveals 0 hits that are not tests for js engines or js linters.

A search of sourcegraph would also not reveal my usages. I know the "no observable get" is very, very specific and agree it's unlikely anyone else happened to need it to be derived for that reason. But do you really think no one else used it as a fill-in for cases where the real heritage isn't immediately known? Solutions that don't break that are possible, so I'm surprised a breaking change would be preferred.

class X extends null {
  constructor() {
    super("okay");
  }
}

class Y {
  constructor(arg) {
    console.log(arg);
  }
}

Object.setPrototypeOf(X, Y);
Object.setPrototypeOf(X.prototype, Y.prototype);

new X;

// -> "okay"

bathos avatar Oct 13 '21 16:10 bathos

@bathos "breaking" is always kind of a subtle question. What we generally mean by "breaking" is that websites or applications no longer serve their purpose for users, not that a test fails. It's not that we are generally opposed to changes to the language which can in principle cause existing code to have different behavior - after all, adding any new syntax can be "breaking" in that strong sense, since code can rely on its absence through eval.

I strongly suspect that there is little-to-no code on the web which relies on the behavior you highlight.

bakkot avatar Oct 13 '21 16:10 bakkot