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

Control flow for constructor initialized properties#37920

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
ahejlsberg merged 10 commits intomasterfromcontrolFlowConstructorProperties
Apr 28, 2020

Conversation

@ahejlsberg
Copy link
Member

@ahejlsbergahejlsberg commentedApr 12, 2020•
edited
Loading

With this PR we use control flow analysis ofthis.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 innoImplicitAny 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 onethis.xxx assignment in the constructor.

In the following example, control flow analysis determines the type ofx to bestring | number based on thethis.x assignments in the constructor:

// Compile with --noImplicitAnyclassC{x;// string | numberconstructor(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 allthis.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 typeany when they are assignment targets in the constructor body. This an effect of how CFA auto-typing works.

Fixes#37900.

Kingwl, hardfist, ExE-Boss, lcswillems, ulrichb, dragomirtitian, AntoineEsteve, danilofuchs, raftario, fernap3, and 14 more reacted with thumbs up emoji
@weswigham
Copy link
Member

weswigham commentedApr 13, 2020•
edited
Loading

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

@ahejlsberg
Copy link
MemberAuthor

@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 usesgetTypeOfSymbol.

weswigham reacted with thumbs up emoji

@ahejlsberg
Copy link
MemberAuthor

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 14, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized community code test suite on this PR atdf98d9c. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished andfailed. I've opened aPR with the baseline diff from master.

@ahejlsberg
Copy link
MemberAuthor

ahejlsberg commentedApr 14, 2020•
edited
Loading

@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
Copy link
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.

(@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
Copy link
MemberAuthor

@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 16, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized community code test suite on this PR at728d9cb. You can monitor the buildhere.

@ahejlsberg
Copy link
MemberAuthor

@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 runningtsc on chrome-devtools-frontend but there is so much variability in the numbers that I can't really see any appreciable difference.

@ahejlsbergahejlsberg changed the titleControl flow for constructor propertiesControl flow for constructor initialized propertiesApr 21, 2020
@ahejlsberg
Copy link
MemberAuthor

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

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 21, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the extended test suite on this PR atab993c2. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 21, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR atab993c2. You can monitor the buildhere.

@typescript-bot
Copy link
Collaborator

typescript-bot commentedApr 21, 2020•
edited
Loading

Heya@ahejlsberg, I've started to run the parallelized community code test suite on this PR atab993c2. You can monitor the buildhere.

// 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];
Copy link
Member

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?

Copy link
MemberAuthor

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.

@ahejlsbergahejlsberg merged commit3919042 intomasterApr 28, 2020
@ahejlsbergahejlsberg deleted the controlFlowConstructorProperties branchApril 28, 2020 23:59
@ahejlsbergahejlsberg added this to theTypeScript 4.0 milestoneApr 29, 2020
@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

@weswighamweswighamweswigham approved these changes

Assignees

@sandersnsandersn

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

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

5 participants

@ahejlsberg@weswigham@typescript-bot@sandersn
[8]ページ先頭
©2009-2025 Movatter.jp