Improve type guard consistiency

[weswigham]

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]

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

[weswigham]

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.

[sandersn]

That makes sense.

[sandersn]

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

[weswigham]

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]

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.

[weswigham]

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]

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]

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.

[DanielRosenwasser]

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

[DanielRosenwasser]

Can you make all of these consts?

[weswigham]

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

[DanielRosenwasser]

Do as your heart tells you. ✨

[weswigham]

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

[weswigham]

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]

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]

You should just check isFunctionLike

[weswigham]

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]

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

[weswigham]

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).

[DanielRosenwasser]

🎉