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

Should we make subclassing easy via this.constructor (or similar)? #30

Closed
domenic opened this issue Sep 17, 2013 · 10 comments
Closed

Should we make subclassing easy via this.constructor (or similar)? #30

domenic opened this issue Sep 17, 2013 · 10 comments

Comments

@domenic
Copy link
Owner

domenic commented Sep 17, 2013

Disclaimer: this should not affect ability to achieve consensus, as it does not have observable effects until @@create is implemented in browsers.

@annevk pointed out that, unlike ES6 arrays, promises as specced are not very subclassable. If you do class MyPromise extends Promise {}, then every static method and instance method will by default return instances of Promise, not MyPromise.

This should probably be fixed to work in the same way as arrays, which create their new instances by saying "if this is an exotic array, use new this.constructor(0) to construct the returned thing."

Similarly, we can use "if this.[[IsPromise]] is true, use new this.constructor(() => {}) to construct the returned thing."


Relatedly, @allenwb noted in today's TC39 meeting that the pattern of new this.constructor(0) is a bit weird because of the implicit knowledge of the single-parameter form being special; @BrendanEich also was perturbed by the this.constructor lookup, as it is forgeable and (for arrays) a potential source of backward-incompatibility.

So, perhaps both promises and arrays could benefit from a hook for "get me a newly-created instance of this object's true, unforgable constructor."

@domenic
Copy link
Owner Author

domenic commented Sep 18, 2013

After a brief talk with Allen over dinner, I think I am going to make this change tonight. There is really no downside.

@erights
Copy link
Collaborator

erights commented Sep 19, 2013

Why is it that this has no observable effect prior to @@create? @@create is not mentioned by the proposed text.

Currently, (s = Promise.cast(p).then(q,r)) protects the caller from any plan interference from code contributed by p,q,or r from running during the execution of this expression. Under this proposal, couldn't p be an instance of a subclass of Promise that violates this guarantee?

@erights
Copy link
Collaborator

erights commented Sep 19, 2013

What does an "object's true, unforgable constructor." mean?

@allenwb
Copy link
Collaborator

allenwb commented Sep 19, 2013

On Sep 17, 2013, at 6:39 PM, Domenic Denicola wrote:

Disclaimer: this should not affect ability to achieve consensus, as it does not have observable effects until @@create is implemented in browsers.

@annevk pointed out that, unlike ES6 arrays, promises as specced are not very subclassable. If you do class MyPromise extends Promise {}, then every static method and instance method will by default return instances of Promise, not MyPromise.

This should probably be fixed to work in the same way as arrays, which create their new instances by saying "if this is an exotic array, use new this.constructor(0) to construct the returned thing."

Similarly, we can use "if this.[[IsPromise]] is true, use new this.constructor(() => {}) to construct the returned thing."

Relatedly, @allenwb noted in today's TC39 meeting that the pattern of new this.constructor(0) is a bit weird because of the implicit knowledge of the single-parameter form being special; @BrendanEich also was perturbed by the this.constructor lookup, as it is forgeable and (for arrays) a potential source of backward-incompatibility.

So, perhaps both promises and arrays could benefit from a hook for "get me a newly-created instance of this object's true, unforgable constructor."

A couple things...

Im not up to speed on the actual interface to promises but here are some patterns you should consider:

class Promise {
static factoryMethod(arg) {
let newPromise = new this(arg); //pass are if it is appropriate to do so for the constructor
//alternatively do something with newPromise and arg
return newPromise;
}
clone(arg) { //hypothetical instance method that creates a new instance of the same
if (!brandTest(this)) throw TypeException("not a promose");
new this.constructor(getCloneData(this));
//do any post cloning initialization
}
}

class PromiseSubclass extends Promise {
}

let newSubP = new PromiseSubclass(stuff); // creates an instance of PromiseSubclass but using the @@create inherited from Promise
console.log(newSubP instanceof PromiseSubclass); //true
console.log(newSubP instanceof Promise) ; //true

let clonedSubP = newSubP.clone(otherStuff);
console.log(clonedSubP instanceof PromiseSubclass); //true

You might want to over-ride either the PromiseSubclass construct or its @@CreateMethod depending upon your requirements, but in many cases this should not be necessary.

Allen

@allenwb
Copy link
Collaborator

allenwb commented Sep 19, 2013

On Sep 18, 2013, at 8:34 PM, Mark S. Miller wrote:

Why is it that this has no observable effect prior to @@create? @@create is not mentioned by the proposed text.

Currently, (s = Promise.cast(p).then(q,r)) protects the caller from any plan interference from code contributed by p,q,or r from running during the execution of this expression. Under this proposal, couldn't p be an instance of a subclass of Promise that violates this guarantee?

If the subclass doesn't override static 'cast' or 'then' (or possibly its constructor) then that invariant would hold. @@create is called without arguments.

Allen


Reply to this email directly or view it on GitHub.

@erights
Copy link
Collaborator

erights commented Sep 19, 2013

@allenwb In responding, I had an aha. If p is an instance of FooPromise, a subclass of Promise, what does Promise.cast(p) actually return? If it returns p itself, then I think we're hosed, both from some attacks I was worried about and from the one you mention: FooPromise could simply override .then (which I'm embarrassed to not have thought of!). However, if Promise.cast(p) returns p coerced to a direct instance of Promise, then we don't lose any safety. FooPromise.cast(p) would be expected to return p itself, but that is of course up to it.

@allenwb
Copy link
Collaborator

allenwb commented Sep 19, 2013

@erights Yes, that sounds right. Promise can certainly decide to refuse to accept subclass instances as being interchangeable with its instance. In that case, a a sublcass that wants to locally circumvent such guards needs to over-ride the methods that use them. Whether that design pattern makes sense depends upon the requirements of the abstraction you are implementing.

This is all pretty much part of the corpus of good OO design patterns that came out of Smalltalk. I'm not sure whether Ruby programmers found or rediscovered them but as we introduce formalized class-based abstractions into JS we should propagate this knowledge.

@domenic
Copy link
Owner Author

domenic commented Sep 19, 2013

Agree, we must make XPromise.cast(p) always return an XPromise (including for the case of XPromise being plain Promise). This must be a very strong guarantee, tamper-proof against e.g. messing with .constructor.

But generally, it would be nice to make the default behavior for subclassing Promise make all its static methods and all its instance methods return instances of the same class as this. That is what this bug is about. (But I agree we cannot do so in a way that sacrifices Promise.cast guarantees.)

As for why this is not observable before @@create is introduced, that is because nothing will pass the "is true promise" check until that works, except for Promise itself. @@create is necessary for setting up inheritance of the type that allows you to pass the "is a true promise" test. [[IsPromise]] is the mechanism for this.

Will type up more complete thing, maybe a pull request, after I get back to my computer.

@erights
Copy link
Collaborator

erights commented Sep 19, 2013

This looks good to me.

@domenic
Copy link
Owner Author

domenic commented Sep 23, 2013

Implemented in #31.

@domenic domenic closed this as completed Sep 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants