Control flow for constructor initialized properties

[ahejlsberg]

With this PR we use control flow analysis of this.xxx assignments in constructors to determine the types of properties that have no type annotations or initializers.

• In .ts files we perform control flow analysis for properties declared with no type annotation or initializer when in noImplicitAny mode.
• In .js files we perform control flow analysis for properties declared with no JSDoc type annotation or initializer, and properties with no explicit declaration but at least one this.xxx assignment in the constructor.

In the following example, control flow analysis determines the type of x to be string | number based on the this.x assignments in the constructor:

// Compile with --noImplicitAny
class C {
    x;  // string | number
    constructor(b: boolean) {
        if (b) {
            this.x = 'hello';
        }
        else {
            this.x = 42;
        }
    }
}

In .js files we would previously determine the type of a property with no explicit declaration from local analysis of all this.xxx assignments seen in the constructor and methods of the class. This analysis is less precise than control flow analysis, but we still use it as a fallback. With the increased precision of control flow analysis we now correctly discover properties that are conditionally (as opposed to definitely) initialized in constructors. We're also able to handle assignments of values that depend on previous assignment to the same property, as well as other scenarios that previously were deemed circular.

The baseline changes are mostly because constructor declared properties are now considered to have type any when they are assignment targets in the constructor body. This an effect of how CFA auto-typing works.

Fixes #37900.

[weswigham]

@ahejlsberg do we have a test for the declaration emit of a property like this? afaik none of the cases in tests/cases/conformance/jsdoc/declarations/jsDeclarationsClasses.ts are that complex.

[ahejlsberg]

@weswigham Best I can tell there are several classes in that test that now have property types computed by CFA with no changes in baselines. I wouldn't expect any either since declaration emit just uses getTypeOfSymbol.

[ahejlsberg]

@typescript-bot user test this

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at df98d9c. You can monitor the build here.

[typescript-bot]

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

[ahejlsberg]

@sandersn I'm looking at the user test suite baseline changes, and so far things seem reasonable. The increased precision of CFA causes more errors in places as expected, for example when CFA discovers that a property is conditionally (instead of definitely) initialized in the constructor or that a property has an auto-type array type.

The chrome-devtools errors are insanely hard to verify because there's no indentation in the code, but best I can tell they're to be expected.

[sandersn]

I ran a before/after perf comparison of 10 runs of chrome-devtools-frontend, since it's a nice big code base with lots of functions.

Here's the average of 10 runs:

Before, time: 15.94 s
After, time: 16.21 s
1.66% slower.

Before, memory: 751.06 MB
After, memory: 757.27 MB
0.82% more memory.

Not that much difference actually.

(@ahejlsberg only one file in chrome-devtools-frontend is indentation-free, and it's a checked-in dependency. I always just skip that one.)

[ahejlsberg]

@typescript-bot user test this

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 728d9cb. You can monitor the build here.

[ahejlsberg]

@sandersn Fixed some over eager widening in auto-typed assignments. User test baselines now look good. More errors because we figure out more types, but the errors are to be expected and some of them actually reveal questionable logic.

I tried to get some perf numbers by running tsc on chrome-devtools-frontend but there is so much variability in the numbers that I can't really see any appreciable difference.

[ahejlsberg]

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

[typescript-bot]

Heya @ahejlsberg, I've started to run the extended test suite on this PR at ab993c2. You can monitor the build here.

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at ab993c2. You can monitor the build here.

[typescript-bot]

Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at ab993c2. You can monitor the build here.

[weswigham]

Thinking of mixins, shouldn't we iterate through the base types list and get the property from the first base type which has it, rather than only checking the first base type?

[ahejlsberg]

No, classes never have more than one base class, mixins or not.