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

Add useDefineForClassFields flag for Set -> Define property declaration #33509

Merged
merged 57 commits into from Sep 26, 2019

Conversation

@sandersn
Copy link
Member

sandersn commented Sep 19, 2019

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:

  • #33470: emit get/set accessors in declaration files. This means that 3.7-emitted d.ts files will be incompatible with 3.5 and below.
  • #33401: accessors may not override properties and vice versa.
  • #33423: uninitialised properties may not override properties.
    • Introduce new syntax, class C extends B { declare x: number }
    • Introduce a codefix that inserts declare where needed.
    • This declaration is erased, like the original declaration in old versions of TS
  • #33509 Introduce a new flag, useDefineForClassFields.
    • When "useDefineForClassFields": false:
      • Initialized fields in class bodies are emitted as constructor assignments (even when targeting ESNext).
      • Uninitialized fields in class bodies are erased.
      • The preceding errors are silenced.
    • When "useDefineForClassFields": true:
      • Fields in class bodies are emitted as-is for ESNext.
      • Fields in class bodies are emitted as Object.defineProperty assignments in the constructor otherwise.
    • In 3.7, "useDefineForClassFields" defaults to false
    • Declaration emit is the same regardless of the flag's value.
  • Bonus: The error "properties/accessors aren't allowed to override methods" was not reported from 3.0 onward. This PR also fixes that error.

Tasks from the design meeting:

  • Abstract properties with initialisers generate code, so should generate the error after all.
  • ES3 should set+require useDefineForClassFields=false
  • useDefineForClassFields=true should also use defineProperty for method emit so we can set enumerability correctly.
  • change flag name to useDefineForClassFields and flip polarity from legacyClassFields

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).
sandersn added 28 commits Sep 12, 2019
Unless the base property or accessor is abstract
This causes quite a few test breaks. We'll probably want to revert many
of 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 no
tests 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.
…property-define-flag
@sandersn

This comment has been minimized.

Copy link
Member Author

sandersn commented Sep 19, 2019

@typescript-bot pack this

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Sep 23, 2019

@sandersn
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..33509

Metric master 33509 Delta Best Worst
Angular - node (v12.1.0, x64)
Memory used 329,713k (± 0.08%) 331,620k (± 0.02%) +1,907k (+ 0.58%) 331,515k 331,783k
Parse Time 1.56s (± 0.43%) 1.56s (± 0.51%) +0.00s (+ 0.13%) 1.54s 1.58s
Bind Time 0.79s (± 1.06%) 0.79s (± 0.92%) +0.00s (+ 0.38%) 0.77s 0.81s
Check Time 4.23s (± 0.74%) 4.26s (± 0.39%) +0.03s (+ 0.73%) 4.21s 4.28s
Emit Time 5.25s (± 0.92%) 5.26s (± 0.68%) +0.01s (+ 0.21%) 5.20s 5.33s
Total Time 11.82s (± 0.55%) 11.87s (± 0.29%) +0.05s (+ 0.41%) 11.82s 11.98s
Monaco - node (v12.1.0, x64)
Memory used 344,573k (± 0.02%) 345,988k (± 0.01%) +1,415k (+ 0.41%) 345,880k 346,067k
Parse Time 1.22s (± 0.54%) 1.23s (± 0.38%) +0.01s (+ 0.82%) 1.22s 1.24s
Bind Time 0.67s (± 1.30%) 0.68s (± 1.53%) +0.01s (+ 1.50%) 0.66s 0.71s
Check Time 4.19s (± 0.41%) 4.26s (± 0.66%) +0.07s (+ 1.72%) 4.20s 4.33s
Emit Time 2.82s (± 0.59%) 2.86s (± 0.57%) +0.03s (+ 1.17%) 2.82s 2.89s
Total Time 8.90s (± 0.29%) 9.02s (± 0.50%) +0.12s (+ 1.37%) 8.94s 9.16s
TFS - node (v12.1.0, x64)
Memory used 300,014k (± 0.02%) 301,478k (± 0.01%) +1,464k (+ 0.49%) 301,383k 301,579k
Parse Time 0.94s (± 0.74%) 0.95s (± 0.55%) +0.01s (+ 0.85%) 0.94s 0.96s
Bind Time 0.62s (± 1.07%) 0.62s (± 0.84%) -0.00s (- 0.32%) 0.61s 0.63s
Check Time 3.77s (± 0.51%) 3.87s (± 0.48%) +0.10s (+ 2.71%) 3.84s 3.92s
Emit Time 2.94s (± 0.54%) 2.96s (± 0.78%) +0.02s (+ 0.75%) 2.88s 2.99s
Total Time 8.27s (± 0.35%) 8.40s (± 0.41%) +0.13s (+ 1.56%) 8.31s 8.47s
Angular - node (v8.9.0, x64)
Memory used 348,440k (± 0.02%) 350,548k (± 0.02%) +2,108k (+ 0.61%) 350,430k 350,677k
Parse Time 2.09s (± 0.35%) 2.09s (± 0.45%) +0.00s (+ 0.05%) 2.08s 2.12s
Bind Time 0.83s (± 0.57%) 0.83s (± 0.54%) -0.00s (- 0.12%) 0.82s 0.84s
Check Time 5.08s (± 0.64%) 5.09s (± 0.36%) +0.02s (+ 0.33%) 5.05s 5.12s
Emit Time 6.00s (± 0.62%) 6.02s (± 1.03%) +0.02s (+ 0.35%) 5.88s 6.15s
Total Time 14.00s (± 0.43%) 14.04s (± 0.54%) +0.04s (+ 0.27%) 13.92s 14.21s
Monaco - node (v8.9.0, x64)
Memory used 362,200k (± 0.01%) 363,752k (± 0.01%) +1,552k (+ 0.43%) 363,670k 363,822k
Parse Time 1.56s (± 0.36%) 1.56s (± 0.41%) 0.00s ( 0.00%) 1.55s 1.58s
Bind Time 0.89s (± 0.75%) 0.89s (± 0.77%) -0.00s (- 0.11%) 0.87s 0.90s
Check Time 5.03s (± 1.53%) 5.12s (± 1.74%) +0.09s (+ 1.83%) 4.96s 5.30s
Emit Time 3.20s (± 4.31%) 3.14s (± 4.58%) -0.07s (- 2.12%) 2.93s 3.35s
Total Time 10.68s (± 0.66%) 10.70s (± 0.75%) +0.02s (+ 0.18%) 10.52s 10.87s
TFS - node (v8.9.0, x64)
Memory used 316,089k (± 0.01%) 317,701k (± 0.01%) +1,611k (+ 0.51%) 317,642k 317,779k
Parse Time 1.25s (± 0.29%) 1.26s (± 0.60%) +0.01s (+ 0.88%) 1.25s 1.29s
Bind Time 0.67s (± 3.24%) 0.67s (± 4.44%) -0.00s (- 0.30%) 0.65s 0.79s
Check Time 4.39s (± 0.99%) 4.48s (± 0.88%) +0.09s (+ 2.03%) 4.33s 4.53s
Emit Time 3.07s (± 0.36%) 3.07s (± 0.74%) +0.00s (+ 0.16%) 3.02s 3.12s
Total Time 9.38s (± 0.40%) 9.48s (± 0.30%) +0.10s (+ 1.11%) 9.43s 9.57s
Angular - node (v8.9.0, x86)
Memory used 197,309k (± 0.02%) 198,411k (± 0.03%) +1,101k (+ 0.56%) 198,310k 198,547k
Parse Time 2.04s (± 0.55%) 2.03s (± 0.87%) -0.00s (- 0.15%) 2.01s 2.09s
Bind Time 0.95s (± 0.70%) 0.95s (± 0.65%) +0.00s (+ 0.11%) 0.94s 0.96s
Check Time 4.60s (± 0.51%) 4.66s (± 0.74%) +0.06s (+ 1.31%) 4.59s 4.76s
Emit Time 5.72s (± 1.14%) 5.75s (± 1.08%) +0.04s (+ 0.63%) 5.64s 5.89s
Total Time 13.30s (± 0.55%) 13.40s (± 0.63%) +0.09s (+ 0.71%) 13.29s 13.64s
Monaco - node (v8.9.0, x86)
Memory used 202,412k (± 0.02%) 203,269k (± 0.02%) +857k (+ 0.42%) 203,172k 203,344k
Parse Time 1.62s (± 0.83%) 1.61s (± 0.65%) -0.00s (- 0.19%) 1.60s 1.65s
Bind Time 0.72s (± 0.81%) 0.71s (± 0.69%) -0.00s (- 0.42%) 0.71s 0.73s
Check Time 4.81s (± 0.57%) 4.87s (± 0.41%) +0.05s (+ 1.12%) 4.83s 4.93s
Emit Time 3.16s (± 1.07%) 3.17s (± 0.75%) +0.02s (+ 0.57%) 3.13s 3.24s
Total Time 10.30s (± 0.53%) 10.37s (± 0.39%) +0.07s (+ 0.67%) 10.28s 10.45s
TFS - node (v8.9.0, x86)
Memory used 177,708k (± 0.01%) 178,583k (± 0.01%) +875k (+ 0.49%) 178,521k 178,643k
Parse Time 1.32s (± 0.81%) 1.31s (± 0.89%) -0.00s (- 0.38%) 1.29s 1.34s
Bind Time 0.64s (± 0.77%) 0.64s (± 0.73%) +0.00s (+ 0.63%) 0.63s 0.65s
Check Time 4.22s (± 0.86%) 4.29s (± 0.44%) +0.07s (+ 1.66%) 4.25s 4.32s
Emit Time 2.86s (± 0.73%) 2.89s (± 0.86%) +0.02s (+ 0.73%) 2.84s 2.94s
Total Time 9.03s (± 0.69%) 9.12s (± 0.44%) +0.09s (+ 0.97%) 9.05s 9.22s
Angular - node (v9.0.0, x64)
Memory used 348,096k (± 0.02%) 350,196k (± 0.02%) +2,100k (+ 0.60%) 350,036k 350,319k
Parse Time 1.82s (± 0.61%) 1.82s (± 0.68%) +0.00s (+ 0.00%) 1.79s 1.85s
Bind Time 0.78s (± 0.57%) 0.78s (± 0.98%) -0.00s (- 0.39%) 0.76s 0.79s
Check Time 4.80s (± 0.50%) 4.88s (± 0.24%) +0.08s (+ 1.63%) 4.85s 4.90s
Emit Time 5.79s (± 1.35%) 5.77s (± 1.36%) -0.03s (- 0.45%) 5.55s 5.87s
Total Time 13.19s (± 0.48%) 13.24s (± 0.68%) +0.05s (+ 0.36%) 13.00s 13.37s
Monaco - node (v9.0.0, x64)
Memory used 361,867k (± 0.01%) 363,471k (± 0.01%) +1,604k (+ 0.44%) 363,376k 363,596k
Parse Time 1.32s (± 0.51%) 1.32s (± 0.52%) -0.00s (- 0.15%) 1.30s 1.33s
Bind Time 0.83s (± 1.41%) 0.83s (± 1.18%) +0.00s (+ 0.12%) 0.81s 0.85s
Check Time 4.96s (± 1.54%) 4.99s (± 1.75%) +0.03s (+ 0.61%) 4.85s 5.16s
Emit Time 3.10s (± 5.84%) 3.16s (± 5.25%) +0.06s (+ 2.07%) 2.87s 3.37s
Total Time 10.21s (± 1.24%) 10.30s (± 0.94%) +0.09s (+ 0.87%) 10.11s 10.51s
TFS - node (v9.0.0, x64)
Memory used 315,870k (± 0.01%) 317,492k (± 0.01%) +1,622k (+ 0.51%) 317,428k 317,580k
Parse Time 1.04s (± 0.45%) 1.04s (± 0.55%) -0.00s (- 0.00%) 1.03s 1.06s
Bind Time 0.62s (± 0.65%) 0.62s (± 1.07%) +0.00s (+ 0.16%) 0.61s 0.64s
Check Time 4.31s (± 0.52%) 4.39s (± 0.68%) +0.08s (+ 1.74%) 4.33s 4.46s
Emit Time 3.16s (± 0.62%) 3.20s (± 0.91%) +0.04s (+ 1.33%) 3.12s 3.26s
Total Time 9.13s (± 0.34%) 9.25s (± 0.54%) +0.12s (+ 1.34%) 9.15s 9.38s
Angular - node (v9.0.0, x86)
Memory used 197,450k (± 0.03%) 198,547k (± 0.03%) +1,098k (+ 0.56%) 198,463k 198,693k
Parse Time 1.72s (± 0.80%) 1.73s (± 0.54%) +0.01s (+ 0.41%) 1.71s 1.75s
Bind Time 0.89s (± 0.55%) 0.90s (± 1.07%) +0.01s (+ 0.78%) 0.88s 0.92s
Check Time 4.29s (± 0.55%) 4.36s (± 0.43%) +0.06s (+ 1.44%) 4.31s 4.39s
Emit Time 5.51s (± 0.43%) 5.51s (± 0.85%) -0.00s (- 0.04%) 5.41s 5.61s
Total Time 12.42s (± 0.32%) 12.49s (± 0.51%) +0.07s (+ 0.59%) 12.38s 12.66s
Monaco - node (v9.0.0, x86)
Memory used 202,486k (± 0.02%) 203,314k (± 0.03%) +828k (+ 0.41%) 203,141k 203,424k
Parse Time 1.34s (± 0.92%) 1.35s (± 1.07%) +0.01s (+ 0.89%) 1.33s 1.39s
Bind Time 0.64s (± 0.97%) 0.64s (± 1.01%) +0.00s (+ 0.47%) 0.63s 0.66s
Check Time 4.64s (± 0.54%) 4.70s (± 0.40%) +0.06s (+ 1.25%) 4.67s 4.75s
Emit Time 3.10s (± 0.71%) 3.10s (± 0.42%) +0.01s (+ 0.26%) 3.07s 3.12s
Total Time 9.72s (± 0.38%) 9.80s (± 0.35%) +0.08s (+ 0.82%) 9.73s 9.87s
TFS - node (v9.0.0, x86)
Memory used 177,748k (± 0.02%) 178,651k (± 0.02%) +903k (+ 0.51%) 178,546k 178,711k
Parse Time 1.06s (± 1.12%) 1.06s (± 0.31%) -0.00s (- 0.19%) 1.05s 1.07s
Bind Time 0.58s (± 1.63%) 0.58s (± 0.59%) +0.00s (+ 0.00%) 0.57s 0.58s
Check Time 4.09s (± 0.56%) 4.15s (± 0.51%) +0.05s (+ 1.25%) 4.10s 4.20s
Emit Time 2.79s (± 0.52%) 2.79s (± 0.81%) +0.00s (+ 0.04%) 2.75s 2.86s
Total Time 8.52s (± 0.37%) 8.57s (± 0.35%) +0.05s (+ 0.58%) 8.51s 8.64s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-161-generic
Architecturex64
Available Memory16 GB
Available Memory9 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
  • node (v9.0.0, x64)
  • node (v9.0.0, x86)
Scenarios
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Angular - node (v9.0.0, x64)
  • Angular - node (v9.0.0, x86)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • Monaco - node (v9.0.0, x64)
  • Monaco - node (v9.0.0, x86)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • TFS - node (v9.0.0, x64)
  • TFS - node (v9.0.0, x86)
Benchmark Name Iterations
Current 33509 10
Baseline master 10

@sandersn

This comment has been minimized.

Copy link
Member Author

sandersn commented Sep 23, 2019

Perf doesn't look significantly different to me; most diffs in emit time are less than 1%.

@sandersn sandersn merged commit 500a0df into master Sep 26, 2019
5 checks passed
5 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
node10 Build #45940 succeeded
Details
node12 Build #45938 succeeded
Details
node8 Build #45939 succeeded
Details
@sandersn

This comment has been minimized.

Copy link
Member Author

sandersn commented Sep 26, 2019

I merged this so it gets an overnight run before the beta snap. @rbuckton if you have more comments about emit, please let me know so I can address them in the beta.

@AlCalzone

This comment has been minimized.

Copy link

AlCalzone commented Oct 2, 2019

I haven't seen any mention of this in the beta release notes. Is that intentional?

@sandersn

This comment has been minimized.

Copy link
Member Author

sandersn commented Oct 3, 2019

Nope! Leading up to the beta release I was too busy to proof the notes and it looks like @DanielRosenwasser just missed it. I'll make sure it gets into the notes for the full release.

@DanielRosenwasser

This comment has been minimized.

Copy link
Member

DanielRosenwasser commented Oct 4, 2019

Apologies! It will be noted in the RC/Final Release.

sandersn added a commit that referenced this pull request Oct 15, 2019
Originally removed incorrectly along with method-override-property error
in #24343, then both were restored in #33509. Only
method-override-property should be an error, since it doesn't actually
work at runtime.
sandersn added a commit that referenced this pull request Oct 15, 2019
Originally removed incorrectly along with method-override-property error
in #24343, then both were restored in #33509. Only
method-override-property should be an error, since it doesn't actually
work at runtime.
@vkrol

This comment has been minimized.

Copy link

vkrol commented Oct 24, 2019

@DanielRosenwasser

It will be noted in the RC/Final Release.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.