Added runtime TypeError for non-function, non-null __extends #37283
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As per the linked issue, although TypeScript already has checking in place that tries to prevent users from extending classes before their declaration, it's still possible to accidentally work around the checker. This adds a block to `__extends` that throws a `TypeError` if the base class `b` isn't a function.
My _hope_ is that this will not have a negative performance impact on community performance-critical applications, as they would likely already prefer newer browser/Node versions and output ES2015+ code. If there is something you can think of that I should do to verify that hope, I'd love to know!
For reference, runtime errors in Node 12.0.0 (Chrome exhibits the same messages):
```js
class X extends null { }
// undefined
class Y extends undefined { }
// TypeError: Class extends value undefined is not a constructor or null
class Z extends 0 { }
// TypeError: Class extends value 0 is not a constructor or null
```
`Class extends value {0} is not a constructor or null` matches the Node.js behavior:
* [Message template](https://github.com/nodejs/node/blob/2bdeb88c27b4d8de3a8f6b7a438cf0bcb88fa927/deps/v8/src/common/message-template.h) for `ExtendsValueNotConstructor`
* [Error thrown with that message](https://github.com/nodejs/node/blob/6ca81ad72a3c6fdf16c683335be748f22aaa9a0d/deps/v8/src/runtime/runtime-classes.cc#L617) when `!super_class->IsConstructor()`
Runtime errors in Firefox 72.0.1:
```js
class X extends null { }
// undefined
class Y extends undefined { }
// TypeError: class heritage undefined is not an object or null
class Z extends 0 { }
// TypeError: class heritage 0 is not an object or null
```
sandersn
reviewed
Mar 17, 2020
There was a problem hiding this comment.
Couple of comments, since I don't feel qualified to sign off on an emit change, especially for __extends.
- Whitespace changed a suspicious amount.
- You probably need to merge from master and accept baselines again because CI failed last time with a missed baseline.
JoshuaKGoldberg
commented
Mar 17, 2020
|
@typescript-bot perf test |
|
Well, I guess @typescript-bot only listens to Microsoft employees 😄 |
|
Yep, sorry! Typescript-bot is (1) powerful (2) ruthless, so we don't trust anybody outside the team to command it. @typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 9adf2f8. You can monitor the build here. Update: The results are in! |
|
Hey @sandersn, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
@sandersn Here they are:Comparison Report - master..37283
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
In general this looks fine, though we'd have to make a similar change to https://github.com/microsoft/tslib. One thing to note is that this does not behave the same as Chrome/v8 for this case: class C extends Symbol.iterator {}
// v8: TypeError: Class extends value Symbol(Symbol.iterator) is not a constructor or null
// this emit: TypeError: Cannot convert a Symbol value to a string.This could be addressed by changing the exception to this instead: throw new TypeError("Class extends value " + String(b) + " is not a constructor or null.");As |
|
@rbuckton should we merge this now or wait for the tslib change? |
|
I am reviewing now. While we should still have a PR for tslib, there are no changes here that would make them incompatible. |
rbuckton
approved these changes
Dec 2, 2020
|
Super, thanks for the reviews! I can send that PR over to tslib. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As per the linked issue, although TypeScript already has checking in place that tries to prevent users from extending classes before their declaration, it's still possible to accidentally work around the checker. This adds a block to
__extendsthat throws aTypeErrorif the base classbisn't a function.This'll be what the new error message is:
My hope is that this will not have a negative performance impact on community performance-critical applications, as they would likely already prefer newer browser/Node versions and output ES2015+ code. If there is something you can think of that I should do to verify that hope, I'd love to know!
These errors match the behavior in Chrome, Node, and Firefox
Runtime errors in Chrome 80 and Node 12.0.0:Class extends value {0} is not a constructor or nullmatches the Node.js behavior:ExtendsValueNotConstructor!super_class->IsConstructor()Runtime errors in Firefox 72.0.1:
Fixes #5794.