Skip to content

Make type guards continue to narrow within classes/nested function declarations #5626

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Wesley Wigham (weswigham) merged 4 commits into from
Nov 19, 2015
Merged

Make type guards continue to narrow within classes/nested function declarations #5626

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c29ddf
loosen walk up containers, add extra test
weswigham Nov 11, 2015
472afc4
add condition to bail at containing level
weswigham Nov 11, 2015
321bb9a
Merge branch 'master' into type-guard-narrowing
weswigham Nov 17, 2015
9531d92
update test with pr feedback
weswigham Nov 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6401,6 +6401,8 @@ namespace ts {
// Only narrow when symbol is variable of type any or an object, union, or type parameter type
if (node && symbol.flags & SymbolFlags.Variable) {
if (isTypeAny(type) || type.flags & (TypeFlags.ObjectType | TypeFlags.Union | TypeFlags.TypeParameter)) {
const declaration = getDeclarationOfKind(symbol, SyntaxKind.VariableDeclaration);

@sandersn Nathan Shively-Sanders (sandersn) Nov 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So now getNarrowedTypeOfSymbol walks up the parent chain twice, and further than it used to. Is there any performance impact?

@mhegazy Mohamed Hegazy (mhegazy) Nov 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

+1

@weswigham Wesley Wigham (weswigham) Nov 12, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

getDeclarationOfKind doesn't walk up any tree, it just iterates over a symbol's declarations. getDeclarationContainer walks out of a binding pattern (which is unavoidable, but also not highly nested) but then just does node.parent.parent.parent or node.parent. I'm not seeing a secondary walk here at all, just static overhead. Now as for weather it's worth doing this just to bail early rather than walking to the top of file... no idea. It probably depends on how deeply nested your code gets. I'll run some perf tests with and without the bailout.

@mhegazy Mohamed Hegazy (mhegazy) Nov 12, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you are walking once to find top then another time until you hit top. we can put them together, and walk up once, and just stop when you find what would be top otherwise.

@weswigham Wesley Wigham (weswigham) Nov 12, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Finding top doesn't involve a walk (beyond moving out of a binding pattern in the declaration), though?

Finding top is a matter of going from here:

function foo() {
  let {x}: {x: string | number} = {x: 1};
  //   ^
  if (typeof x === "string") {
    x;
  }
}

to the enclosing block.

The actual walk up starts at the node to narrow at:

function foo() {
  let {x}: {x: string | number} = {x: 1};
  if (typeof x === "string") {
    x;
 // ^
  }
}

and can either go up to the top level of the file/namespace (though this could traverse a lot of excess nodes if this is the only terminating condition), or can stop at the location defined by top - the scope within which the symbol in question is defined.

@sandersn Nathan Shively-Sanders (sandersn) Nov 12, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I meant getDeclarationContainer but put the comment one line too high by mistake.

@weswigham Wesley Wigham (weswigham) Nov 14, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, getDeclarationContainer isn't a heavy walk. It's a static 2 or 4 references up (based on declaration kind) after walking out of the binding pattern in the declaration. I can't merge these walks as Mohamed Hegazy (@mhegazy) suggests because I'm not walking the same section of the tree twice (declaration -> up to declaration container vs use site -> up to declaration container - I mean, I could merge them, but I'd have to recur into all siblings at each level looking for the declaration, which seems really bad). If k is the stack of nodes between the file/module container and the node we're narrowing at (which is how many nodes I would walk in the simple case without looking for the declaration container of the symbol's declaration), and n is the number of nodes I walk up when computing narrowing before stopping at top, and m is the number of nodes I walk up when computing the declaration container to compute top (which is to say the recursive depth of the binding pattern), I'm wagering that walking over m+n is practically more performant than than k, given that m has a tendency to be very small and k has a tendency to be very large.

Now, if it turns out that the declaration container is the top level of the namespace/module, then calculating it was extra work we didn't need to do as n=k and we've walked over m+k nodes rather than potentially just k nodes.

Really, this could be avoided if a declaration just kept a reference somewhere to its container, but that would be awkward, I suppose.

In effect, I'm trying to assert that getDeclarationContainer is close enough to constant time in all real-world cases, that its usage is warranted to be able to bail out of our narrowing walk up the tree early.

@weswigham Wesley Wigham (weswigham) Nov 18, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Current changes vs master baseline (5 iterations of perf test):

Project                         Baseline (ms)     Current (ms)      Delta (%) Best (ms) Worst (ms)
-------                         -------------     ------------      --------- --------- ----------
Compiler - tsc (ParseTime)      0.73 (± 46.94 %)  0.60 (± 2.76 %)   -17.71 %       0.59       0.62
Compiler - tsc (BindTime)       0.24 (± 4.36 %)   0.24 (± 2.88 %)   -0.84 %        0.23       0.24
Compiler - tsc (CheckTime)      1.11 (± 2.55 %)   1.07 (± 1.32 %)   -3.42 %        1.06       1.09
Compiler - tsc (EmitTime)       2.02 (± 12.41 %)  1.92 (± 7.18 %)   -4.94 %        1.73       1.99
Compiler - tsc (TotalTime)      4.11 (± 13.65 %)  3.83 (± 3.09 %)   -6.62 %        3.67        3.9
Monaco - tsc (ParseTime)        2.47 (± 70.11 %)  1.70 (± 1.11 %)   -31.45 %       1.68       1.72
Monaco - tsc (BindTime)         0.70 (± 2.67 %)   0.66 (± 1.69 %)   -6.82 %        0.65       0.67
Monaco - tsc (CheckTime)        3.35 (± 12.81 %)  3.22 (± 11.00 %)  -3.88 %        2.99       3.57
Monaco - tsc (EmitTime)         6.61 (± 22.60 %)  6.30 (± 15.68 %)  -4.69 %        5.55       7.22
Monaco - tsc (TotalTime)        13.14 (± 18.82 %) 11.87 (± 11.16 %) -9.62 %       10.93      13.14
TFS - tsc (ParseTime)           1.46 (± 70.93 %)  1.08 (± 1.41 %)   -26.23 %       1.06       1.09
TFS - tsc (BindTime)            0.49 (± 14.66 %)  0.46 (± 1.47 %)   -4.92 %        0.46       0.47
TFS - tsc (CheckTime)           2.28 (± 1.83 %)   2.45 (± 6.33 %)   7.73 %         2.29       2.57
TFS - tsc (EmitTime)            3.65 (± 5.27 %)   3.85 (± 22.65 %)  5.59 %         3.49       5.11
TFS - tsc (TotalTime)           7.88 (± 16.79 %)  7.85 (± 12.08 %)  -0.41 %        7.36        9.2
Encyclopedia - tsc (ParseTime)  0.42 (± 22.49 %)  0.38 (± 1.81 %)   -9.62 %        0.37       0.38
Encyclopedia - tsc (BindTime)   0.14 (± 9.58 %)   0.14 (± 7.71 %)   1.41 %         0.13       0.15
Encyclopedia - tsc (CheckTime)  0.50 (± 1.11 %)   0.51 (± 4.71 %)   3.21 %          0.5       0.54
Encyclopedia - tsc (EmitTime)   0.08 (± 28.47 %)  0.08 (± 28.47 %)  0.00 %         0.07       0.11
Encyclopedia - tsc (TotalTime)  1.13 (± 10.39 %)  1.11 (± 3.40 %)   -2.12 %        1.08       1.15
Compiler - node (ParseTime)     0.78 (± 8.57 %)   0.75 (± 1.81 %)   -4.08 %        0.74       0.76
Compiler - node (BindTime)      0.39 (± 10.07 %)  0.36 (± 2.90 %)   -8.21 %        0.35       0.37
Compiler - node (CheckTime)     1.70 (± 10.04 %)  1.64 (± 3.80 %)   -3.30 %        1.57       1.69
Compiler - node (EmitTime)      1.97 (± 4.16 %)   1.90 (± 11.51 %)  -3.26 %        1.74       2.18
Compiler - node (TotalTime)     4.84 (± 6.30 %)   4.66 (± 5.44 %)   -3.84 %        4.42       4.98
Monaco - node (ParseTime)       1.92 (± 8.11 %)   1.90 (± 7.76 %)   -1.14 %        1.78       2.08
Monaco - node (BindTime)        0.99 (± 7.98 %)   0.95 (± 6.98 %)   -4.23 %        0.89       1.03
Monaco - node (CheckTime)       5.59 (± 4.20 %)   5.60 (± 4.81 %)   0.07 %         5.32       5.88
Monaco - node (EmitTime)        5.96 (± 1.06 %)   5.87 (± 5.12 %)   -1.48 %        5.62       6.21
Monaco - node (TotalTime)       14.47 (± 2.89 %)  14.32 (± 4.56 %)  -1.05 %        13.6      14.81
TFS - node (ParseTime)          1.33 (± 4.01 %)   1.32 (± 3.88 %)   -0.45 %        1.28       1.39
TFS - node (BindTime)           0.83 (± 2.36 %)   0.84 (± 2.25 %)   0.72 %         0.82       0.86
TFS - node (CheckTime)          3.75 (± 2.66 %)   3.77 (± 2.40 %)   0.53 %         3.65       3.84
TFS - node (EmitTime)           4.13 (± 4.28 %)   4.20 (± 2.82 %)   1.55 %         4.13       4.36
TFS - node (TotalTime)          10.04 (± 2.89 %)  10.13 (± 2.27 %)  0.82 %         9.98      10.45
Encyclopedia - node (ParseTime) 0.29 (± 1.93 %)   0.30 (± 1.86 %)   3.47 %         0.29        0.3
Encyclopedia - node (BindTime)  0.22 (± 2.50 %)   0.23 (± 4.75 %)   5.41 %         0.22       0.24
Encyclopedia - node (CheckTime) 1.03 (± 2.41 %)   1.08 (± 3.40 %)   4.66 %         1.03        1.1
Encyclopedia - node (EmitTime)  0.13 (± 13.22 %)  0.13 (± 12.65 %)  1.59 %         0.12       0.15
Encyclopedia - node (TotalTime) 1.67 (± 1.93 %)   1.73 (± 2.17 %)   3.84 %         1.68       1.75

It causes minimal/no changes to overall perf in real-world scenarios. (Though I imagine this is the kind of change were you'd only see perf changes in a micro-benchmark anyway)

const top = declaration && getDeclarationContainer(declaration);
const originalType = type;
const nodeStack: {node: Node, child: Node}[] = [];
loop: while (node.parent) {
Expand All @@ -6414,15 +6416,12 @@ namespace ts {
break;
case SyntaxKind.SourceFile:
case SyntaxKind.ModuleDeclaration:
case SyntaxKind.FunctionDeclaration:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.MethodSignature:
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
case SyntaxKind.Constructor:
// Stop at the first containing function or module declaration
// Stop at the first containing file or module declaration
break loop;
}
if (node === top) {
break;
}
}

let nodes: {node: Node, child: Node};
Expand Down
37 changes: 37 additions & 0 deletions tests/baselines/reference/typeGuardInClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
//// [typeGuardInClass.ts]
let x: string | number;

if (typeof x === "string") {
let n = class {
constructor() {
let y: string = x;
}
}
}
else {
let m = class {
constructor() {
let y: number = x;
}
}
}


//// [typeGuardInClass.js]
var x;
if (typeof x === "string") {
var n = (function () {
function class_1() {
var y = x;
}
return class_1;
})();
}
else {
var m = (function () {
function class_2() {
var y = x;
}
return class_2;
})();
}
29 changes: 29 additions & 0 deletions tests/baselines/reference/typeGuardInClass.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardInClass.ts ===
let x: string | number;
>x : Symbol(x, Decl(typeGuardInClass.ts, 0, 3))

if (typeof x === "string") {
>x : Symbol(x, Decl(typeGuardInClass.ts, 0, 3))

let n = class {
>n : Symbol(n, Decl(typeGuardInClass.ts, 3, 7))

constructor() {
let y: string = x;
>y : Symbol(y, Decl(typeGuardInClass.ts, 5, 15))
>x : Symbol(x, Decl(typeGuardInClass.ts, 0, 3))
}
}
}
else {
let m = class {
>m : Symbol(m, Decl(typeGuardInClass.ts, 10, 7))

constructor() {
let y: number = x;
>y : Symbol(y, Decl(typeGuardInClass.ts, 12, 15))
>x : Symbol(x, Decl(typeGuardInClass.ts, 0, 3))
}
}
}

34 changes: 34 additions & 0 deletions tests/baselines/reference/typeGuardInClass.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardInClass.ts ===
let x: string | number;
>x : string | number

if (typeof x === "string") {
>typeof x === "string" : boolean
>typeof x : string
>x : string | number
>"string" : string

let n = class {
>n : typeof (Anonymous class)
>class { constructor() { let y: string = x; } } : typeof (Anonymous class)

constructor() {
let y: string = x;
>y : string
>x : string
}
}
}
else {
let m = class {
>m : typeof (Anonymous class)
>class { constructor() { let y: number = x; } } : typeof (Anonymous class)

constructor() {
let y: number = x;
>y : number
>x : number
}
}
}

8 changes: 4 additions & 4 deletions tests/baselines/reference/typeGuardsInFunctionAndModuleBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,12 @@ function foo4(x: number | string | boolean) {
: x.toString(); // number
})(x); // x here is narrowed to number | boolean
}
// Type guards affect nested function expressions, but not nested function declarations
// Type guards affect nested function expressions and nested function declarations
function foo5(x: number | string | boolean) {
if (typeof x === "string") {
var y = x; // string;
function foo() {
var z = x; // number | string | boolean, type guard has no effect
var z = x; // string
}
}
}
Expand Down Expand Up @@ -121,12 +121,12 @@ function foo4(x) {
: x.toString(); // number
})(x); // x here is narrowed to number | boolean
}
// Type guards affect nested function expressions, but not nested function declarations
// Type guards affect nested function expressions and nested function declarations
function foo5(x) {
if (typeof x === "string") {
var y = x; // string;
function foo() {
var z = x; // number | string | boolean, type guard has no effect
var z = x; // string
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/typeGuardsInFunctionAndModuleBlock.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ function foo4(x: number | string | boolean) {
})(x); // x here is narrowed to number | boolean
>x : Symbol(x, Decl(typeGuardsInFunctionAndModuleBlock.ts, 32, 14))
}
// Type guards affect nested function expressions, but not nested function declarations
// Type guards affect nested function expressions and nested function declarations
function foo5(x: number | string | boolean) {
>foo5 : Symbol(foo5, Decl(typeGuardsInFunctionAndModuleBlock.ts, 41, 1))
>x : Symbol(x, Decl(typeGuardsInFunctionAndModuleBlock.ts, 43, 14))
Expand All @@ -145,7 +145,7 @@ function foo5(x: number | string | boolean) {
function foo() {
>foo : Symbol(foo, Decl(typeGuardsInFunctionAndModuleBlock.ts, 45, 18))

var z = x; // number | string | boolean, type guard has no effect
var z = x; // string
>z : Symbol(z, Decl(typeGuardsInFunctionAndModuleBlock.ts, 47, 15))
>x : Symbol(x, Decl(typeGuardsInFunctionAndModuleBlock.ts, 43, 14))
}
Expand Down
8 changes: 4 additions & 4 deletions tests/baselines/reference/typeGuardsInFunctionAndModuleBlock.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ function foo4(x: number | string | boolean) {
})(x); // x here is narrowed to number | boolean
>x : number | boolean
}
// Type guards affect nested function expressions, but not nested function declarations
// Type guards affect nested function expressions and nested function declarations
function foo5(x: number | string | boolean) {
>foo5 : (x: number | string | boolean) => void
>x : number | string | boolean
Expand All @@ -199,9 +199,9 @@ function foo5(x: number | string | boolean) {
function foo() {
>foo : () => void

var z = x; // number | string | boolean, type guard has no effect
>z : number | string | boolean
>x : number | string | boolean
var z = x; // string
>z : string
>x : string
}
}
}
Expand Down
16 changes: 16 additions & 0 deletions tests/cases/conformance/expressions/typeGuards/typeGuardInClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
let x: string | number;

if (typeof x === "string") {
let n = class {
constructor() {
let y: string = x;
}
}
}
else {
let m = class {
constructor() {
let y: number = x;
}
}
}
4 changes: 2 additions & 2 deletions tests/cases/conformance/expressions/typeGuards/typeGuardsInFunctionAndModuleBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ function foo4(x: number | string | boolean) {
: x.toString(); // number
})(x); // x here is narrowed to number | boolean
}
// Type guards affect nested function expressions, but not nested function declarations
// Type guards affect nested function expressions and nested function declarations
function foo5(x: number | string | boolean) {
if (typeof x === "string") {
var y = x; // string;
function foo() {
var z = x; // number | string | boolean, type guard has no effect
var z = x; // string
}
}
}
Expand Down