Improve type guard consistiency #5442

weswigham · 2015-10-28T23:37:21Z

Building on what was mentioned in #5427, modifies getNarrowedTypeOfSymbol to handle an empty union only at the top level (so we handle compound expressions correctly), thereby allowing enabling returning empty sets at the correct time when mid-narrow.

Additionally, I've also preserved the behavior for narrowing any by a primitive type, and incidentally fixed issues where narrowing wasn't being correctly applies to an else clause or with enums.

Long story short, this fixes a bunch of bugs, but preserves our current behaviors in meaningful edge cases (according to our test suite).

Fixes #1270, #5439, #5100, #4874. (Or it should, anyway)

To summarize the changes:
narrowTypeByEquality was mostly rewritten to unify the positive case and negative case logic, while also allowing it to actually return an empty union. removeTypesFromUnion was removed in the process, and flags were set to not merge subtypes in unions when narrowing (thus preserving enums/future string types in narrowed unions). To preserve old (desired?) behavior with respect to type guards narrowing a type to the empty set acting as the original type, a check was added after the loop in getNarrowedTypeOfSymbol to do just that. While looking at getNarrowedTypeOfSymbol, I noticed we were narrowing from the outside in (for some reason), which was the root cause of nested narrowing not working as intended. To fix this, I instead create a stack of parent nodes and visit them in reverse order from before. This, combined with actually returning an empty set from the internal narrowing functions, corrected the tautological case.

@sandersn how do you feel about it?

If you feel strongly about allowing narrowing to the empty set, we can now do that just by removing a top-level check in getNarrowedTypeOfSymbol.

sandersn · 2015-10-29T17:39:34Z

src/compiler/checker.ts

@@ -6338,6 +6338,10 @@ namespace ts {
                                // Stop at the first containing function or module declaration
                                break loop;
                        }
+                        // Preserve old top-level behavior - if the branch is really an empty set, revert to prior type
+                        if (narrowedType === getUnionType(emptyArray)) {


Isn't getUnionType(emptyArray) === emptyObjectType? Why not just use that?

I respected the note left in the narrowing code about this - if we change how we represent empty unions, then this is the canonical way to find that representation.

That makes sense.

sandersn · 2015-10-29T18:21:28Z

What happens with conflicting guards with this change? You'll still get the original union, right?

weswigham · 2015-10-29T18:24:40Z

Correct - I moved the check to replace an empty object with the original type from within the narrowing functions to the top level of the getNarrowedTypeOfSymbol function to preserve current behaviors as much as possible.

sandersn · 2015-10-29T18:26:55Z

src/compiler/checker.ts

                if (assumeTrue) {
-                    // Assumed result is true. If check was not for a primitive type, remove all primitive types
-                    if (!typeInfo) {
-                        return removeTypesFromUnionType(type, /*typeKind*/ TypeFlags.StringLike | TypeFlags.NumberLike | TypeFlags.Boolean | TypeFlags.ESSymbol,


removeTypesFromUnionType is only used one place now, I think. I bet you could considerably simplify the interface and implementation (typeKind and isTypeOfKind and allowEmptyUnionResult at least).

Actually, the lone usage might also be replaceable by getUnionType . filter as well.

Seems reasonable, I was thinking about the same thing - just wanted to avoid making the change cascade into too many other places. I'll replace the invocation.

sandersn · 2015-10-29T18:34:06Z

The change is convincing to me -- seems like a small but reasonable change to the semantics -- so here's a tentative 👍
But you should have somebody who understands all the rules for union types take a look too.

DanielRosenwasser · 2015-11-01T09:05:03Z

src/compiler/checker.ts

                        }
                    }
+
+                    // Preserve old top-level behavior - if the branch is really an empty set, revert to prior type
+                    if (type === getUnionType(emptyArray)) {


Just check against the emptyObjectType, or if you want to avoid tying this code against the fact that it will return the emptyObjectType, calculate it once at the top.

Actually I might declare an emptyUnionType which is just aliased to emptyObjectType, then return it in getUnionType and use it here

DanielRosenwasser · 2015-11-03T20:57:44Z

src/compiler/checker.ts

@@ -112,6 +112,7 @@ namespace ts {
        let circularType = createIntrinsicType(TypeFlags.Any, "__circular__");

        let emptyObjectType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);
+        let emptyUnionType = emptyObjectType;
        let emptyGenericType = <GenericType><ObjectType>createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined);


Can you make all of these consts?

Can I remove circularType while I'm at it? (It's referenced nowhere)

Do as your heart tells you. ✨

weswigham · 2015-11-09T21:03:56Z

@sandersn @DanielRosenwasser @mhegazy is there anything else you'd like from this PR, or can it be merged?

weswigham · 2015-11-09T23:12:28Z

I've run perf tests (10 iterations) against this branch vs master:

Project                         Baseline (ms)    Current (ms)     Delta (%) Best (ms) Worst (ms)
-------                         -------------    ------------     --------- --------- ----------
Compiler - tsc (ParseTime)      0.59 (± 1.61 %)  0.58 (± 1.01 %)  -2.19 %        0.57       0.59
Compiler - tsc (BindTime)       0.23 (± 1.98 %)  0.23 (± 2.25 %)  -1.32 %        0.22       0.24
Compiler - tsc (CheckTime)      1.02 (± 1.15 %)  1.01 (± 2.04 %)  -0.88 %        0.96       1.04
Compiler - tsc (EmitTime)       1.93 (± 5.98 %)  1.83 (± 5.15 %)  -5.04 %        1.73       2.13
Compiler - tsc (TotalTime)      3.76 (± 2.99 %)  3.64 (± 2.61 %)  -3.24 %         3.5       3.94
Monaco - tsc (ParseTime)        1.73 (± 3.39 %)  1.77 (± 4.07 %)  2.20 %         1.64       1.97
Monaco - tsc (BindTime)         0.64 (± 2.53 %)  0.67 (± 6.95 %)  4.06 %         0.62       0.84
Monaco - tsc (CheckTime)        3.21 (± 6.67 %)  3.29 (± 5.58 %)  2.53 %         2.82       3.56
Monaco - tsc (EmitTime)         6.65 (± 9.69 %)  6.90 (± 9.02 %)  3.70 %         5.68       8.12
Monaco - tsc (TotalTime)        12.22 (± 6.54 %) 12.62 (± 6.54 %) 3.22 %        10.92      14.09
TFS - tsc (ParseTime)           1.08 (± 1.95 %)  1.12 (± 4.79 %)  3.33 %         1.04        1.3
TFS - tsc (BindTime)            0.47 (± 4.66 %)  0.48 (± 4.32 %)  1.71 %         0.44       0.52
TFS - tsc (CheckTime)           2.36 (± 3.91 %)  2.48 (± 5.24 %)  5.26 %          2.2       2.74
TFS - tsc (EmitTime)            3.90 (± 12.64 %) 4.05 (± 9.40 %)  3.95 %         3.62       5.01
TFS - tsc (TotalTime)           7.80 (± 7.50 %)  8.12 (± 6.11 %)  4.13 %         7.45       9.15
Encyclopedia - tsc (ParseTime)  0.37 (± 1.82 %)  0.40 (± 3.66 %)  8.31 %         0.38       0.44
Encyclopedia - tsc (BindTime)   0.14 (± 4.30 %)  0.15 (± 7.96 %)  11.68 %        0.13       0.18
Encyclopedia - tsc (CheckTime)  0.47 (± 1.92 %)  0.50 (± 4.45 %)  7.92 %         0.47       0.56
Encyclopedia - tsc (EmitTime)   0.08 (± 14.85 %) 0.08 (± 12.87 %) 7.89 %         0.07       0.12
Encyclopedia - tsc (TotalTime)  1.05 (± 2.08 %)  1.14 (± 3.95 %)  8.26 %         1.06       1.23
Compiler - node (ParseTime)     0.74 (± 1.71 %)  0.75 (± 1.90 %)  1.49 %         0.72       0.78
Compiler - node (BindTime)      0.36 (± 2.12 %)  0.38 (± 1.80 %)  5.60 %         0.36       0.39
Compiler - node (CheckTime)     1.49 (± 2.12 %)  1.57 (± 2.85 %)  5.30 %          1.5       1.68
Compiler - node (EmitTime)      1.75 (± 2.50 %)  1.81 (± 3.46 %)  3.49 %         1.72          2
Compiler - node (TotalTime)     4.33 (± 1.54 %)  4.50 (± 2.18 %)  4.00 %         4.34       4.73
Monaco - node (ParseTime)       1.79 (± 1.76 %)  1.78 (± 1.36 %)  -0.84 %        1.73       1.85
Monaco - node (BindTime)        0.90 (± 5.29 %)  0.91 (± 3.86 %)  1.44 %         0.85          1
Monaco - node (CheckTime)       5.11 (± 2.32 %)  5.11 (± 1.62 %)  -0.12 %        4.88       5.27
Monaco - node (EmitTime)        5.82 (± 1.63 %)  5.94 (± 2.45 %)  2.10 %          5.6       6.31
Monaco - node (TotalTime)       13.62 (± 1.36 %) 13.74 (± 1.39 %) 0.84 %        13.28      14.14
TFS - node (ParseTime)          1.31 (± 2.15 %)  1.31 (± 2.19 %)  0.00 %         1.27        1.4
TFS - node (BindTime)           0.83 (± 4.08 %)  0.87 (± 14.34 %) 4.72 %         0.77       1.35
TFS - node (CheckTime)          3.82 (± 4.77 %)  3.70 (± 2.42 %)  -2.96 %        3.48       3.87
TFS - node (EmitTime)           4.21 (± 3.00 %)  4.18 (± 1.26 %)  -0.88 %        4.07       4.29
TFS - node (TotalTime)          10.17 (± 2.92 %) 10.06 (± 1.93 %) -1.08 %        9.64      10.59
Encyclopedia - node (ParseTime) 0.29 (± 2.53 %)  0.28 (± 2.71 %)  -2.74 %        0.27       0.31
Encyclopedia - node (BindTime)  0.23 (± 3.24 %)  0.23 (± 5.88 %)  0.00 %          0.2       0.27
Encyclopedia - node (CheckTime) 1.07 (± 1.46 %)  1.07 (± 10.95 %) 0.47 %         0.99       1.54
Encyclopedia - node (EmitTime)  0.13 (± 9.72 %)  0.13 (± 7.78 %)  3.17 %         0.11       0.16
Encyclopedia - node (TotalTime) 1.71 (± 1.67 %)  1.72 (± 8.02 %)  0.29 %          1.6       2.26

I don't see any meaningful change in perf.

sandersn · 2015-11-11T18:05:59Z

Can we merge this now? I talked to @mhegazy yesterday and we are both in favor of it. @DanielRosenwasser can you take another look?

DanielRosenwasser · 2015-11-11T20:13:27Z

src/compiler/checker.ts

+                            case SyntaxKind.MethodSignature:
+                            case SyntaxKind.GetAccessor:
+                            case SyntaxKind.SetAccessor:
+                            case SyntaxKind.Constructor:


You should just check isFunctionLike

isFunctionLike also returns affirmatively for FunctionExpression, ArrowFunction, CallSignature, ConstructSignature, IndexSignature, FunctionType, and ConstructorType - the difference visibly changes our behavior (as in: tests fail). I think these kinds are moreso to find "top-level" declarations where logic outside of it is unlikely to affect the type within the declaration.

DanielRosenwasser · 2015-11-11T20:14:41Z

Perf impact seems decently small, apart from my last comment 👍

weswigham · 2015-11-11T20:52:08Z

Spoke with @DanielRosenwasser about that switch and the implications for changing it - opened #5619 as a related issue, and will merge this as-is since it matches existing behavior for selecting where to stop collecting type narrowing information (bugs included).

Improve type guard consistiency

DanielRosenwasser · 2015-11-11T20:57:16Z

🎉

weswigham added 3 commits October 28, 2015 13:48

naive change and new tests

062495c

Improve type narrowing algorithm with typeof

febda00

restore any narrowing

168c664

msftclas added the cla-already-signed label Oct 28, 2015

sandersn reviewed Oct 29, 2015
View reviewed changes

weswigham added 3 commits October 29, 2015 17:51

feedback from pr, new tests

9f12bc8

More correctness

19e796d

that feling when you ctrl-z 1 too many times

0d0c05d

weswigham mentioned this pull request Oct 30, 2015

Narrow on else branch of multi-conditional guard #5427

Closed

DanielRosenwasser reviewed Nov 1, 2015
View reviewed changes

fix nits

d48a4f0

DanielRosenwasser reviewed Nov 3, 2015
View reviewed changes

weswigham added 2 commits November 3, 2015 14:35

const all the things

77c69da

Merge branch 'master' into empty-set

d4353fd

sandersn mentioned this pull request Nov 4, 2015

type checking after instainceOf condition in else block is working incorrectly #5523

Closed

Merge branch 'master' into empty-set

5f18486

DanielRosenwasser reviewed Nov 11, 2015
View reviewed changes

weswigham added a commit that referenced this pull request Nov 11, 2015

Merge pull request #5442 from weswigham/empty-set

b3f0a71

Improve type guard consistiency

weswigham merged commit b3f0a71 into microsoft:master Nov 11, 2015

This was referenced Nov 11, 2015

Behavior for conflicting type guards is inconsistent #4874

Closed

Enums erroneously removed from unions by type guards #5439

Closed

weswigham mentioned this pull request Nov 11, 2015

Type guards doesn't work on multilevel if statements #5100

Closed

weswigham mentioned this pull request Dec 2, 2015

Type guard not working as expected in function overloading with union types #5826

Closed

weswigham deleted the empty-set branch August 17, 2017 23:04

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve type guard consistiency #5442

Improve type guard consistiency #5442

weswigham commented Oct 28, 2015

sandersn Oct 29, 2015

weswigham Oct 29, 2015

sandersn Oct 29, 2015

sandersn commented Oct 29, 2015

weswigham commented Oct 29, 2015

sandersn Oct 29, 2015

weswigham Oct 29, 2015

sandersn commented Oct 29, 2015

DanielRosenwasser Nov 1, 2015

DanielRosenwasser Nov 1, 2015

DanielRosenwasser Nov 3, 2015

weswigham Nov 3, 2015

DanielRosenwasser Nov 3, 2015

weswigham commented Nov 9, 2015

weswigham commented Nov 9, 2015

sandersn commented Nov 11, 2015

DanielRosenwasser Nov 11, 2015

weswigham Nov 11, 2015

DanielRosenwasser commented Nov 11, 2015

weswigham commented Nov 11, 2015

DanielRosenwasser commented Nov 11, 2015

Improve type guard consistiency #5442

Improve type guard consistiency #5442

Conversation

weswigham commented Oct 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Oct 29, 2015

weswigham commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn commented Oct 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Nov 9, 2015

weswigham commented Nov 9, 2015

sandersn commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Nov 11, 2015

weswigham commented Nov 11, 2015

DanielRosenwasser commented Nov 11, 2015