Skip to content

more precise type facts for intersection - #47282

Closed
Gabriela Araujo Britto (gabritto) wants to merge 5 commits into
mainfrom
gabritto/issue45801
Closed

more precise type facts for intersection#47282
Gabriela Araujo Britto (gabritto) wants to merge 5 commits into
mainfrom
gabritto/issue45801

Conversation

@gabritto

@gabrittoGabriela Araujo Britto (gabritto) commented Dec 31, 2021

Copy link
Copy Markdown
Member

Fixes#45801

Brief background

When narrowing a type, we rely on TypeFacts flags that tell us what narrowing facts could be true for a value of a given type. The flags cover the possible results of narrowing using typeof (e.g. TypeFacts.TypeofEQString), comparsions to null or undefined (e.g. TypeFacts.UndefinedOrNull means if (v === null || v === undefined) could be true), or if the value is truthy or falsy (e.g. TypeFacts.Truthy means if (v) { ... } could be true).
To compute the type facts for a type T, we call getTypeFacts(T).

The issue

Up until v4.2, when computing type facts for an intersection type, we ored the facts of each of the intersection type's component type. e.g. getTypeFacts(A & B) = getTypeFacts(A) | getTypeFacts(B).
Starting at v4.3, this changed to an and of the facts of each intersection type's component type, e.g. getTypeFacts(A & B) = getTypeFacts(A) & getTypeFacts(B), to fix cases like the following:

functionf3<T>(x: T&string|T&number){if(typeofx==="string"){x;// Should narrow to T & string// <= 4.2: x: T & string | T & number}else{x;// Should narrow to T & number// <= 4.2: x: T & string | T & number}}

If we have T & number, we know the value can only be of type number (or null or undefined, in non-strict), never of type string. However, we don't know anything about T, so when computing type facts for T, we think it can be anything at all (including of type string). If we or the type facts for T and number, then, we get that T & number could be string. If we and the type facts, we know T & number can't be of type string, because number can't be of type string.

However, in some cases we don't want an and of the type facts. Consider the example from issue #45801:

typeMeta={foo: string}interfaceF{(): string}constx=(a: (F&Meta)|string)=>{if(typeofa==="function"){// ts.version >= 4.3.5: never -- unexpected// ts.version <= 4.2.3: F & Meta -- expecteda;}else{// ts.version >= 4.3.5: string -- expected// ts.version <= 4.2.3: string | (F & Meta) -- unexpecteda;}}

For type F & Meta, we know values of F have to be functions, since F has a call signature declared. But type Meta doesn't, it is a regular object. If we and those facts, we conclude F & Meta cannot be a function, because we don't think values of Meta could be functions (no call signature). So in this case, we probably want to or the type facts.

(Note: We could include TypeFacts.TypeofEQFunction in the flags returned for object types such as Meta above, and keep using and for computing type facts of intersection types. I think we don't want to do that, since we explicitly make that distinction in getTypeFacts's code and it generally seems a useful distinction.)

The possible fix

Considering both examples above, it seems we sometimes know a fact will always be true (i.e. true for all values of a type), e.g. values of type T & number can't be strings (if (typeof v === 'string') can't be true). This contrasts with the other uses of type facts, which are facts that can be true for values of type T, but are not necessarily always true/true for all values.
So that's why sometimes we want to use an and to compute facts of intersection types, and sometimes we want to use an or.

tl;dr

This PR changes the way we compute type facts of intersection types. For each component type, in addition to computing its type facts, we also compute which facts are always true (true for all values of that type) and which are always false.
We then or the regular type facts of the component types, and we also or the always false facts. We then exclude the facts that are always false from the regular facts.
So, in cases like T & number, even though when we or the components' type facts, we include a flag such as TypeFacts.TypeofEQString (because that's one of the flags for T), we will delete it later because TypeFacts.TypeofEQString is always false for type number.

Questions I had

  • When is TypeFacts.TypeofEQHostObject always false? It seems typeof v === "xxx" ("xxx" being something other than the usual strings returned by typeof) can only be true if v is a host object (and hosts return a custom typeof for those), or if we don't yet recognize the "xxx" in question (e.g. if there's ever a new Fraction type and typeof v === "fraction"). So it should always be false for known primitives at least.
  • Should we care about contradictory types (e.g. string & number) in getTypeFacts or getAlwaysFalseTypeFacts? I'm assuming we shouldn't, because existing code for getTypeFacts doesn't address this, and this looks like something getIntersectionType takes care of.

@gabritto
Gabriela Araujo Britto (gabritto) marked this pull request as ready for review January 4, 2022 20:12
@andrewbranch

Copy link
Copy Markdown
Member

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the extended test suite on this PR at 2489916. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the perf test suite on this PR at 2489916. You can monitor the build here.

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the parallelized Definitely Typed test suite on this PR at 2489916. You can monitor the build here.

@typescript-bot

TypeScript Bot (typescript-bot) commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

Heya Andrew Branch (@andrewbranch), I've started to run the inline community code test suite on this PR at 2489916. You can monitor the build here.

Update: The results are in!

@typescript-bot

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch)
Great news! no new errors were found between main..refs/pull/47282/merge

@typescript-bot

Copy link
Copy Markdown
Contributor

Andrew Branch (@andrewbranch)
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..47282

Metricmain47282DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used355,048k (± 0.02%)355,060k (± 0.03%)+12k (+ 0.00%)354,804k355,339k
Parse Time1.95s (± 0.61%)1.95s (± 0.38%)-0.00s (- 0.10%)1.93s1.96s
Bind Time0.84s (± 0.73%)0.84s (± 0.35%)-0.00s (- 0.36%)0.83s0.84s
Check Time5.55s (± 0.37%)5.54s (± 0.37%)-0.02s (- 0.34%)5.48s5.58s
Emit Time5.90s (± 0.70%)5.90s (± 0.62%)-0.00s (- 0.07%)5.85s6.01s
Total Time14.25s (± 0.36%)14.22s (± 0.39%)-0.03s (- 0.19%)14.11s14.39s
Compiler-Unions - node (v10.16.3, x64)
Memory used204,146k (± 0.02%)204,305k (± 0.03%)+160k (+ 0.08%)204,221k204,494k
Parse Time0.79s (± 0.85%)0.79s (± 0.78%)+0.00s (+ 0.51%)0.78s0.80s
Bind Time0.52s (± 1.25%)0.52s (± 1.49%)+0.00s (+ 0.58%)0.51s0.54s
Check Time7.87s (± 0.53%)7.89s (± 0.45%)+0.02s (+ 0.23%)7.82s8.01s
Emit Time2.47s (± 0.69%)2.48s (± 0.60%)+0.01s (+ 0.53%)2.45s2.51s
Total Time11.64s (± 0.39%)11.68s (± 0.36%)+0.04s (+ 0.36%)11.58s11.82s
Monaco - node (v10.16.3, x64)
Memory used342,515k (± 0.02%)342,595k (± 0.02%)+80k (+ 0.02%)342,486k342,729k
Parse Time1.48s (± 0.42%)1.48s (± 0.55%)+0.00s (+ 0.07%)1.46s1.50s
Bind Time0.75s (± 0.78%)0.75s (± 0.80%)-0.00s (- 0.13%)0.74s0.76s
Check Time5.50s (± 0.51%)5.53s (± 0.49%)+0.03s (+ 0.45%)5.45s5.58s
Emit Time3.22s (± 1.31%)3.22s (± 0.79%)0.00s ( 0.00%)3.18s3.30s
Total Time10.95s (± 0.39%)10.98s (± 0.25%)+0.02s (+ 0.22%)10.90s11.02s
TFS - node (v10.16.3, x64)
Memory used305,631k (± 0.01%)305,632k (± 0.02%)+1k (+ 0.00%)305,431k305,751k
Parse Time1.20s (± 0.57%)1.19s (± 0.50%)-0.00s (- 0.08%)1.18s1.21s
Bind Time0.71s (± 0.84%)0.71s (± 0.66%)+0.00s (+ 0.57%)0.70s0.72s
Check Time5.03s (± 0.53%)5.06s (± 0.47%)+0.02s (+ 0.48%)4.99s5.11s
Emit Time3.34s (± 1.57%)3.40s (± 1.79%)+0.06s (+ 1.71%)3.31s3.58s
Total Time10.27s (± 0.64%)10.36s (± 0.77%)+0.08s (+ 0.82%)10.20s10.60s
material-ui - node (v10.16.3, x64)
Memory used471,429k (± 0.02%)471,484k (± 0.01%)+55k (+ 0.01%)471,340k471,632k
Parse Time1.77s (± 0.43%)1.77s (± 0.37%)-0.00s (- 0.17%)1.76s1.79s
Bind Time0.66s (± 0.79%)0.66s (± 0.88%)-0.00s (- 0.30%)0.65s0.67s
Check Time14.22s (± 0.57%)14.24s (± 0.61%)+0.02s (+ 0.12%)14.08s14.48s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time16.65s (± 0.51%)16.67s (± 0.55%)+0.01s (+ 0.09%)16.49s16.93s
xstate - node (v10.16.3, x64)
Memory used569,251k (± 0.02%)572,922k (± 1.39%)+3,671k (+ 0.64%)569,102k605,017k
Parse Time2.56s (± 0.28%)2.55s (± 0.40%)-0.01s (- 0.27%)2.53s2.57s
Bind Time1.01s (± 0.29%)1.00s (± 0.37%)-0.00s (- 0.30%)1.00s1.01s
Check Time1.48s (± 0.61%)1.50s (± 0.90%)+0.01s (+ 0.95%)1.47s1.53s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.11s (± 0.24%)5.11s (± 0.33%)+0.00s (+ 0.02%)5.08s5.16s
Angular - node (v12.1.0, x64)
Memory used332,718k (± 0.09%)332,914k (± 0.03%)+196k (+ 0.06%)332,716k333,057k
Parse Time1.94s (± 0.75%)1.96s (± 0.78%)+0.01s (+ 0.67%)1.93s1.99s
Bind Time0.82s (± 1.10%)0.82s (± 0.58%)-0.00s (- 0.49%)0.81s0.83s
Check Time5.36s (± 0.61%)5.37s (± 0.48%)+0.01s (+ 0.22%)5.33s5.46s
Emit Time6.17s (± 1.28%)6.17s (± 1.34%)+0.00s (+ 0.02%)6.08s6.49s
Total Time14.29s (± 0.74%)14.31s (± 0.56%)+0.02s (+ 0.13%)14.19s14.59s
Compiler-Unions - node (v12.1.0, x64)
Memory used191,687k (± 0.03%)191,734k (± 0.08%)+46k (+ 0.02%)191,165k191,922k
Parse Time0.78s (± 0.79%)0.79s (± 0.76%)+0.01s (+ 0.77%)0.77s0.80s
Bind Time0.53s (± 0.89%)0.53s (± 0.89%)0.00s ( 0.00%)0.52s0.54s
Check Time7.33s (± 0.61%)7.31s (± 0.37%)-0.02s (- 0.30%)7.26s7.38s
Emit Time2.47s (± 0.47%)2.50s (± 0.83%)+0.02s (+ 0.93%)2.47s2.56s
Total Time11.11s (± 0.42%)11.12s (± 0.30%)+0.01s (+ 0.07%)11.04s11.20s
Monaco - node (v12.1.0, x64)
Memory used325,567k (± 0.02%)325,648k (± 0.03%)+81k (+ 0.02%)325,427k325,797k
Parse Time1.47s (± 0.79%)1.47s (± 0.64%)0.00s ( 0.00%)1.45s1.49s
Bind Time0.73s (± 0.61%)0.73s (± 0.55%)-0.00s (- 0.41%)0.72s0.74s
Check Time5.38s (± 0.35%)5.39s (± 0.55%)+0.00s (+ 0.09%)5.32s5.45s
Emit Time3.23s (± 0.70%)3.25s (± 0.62%)+0.02s (+ 0.62%)3.22s3.31s
Total Time10.81s (± 0.32%)10.83s (± 0.35%)+0.02s (+ 0.17%)10.74s10.91s
TFS - node (v12.1.0, x64)
Memory used290,340k (± 0.02%)290,343k (± 0.01%)+3k (+ 0.00%)290,232k290,433k
Parse Time1.21s (± 0.61%)1.22s (± 0.53%)+0.01s (+ 0.66%)1.20s1.23s
Bind Time0.70s (± 1.19%)0.69s (± 0.65%)-0.01s (- 1.15%)0.68s0.70s
Check Time4.96s (± 0.30%)4.94s (± 0.59%)-0.02s (- 0.40%)4.87s5.00s
Emit Time3.43s (± 0.63%)3.40s (± 1.16%)-0.03s (- 0.84%)3.32s3.51s
Total Time10.30s (± 0.27%)10.25s (± 0.62%)-0.05s (- 0.46%)10.10s10.43s
material-ui - node (v12.1.0, x64)
Memory used450,125k (± 0.06%)450,258k (± 0.02%)+133k (+ 0.03%)450,149k450,451k
Parse Time1.78s (± 0.39%)1.77s (± 0.42%)-0.01s (- 0.62%)1.76s1.79s
Bind Time0.64s (± 0.73%)0.64s (± 0.81%)-0.00s (- 0.16%)0.63s0.65s
Check Time12.69s (± 0.52%)12.71s (± 0.27%)+0.02s (+ 0.16%)12.63s12.76s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.11s (± 0.43%)15.12s (± 0.22%)+0.01s (+ 0.08%)15.04s15.17s
xstate - node (v12.1.0, x64)
Memory used542,181k (± 1.90%)538,809k (± 1.44%)-3,372k (- 0.62%)535,116k570,145k
Parse Time2.49s (± 0.51%)2.49s (± 0.52%)+0.00s (+ 0.12%)2.48s2.53s
Bind Time1.03s (± 0.84%)1.04s (± 0.93%)+0.01s (+ 0.68%)1.02s1.06s
Check Time1.43s (± 0.65%)1.43s (± 0.67%)-0.00s (- 0.14%)1.41s1.45s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.02s (± 0.29%)5.03s (± 0.44%)+0.00s (+ 0.06%)4.99s5.09s
Angular - node (v14.15.1, x64)
Memory used331,252k (± 0.01%)331,291k (± 0.01%)+39k (+ 0.01%)331,250k331,362k
Parse Time1.95s (± 0.59%)1.94s (± 0.39%)-0.01s (- 0.56%)1.93s1.97s
Bind Time0.86s (± 0.75%)0.86s (± 0.47%)-0.00s (- 0.35%)0.85s0.87s
Check Time5.41s (± 0.45%)5.39s (± 0.31%)-0.02s (- 0.37%)5.35s5.42s
Emit Time6.16s (± 0.40%)6.16s (± 0.63%)+0.00s (+ 0.03%)6.10s6.26s
Total Time14.39s (± 0.24%)14.36s (± 0.33%)-0.03s (- 0.20%)14.28s14.49s
Compiler-Unions - node (v14.15.1, x64)
Memory used192,219k (± 0.62%)192,600k (± 0.58%)+382k (+ 0.20%)190,254k193,661k
Parse Time0.81s (± 1.09%)0.81s (± 0.96%)+0.00s (+ 0.50%)0.80s0.83s
Bind Time0.55s (± 0.73%)0.55s (± 0.66%)+0.00s (+ 0.54%)0.55s0.56s
Check Time7.42s (± 0.62%)7.42s (± 0.38%)0.00s ( 0.00%)7.37s7.50s
Emit Time2.47s (± 0.79%)2.48s (± 0.94%)+0.01s (+ 0.32%)2.44s2.56s
Total Time11.26s (± 0.43%)11.27s (± 0.24%)+0.01s (+ 0.11%)11.21s11.34s
Monaco - node (v14.15.1, x64)
Memory used324,483k (± 0.00%)324,512k (± 0.00%)+29k (+ 0.01%)324,470k324,547k
Parse Time1.50s (± 0.40%)1.52s (± 0.82%)+0.01s (+ 0.80%)1.50s1.55s
Bind Time0.76s (± 0.68%)0.76s (± 0.85%)-0.00s (- 0.13%)0.75s0.78s
Check Time5.37s (± 0.43%)5.37s (± 0.50%)-0.00s (- 0.04%)5.30s5.42s
Emit Time3.26s (± 0.67%)3.27s (± 0.44%)+0.01s (+ 0.34%)3.24s3.30s
Total Time10.89s (± 0.36%)10.91s (± 0.35%)+0.02s (+ 0.20%)10.83s10.98s
TFS - node (v14.15.1, x64)
Memory used289,246k (± 0.01%)289,298k (± 0.01%)+53k (+ 0.02%)289,258k289,333k
Parse Time1.23s (± 0.74%)1.23s (± 0.41%)-0.00s (- 0.33%)1.21s1.23s
Bind Time0.73s (± 0.84%)0.73s (± 0.61%)-0.00s (- 0.41%)0.72s0.74s
Check Time4.98s (± 0.60%)4.97s (± 0.60%)-0.01s (- 0.14%)4.89s5.03s
Emit Time3.52s (± 0.53%)3.51s (± 0.62%)-0.01s (- 0.34%)3.47s3.57s
Total Time10.46s (± 0.35%)10.44s (± 0.41%)-0.03s (- 0.25%)10.33s10.52s
material-ui - node (v14.15.1, x64)
Memory used448,434k (± 0.00%)448,370k (± 0.06%)-64k (- 0.01%)447,366k448,539k
Parse Time1.84s (± 0.82%)1.83s (± 0.44%)-0.01s (- 0.49%)1.81s1.84s
Bind Time0.67s (± 0.54%)0.68s (± 0.70%)+0.00s (+ 0.45%)0.67s0.69s
Check Time12.87s (± 0.28%)12.85s (± 0.34%)-0.02s (- 0.16%)12.75s12.97s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.38s (± 0.28%)15.35s (± 0.28%)-0.03s (- 0.18%)15.27s15.47s
xstate - node (v14.15.1, x64)
Memory used532,909k (± 0.00%)533,039k (± 0.00%)+130k (+ 0.02%)532,999k533,081k
Parse Time2.55s (± 0.32%)2.56s (± 0.42%)+0.01s (+ 0.35%)2.54s2.58s
Bind Time1.15s (± 0.63%)1.15s (± 1.10%)+0.00s (+ 0.26%)1.13s1.19s
Check Time1.47s (± 0.46%)1.48s (± 0.40%)+0.00s (+ 0.14%)1.47s1.49s
Emit Time0.07s (± 0.00%)0.07s (± 0.00%)0.00s ( 0.00%)0.07s0.07s
Total Time5.25s (± 0.20%)5.26s (± 0.40%)+0.01s (+ 0.19%)5.21s5.30s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory8 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v10.16.3, x64)
  • Compiler-Unions - node (v12.1.0, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v10.16.3, x64)
  • xstate - node (v12.1.0, x64)
  • xstate - node (v14.15.1, x64)
BenchmarkNameIterations
Current4728210
Baselinemain10

Developer Information:

Download Benchmark

if (flags & TypeFlags.Object && !ignoreObjects) {
if (flags & TypeFlags.Object) {
if (ignoreObjects) {
return TypeFacts.None;

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ignoreObjects is only going to be true if we are in the middle of computing type facts for an intersection type. In this case, the neutral value returned by getTypeFacts can't be TypeFacts.All (as it was before), because we are oring the type facts. So it should be TypeFacts.None.

Comment threadsrc/compiler/checker.ts Outdated
Comment threadsrc/compiler/checker.ts Outdated
@gabritto
Gabriela Araujo Britto (gabritto) marked this pull request as ready for review January 12, 2022 18:23
@ahejlsberg

Copy link
Copy Markdown
Member

I'm definitely in favor of fixing this issue, but I'm not crazy about adding 175 lines of code to do it. Couldn't we just say that certain type facts for intersections are computed as and and others are computed as or, but otherwise use the existing single function? For example, TypeFacts.TypeofEQString would be an and whereas TypeFacts.TypeofEQFunction would be an or.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Type narrowing based on function typeof is broken when function is intersected with a record

6 participants

@gabritto@andrewbranch@typescript-bot@ahejlsberg@RyanCavanaugh@sandersn