Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
sandersn merged 57 commits intomasterfromsandersn/add-property-define-flag
Sep 26, 2019

Conversation

@sandersn
Copy link
Member

@sandersnsandersn commentedSep 19, 2019
edited
Loading

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:

  • Always emit accessors in .d.ts files #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.
  • Disallow property/accessor overrides #33401: accessors may not override properties and vice versa.
  • Disallow uninitialised property overrides #33423: uninitialised properties may not override properties.
    • Introduce new syntax,class C extends B { declare x: number }
    • Introduce a codefix that insertsdeclare where needed.
    • This declaration is erased, like the original declaration in old versions of TS
  • Add useDefineForClassFields flag for Set -> Define property declaration #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 tofalse
    • 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 prints2, the derived value, but italso printsset 1. That's because the basep = 1 calls derived accessorset p.

classB{p=1}classDextendsB{_p=2getp(){returnthis._p}setp(value){console.log('set',value);this._p=value}}vard=newD()console.log(d.p)

In fact, if the derived class attempts to makep readonly 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:

classCleverBase{getp(){// caching, etc.}setp(){}}classSimpleextendsCleverBase{p='just a value'}

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

classLegacyBase{p=1}classSmartDerivedextendsLegacyBase{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

classCleverBase{_p:unknowngetp(){return_p}setp(value){// cache or transform or register or ..._p=value}}classSimpleUserextendsCleverBase{// base setter runs, caching happensp="just fill in some property values"}

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

classSimpleUserextendsCleverBase{constructor(){// base setter runs, caching happensthis.p="just fill in some property values"}}

SinceCleverBase declaresp, there's no need forSimpleUser to do so.

Accessor overrides property

classLegacyBase{p=1}classSmartDerivedextendsLegacyBase{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 forp. The fix is to move them into the constructor as anObject.defineProperty:

classSmarterDerivedextendsLegacyBase{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 forp. 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 ofundefined. This will be a major breaking change:

classB{p:number}classCextendsB{p:256|1000// use whatever value you need here; it}

Previously this emitted

classB{}classCextendsB{}

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

classB{p}classCextendsB{p}

which will give bothB andC and propertyp with the valueundefined. (This is an error today withstrictNullChecks: true.)

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

classB{p:number}classCextendsB{declarep: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 whenstrictNullChecks: true. I chose to be conservative and always issue the error forstrictNullChecks: 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-! willnot fix the error;! is always erased, so, in the last example,p!: 256 | 1000 would still result inp === undefined for [[Define]] semantics.
  2. You can adddeclare 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 canalmost write a regex for it).

trotyl, yokots, chicoxyzzy, and jrumandal reacted with thumbs up emojidragomirtitian and trotyl reacted with hooray emojitrotyl reacted with heart emoji
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.
@sandersn
Copy link
MemberAuthor

@typescript-bot pack this

sandersn added a commit that referenced this pull requestOct 15, 2019
Originally removed incorrectly along with method-override-property errorin#24343, then both were restored in#33509. Onlymethod-override-property should be an error, since it doesn't actuallywork at runtime.
sandersn added a commit that referenced this pull requestOct 15, 2019
Originally removed incorrectly along with method-override-property errorin#24343, then both were restored in#33509. Onlymethod-override-property should be an error, since it doesn't actuallywork at runtime.
@vkrol
Copy link

vkrol commentedOct 24, 2019
edited
Loading

@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?

SlurpTheo reacted with thumbs up emoji

@agrozyme
Copy link

import assert = require('assert');describe('BaseClass', () => {  interface YI {}  abstract class X extends Base implements YI {    xa!: string;    xf!: string;    xfr!: string;    protected ef: string = 'ex';    protected readonly efr: string = 'ex';    constructor(...items: any[]) {      this.setup(...items);    }    setup(...items: any[]): void {}    protected get ea() {      return 'ex';    }    static z() {      return 'X';    }    type() {      return X;    }  }  class Y extends X {    protected ef: string = 'ey';    protected readonly efr: string = 'ey';    protected get ea() {      return 'ey';    }    static z() {      return 'Y';    }    static zz() {      return super.z();    }    setup(...items: any[]): void {      this.xf = 'ey' === this.ef ? 'TRUE' : 'FALSE';      this.xfr = 'ey' === this.efr ? 'TRUE' : 'FALSE';      this.xa = 'ey' === this.ea ? 'TRUE' : 'FALSE';    }    type() {      return Y;    }  }  const y = new Y();  it('Y', () => {    console.dir(y);    assert.strictEqual('FALSE', y.xf);    assert.strictEqual('FALSE', y.xfr);    assert.strictEqual('TRUE', y.xa);    assert.strictEqual('Y', y.type().z());    assert.strictEqual('X', y.type().zz());  });});

when useDefineForClassFields = false
result:

Y { xf: 'FALSE', xfr: 'FALSE', xa: 'TRUE', ef: 'ey', efr: 'ey' }

when useDefineForClassFields = true
result:

Y {  xf: undefined,  xfr: undefined,  xa: undefined,  ef: 'ey',  efr: 'ey' }

@sandersn
Copy link
MemberAuthor

@agrozyme

  1. Can you open a new issue for this?
  2. I can't repro your output in a version of the code in which I removed the unit testing:http://www.typescriptlang.org/play/#code/JYOwLgpgTgZghgYwgAgJoElkG8C+AoPOAIwGcwpExkEAbOEk5ADWWAFsAHGiNicRjNjwAPOAEIAXMjJRQAcwDcImJOnl5S4TCiqZGghygB7SAkgATZBBhS9IOcgC8yAOQRhLvIZMQzES1AQcOZGIDQAnlbatur2Tq7unngIoTIArmZGUAAUAHT5wJBsJFJwIOEA2gC6AJRCyA3IYAAWwCS5JBBgaRx5BUUkNUr4eJ3dvfm5hTwlyGWVtVIAbkbAlrgGxqYWyHJdVnDZdVh4jciB3VAgCR7DBGRwYMAIyABeR-WNF2lXrkwudzwYHCHAgHxOZ2+vyYdxGtHoAiswkgIHMjBYJ28238URisjizjc4QBXi2vh2gWCoQiUSgePk8SJJNJPj8lj2VCC4NOXy6P2uTMBDyeL3exx5DShAtQJJGwuebzFn0lfN+JB60FyYqFfIm-RmpXK1Rqy1W6wlTVa7S0jIgxKcjmcLTauWsyAA-K4ACoAJQAqgBRFzIKQuABiAEEADIAZSDSjOzut2lt9sdTqtrpTnpcvsDwdDkdj8YtSdyolTwfTlpdQQ93v9QZDriLcdlBGBoO5kNV11QsIIKRAZGQkWcIAgAHc0EclEOSEZuLlzMAcuEhhaAPSbhHQMAddRmAMARzScBo2XD0bbABpR+WYBuztvd1B93oj6fz5fW0G7+EHygJ9GhfBg9wPWRPzPC9c0bFx-3LOBgIaUDOjfCDnjAE9oMvGUEM7MEai1I5kOQVDwI-LCvxg-58JBQitTFDcgA

@agrozyme
Copy link

OK
I create a new issue:#35563
maybe it only repro in unit testing?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull requestDec 22, 2023
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull requestJan 1, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull requestJan 1, 2024
…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
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull requestJan 1, 2024
…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
jwidar pushed a commit to jwidar/LatencyZeroGithub that referenced this pull requestSep 16, 2025
…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
@microsoftmicrosoft locked asresolvedand limited conversation to collaboratorsOct 21, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@andrewbranchandrewbranchandrewbranch approved these changes

Assignees

@rbucktonrbuckton

Labels

Update Docs on Next ReleaseIndicates that this PR affects docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@sandersn@typescript-bot@AlCalzone@DanielRosenwasser@vkrol@agrozyme@andrewbranch@rbuckton

[8]ページ先頭

©2009-2025 Movatter.jp