GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Control flow for constructor initialized properties by ahejlsberg · Pull Request #37920 · microsoft/TypeScript

Conversation

@ahejlsberg

@ahejlsberg Anders Hejlsberg (ahejlsberg) commented Apr 12, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Kingwl, hardfist, ExE-Boss, lcswillems, ulrichb, dragomirtitian, AntoineEsteve, danilofuchs, raftario, fernap3, and 14 more reacted with thumbs up emoji
All reactions
  • 👍 24 reactions
Anders Hejlsberg (ahejlsberg) added 2 commits April 12, 2020 15:27
@ahejlsberg
Use CFA to determine types of properties declared by this.xxx assignm…
2fbab38 
…ents
@ahejlsberg
Accept new baselines
957d9e6
@ahejlsberg Anders Hejlsberg (ahejlsberg) mentioned this pull request Apr 12, 2020
this-property assignments should use autoType to get control flow narrowing #37900
Closed
@weswigham

Wesley Wigham (weswigham) commented Apr 13, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Anders Hejlsberg (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 13, 2020

Copy link
Copy Markdown
Member Author

Wesley Wigham (@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.
weswigham reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Anders Hejlsberg (ahejlsberg) added 2 commits April 14, 2020 10:13
@ahejlsberg
Also use CFA in constructor functions
6e0274a
@ahejlsberg
Accept new baselines
df98d9c
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 14, 2020

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) user test this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 14, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg
Fix lint error
0c9c76a
@typescript-bot TypeScript Bot (typescript-bot) mentioned this pull request Apr 14, 2020
🤖 User test baselines have changed for controlFlowConstructorProperties typescript-bot/TypeScript#21
Open
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 14, 2020

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 14, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Nathan Shively-Sanders (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sandersn

Nathan Shively-Sanders (sandersn) commented Apr 14, 2020

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg
Only widen fresh literal types in CFA of assignment to auto-typed
728d9cb
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 16, 2020

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) user test this
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 16, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 16, 2020

Copy link
Copy Markdown
Member Author

Nathan Shively-Sanders (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Anders Hejlsberg (ahejlsberg) added 4 commits April 20, 2020 20:33
@ahejlsberg
Auto-typing for declared properties with no type annotation or initia…
fa6fee8 
…lizer
@ahejlsberg
Add optionality if declaration includes '?' modifier
e6d7607
@ahejlsberg
Always use CFA for properties with no initializer in .js files
33cbb82
@ahejlsberg
Small fix
ab993c2
@ahejlsberg Anders Hejlsberg (ahejlsberg) changed the title Control flow for constructor properties Control flow for constructor initialized properties Apr 21, 2020
@ahejlsberg

Anders Hejlsberg (ahejlsberg) commented Apr 21, 2020

Copy link
Copy Markdown
Member Author

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this
TypeScript Bot (@typescript-bot) run dt
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Apr 21, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Apr 21, 2020
Design Meeting Notes, 4/17/2020 #38096
Closed
@sandersn Nathan Shively-Sanders (sandersn) self-assigned this Apr 21, 2020
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request Apr 24, 2020
Design Meeting Notes, 4/24/2020 #38168
Closed
weswigham
Wesley Wigham (weswigham) reviewed Apr 28, 2020
View reviewed changes
Comment thread src/compiler/checker.ts
// Return the inherited type of the given property or undefined if property doesn't exist in a base class.
function getTypeOfPropertyInBaseClass(property: Symbol) {
const classType = getDeclaringClass(property);
const baseClassType = classType && getBaseTypes(classType)[0];

@weswigham Wesley Wigham (weswigham) Apr 28, 2020

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ahejlsberg Anders Hejlsberg (ahejlsberg) Apr 28, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
weswigham
Wesley Wigham (weswigham) approved these changes Apr 28, 2020
View reviewed changes
@ahejlsberg
Anders Hejlsberg (ahejlsberg) merged commit 3919042 into master Apr 28, 2020
@ahejlsberg
Anders Hejlsberg (ahejlsberg) deleted the controlFlowConstructorProperties branch April 28, 2020 23:59
@ahejlsberg Anders Hejlsberg (ahejlsberg) added this to the TypeScript 4.0 milestone Apr 29, 2020
@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) mentioned this pull request May 12, 2020
TypeScript 4.0 Iteration Plan #38510
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@sandersn Nathan Shively-Sanders (sandersn)

Labels

None yet

Projects

PR Backlog
Archived in project

Milestone

TypeScript 4.0.0

Development

Successfully merging this pull request may close these issues.

this-property assignments should use autoType to get control flow narrowing

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@ahejlsberg @weswigham @typescript-bot @sandersn
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.