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

Editorial: Remove 'designed to be subclassable' #2360

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

ptomato
Copy link
Contributor

@ptomato ptomato commented Mar 25, 2021

As discussed during the March 2021 plenary, this note on "is designed to
be subclassable" can be read as an encouragement or value judgement on
whether it is desirable to subclass built-in classes like Boolean. We
don't want to actively encourage this.

@ptomato
Copy link
Contributor Author

ptomato commented Mar 25, 2021

Hi, I thought it'd be good to start a discussion around the removal of this phrase (which was mentioned in plenary) in order to provide a basis for the language in Temporal to be consistent with. Please do suggest alternatives 😄

spec.html Outdated
@@ -24492,7 +24492,7 @@ <h1>The Object Constructor</h1>
<li>is the initial value of the *"Object"* property of the global object.</li>
<li>creates a new ordinary object when called as a constructor.</li>
<li>performs a type conversion when called as a function rather than as a constructor.</li>
<li>is designed to be subclassable. It may be used as the value of an `extends` clause of a class definition.</li>
<li>may be used as the value of an `extends` clause of a class definition.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is definitely better! but:

Suggested change
<li>may be used as the value of an `extends` clause of a class definition.</li>
<li>may be used as the value of an `extends` clause of a class definition, but is not intended to be subclassed.</li>

perhaps? (at least for all the non-nullish primitive constructors, where it just doesn't make sense to ever subclass them)

spec.html Outdated
@@ -27366,7 +27366,7 @@ <h1>The Date Constructor</h1>
<li>creates and initializes a new Date object when called as a constructor.</li>
<li>returns a String representing the current time (UTC) when called as a function rather than as a constructor.</li>
<li>is a function whose behaviour differs based upon the number and types of its arguments.</li>
<li>is designed to be subclassable. It may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Date behaviour must include a `super` call to the Date constructor to create and initialize the subclass instance with a [[DateValue]] internal slot.</li>
<li>may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Date behaviour must include a `super` call to the Date constructor to create and initialize the subclass instance with a [[DateValue]] internal slot.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date seems like it works fine as being subclassable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar response here as above for Error.

spec.html Outdated
@@ -30767,7 +30767,7 @@ <h1>The RegExp Constructor</h1>
<li>is <dfn>%RegExp%</dfn>.</li>
<li>is the initial value of the *"RegExp"* property of the global object.</li>
<li>creates and initializes a new RegExp object when called as a function rather than as a constructor. Thus the function call `RegExp(&hellip;)` is equivalent to the object creation expression `new RegExp(&hellip;)` with the same arguments.</li>
<li>is designed to be subclassable. It may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified RegExp behaviour must include a `super` call to the RegExp constructor to create and initialize subclass instances with the necessary internal slots.</li>
<li>may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified RegExp behaviour must include a `super` call to the RegExp constructor to create and initialize subclass instances with the necessary internal slots.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly, RegExp was explicitly designed to be subclassable

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still curious. Does anyone use subclassable RegExp in wild? I never seen that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither have i, but the committee decided not to remove that plumbing, and it was definitely designed for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sadly, RegExp was explicitly designed to be subclassable

Doesn't mean we need to mention that in the specification, though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s reasonable to denote the things that are built for it, to separate them from the ones that aren’t.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If “properly work” means “RegExp subclasses don’t work, which is fine because nobody actually uses them except for core-js feature detection”, then they’d just use those methods directly. If by “properly work” you mean that RegExp subclasses continue to work, then no, of course those are required.

Copy link

@zloirock zloirock Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb by "properly" work I mean how to make work extended transpiled functionality of RegExp - NCG in this case - work with String.prototype.match and String.prototype.replace. It's absolutely not related to polyfills - it's the implementation of ES2018 feature on ES2015. You say that "they could drastically simplify their output" - I have no ideas how it could be implemented without ES2015 subclassing with delegation to .exec and @@replace how it's implemented at this moment or without patching built-ins (unacceptable on the transpilers side).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that, you’re probably right - but “making transpilers possible” isn’t a primary concern when designing features.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, it's a significant plus.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When possible, yes, but it's not worth the complexity and performance hits that the RegExp system takes. Unfortunately, we can't remove it because there's no practical way to tease apart polyfill feature detections from real code when gathering usage data.

spec.html Outdated
@@ -25626,7 +25626,7 @@ <h1>The Error Constructor</h1>
<li>is <dfn>%Error%</dfn>.</li>
<li>is the initial value of the *"Error"* property of the global object.</li>
<li>creates and initializes a new Error object when called as a function rather than as a constructor. Thus the function call `Error(&hellip;)` is equivalent to the object creation expression `new Error(&hellip;)` with the same arguments.</li>
<li>is designed to be subclassable. It may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Error behaviour must include a `super` call to the Error constructor to create and initialize subclass instances with an [[ErrorData]] internal slot.</li>
<li>may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Error behaviour must include a `super` call to the Error constructor to create and initialize subclass instances with an [[ErrorData]] internal slot.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I do see and use subclassing on the Error constructor. Isn't it is designed subclassable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error doesn't have any methods outside of toString, which side steps the issues with subclassing. Still I don't see any reason to deviate from the proposed language here. I see this PR as removing encouragement, and IMO, the fact that Error can be successfully subclassed doesn't mean the spec needs to encourage it over other built-ins that can't be easily successfully subclassed.

Further, by removing the encouragement, we're also saying something about future proposals for extending Error: that they shouldn't accommodate subclassing beyond the constructor being usable in extends.

spec.html Outdated
@@ -31492,7 +31492,7 @@ <h1>The Array Constructor</h1>
<li>creates and initializes a new Array exotic object when called as a constructor.</li>
<li>also creates and initializes a new Array object when called as a function rather than as a constructor. Thus the function call `Array(&hellip;)` is equivalent to the object creation expression `new Array(&hellip;)` with the same arguments.</li>
<li>is a function whose behaviour differs based upon the number and types of its arguments.</li>
<li>is designed to be subclassable. It may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the exotic Array behaviour must include a `super` call to the Array constructor to initialize subclass instances that are Array exotic objects. However, most of the `Array.prototype` methods are generic methods that are not dependent upon their *this* value being an Array exotic object.</li>
<li>may be used as the value of an `extends` clause of a class definition. Subclass constructors that intend to inherit the exotic Array behaviour must include a `super` call to the Array constructor to initialize subclass instances that are Array exotic objects. However, most of the `Array.prototype` methods are generic methods that are not dependent upon their *this* value being an Array exotic object.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same, subclassing Array is really an important feature in ES6, why discourage that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree? I guess it depends on what you mean by subclassing, since in JS it encompasses stuff like species construction, and that methods can deal with non-Array objects that still have the Array exotic objects' slots and essential methods.

What about subclassing Arrays is important beyond being able to extend them?

spec.html Outdated
@@ -34249,7 +34249,7 @@ <h1>The Map Constructor</h1>
<li>is the initial value of the *"Map"* property of the global object.</li>
<li>creates and initializes a new Map object when called as a constructor.</li>
<li>is not intended to be called as a function and will throw an exception when called in that manner.</li>
<li>is designed to be subclassable. It may be used as the value in an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Map behaviour must include a `super` call to the Map constructor to create and initialize the subclass instance with the internal state necessary to support the `Map.prototype` built-in methods.</li>
<li>may be used as the value in an `extends` clause of a class definition. Subclass constructors that intend to inherit the specified Map behaviour must include a `super` call to the Map constructor to create and initialize the subclass instance with the internal state necessary to support the `Map.prototype` built-in methods.</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as Array/Set/Error case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above then: what about the subclassing facilities, aside from being able to extend Maps, is important?

@Jack-Works
Copy link
Member

In my personal programming use case, I have subclasses the following classes and found they're really useful. I don't think we should discourage those classes to be extended.

  • Array
  • Error
  • Map
  • Set
  • WeakMap
  • WeakSet

@bakkot
Copy link
Contributor

bakkot commented Mar 26, 2021

Many of these were originally designed to be subclassable. All of them work to at least some extent in an extends clause (though good luck extending Function if you might be running on webpages with CSP). Some of them it makes more sense to subclass, others less, though exactly which and to what extent will depend on your point of view.

However, we aren't obligated to mention everything design decision we've ever made. My preference is to just drop mention of subclassing from all built-in constructors. We don't need to take a position on whether it's a good idea to subclass these, whether in general or for some particular subset. We can simply not discuss it at all.

@ljharb
Copy link
Member

ljharb commented Mar 26, 2021

I think it’s very important that we actively discourage subclassing of the things that aren’t meant for it. I’m not sure how to do that without explicitly mentioning that some things are meant for it.

@ptomato
Copy link
Contributor Author

ptomato commented Mar 26, 2021

Personally I agree with @bakkot here: removing the phrase doesn't discourage subclassing. But I'm not an editor, so having given my opinion I'll happily change this to reflect whatever comes out of the discussion, and then change Temporal accordingly.

@bakkot bakkot added the editor call to be discussed in the next editor call label Mar 26, 2021
@ljharb
Copy link
Member

ljharb commented Mar 26, 2021

I think this goes beyond a purely editorial question, personally, since user expectations from prose in the spec do often impact normative behaviors - just like the same discussion around Temporal.

@syg
Copy link
Contributor

syg commented Mar 31, 2021

Strong agree with @bakkot. "Subclassing" as an umbrella term encompasses several mechanisms, some of which I actively despise, and some of which are fine. I like this PR in that it calls out a particular mechanism as being actively maintained for future evolution of the language, and removing encouragement for using the whole set of mechanisms.

@ljharb and @Jack-Works, if you feel some of those mechanisms should be encouraged, then let's discuss those. I'm otherwise willing to die on the hill of removing blanket encouragement of "subclassing".

@Jack-Works
Copy link
Member

"Subclassing" as an umbrella term encompasses several mechanisms, some of which I actively despise, and some of which are fine.

do you mean https://github.com/tc39/proposal-rm-builtin-subclassing#taxonomy-of-subclassing?

@ljharb
Copy link
Member

ljharb commented Mar 31, 2021

@syg I'm not attached to the term "subclassing" for the precise reason you mention - that it conflates multiple mechanisms. I think it is important to explicitly distinguish the builtins that can meaningfully be the RHS of an extends - either by calling them out, or by calling out the rest. Simply removing the terminology leaves us in the same unfortunate position we're in now: users receive no guidance as to which builtin subclasses make sense (even if only a little) and which make zero sense.

@bathos
Copy link
Contributor

bathos commented Mar 31, 2021

I agree with @bakkot that this is very subjective and it's apparent that people don't share the same definition of "subclass". Some of it is provably untrue ("must include a super call"). Why replace one iffy/fuzzy text with an iffier, fuzzier one?

@syg
Copy link
Contributor

syg commented Mar 31, 2021

@syg
Copy link
Contributor

syg commented Mar 31, 2021

I agree with @bakkot that this is very subjective and it's apparent that people don't share the same definition of "subclass". Some of it is provably untrue ("must include a super call"). Why replace one iffy/fuzzy text with an iffier, fuzzier one?

What's being replaced? This PR drops the "designed to be subclassable" language, the other language is existing.

@bathos
Copy link
Contributor

bathos commented Apr 1, 2021

@syg You’re right, sorry — that stemmed from misreading. The real changes here seem like improvements to me.

@bakkot bakkot removed the editor call to be discussed in the next editor call label Apr 14, 2021
@ptomato
Copy link
Contributor Author

ptomato commented May 14, 2021

Have the editors had a chance to discuss this? It seems like it is blocked on whether the removal of the phrase "is designed to be subclassable" is interpreted as a deprecation, or as a neutral removal of encouragement.

@bakkot
Copy link
Contributor

bakkot commented Jun 9, 2021

Editors discussed this and are happy about it as-is among ourselves. Since there seems to be disagreement about whether this is strictly an editorial decision, I've put together a very short presentation and will ask that we get consensus on this particular change at the next meeting, emphasizing that a.) this PR does not take a position against (or for) subclassing any particular constructors and b.) the current state of affairs, which says that Boolean is "designed to be subclassable", is silly. If people want to call out some particular subset as being "designed to be subclassable", and/or call out some other subset as explicitly not for subclassing, they are welcome to drive that through committee as a followup.

@allenwb
Copy link
Member

allenwb commented Jun 10, 2021

which says that Boolean is "designed to be subclassable", is silly.

"designed to be subclassable" was used in the ES6 specification with a very specific meaning:

  • if the constructor is correctly super invoked by a subclass constructor
    • subclass instances will be the same kind of ordinary/exotic object as those created by the "superclass" constructor
    • subclass instances will have the same internal slots as defined by the "superclass"
    • prototype methods defined by the "superclass" that have dependencies upon internal slots or exotic internal methods will function correctly when invoked upon subclass instances

ES6 Boolean was, in fact, "defined to be subclassable" exactly according to those criteria. Specifically, Boolean.prototype.toString and valueOf are specified such that they make use of [[BooleanData]] internal slot of a Boolean subclass.

There may be very little utility in subclassing Boolean, but if somebody does it will work. The incorrect operation of ad hoc subclasses of Array and other builtins was something that developers complained about in the ES3/ES5 era. Fixing that general problem was an ES6 objective. For both internal and external consistency "defined to be subclassable" was the default design principle for ES6 builtins, even with there was minimal utility in subclassing some builtins. Only in a few cases such as Symbol, were there strong reasons to violate the consistency and design principle and make a builtin non-"subclassable".

Because "designed to be subclassable" was the ES6 norm, that label arguably was not need on every builtin constructor. But as a change from prior editions of the specification the label served as an important notice to both ES implementors and developers. Perhaps it is no longer needed, but the fact that has been this ongoing discussion make me think it still has value.

@bakkot
Copy link
Contributor

bakkot commented Jun 10, 2021

@allenwb, thanks for giving the history. It's the opinion of the current editor group that the sense of "designed to be subclassable" used in your comment is adequately captured by the "may be used as the value of an extends clause of a class definition" part, which will remain. The advantage of removing specifically the "subclassable" part is that some people read it to imply more things than are present in the definition you intended (for example, people have read it as implicitly encouraging subclassing of built-ins, rather than merely noting that it would work).

@bakkot bakkot removed the editor call to be discussed in the next editor call label Jun 16, 2021
@ljharb ljharb force-pushed the master branch 2 times, most recently from 1320e04 to 3d0c24c Compare June 29, 2021 02:17
@ljharb ljharb added the has consensus This has committee consensus. label Jul 13, 2021
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 15, 2021
As discussed during the March 2021 plenary, this note on "is designed to
be subclassable" can be read as an encouragement or value judgement on
whether it is desirable to subclass built-in classes like Boolean. We
don't want to actively encourage this.
@ljharb ljharb force-pushed the designed-to-be-subclassable branch from 1c01ea5 to a44f010 Compare July 15, 2021 03:38
@ljharb ljharb merged commit a44f010 into tc39:master Jul 15, 2021
@ptomato ptomato deleted the designed-to-be-subclassable branch July 15, 2021 03:52
ptomato added a commit to tc39/proposal-temporal that referenced this pull request Jul 23, 2021
See tc39/ecma262#2360

Bring the language around this in line with the main ECMA-262 text, using
the same unordered list form and the same bullet points everywhere.
Ms2ger pushed a commit to tc39/proposal-temporal that referenced this pull request Jul 27, 2021
See tc39/ecma262#2360

Bring the language around this in line with the main ECMA-262 text, using
the same unordered list form and the same bullet points everywhere.
mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021
As discussed during the March 2021 plenary, this note on "is designed to
be subclassable" can be read as an encouragement or value judgement on
whether it is desirable to subclass built-in classes like Boolean. We
don't want to actively encourage this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial change has consensus This has committee consensus. ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants