Add useDefineForClassFields flag for Set -> Define property declaration by sandersn · Pull Request #33509 · microsoft/TypeScript (original) (raw)

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:

Tasks 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

  1. If the base property is abstract or in an interface, there is no error unless the abstract base property has an initialiser.
  2. If the symbol has both Property and Accessor set, there is no error. (I'm not sure when this can happen, although @weswigham says he can produce a Javascript example pretty easily.)

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 it also prints set 1. That's because the base p = 1 calls derived accessor set p.

class B { p = 1 } class D extends B { _p = 2 get p() { return this._p } set p(value) { console.log('set', value); this._p = value } } var d = new D() console.log(d.p)

In fact, if the derived class attempts to make p readonly by leaving off set 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:

class CleverBase { get p() { // caching, etc. } set p() { } } class Simple extends CleverBase { p = 'just a value' }

CleverBase is a framework class that intends to cache or transform the value provided by Simple. Simple is written by a framework user who just knows that they need to extend CleverBase and 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.

class LegacyBase { p = 1 } class SmartDerived extends LegacyBase { get() { // clever work on get } set(value) { // additional work to skip initial set from the base // clever work on set } }

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

class CleverBase { _p: unknown get p() { return _p } set p(value) { // cache or transform or register or ... _p = value } } class SimpleUser extends CleverBase { // base setter runs, caching happens p = "just fill in some property values" }

SimpleUser will have an error on the property declaration p. The fix is to move it into the constructor as an assignment:

class SimpleUser extends CleverBase { constructor() { // base setter runs, caching happens this.p = "just fill in some property values" } }

Since CleverBase declares p, there's no need for SimpleUser to do so.

Accessor overrides property

class LegacyBase { p = 1 } class SmartDerived extends LegacyBase { get() p { // clever work on get } set(value) p { // additional work to skip initial set from the base // clever work on set } }

SmartDerived will have an error on the get/set declarations for p. The fix is to move them into the constructor as an Object.defineProperty:

class SmarterDerived extends LegacyBase { constructor() { Object.defineProperty(this, "p", { get() { // clever work on get }, set(value) { // clever work on set } }) } }

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:

class B { p: number } class C extends B { p: 256 | 1000 // use whatever value you need here; it }

Previously this emitted

class B {} class C extends B {}

When Typescript supports [[Define]] semantics, it will instead emit

class B { p } class C extends B { p }

which will give both B and C and property p with the value undefined. (This is an error today with strictNullChecks: true.)

The new syntax will cause Typescript to emit the original JS output:

class B { p: number } class C extends B { declare p: 256 | 1000 }

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 for strictNullChecks: false.

Other ways to fix the error

  1. Make the overriding property abstract.
  2. Make the base property abstract.
  3. Give the overriding property an initialiser.
  4. Initialise the overriding property in the constructor -- but this only works with "strictNullChecks": true.

Notes

  1. Adding a postfix-! will not fix the error; ! is always erased, so, in the last example, p!: 256 | 1000 would still result in p === undefined for [[Define]] semantics.
  2. You can add declare to 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 can almost write a regex for it).