Disallow uninitialised property overrides #33423

sandersn · 2019-09-13T21:45:39Z

This PR is part of the migration from [[Set]] semantics to [[Define]] semantics. #27644 tracks the complete migration. This is the third step.

Briefly, uninitialised property declarations that must now use new syntax when the property overrides a property in a base class. The new syntax is only allowed in this case and is an error elsewhere. That's because when Typescript supports [[Define]] semantics, it will start emitting uninitialised property declarations, where previously it did not. 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

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

Notes

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.
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).

To test this PR

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/44156/artifacts?artifactName=tgz&fileId=1E6A72F0C6E099FF6B046F9DBFDBA5D1CF45DD963781CEFBCFDBBDE15FD60F7C02&fileName=/typescript-3.7.0-insiders.20190916.tgz"
    }
}

Future work

The upcoming flag to change class field emit to [[Define]] semantics should disable this error.

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.

sandersn · 2019-09-13T21:46:11Z

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

typescript-bot · 2019-09-13T21:46:13Z

Heya @sandersn, I've started to run the extended test suite on this PR at ac96739. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-13T21:46:13Z

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at ac96739. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-13T21:46:13Z

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at ac96739. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-13T22:17:44Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

AlCalzone · 2019-09-14T10:23:25Z

Is there any way how I can test my code against this? From reading your recent PRs it seems that one of my libraries will be affected by the change.

Will update after checking out other branch for a minute

sandersn · 2019-09-16T17:59:50Z

@typescript-bot pack this
@AlCalzone the code isn't quite done yet, but typescript-bot will produce a tgz on Azure that you can point your package.json to.

The extended tests look pretty grim, but at least this error will be pretty easy to fix.

typescript-bot · 2019-09-16T18:00:51Z

Heya @sandersn, I've started to run the tarball bundle task on this PR at ac96739. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-16T18:07:05Z

Hey @sandersn, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/44156/artifacts?artifactName=tgz&fileId=1E6A72F0C6E099FF6B046F9DBFDBA5D1CF45DD963781CEFBCFDBBDE15FD60F7C02&fileName=/typescript-3.7.0-insiders.20190916.tgz"
    }
}

and then running npm install.

Need to test properties initialised in constructor

sandersn · 2019-09-16T22:18:57Z

@typescript-bot test this
@typescript-bot run dt
@typescript-bot user test this

typescript-bot · 2019-09-16T22:19:03Z

Heya @sandersn, I've started to run the parallelized community code test suite on this PR at 8686242. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-16T22:19:03Z

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 8686242. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-16T22:19:03Z

Heya @sandersn, I've started to run the extended test suite on this PR at 8686242. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-09-16T22:50:38Z

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

sandersn · 2019-09-16T23:22:25Z

@rkirov @mprobst @evmar You will probably want to take a look at this PR too. The errors are easier to fix compared to #33401.

sandersn · 2019-09-17T16:23:44Z

The codefix is in -- @rbuckton I think this is ready for review now.

And simplify redundant parts of check.

sandersn · 2019-09-17T18:31:07Z

src/compiler/checker.ts

@@ -29583,6 +29603,9 @@ namespace ts {
            }
            const constructor = findConstructorDeclaration(node);
            for (const member of node.members) {
+                if (getModifierFlags(member) & ModifierFlags.Ambient) {


do not require ! on declare x: number (since it is in fact not even allowed)

ljharb · 2019-09-17T23:38:26Z

It seems useful to ship declare syntax ASAP, and encourage people to migrate to it, before separately shipping the Set → Define change. Is that an option?

sandersn · 2019-09-18T15:40:08Z

@ljharb Our current plan is to ship the new syntax, the new errors, and the flag to switch to Define semantics at the same time. The flag will initially default to Set semantics, which also silences the errors. People can switch the flag on, fix errors, then switch it off again. Notably, the fixes for both errors are the same with both Set and Define semantics.

We'll discuss this at our design meeting on Friday so we should have a final decision then.

ljharb · 2019-09-18T15:47:44Z

Mainly thinking that the declare syntax is at least explicit, and that you may want to hold off merging the rest until after the next TC39 meeting, in case the topic comes up. (not trying to imply anything will change about the proposal)

sandersn · 2019-09-18T15:50:55Z

The errors in #33401 that disable accessor/property override mismatch are independently useful, although admittedly we haven't got many complaints from people confused by the current Set behaviour.

sandersn · 2019-09-18T23:24:26Z

Note: this-property assignments are still incorrectly erroring in JS.

littledan · 2019-09-21T23:01:29Z

@ljharb I don't see anything on the October 2019 TC39 agenda about this topic, and the deadline for adding agenda items has passed. Class fields have been stable with Define semantics for the whole time they have been at Stage 3, since 2017, and these semantics have shipped unflagged in Firefox, Chrome, and Node.js, as well as other environments.

ljharb · 2019-09-22T07:22:59Z

The deadline is for stage advancement; there’s no deadline for discussion topics, which is what i was alluding to.

AlCalzone · 2019-09-23T14:29:39Z

@sandersn Surprisingly little errors (only 3) in my codebase. I guess I squashed most of them when I switched testing to Babel+Jest. Back then I switched from redefining the class properties to interface merging, i.e.

class BaseClass {
  property: number
}

interface SubClass {
  property: 1 | 2 | 3
}

class SubClass extends BaseClass {
  // ...implementation
}

which is the ugly brother of the define syntax that has been proposed.

sandersn · 2019-09-23T20:16:50Z

This is now included in #33509. Thanks all for testing.

@AlCalzone we decided that interface merging is too esoteric a fix to recommend for the number of errors this causes. Also it's harder to write a codefix for.

@ljharb After our design meeting: #33509 has everything in one PR, and will keep the errors behind a flag while adding the new syntax for everyone to use.

AlCalzone · 2019-09-24T06:11:12Z

@sandersn is define inside classes going to be in 3.7?

sandersn · 2019-10-01T20:28:17Z

Yes. Well, declare is, I assume that's what you meant.

AlCalzone · 2019-10-02T05:24:43Z

👍🏻

Disallow uninitialised property overrides

ac96739

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.

typescript-bot mentioned this pull request Sep 13, 2019

🤖 User test baselines have changed for disallow-uninitialised-property-override #33424

Closed

sandersn mentioned this pull request Sep 16, 2019

Disallow property/accessor overrides #33401

Closed

Need to add a couple of errors and squash one

70a15bd

Will update after checking out other branch for a minute

sandersn added 2 commits September 16, 2019 13:36

Everything works so far

feb0fb6

Need to test properties initialised in constructor

Check for constructor initialisation

8686242

change error wording

2916c29

typescript-bot mentioned this pull request Sep 16, 2019

🤖 User test baselines have changed for disallow-uninitialised-property-override #33456

Closed

Improve error wording

2e5f57c

Add codefix to add missing 'declare'

f11f2be

sandersn added 4 commits September 17, 2019 09:47

Merge branch 'master' into disallow-uninitialised-property-override

a8f4610

Allow 'declare' on any uninitialised property decl

fcf0ff1

Undo code moves

810f923

Let sleeping dogs lie

6408d7a

sandersn added 2 commits September 17, 2019 13:45

Correctly set NodeFlags.Ambient

5ee3271

And simplify redundant parts of check.

Remove more unneeded code

e0ddced

sandersn commented Sep 17, 2019

View reviewed changes

Update baselines

6a066b9

RyanCavanaugh mentioned this pull request Sep 17, 2019

Handle breaking change in class property runtime behavior #27644

Closed

9 tasks

sandersn added 4 commits September 19, 2019 10:51

Do not error when base parent is interface

a645ca9

Fix base interface check

030c768

Add missed baselines

003d041

Fix new errors in services

1c5f699

sandersn mentioned this pull request Sep 23, 2019

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

Merged

9 tasks

sandersn closed this Sep 23, 2019

sandersn mentioned this pull request Jan 24, 2020

useDefineForClassFields and parameter properties: missing error or bad emit #36410

Closed

jakebailey deleted the disallow-uninitialised-property-override branch November 7, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow uninitialised property overrides #33423

Disallow uninitialised property overrides #33423

sandersn commented Sep 13, 2019 •

edited

Loading

sandersn commented Sep 13, 2019

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019

AlCalzone commented Sep 14, 2019

sandersn commented Sep 16, 2019

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019

sandersn commented Sep 16, 2019

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019

sandersn commented Sep 16, 2019

sandersn commented Sep 17, 2019

sandersn Sep 17, 2019

ljharb commented Sep 17, 2019

sandersn commented Sep 18, 2019

ljharb commented Sep 18, 2019

sandersn commented Sep 18, 2019

sandersn commented Sep 18, 2019

littledan commented Sep 21, 2019

ljharb commented Sep 22, 2019

AlCalzone commented Sep 23, 2019

sandersn commented Sep 23, 2019

AlCalzone commented Sep 24, 2019

sandersn commented Oct 1, 2019

AlCalzone commented Oct 2, 2019

Disallow uninitialised property overrides #33423

Disallow uninitialised property overrides #33423

Conversation

sandersn commented Sep 13, 2019 • edited Loading

Other ways to fix the error

Notes

To test this PR

Future work

sandersn commented Sep 13, 2019

typescript-bot commented Sep 13, 2019 • edited Loading

typescript-bot commented Sep 13, 2019 • edited Loading

typescript-bot commented Sep 13, 2019 • edited Loading

typescript-bot commented Sep 13, 2019

AlCalzone commented Sep 14, 2019

sandersn commented Sep 16, 2019

typescript-bot commented Sep 16, 2019 • edited Loading

typescript-bot commented Sep 16, 2019

sandersn commented Sep 16, 2019

typescript-bot commented Sep 16, 2019 • edited Loading

typescript-bot commented Sep 16, 2019 • edited Loading

typescript-bot commented Sep 16, 2019 • edited Loading

typescript-bot commented Sep 16, 2019

sandersn commented Sep 16, 2019

sandersn commented Sep 17, 2019

sandersn Sep 17, 2019

Choose a reason for hiding this comment

ljharb commented Sep 17, 2019

sandersn commented Sep 18, 2019

ljharb commented Sep 18, 2019

sandersn commented Sep 18, 2019

sandersn commented Sep 18, 2019

littledan commented Sep 21, 2019

ljharb commented Sep 22, 2019

AlCalzone commented Sep 23, 2019

sandersn commented Sep 23, 2019

AlCalzone commented Sep 24, 2019

sandersn commented Oct 1, 2019

AlCalzone commented Oct 2, 2019

sandersn commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 13, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading

typescript-bot commented Sep 16, 2019 •

edited

Loading