tc39 / proposal-rm-builtin-subclassing

Remove ES6 built-in subclassing
31 stars 5 forks source link

Semantics suggestion: Use SpeciesConstructor just for new.target #1

Open littledan opened 4 years ago

littledan commented 4 years ago

Great writeup! I'm really excited about reconsidering this stuff. Two things occur to me:

These might both be solved with a simple tweak: Rather than totally abandoning the this.constructor[@@species] pattern, what if we continue to use it, but only to determine the new.target, and then we still go through the built-in constructor codepath? Similarly, for static methods, you can use this as the new.target but still go through the original constructor path. Some implications:

littledan commented 4 years ago

Note that, a downside of this proposal which @jandem mentioned is that, you still have two Get's, for this.constructor and then [Symbol.species], which have arbitrary side effects. Inserting these arbitrary side effects in the middle of previously simpler code caused security issues in the past in some engines. So, from an implementation perspective, this only solves some of the problems, having to do with processing after instantiation.

syg commented 4 years ago

Interesting semantics.

The current proposal is overly ambitious in "all subclassing machinery delenda est". I agree with you that subclass creation in itself is a nice intuition, and that without continued support for it, given libraries like Buffer, the compat risk is an order of magnitude more.

Fortunately the test for any of these ideas is to go ahead and try to clean up a single built-in method and see how much complexity in existing engines it actually gets rid of. Could @jandem give some data for SpiderMonkey? I'll do a similar investigation in V8.

syg commented 4 years ago

An alternative to this proposal is to still drop @@species and only use this.constructor as new.target in instance methods. That's only one Get, but unfortunately still requires a protector in V8 for modifying .constructor on instances of built-ins.

On the compat side, I've been told Microsoft tried shipping subclassing by delegating only to .constructor in ES6 days and found that it was not compatible, and that's how we got @@species. Can anyone from MS dig anything up? @bterlson?

All in all I'd still prefer to get rid of support for Type II subclassing entirely.

syg commented 4 years ago

I looked a bit in V8 for the code to remove for both @littledan's original proposal of using constructor[@@species] as new.target and my alternative suggestion of only using constructor.

Both don't really completely eliminate code complexity and slow paths, as you might expect, but do seem on paper to increase security guarantees by not having arbitrary subclass constructor run, as Dan said. They do removal some logic, but it is unclear to me at this point if that limited removal is worth the trouble to change from the current behavior.

littledan commented 4 years ago

The main thing is, if you call Array.prototype.map.call(not an array subclass instance), it should instantiate an Array, not this.constructor (think a NodeList, or Array with null prototype, or an object literal with indexed properties). This is fixable with this.constructor in other ways.

littledan commented 4 years ago

So, I guess my suggestion here is in between Type I and Type II. Would it be worth giving it some treatment in the README?

jandem commented 4 years ago

Fortunately the test for any of these ideas is to go ahead and try to clean up a single built-in method and see how much complexity in existing engines it actually gets rid of. Could @jandem give some data for SpiderMonkey?

The big pieces in SM are, as far as I can tell:

  1. A cache to make @@species lookups fast in C++ builtins (called ArraySpeciesLookup).
  2. The self-hosted ArraySpeciesCreate implementation.
  3. Various C++ intrinsics called by the self-hosted code and potentially JIT-optimized.

Type I would let us remove each of these. The other options would let us simplify code, but we'd still need the basic machinery in some form. It also depends quite a lot on what ArraySpeciesCreate steps 6-10 would look like, for example the Realm stuff in step 6 is pretty unfortunate as well.

@anba, do you have any thoughts on this? (He added the @@species lookup cache to SpiderMonkey.)

anba commented 4 years ago

The savings will probably depend on the built-in:

littledan commented 4 years ago

OK, sounds like this wouldn't be a big benefit; no need to focus on this idea.