- Notifications
You must be signed in to change notification settings - Fork13.2k
Add useDefineForClassFields flag for Set -> Define property declaration#33509
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Unless the base property or accessor is abstract
This causes quite a few test breaks. We'll probably want to revert manyof them by switching to the upcoming `declare x: number` syntax.
1. Don't error when overriding properties from interfaces.2. Fix error when overriding methods with other things. This had notests so I assume that the code was always dead and never worked.
Will update after checking out other branch for a minute
Need to test properties initialised in constructor
And simplify redundant parts of check.
…/add-property-define-flag
…property-define-flag
sandersn commentedSep 19, 2019
@typescript-bot pack this |
vkrol commentedOct 24, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There is nothing about it in the RC release notes. Will it be mentioned in the final release notes or it will be postponed to the next release? |
agrozyme commentedDec 6, 2019
when useDefineForClassFields = false when useDefineForClassFields = true |
sandersn commentedDec 6, 2019
agrozyme commentedDec 7, 2019
OK |
…nce r=robwuThese fall under 5 main categories: 1) Declare and/or initialize all class fiels in the constructor. (general good practise) 2) Use real getters and redefineGetter instead of defineLazyGetter. (also keeps related code closer together) 3) When subclassing, don't override class fields with getters (or vice versa).microsoft/TypeScript#33509 4) Declare and assign object literals at the same time, not separatelly. (don't use `let foo;` at the top of the file, use `var foo = {`) 5) Don't re-use local variables unnecesarily with different types. (general good practise, local variables are "free")Differential Revision:https://phabricator.services.mozilla.com/D196386
…nce r=robwuThese fall under 5 main categories: 1) Declare and/or initialize all class fiels in the constructor. (general good practise) 2) Use real getters and redefineGetter instead of defineLazyGetter. (also keeps related code closer together) 3) When subclassing, don't override class fields with getters (or vice versa).microsoft/TypeScript#33509 4) Declare and assign object literals at the same time, not separatelly. (don't use `let foo;` at the top of the file, use `var foo = {`) 5) Don't re-use local variables unnecesarily with different types. (general good practise, local variables are "free")Differential Revision:https://phabricator.services.mozilla.com/D196386UltraBlame original commit: 8e768446e17cc306729e3b0f705b0285c69321cf
…nce r=robwuThese fall under 5 main categories: 1) Declare and/or initialize all class fiels in the constructor. (general good practise) 2) Use real getters and redefineGetter instead of defineLazyGetter. (also keeps related code closer together) 3) When subclassing, don't override class fields with getters (or vice versa).microsoft/TypeScript#33509 4) Declare and assign object literals at the same time, not separatelly. (don't use `let foo;` at the top of the file, use `var foo = {`) 5) Don't re-use local variables unnecesarily with different types. (general good practise, local variables are "free")Differential Revision:https://phabricator.services.mozilla.com/D196386UltraBlame original commit: 8e768446e17cc306729e3b0f705b0285c69321cf
…nce r=robwuThese fall under 5 main categories: 1) Declare and/or initialize all class fiels in the constructor. (general good practise) 2) Use real getters and redefineGetter instead of defineLazyGetter. (also keeps related code closer together) 3) When subclassing, don't override class fields with getters (or vice versa).microsoft/TypeScript#33509 4) Declare and assign object literals at the same time, not separatelly. (don't use `let foo;` at the top of the file, use `var foo = {`) 5) Don't re-use local variables unnecesarily with different types. (general good practise, local variables are "free")Differential Revision:https://phabricator.services.mozilla.com/D196386UltraBlame original commit: 8e768446e17cc306729e3b0f705b0285c69321cf
…nce r=robwuThese fall under 5 main categories: 1) Declare and/or initialize all class fiels in the constructor. (general good practise) 2) Use real getters and redefineGetter instead of defineLazyGetter. (also keeps related code closer together) 3) When subclassing, don't override class fields with getters (or vice versa).microsoft/TypeScript#33509 4) Declare and assign object literals at the same time, not separatelly. (don't use `let foo;` at the top of the file, use `var foo = {`) 5) Don't re-use local variables unnecesarily with different types. (general good practise, local variables are "free")Differential Revision:https://phabricator.services.mozilla.com/D196386
Uh oh!
There was an error while loading.Please reload this page.
This PR is part of the migration from [[Set]] semantics to [[Define]] semantics.#27644 tracks the complete migration. This PR includes steps 1-4 for Typescript 3.7:
class C extends B { declare x: number }declarewhere needed.useDefineForClassFields."useDefineForClassFields": false:"useDefineForClassFields": true:"useDefineForClassFields"defaults tofalseTasks from the design meeting:
Accessors may not override properties and vice versa
Briefly, properties can now only override properties, and accessors can only override accessors. The tests come from the user-submitted horrorshow parade in#27644. Thanks to all who contributed there.
Exceptions
Motivation
Accessors that override properties have always been error-prone with [[Set]] semantics, so these new errors are useful with [[Set]]or [[Define]] semantics. For example, base-class properties call derived accessors, which may be unexpected. The code below prints
2, the derived value, but italso printsset 1. That's because the basep = 1calls derived accessorset p.In fact, if the derived class attempts to make
preadonly by leaving offset p, the code crashes with"Cannot set property p of #<D> which has only a getter."This should always have been an error. However, because we haven’t given errors before, and because fixing the errors is likely to be difficult, we probably would not have added them otherwise, or added them in so many cases. Here are some examples of code that will be tricky to change:CleverBaseis a framework class that intends to cache or transform the value provided bySimple.Simpleis written by a framework user who just knows that they need toextend CleverBaseand provide some configuration properties. This pattern no longer works with [[Define]] semantics. I believe the Angular 2 failure I found below is similar to this case.This code is the same as the first example — accessors override a property — but the intent is different: to ignore the base's property. With [[Set]] semantics, it's possible to work around the initial set from the base, but with [[Define]] semantics, the base property will override the derived accessors. Sometimes a derived accessor can be replaced with a property, but often the accessor needs to run additional code to work around base class limitations. In this case, the fix is not simple. I saw this in a couple of Microsoft apps, and in one place it had a comment "has to be a getter so overriding the base class works correctly".
How to fix the errors
Property overrides accessor
SimpleUser will have an error on the property declaration
p. The fix is to move it into the constructor as an assignment:Since
CleverBasedeclaresp, there's no need forSimpleUserto do so.Accessor overrides property
SmartDerived will have an error on the get/set declarations for
p. The fix is to move them into the constructor as anObject.defineProperty:This doesn't have exactly the same semantics; the base never calls the derived setter for
p. If the original setter does have skip-initial-set code to work around the current weird Typescript semantics, that code will need to be removed. However, all the real examples I saw were just using accessors to make the property readonly, so they could instead use a readonly property.Uninitialised property declarations may not override properties
Briefly, uninitialised property declarations that must now use new syntax when the property overrides a property in a base class. That's because when Typescript supports [[Define]] semantics, previously uninitialised property declarations will instead have an initial value of
undefined. This will be a major breaking change:Previously this emitted
When Typescript supports [[Define]] semantics, it will instead emit
which will give both
BandCand propertypwith the valueundefined. (This is an error today withstrictNullChecks: true.)The new syntax will cause Typescript to emit the original JS output:
This PR adds an error prompting the author to add the new syntax in order to avoid the breaking change. As you can see from the baselines, this error is pretty common. From my first run of our extended test suites, it looks pretty common in real code as well.
Note that the compiler can only check constructors for initialisation when
strictNullChecks: true. I chose to be conservative and always issue the error forstrictNullChecks: false.Other ways to fix the error
"strictNullChecks": true.Notes
!is always erased, so, in the last example,p!: 256 | 1000would still result inp === undefinedfor [[Define]] semantics.declareto any uninitialised property, even if it's not an override. I previously had a check that prevented this, but I think an easy upgrade is valuable (you canalmost write a regex for it).