Removing observable lookup/call of %SetPrototype%.add in new Set

[bakkot]

The Set constructor when called with an iterable behaves as follows:

1. Create a new object with the appropriate prototypes derived from NewTarget
2. Look up add on that object
3. Repeatedly invoke add on the new object passing each member of the iterable

I believe the lookup for add is to support subclasses overriding add, such that

class A extends Set {
  add(x) {
    console.log(x);
  }
}
new A([0, 1]);

But this also has the consequence that changing Set.prototype.add changes the behavior of the Set constructor, which adds implementation overhead. I don't think it's intended or necessary that Set.prototype.add be monkey-patchable in this way. Perhaps we could say that if NewTarget is the Set constructor itself (edit: from the same realm), it can just use the original Set.prototype.add. This would preserve the subclassing behavior (i.e., my example would work the same) while reducing complexity in the common non-subclassed case.

This would debatably be an inconsistency, but it's only observable if you're patching intrinsics, which is a case where I don't think we should worry too much about trying to maintain consistency.

This all applies to WeakSet, Map, and WeakMap as well.

[bakkot]

Note that this would break anyone currently relying on the monkey-patching working. My hope is that no one is relying on that.

[allenwb]

I don't think it's intended or necessary that Set.prototype.add be monkey-patchable in this way.

This was absolutely intended and is necessary to allow allow a subclass to share the inherited constructor iteration behavior while changing the internal representation details. set is intended to be the sole method with the responsibility and knowledge of how to add an item to the internal representation of a set. Other methods, such as the constructor are intended to work through it.

Perhaps we could say that if NewTarget is the Set constructor itself (edit: from the same realm), it can just use the original Set.prototype.add.

The design already allows an implementation to optimize the constructor. Note that add is accessed exactly once, before iterating over the elements to be initially added to the set. This permits an implementation to verify that add is the Set.prototype.add intrinsic and to run a specialized version of the loop if it is.

Prior to the development of ES6 there was significant feedback from users who were unhappy because the ES3 era built-ins were not designed to be subclassed. Hence one of the goals of ES6 was to make the new and existing intrinsics work as subclassable base classes. To that end the ES6 methods were designed by somebody (me) with lots of experience with designing for subclass extensibility. It's all intentional.

Note that TC39 could have said, "no we don't want to make things subclassable because such extensibility often adds overhead that takes extra effort to optimize away" . But that wasn't the decision that TC39 made for ES6 and it s now water under the bridge.

Note that this would break anyone currently relying on the monkey-patching working. My hope is that no one is relying on that.

This is a breaking change and you have no way to determine how many hundreds, thousands, or millions of JavaScript programs depend upon the ES6 specified behavior. Why are you even considering this?

[bakkot]

This was absolutely intended and is necessary to allow allow a subclass to share the inherited constructor iteration behavior while changing the internal representation details.

A subclass would presumably shadow Set.prototype.add, not monkey-patch it, and I am not proposing any changes to the behavior when invoking the Set constructor via a subclass. The change I am proposing would only affect programs which actually overwrote Set.prototype.add, which a well-behaved subclass should not need to do.

I don't understand why this particular behavior is at all necessary for subclassing. Say more?

This is a breaking change and you have no way to determine how many hundreds, thousands, or millions of JavaScript programs depend upon the ES6 specified behavior.

I don't have a 100% reliable way, but if we check GitHub and npm and add a use-counter to browsers looking for places which are overriding Set.prototype.add, and find none, we can be reasonably confident that few if any do. I would give very good odds that no one is relying on this particular behavior at this time.

Why are you even considering this?

Mainly because I find it strange that built-ins are less defensible than user code, in a way which adds cost to engines for what seems to me to be no benefit. But perhaps I have not understood the benefit; can you say more about why this is desirable?

I agree that subclassing is an important design goal (though not everyone on the committee does). But I don't see how this particular behavior, which is only observable when overwriting intrinsics and invoking the Set constructor directly (in particular, not using super), helps that goal.

Edit: let me give a specific example of why this change would be helpful. The current behavior means that if I want to use the built-in Set constructor in a defensible way I cannot pass it an iterable. I have to manually invoke the original Set.prototype.add, just in case someone patches it. In my initial implementation of the linked code I missed this and was therefore not fully defensible. I don't think this sort of thing ought to be necessary to use built-ins in a predictable way.

[allenwb]

I don't understand why this particular behavior is at all necessary for subclassing. Say more?

The example given in you're original issue is a perfect example. Subclass A has a default constructor that implicitly super calls the Set constructor. %Set% creates an instance of A (using newTarget) and accesses the set property of the new instance. This retrieves A.prototype.set. %Set% then enumerates its argument calling A.prototype.set with each enumerated value. If %Set% did not behave in exactly this way then each subclass of Set would have to have an explicit constructor that reimplement all of the logic of %Set%.

I don't have a 100% reliable way, but if we check GitHub and npm and add a use-counter to browsers looking for places which are overriding Set.prototype.add, and find none, we can be reasonably confident that few if any do. I would give very good odds that no one is relying on this particular behavior at this time.

It is naive to think that everybody who matter makes their code publicly available on github, or npm, or runs in a browser. The TC39 perspective has always been to strive for absolute backwards compat and only make breaking changes in exceptional circumstances. If a feature is there (even a bug) we have to assume that somebody depends upon it. In this cases, you are talking about an intentional design decision with a real use case

If you disagree with the subclassing goal that ES6 was designed under then it would probably be better to try to convince T39 to totally reverse all decisions derived from that goal rather than chipping away at individual methods like this.

But I don't see how this particular behavior, which is only observable when overwriting intrinsics and invoking the Set constructor directly (in particular, not using super),

I don't understand this statement. You example A exploits this behavior because it implicitly makes a super constructor call to Set. It does not have an explicit invocation of Set.

Why isn't freeing the Set.prototype objet and making the Set.prototype property non-writable, non-configurable sufficient to make Set defensible? If you freeze it at startup, then any new Set(...) invocation will create an instance of the intrinsic Set using the intrinsic Set.prototype.add method. Subclasses can still do whatever they want, but by definition a subclass is not a defensible intrinsic.

Finally, if you want to make a frozen defensible intrinsics Set that isn't subclassable why don't you propose a built-in that export such a think rather than breaking stuff that is already there.

My sense is that your use case and the subclassability use case are ultimately incompatible with each other. Rather than compromising both, it would be better to have two separate intrinsics Set classes.

[bakkot]

If %Set% did not behave in exactly this way then each subclass of Set would have to have an explicit constructor that reimplement all of the logic of %Set%.

Ah, ok, I think there was a misunderstanding. To be clear, I am not proposing to change this behavior. I understand why it's important that the example I gave works as it does (which is why I gave it), and specifically meant to highlight that it should continue to work exactly the same under my proposed change. That's what I meant by "This would preserve the subclassing behavior".

I am only proposing to change what happens when calling Set in cases where NewTarget is the original Set - i.e., cases where the lookup can be guaranteed to result in the current value Set.prototype.add (assuming it still exists), not the add defined by some subclass.

The change would be to replace step 6 of the Set constructor algorithm, which currently reads Let _adder_ be ? Get(set, "add"), with If NewTarget is %Set%, let _adder_ be %SetProto_add%. Otherwise, let _adder_ be ? Get(set, "add").

You example A exploits this behavior because it implicitly makes a super constructor call to Set.

Hopefully this is clear from the above, but that's not what I meant by "this behavior". The behavior I am referring to is that

Set.prototype.add = foo;
new Set([0]);

calls foo. I am concerned only with the case where Set.prototype.add has been overwritten, not the case where it has been shadowed.

Why isn't freeing the Set.prototype objet and making the Set.prototype property non-writable, non-configurable sufficient to make Set defensible?

This is now somewhat off-topic, but generally speaking most libraries try not to have side effects, including affecting the configurability of builtins.

[littledan]

You could imagine doing something similar to what the original post proposes with, e.g., RegExp subclassing. Such a change could lead to a significantly simpler implementation.

[ljharb]

@bakkot is this something you want to make a PR for, and/or place on the June or July agendas?