Favor asserted type in type predicate narrowing

[Anders Hejlsberg (ahejlsberg)]

With this PR we consistently favor the asserted type in a type predicate narrowing when both the argument type and the asserted type are subtypes of each other. We used to do this for non-union argument types, but not for unions. Fixes the issue reported here.

In an ideal world we'd fix this issue by having the subtype relationship only consider fresh object literal types subtypes of object types with index signatures, but unfortunately this is too much of a breaking change (I tried in #50013, but I will need to abandon that PR).

Adding a bit more context...

This is our inconsistent behavior in 4.7 (adding undefined shouldn't cause an error):

declarefunctionisObject(value: unknown): value is Record<string,unknown>;declareconstobj1: {};obj1;// {}if(isObject(obj1)){obj1;// Record<string, unknown>obj1['attr'];}obj1;// Record<string, unknown>declareconstobj2: {}|undefined;obj2;// {} | undefinedif(isObject(obj2)){obj2;// {}obj2['attr'];// Error!}obj2;// {} | undefined

In #49625 we changed both of the above to error. That's more consistent, but a breaking change with the new error. With this PR we instead consistently favor the asserted type:

declarefunctionisObject(value: unknown): value is Record<string,unknown>;declareconstobj1: {};obj1;// {}if(isObject(obj1)){obj1;// Record<string, unknown>obj1['attr'];}obj1;// Record<string, unknown>declareconstobj2: {}|undefined;obj2;// {} | undefinedif(isObject(obj2)){obj2;// Record<string, unknown>obj2['attr'];}obj2;// Record<string, unknown> | undefined

Note that one issue with favoring the asserted type is that CFA continues with that type after the conditional block. Though undesired, that's an effect of how CFA works in the face of mutual subtypes. This behavior was already present for singleton types, but now also extends to union types.

Fixes#49988 (at least partially).

[Anders Hejlsberg (ahejlsberg)]

TypeScript Bot (@typescript-bot) test this
TypeScript Bot (@typescript-bot) user test this inline
TypeScript Bot (@typescript-bot) run dt
TypeScript Bot (@typescript-bot) perf test faster

[TypeScript Bot (typescript-bot)]

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the extended test suite on this PR at a9484d3. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the abridged perf test suite on this PR at a9484d3. You can monitor the build here.

Update: The results are in!

[TypeScript Bot (typescript-bot)]

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the parallelized Definitely Typed test suite on this PR at a9484d3. You can monitor the build here.

[TypeScript Bot (typescript-bot)]

Heya Anders Hejlsberg (@ahejlsberg), I've started to run the diff-based user code test suite on this PR at a9484d3. You can monitor the build here.

Update: The results are in!

[TypeScript Bot (typescript-bot)]

Heya Anders Hejlsberg (@ahejlsberg), I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here.

[TypeScript Bot (typescript-bot)]

Anders Hejlsberg (@ahejlsberg)
Great news! no new errors were found between main..refs/pull/50044/merge

[Anders Hejlsberg (ahejlsberg)]

One new error in the RWC test suite. In the ant-design project, there's this sequence:

if(!React.isValidElement(element)){return;}constcolumn: any={
...element.props,// New error here};

where element is of type ReactElement<any> | ReactText. The isValidElement type predicate narrows to ReactElement<unknown> which is a mutual subtype with ReactElement<any>. So we now go with the unknown and subsequently can't access through a property that previously had type any. This is consistent with what would happen for a singleton, but it's still a new error.

[TypeScript Bot (typescript-bot)]

Anders Hejlsberg (@ahejlsberg)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..50044

Metric	main	50044	Delta	Best	Worst
Angular - node (v14.15.1, x64)
Memory used	335,955k (± 0.01%)	335,942k (± 0.01%)	-13k (- 0.00%)	335,899k	336,013k
Parse Time	2.06s (± 0.64%)	2.07s (± 0.69%)	+0.01s (+ 0.53%)	2.04s	2.10s
Bind Time	0.89s (± 0.58%)	0.90s (± 0.53%)	+0.01s (+ 0.67%)	0.89s	0.91s
Check Time	5.82s (± 0.26%)	5.82s (± 0.52%)	-0.00s (- 0.09%)	5.74s	5.88s
Emit Time	6.34s (± 0.52%)	6.38s (± 0.63%)	+0.04s (+ 0.65%)	6.30s	6.48s
Total Time	15.12s (± 0.32%)	15.17s (± 0.36%)	+0.05s (+ 0.34%)	15.03s	15.28s
Compiler-Unions - node (v14.15.1, x64)
Memory used	193,148k (± 0.01%)	193,139k (± 0.01%)	-10k (- 0.00%)	193,077k	193,191k
Parse Time	0.85s (± 0.47%)	0.85s (± 0.78%)	+0.00s (+ 0.47%)	0.84s	0.87s
Bind Time	0.57s (± 1.02%)	0.57s (± 0.65%)	+0.01s (+ 1.41%)	0.57s	0.58s
Check Time	6.70s (± 0.50%)	6.75s (± 0.49%)	+0.06s (+ 0.84%)	6.67s	6.83s
Emit Time	2.49s (± 0.48%)	2.50s (± 0.75%)	+0.01s (+ 0.40%)	2.46s	2.55s
Total Time	10.60s (± 0.32%)	10.68s (± 0.45%)	+0.08s (+ 0.72%)	10.56s	10.78s
Monaco - node (v14.15.1, x64)
Memory used	325,639k (± 0.01%)	325,645k (± 0.01%)	+6k (+ 0.00%)	325,611k	325,698k
Parse Time	1.58s (± 0.48%)	1.59s (± 0.47%)	+0.01s (+ 0.70%)	1.57s	1.60s
Bind Time	0.78s (± 0.67%)	0.78s (± 0.67%)	+0.00s (+ 0.00%)	0.77s	0.79s
Check Time	5.68s (± 0.36%)	5.66s (± 0.30%)	-0.02s (- 0.39%)	5.64s	5.71s
Emit Time	3.36s (± 0.72%)	3.35s (± 0.60%)	-0.01s (- 0.24%)	3.31s	3.39s
Total Time	11.41s (± 0.28%)	11.38s (± 0.26%)	-0.02s (- 0.20%)	11.32s	11.47s
TFS - node (v14.15.1, x64)
Memory used	288,811k (± 0.01%)	288,819k (± 0.01%)	+8k (+ 0.00%)	288,773k	288,867k
Parse Time	1.31s (± 1.16%)	1.35s (± 2.64%)	+0.04s (+ 2.74%)	1.30s	1.42s
Bind Time	0.78s (± 5.28%)	0.76s (± 3.50%)	🟩-0.03s (- 3.58%)	0.73s	0.86s
Check Time	5.34s (± 0.37%)	5.35s (± 0.51%)	+0.01s (+ 0.09%)	5.29s	5.39s
Emit Time	3.59s (± 2.08%)	3.62s (± 2.05%)	+0.03s (+ 0.81%)	3.47s	3.73s
Total Time	11.03s (± 0.60%)	11.07s (± 0.93%)	+0.04s (+ 0.39%)	10.86s	11.29s
material-ui - node (v14.15.1, x64)
Memory used	446,707k (± 0.01%)	446,712k (± 0.01%)	+4k (+ 0.00%)	446,638k	446,771k
Parse Time	1.87s (± 0.37%)	1.88s (± 0.52%)	+0.02s (+ 0.91%)	1.86s	1.90s
Bind Time	0.72s (± 1.17%)	0.73s (± 1.19%)	+0.01s (+ 1.25%)	0.71s	0.75s
Check Time	13.14s (± 0.55%)	13.24s (± 0.74%)	+0.10s (+ 0.73%)	13.03s	13.44s
Emit Time	0.00s (± 0.00%)	0.00s (± 0.00%)	0.00s ( NaN%)	0.00s	0.00s
Total Time	15.73s (± 0.49%)	15.85s (± 0.63%)	+0.12s (+ 0.75%)	15.62s	16.06s
xstate - node (v14.15.1, x64)
Memory used	541,283k (± 0.00%)	541,417k (± 0.00%)	+134k (+ 0.02%)	541,346k	541,489k
Parse Time	2.60s (± 0.36%)	2.62s (± 0.56%)	+0.02s (+ 0.69%)	2.59s	2.65s
Bind Time	1.15s (± 0.98%)	1.15s (± 0.96%)	+0.00s (+ 0.26%)	1.12s	1.17s
Check Time	1.54s (± 0.32%)	1.56s (± 0.44%)	+0.01s (+ 0.78%)	1.54s	1.57s
Emit Time	0.07s (± 3.14%)	0.07s (± 3.14%)	0.00s ( 0.00%)	0.07s	0.08s
Total Time	5.36s (± 0.37%)	5.39s (± 0.26%)	+0.03s (+ 0.54%)	5.36s	5.42s

System

Machine Name	ts-ci-ubuntu
Platform	linux 4.4.0-210-generic
Architecture	x64
Available Memory	16 GB
Available Memory	15 GB
CPUs	4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz

Hosts

• node (v14.15.1, x64)

Scenarios

• Angular - node (v14.15.1, x64)
• Compiler-Unions - node (v14.15.1, x64)
• Monaco - node (v14.15.1, x64)
• TFS - node (v14.15.1, x64)
• material-ui - node (v14.15.1, x64)
• xstate - node (v14.15.1, x64)

Benchmark	Name	Iterations
Current	50044	10
Baseline	main	10

Developer Information:

Download Benchmark

[Anders Hejlsberg (ahejlsberg)]

The is-object DT error from #49988 is now gone, and DT test suite is clean except for a few $ExpectType changes in lodash. It defines these two functions:

interfaceLodashStatic{isFunction(value: any): value is (...args: any[])=>any;isNative(value: any): value is (...args: any[])=>any;}

In lodash-tests.ts these are applied to the union number | (() => void). This used to produce () => void but now produces (...args: any[]) => any. Seems right given that's the asserted type and the type you already get when narrowing the singleton () => void. Pretty easy $ExpectType fix in lodash-tests.ts.

[Jake Bailey (jakebailey)]

How would one write a type guard that pulls a function out of a union of types, then? It seems unfortunate to lose that information. Is this solely because () => void is assignable to (...args: any[]) => any and vice versa?

[Anders Hejlsberg (ahejlsberg)]

How would one write a type guard that pulls a function out of a union of types, then?

isFunction(value: any): value is Function;

It's shorter, clearer, and does what you want, i.e. keeps the argument type for any function type, otherwise produces type Function. Reason being that every function type is a subtype of Function, but not vice versa.

[Daniel Rosenwasser (DanielRosenwasser)]

Why do you need the new isTypeDerivedFrom checks too?

[Anders Hejlsberg (ahejlsberg)]

The isTypeDerivedFrom check isn't new. It's just that I'm calling it directly instead of indirectly through the isRelated local. Once we know checkDerived is true, we also know that isRelated references isTypeDerivedFrom, so might as well call it directly.