Control flow based type analysis #8010
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1408,6 +1408,31 @@ namespace ts { | |
| return !!node && (node.kind === SyntaxKind.ArrayBindingPattern || node.kind === SyntaxKind.ObjectBindingPattern); | ||
| } | ||
|
|
||
| // A node is an assignment target if it is on the left hand side of an '=' token, if it is parented by a property | ||
| // assignment in an object literal that is an assignment target, or if it is parented by an array literal that is | ||
| // an assignment target. Examples include 'a = xxx', '{ p: a } = xxx', '[{ p: a}] = xxx'. | ||
| export function isAssignmentTarget(node: Node): boolean { | ||
| while (node.parent.kind === SyntaxKind.ParenthesizedExpression) { | ||
| node = node.parent; | ||
| } | ||
| while (true) { | ||
| const parent = node.parent; | ||
| if (parent.kind === SyntaxKind.ArrayLiteralExpression || parent.kind === SyntaxKind.SpreadElementExpression) { | ||
| node = parent; | ||
| continue; | ||
| } | ||
| if (parent.kind === SyntaxKind.PropertyAssignment || parent.kind === SyntaxKind.ShorthandPropertyAssignment) { | ||
| node = parent.parent; | ||
| continue; | ||
| } | ||
| return parent.kind === SyntaxKind.BinaryExpression && | ||
| (<BinaryExpression>parent).operatorToken.kind === SyntaxKind.EqualsToken && | ||
| (<BinaryExpression>parent).left === node || | ||
| (parent.kind === SyntaxKind.ForInStatement || parent.kind === SyntaxKind.ForOfStatement) && | ||
| (<ForInStatement | ForOfStatement>parent).initializer === node; | ||
| } | ||
| } | ||
|
|
||
| export function isNodeDescendentOf(node: Node, ancestor: Node): boolean { | ||
| while (node) { | ||
| if (node === ancestor) return true; | ||
|
|
@@ -1504,7 +1529,7 @@ namespace ts { | |
| } | ||
|
|
||
| // True if the given identifier, string literal, or number literal is the name of a declaration node | ||
| export function isDeclarationName(name: Node): boolean { | ||
|
There was a problem hiding this comment. Why was the type-guardiness of this function removed? There was a problem hiding this comment. Probably because this function can also return There was a problem hiding this comment. If that was the case, then wouldn't the name of the function be misleading? There was a problem hiding this comment. Not in my opinion, it checks whether a node is a declaration name, but not every identifier is a declaration name. So the type annotation was wrong, not the name. There was a problem hiding this comment. Yes, @ivogabe is exactly right about why I removed the type predicate annotation. |
||
| if (name.kind !== SyntaxKind.Identifier && name.kind !== SyntaxKind.StringLiteral && name.kind !== SyntaxKind.NumericLiteral) { | ||
| return false; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| //// [assignmentTypeNarrowing.ts] | ||
| let x: string | number | boolean | RegExp; | ||
|
|
||
| x = ""; | ||
| x; // string | ||
|
|
||
| [x] = [true]; | ||
| x; // boolean | ||
|
|
||
| [x = ""] = [1]; | ||
| x; // string | number | ||
|
|
||
| ({x} = {x: true}); | ||
| x; // boolean | ||
|
|
||
| ({y: x} = {y: 1}); | ||
| x; // number | ||
|
|
||
| ({x = ""} = {x: true}); | ||
| x; // string | boolean | ||
|
|
||
| ({y: x = /a/} = {y: 1}); | ||
| x; // number | RegExp | ||
|
|
||
| let a: string[]; | ||
|
|
||
| for (x of a) { | ||
| x; // string | ||
| } | ||
|
|
||
|
|
||
| //// [assignmentTypeNarrowing.js] | ||
| var x; | ||
| x = ""; | ||
| x; // string | ||
| x = [true][0]; | ||
| x; // boolean | ||
| _a = [1][0], x = _a === void 0 ? "" : _a; | ||
| x; // string | number | ||
| (_b = { x: true }, x = _b.x, _b); | ||
| x; // boolean | ||
| (_c = { y: 1 }, x = _c.y, _c); | ||
| x; // number | ||
| (_d = { x: true }, _e = _d.x, x = _e === void 0 ? "" : _e, _d); | ||
| x; // string | boolean | ||
| (_f = { y: 1 }, _g = _f.y, x = _g === void 0 ? /a/ : _g, _f); | ||
| x; // number | RegExp | ||
| var a; | ||
| for (var _i = 0, a_1 = a; _i < a_1.length; _i++) { | ||
| x = a_1[_i]; | ||
| x; // string | ||
| } | ||
| var _a, _b, _c, _d, _e, _f, _g; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| === tests/cases/conformance/expressions/assignmentOperator/assignmentTypeNarrowing.ts === | ||
| let x: string | number | boolean | RegExp; | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
| >RegExp : Symbol(RegExp, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --)) | ||
|
|
||
| x = ""; | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| x; // string | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| [x] = [true]; | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| x; // boolean | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| [x = ""] = [1]; | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| x; // string | number | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| ({x} = {x: true}); | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 11, 2)) | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 11, 8)) | ||
|
|
||
| x; // boolean | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| ({y: x} = {y: 1}); | ||
| >y : Symbol(y, Decl(assignmentTypeNarrowing.ts, 14, 2)) | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
| >y : Symbol(y, Decl(assignmentTypeNarrowing.ts, 14, 11)) | ||
|
|
||
| x; // number | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| ({x = ""} = {x: true}); | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 17, 2)) | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 17, 13)) | ||
|
|
||
| x; // string | boolean | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| ({y: x = /a/} = {y: 1}); | ||
| >y : Symbol(y, Decl(assignmentTypeNarrowing.ts, 20, 2)) | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
| >y : Symbol(y, Decl(assignmentTypeNarrowing.ts, 20, 17)) | ||
|
|
||
| x; // number | RegExp | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
|
|
||
| let a: string[]; | ||
| >a : Symbol(a, Decl(assignmentTypeNarrowing.ts, 23, 3)) | ||
|
|
||
| for (x of a) { | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
| >a : Symbol(a, Decl(assignmentTypeNarrowing.ts, 23, 3)) | ||
|
|
||
| x; // string | ||
| >x : Symbol(x, Decl(assignmentTypeNarrowing.ts, 0, 3)) | ||
| } | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| === tests/cases/conformance/expressions/assignmentOperator/assignmentTypeNarrowing.ts === | ||
| let x: string | number | boolean | RegExp; | ||
| >x : string | number | boolean | RegExp | ||
| >RegExp : RegExp | ||
|
|
||
| x = ""; | ||
| >x = "" : string | ||
| >x : string | number | boolean | RegExp | ||
| >"" : string | ||
|
|
||
| x; // string | ||
| >x : string | ||
|
|
||
| [x] = [true]; | ||
| >[x] = [true] : [boolean] | ||
| >[x] : [string | number | boolean | RegExp] | ||
| >x : string | number | boolean | RegExp | ||
| >[true] : [boolean] | ||
| >true : boolean | ||
|
|
||
| x; // boolean | ||
| >x : boolean | ||
|
|
||
| [x = ""] = [1]; | ||
| >[x = ""] = [1] : [number] | ||
| >[x = ""] : [string] | ||
| >x = "" : string | ||
| >x : string | number | boolean | RegExp | ||
| >"" : string | ||
| >[1] : [number] | ||
| >1 : number | ||
|
|
||
| x; // string | number | ||
| >x : string | number | ||
|
|
||
| ({x} = {x: true}); | ||
| >({x} = {x: true}) : { x: boolean; } | ||
| >{x} = {x: true} : { x: boolean; } | ||
| >{x} : { x: string | number | boolean | RegExp; } | ||
| >x : string | number | boolean | RegExp | ||
| >{x: true} : { x: boolean; } | ||
| >x : boolean | ||
| >true : boolean | ||
|
|
||
| x; // boolean | ||
| >x : boolean | ||
|
|
||
| ({y: x} = {y: 1}); | ||
| >({y: x} = {y: 1}) : { y: number; } | ||
| >{y: x} = {y: 1} : { y: number; } | ||
| >{y: x} : { y: string | number | boolean | RegExp; } | ||
| >y : string | number | boolean | RegExp | ||
| >x : string | number | boolean | RegExp | ||
| >{y: 1} : { y: number; } | ||
| >y : number | ||
| >1 : number | ||
|
|
||
| x; // number | ||
| >x : number | ||
|
|
||
| ({x = ""} = {x: true}); | ||
| >({x = ""} = {x: true}) : { x?: boolean; } | ||
| >{x = ""} = {x: true} : { x?: boolean; } | ||
| >{x = ""} : { x?: string | number | boolean | RegExp; } | ||
| >x : string | number | boolean | RegExp | ||
| >{x: true} : { x?: boolean; } | ||
| >x : boolean | ||
| >true : boolean | ||
|
|
||
| x; // string | boolean | ||
| >x : string | boolean | ||
|
|
||
| ({y: x = /a/} = {y: 1}); | ||
| >({y: x = /a/} = {y: 1}) : { y?: number; } | ||
| >{y: x = /a/} = {y: 1} : { y?: number; } | ||
| >{y: x = /a/} : { y?: RegExp; } | ||
| >y : RegExp | ||
| >x = /a/ : RegExp | ||
| >x : string | number | boolean | RegExp | ||
| >/a/ : RegExp | ||
| >{y: 1} : { y?: number; } | ||
| >y : number | ||
| >1 : number | ||
|
|
||
| x; // number | RegExp | ||
| >x : number | RegExp | ||
|
|
||
| let a: string[]; | ||
| >a : string[] | ||
|
|
||
| for (x of a) { | ||
| >x : string | number | boolean | RegExp | ||
| >a : string[] | ||
|
|
||
| x; // string | ||
| >x : string | ||
| } | ||
|
|
Oops, something went wrong.
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.
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.
What's the new type annotation and
// !!!comment for?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.
Since
Entityis a type alias of a union, it will be narrowed after its assignment. This creates a circular dependency during the type analysis. Because of this,nodewould be typed asanyif the type annotation was not given.@ahejlsberg Would it be possible to give the inference of the type of the initializer precedence over the narrowing after assignments?
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.
The exact problem here is the following: We infer the type of
nodefrom the call tocreateNode. To evaluate that call we need to know the type ofentityso we can find the type of theposproperty. To find the type ofentitywe look at the preceding code paths. One leads from the top of the function. In that code path,entityhas typeIdentifier, which is less than its full declared type (Identifier | QualifiedName). Therefore we need to also analyze the second code path that comes from the bottom of the while statement (the loop around case). In that code path, we have the assignmententity = finishNode(node)which requires us to know the type ofnode. Boom! Circularity. Not exactly clear what rule we'd introduce to break that circularity.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.
Would just assuming that
entityhas its original declared type be good enough?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.
One other idea is that while doing the analysis, if you hit a type assertion, ignore the RHS, and just use the type associated with the type assertion.
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.
Yes that's a good point. It's not a simple matter of using the declared type. But I think the overloaded
nextfunction wouldn't even accept a union type as an argument.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, it wouldn't with how TS resolves signatures at present - at least not without another signature declaring it as taking a parameter of type
anyor a union of the other types. (Which, for some reason feels off to me - if every member of a union of argument types can be fulfilled by an overload and there's no better match, why can't the return be the union of those overloads' return values? Question for another time.)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.
(Answer for another time ;)
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.
@weswigham @JsonFreeman There are actually several issues being debated here:
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.
Thanks Anders, I think that's a great breakdown. I'll add that (if I'm not mistaken), this separation of resolution and checking is in place for bodies of function expressions. The idea here would be to generalize it for other expressions.
I agree that iterating to a fixed point may be necessary for loops in which the variable is reassigned in the loop body.