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

What's the reason for the fallback in OrdinaryCreateFromConstructor? #1264

Open
domenic opened this Issue Jul 12, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@domenic
Member

domenic commented Jul 12, 2018

@devsnek brought up in IRC that the built-ins deviate from classes created with class syntax in having a fallback, if new.target.prototype does not exist. For example, this test shows how using Reflect.construct on the Promise constructor where new.target.prototype is null will cause it to use new.target's realm's %PromisePrototype%.

From the perspective of the built-ins being self-hostable, and aligned with normal class syntax, this seems weird. @devsnek had to essentially re-implement OrdinaryCreateFromConstructor and GetPrototypeFromConstructor, including the lookup table of intrinsics, to implement a spec-compliant Promise in JavaScript. https://github.com/devsnek/promise-polyfill/blob/1763f4596173af79ed3c1d2a12a119f89a9c20b8/Promise.js#L42

I imagine this decision and added complexity was done with good reason, though. I'm not necessarily asking to reevaluate or change it. But, we should at least add an explanatory note to OrdinaryCreateFromConstructor, explaining why this complexity was added, and how this means it's incorrect to think of their constructors as operating in the "normal" class C { constructor() { /* operate on this */ } } fashion.

Calling @allenwb as the font of knowledge on these sort of historical decisions.

@ljharb ljharb added the question label Jul 12, 2018

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Jul 12, 2018

Member

It's mostly about matching ES3 era web reality for Ordinary ECMAScript Function objects [[Construct]]. Especially the call in step 5.a. As explained in the note on GetPrototypeFromConstructor it is dealing with the case where the constructor function does not provide an object via its prototype property to be used as the [[Prototype]] of the new object. In particular, it makes sure that the default prototype provided comes from the realm of the constructor rather than the realm of the code doing the construction.

The most common place where this would come up (in the ES1-5 era) is by somebody defining a constructor function and deleting it's prototype property or setting it to any non-object type. Since the prototype of built-in constructors are read-only non-configurable it wouldn't be a problem when directly invoking new on them. But in the ES6 era somebody could "subclass" a built-in constructor using such a subclass constructor that has such a broken prototype (or as you point out using Reflect.construct). For that case, it seemed to make more sense to specify that the fall-back prototype would be the standard prototype of the built-in rather than its realm's %ObjectPrototype%.

Because of the immutability of the built-in prototype properties it would have been safe for the spec. to use that value as the fallback. But that would mean that there would have to be a separate OrdinaryCreateFromConstructor spec. code path for built-ins that is different from non-built-in. It seems like cleaner spec. code to have a single common OrdinaryCreateFromConstructor/GetPrototypeFromConstructor with the fallback parameter then have two difference variations for the built-in/non-built-in cases.

Regarding self-hosting built-ins. If you are doing that and correctly following the spec. then you know that your constructor will have a valid immutable prototype property and you can safely bypass some of the OrdinaryCreateFromConstructor steps based upon that knowledge (although you still have to take into account the possibility of a bogus subclass constructor).

More generally about self-hosting. There are definitely places where the spec. needs to directly refer to some of the built-in (intrinsic) objects so if an implementation is going to support self-hosting of such objects it needs to provide an implementation-dependent mechanism for making such self-hosted known to the implementation code. That was the original reason for the existence of the well-known intrinsic objects table. I don't know whether or not that table has been carefully kept up to date. But I notice it has some new entries that aren't actually referenced meaningfully anywhere else in the spec (for example, %ArrayProto_entries%, etc; only %ArrayProto_values% seems to be needed).

Member

allenwb commented Jul 12, 2018

It's mostly about matching ES3 era web reality for Ordinary ECMAScript Function objects [[Construct]]. Especially the call in step 5.a. As explained in the note on GetPrototypeFromConstructor it is dealing with the case where the constructor function does not provide an object via its prototype property to be used as the [[Prototype]] of the new object. In particular, it makes sure that the default prototype provided comes from the realm of the constructor rather than the realm of the code doing the construction.

The most common place where this would come up (in the ES1-5 era) is by somebody defining a constructor function and deleting it's prototype property or setting it to any non-object type. Since the prototype of built-in constructors are read-only non-configurable it wouldn't be a problem when directly invoking new on them. But in the ES6 era somebody could "subclass" a built-in constructor using such a subclass constructor that has such a broken prototype (or as you point out using Reflect.construct). For that case, it seemed to make more sense to specify that the fall-back prototype would be the standard prototype of the built-in rather than its realm's %ObjectPrototype%.

Because of the immutability of the built-in prototype properties it would have been safe for the spec. to use that value as the fallback. But that would mean that there would have to be a separate OrdinaryCreateFromConstructor spec. code path for built-ins that is different from non-built-in. It seems like cleaner spec. code to have a single common OrdinaryCreateFromConstructor/GetPrototypeFromConstructor with the fallback parameter then have two difference variations for the built-in/non-built-in cases.

Regarding self-hosting built-ins. If you are doing that and correctly following the spec. then you know that your constructor will have a valid immutable prototype property and you can safely bypass some of the OrdinaryCreateFromConstructor steps based upon that knowledge (although you still have to take into account the possibility of a bogus subclass constructor).

More generally about self-hosting. There are definitely places where the spec. needs to directly refer to some of the built-in (intrinsic) objects so if an implementation is going to support self-hosting of such objects it needs to provide an implementation-dependent mechanism for making such self-hosted known to the implementation code. That was the original reason for the existence of the well-known intrinsic objects table. I don't know whether or not that table has been carefully kept up to date. But I notice it has some new entries that aren't actually referenced meaningfully anywhere else in the spec (for example, %ArrayProto_entries%, etc; only %ArrayProto_values% seems to be needed).

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 12, 2018

Member

Thanks very much for the explanation, Allen! Fascinating stuff, as I expected.

Because of the immutability of the built-in prototype properties it would have been safe for the spec. to use that value as the fallback. But that would mean that there would have to be a separate OrdinaryCreateFromConstructor spec. code path for built-ins that is different from non-built-in.

I was following up until this point. If we just defined OrdinaryCreateFromConstructor to always use %ObjectPrototype% as the fallback, i.e. Return ObjectCreate(? GetPrototypeFromConstructor(constructor, "%ObjectPrototype%"), internalSlotsList), then this seems like it would be a single code path for both built-ins and non-built-ins. It'd have the same behavior as today for non-built-ins (per ordinary [[Construct]] 5a) and would be safe for built-ins, as you mention. What am I missing?

Regarding self-hosting built-ins. If you are doing that and correctly following the spec. then you know that your constructor will have a valid immutable prototype property and you can safely bypass some of the OrdinaryCreateFromConstructor steps based upon that knowledge (although you still have to take into account the possibility of a bogus subclass constructor).

I'm not sure you could bypass any steps if you wanted to behave the same as built-ins for Reflect.construct/bogus subclasses. That is, there's no way to get the per-spec behavior while using class Promise { constructor(func) { /* use this, instead of creating a new object */ } }. Do you agree? Just checking my understanding here.

Member

domenic commented Jul 12, 2018

Thanks very much for the explanation, Allen! Fascinating stuff, as I expected.

Because of the immutability of the built-in prototype properties it would have been safe for the spec. to use that value as the fallback. But that would mean that there would have to be a separate OrdinaryCreateFromConstructor spec. code path for built-ins that is different from non-built-in.

I was following up until this point. If we just defined OrdinaryCreateFromConstructor to always use %ObjectPrototype% as the fallback, i.e. Return ObjectCreate(? GetPrototypeFromConstructor(constructor, "%ObjectPrototype%"), internalSlotsList), then this seems like it would be a single code path for both built-ins and non-built-ins. It'd have the same behavior as today for non-built-ins (per ordinary [[Construct]] 5a) and would be safe for built-ins, as you mention. What am I missing?

Regarding self-hosting built-ins. If you are doing that and correctly following the spec. then you know that your constructor will have a valid immutable prototype property and you can safely bypass some of the OrdinaryCreateFromConstructor steps based upon that knowledge (although you still have to take into account the possibility of a bogus subclass constructor).

I'm not sure you could bypass any steps if you wanted to behave the same as built-ins for Reflect.construct/bogus subclasses. That is, there's no way to get the per-spec behavior while using class Promise { constructor(func) { /* use this, instead of creating a new object */ } }. Do you agree? Just checking my understanding here.

@devsnek

This comment has been minimized.

Show comment
Hide comment
@devsnek

devsnek Jul 12, 2018

Contributor

from my understanding, you can get really close to spec behaviour except for the part where the intrinsic lookup happens in the environment for constructor, not the current execution environment. if you happen to own the runtime you can polyfill that too but i must admit it was quite confusing at first.

Contributor

devsnek commented Jul 12, 2018

from my understanding, you can get really close to spec behaviour except for the part where the intrinsic lookup happens in the environment for constructor, not the current execution environment. if you happen to own the runtime you can polyfill that too but i must admit it was quite confusing at first.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Jul 12, 2018

Member

@domenic
I was think of cases like this step in the Map constructor:
2. Let map be ? OrdinaryCreateFromConstructor(NewTarget, "%MapPrototype%", « [[MapData]] »).

Assume this is invoked via super from a subclass constructor (or via Reflect.construct, it really doesn't make a difference) then the value of NewTarget is the subclass constructor. If it has a bogus prototype we still want it to be created with %MapPrototype% not %ObjectPrototype%.

I guess it could be refactored as:
2. Let map be ? OrdinaryCreate(? GetPrototypeFromConstructor(constructor, "%MapPrototype%"), « [[MapData]] »).

but, it isn't clear if that idiom clearer and it is probably harder to remember.

In reality I think what happened was that at first there was ObjectCreate which evolved into OrdinaryCreateFromConstructor to deal with thing like NewTarget and included the GetPrototypeFrom logic. Then later the GetPrototype... logic was needed somewhere else so it was refactored out into a separate abstract operation.

Regarding optimizing self hosted implementations. The only thing that has to be preserved in addition to the final result are the ordering of any observable intermediate results. Accessing the value of an immutable data property of an object that is known to not be a Proxy is not directly observable (unless there is something I'm overlooking). When you are writing the JS code to self host Promise you know that its 'prototype' property has that characteristic. And if you want, you can structure you code such that you have direct access to the object that is the value of that property (for example, by closure capturing a reference to the object before (or after)) you initialize the prototype property. So, you can completely skip the property lookup on Promise when the constructor actually runs.

Member

allenwb commented Jul 12, 2018

@domenic
I was think of cases like this step in the Map constructor:
2. Let map be ? OrdinaryCreateFromConstructor(NewTarget, "%MapPrototype%", « [[MapData]] »).

Assume this is invoked via super from a subclass constructor (or via Reflect.construct, it really doesn't make a difference) then the value of NewTarget is the subclass constructor. If it has a bogus prototype we still want it to be created with %MapPrototype% not %ObjectPrototype%.

I guess it could be refactored as:
2. Let map be ? OrdinaryCreate(? GetPrototypeFromConstructor(constructor, "%MapPrototype%"), « [[MapData]] »).

but, it isn't clear if that idiom clearer and it is probably harder to remember.

In reality I think what happened was that at first there was ObjectCreate which evolved into OrdinaryCreateFromConstructor to deal with thing like NewTarget and included the GetPrototypeFrom logic. Then later the GetPrototype... logic was needed somewhere else so it was refactored out into a separate abstract operation.

Regarding optimizing self hosted implementations. The only thing that has to be preserved in addition to the final result are the ordering of any observable intermediate results. Accessing the value of an immutable data property of an object that is known to not be a Proxy is not directly observable (unless there is something I'm overlooking). When you are writing the JS code to self host Promise you know that its 'prototype' property has that characteristic. And if you want, you can structure you code such that you have direct access to the object that is the value of that property (for example, by closure capturing a reference to the object before (or after)) you initialize the prototype property. So, you can completely skip the property lookup on Promise when the constructor actually runs.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jul 12, 2018

Member

If it has a bogus prototype we still want it to be created with %MapPrototype% not %ObjectPrototype%.

That's the part I'm not following. There's no pre-ES2015 behavior constraint here. And non-built-in-classes don't do this. So why do we want it?

So, you can completely skip the property lookup on Promise when the constructor actually runs.

But, you can't skip it on new.target, right? Since new.target is not necessarily Promise.

Member

domenic commented Jul 12, 2018

If it has a bogus prototype we still want it to be created with %MapPrototype% not %ObjectPrototype%.

That's the part I'm not following. There's no pre-ES2015 behavior constraint here. And non-built-in-classes don't do this. So why do we want it?

So, you can completely skip the property lookup on Promise when the constructor actually runs.

But, you can't skip it on new.target, right? Since new.target is not necessarily Promise.

@devsnek

This comment has been minimized.

Show comment
Hide comment
@devsnek

devsnek Jul 12, 2018

Contributor

i think the argument is when there's no prototype, why not use the prototype related to the thing you're constructing. since there was no per-existing behaviour to match they were able to implement that.

the thing about non-built-in-classes confuses me too

Contributor

devsnek commented Jul 12, 2018

i think the argument is when there's no prototype, why not use the prototype related to the thing you're constructing. since there was no per-existing behaviour to match they were able to implement that.

the thing about non-built-in-classes confuses me too

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jul 12, 2018

Member

But I notice it has some new entries that aren't actually referenced meaningfully anywhere else in the spec (for example, %ArrayProto_entries%, etc; only %ArrayProto_values% seems to be needed).

This is because they're referenced in the HTML spec, I believe.

Member

ljharb commented Jul 12, 2018

But I notice it has some new entries that aren't actually referenced meaningfully anywhere else in the spec (for example, %ArrayProto_entries%, etc; only %ArrayProto_values% seems to be needed).

This is because they're referenced in the HTML spec, I believe.

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