Skip to content

Consistent narrowing by discriminant - #38311

Merged
Anders Hejlsberg (ahejlsberg) merged 2 commits into
masterfrom
fix36777
May 6, 2020
Merged

Consistent narrowing by discriminant#38311
Anders Hejlsberg (ahejlsberg) merged 2 commits into
masterfrom
fix36777

Conversation

@ahejlsberg

@ahejlsbergAnders Hejlsberg (ahejlsberg) commented May 3, 2020

Copy link
Copy Markdown
Member

With this PR we fix inconsistencies in our narrowing logic for discriminated unions. Previously we required the declared type of a reference to be a union type before we'd narrow by discriminants. This meant narrowing by discriminant wouldn't work in conjunction with user defined type predicate narrowing, with the notable (and inconsistent) exception of switch statements. We now consistently allow narrowing by discriminant of a reference with a declared type that isn't a union but a control flow type that is (which, for example, might result from first narrowing an unknown to a union type using a user defined type predicate and then narrowing the union type by a discriminant).

Fixes#30557.
Fixes#36777.

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

@typescript-bot

TypeScript Bot (typescript-bot) commented May 3, 2020

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented May 3, 2020

Copy link
Copy Markdown
Contributor

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

Update: The results are in!

@typescript-bot

TypeScript Bot (typescript-bot) commented May 3, 2020

Copy link
Copy Markdown
Contributor

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

@typescript-bot

TypeScript Bot (typescript-bot) commented May 3, 2020

Copy link
Copy Markdown
Contributor

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

@typescript-bot

Copy link
Copy Markdown
Contributor

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot

Copy link
Copy Markdown
Contributor

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

Here they are:

Comparison Report - master..38311

Metricmaster38311DeltaBestWorst
Angular - node (v10.16.3, x64)
Memory used328,207k (± 0.02%)327,547k (± 0.03%)-661k (- 0.20%)327,354k327,707k
Parse Time1.65s (± 0.57%)1.64s (± 0.62%)-0.01s (- 0.67%)1.61s1.66s
Bind Time0.91s (± 0.44%)0.90s (± 1.16%)-0.01s (- 1.43%)0.88s0.92s
Check Time4.83s (± 0.44%)4.81s (± 0.56%)-0.01s (- 0.23%)4.75s4.86s
Emit Time5.44s (± 0.84%)5.42s (± 0.53%)-0.01s (- 0.26%)5.37s5.52s
Total Time12.83s (± 0.42%)12.77s (± 0.32%)-0.05s (- 0.42%)12.70s12.87s
Monaco - node (v10.16.3, x64)
Memory used327,218k (± 0.02%)327,242k (± 0.02%)+24k (+ 0.01%)327,120k327,366k
Parse Time1.28s (± 0.55%)1.28s (± 0.71%)-0.00s (- 0.00%)1.26s1.30s
Bind Time0.79s (± 0.63%)0.79s (± 0.47%)0.00s ( 0.00%)0.78s0.79s
Check Time4.83s (± 0.51%)4.80s (± 0.49%)-0.03s (- 0.70%)4.75s4.84s
Emit Time2.95s (± 0.85%)2.93s (± 0.79%)-0.02s (- 0.74%)2.86s2.97s
Total Time9.85s (± 0.38%)9.79s (± 0.44%)-0.06s (- 0.57%)9.68s9.88s
TFS - node (v10.16.3, x64)
Memory used292,293k (± 0.02%)292,293k (± 0.02%)+0k (+ 0.00%)292,172k292,454k
Parse Time0.97s (± 0.67%)0.97s (± 0.78%)-0.00s (- 0.21%)0.94s0.98s
Bind Time0.75s (± 0.94%)0.76s (± 0.79%)+0.01s (+ 0.93%)0.75s0.77s
Check Time4.34s (± 0.61%)4.35s (± 0.58%)+0.01s (+ 0.21%)4.29s4.41s
Emit Time3.11s (± 0.61%)3.08s (± 1.02%)-0.03s (- 0.90%)3.00s3.13s
Total Time9.16s (± 0.39%)9.15s (± 0.51%)-0.01s (- 0.10%)9.06s9.23s
material-ui - node (v10.16.3, x64)
Memory used449,256k (± 0.01%)449,062k (± 0.01%)-195k (- 0.04%)448,923k449,162k
Parse Time1.79s (± 0.67%)1.78s (± 0.60%)-0.01s (- 0.28%)1.77s1.82s
Bind Time0.70s (± 0.88%)0.70s (± 1.00%)-0.00s (- 0.00%)0.69s0.72s
Check Time12.71s (± 1.05%)12.65s (± 0.76%)-0.06s (- 0.47%)12.45s12.82s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time15.20s (± 0.94%)15.13s (± 0.65%)-0.07s (- 0.44%)14.93s15.29s
Angular - node (v12.1.0, x64)
Memory used303,676k (± 0.01%)303,182k (± 0.02%)-494k (- 0.16%)302,966k303,294k
Parse Time1.58s (± 0.60%)1.58s (± 0.52%)0.00s ( 0.00%)1.57s1.61s
Bind Time0.89s (± 0.50%)0.89s (± 0.86%)-0.00s (- 0.34%)0.87s0.90s
Check Time4.70s (± 0.60%)4.69s (± 0.46%)-0.01s (- 0.21%)4.64s4.74s
Emit Time5.55s (± 0.68%)5.54s (± 0.58%)-0.00s (- 0.04%)5.49s5.62s
Total Time12.72s (± 0.40%)12.71s (± 0.41%)-0.01s (- 0.11%)12.59s12.84s
Monaco - node (v12.1.0, x64)
Memory used307,171k (± 0.02%)307,179k (± 0.02%)+8k (+ 0.00%)307,104k307,360k
Parse Time1.23s (± 0.83%)1.22s (± 0.48%)-0.01s (- 0.98%)1.20s1.23s
Bind Time0.76s (± 1.09%)0.76s (± 0.99%)-0.01s (- 1.05%)0.75s0.78s
Check Time4.65s (± 0.53%)4.64s (± 0.59%)-0.00s (- 0.02%)4.57s4.69s
Emit Time3.01s (± 1.49%)3.00s (± 0.79%)-0.01s (- 0.43%)2.95s3.06s
Total Time9.65s (± 0.56%)9.62s (± 0.48%)-0.04s (- 0.37%)9.49s9.71s
TFS - node (v12.1.0, x64)
Memory used274,546k (± 0.02%)274,522k (± 0.02%)-24k (- 0.01%)274,393k274,634k
Parse Time0.94s (± 1.50%)0.93s (± 0.70%)-0.01s (- 1.59%)0.92s0.94s
Bind Time0.73s (± 0.96%)0.72s (± 0.47%)-0.01s (- 1.09%)0.72s0.73s
Check Time4.27s (± 0.40%)4.24s (± 0.51%)-0.03s (- 0.63%)4.20s4.28s
Emit Time3.15s (± 0.70%)3.11s (± 1.04%)-0.04s (- 1.27%)3.06s3.21s
Total Time9.10s (± 0.31%)9.01s (± 0.51%)-0.09s (- 0.95%)8.91s9.14s
material-ui - node (v12.1.0, x64)
Memory used426,512k (± 0.07%)426,448k (± 0.01%)-64k (- 0.02%)426,355k426,552k
Parse Time1.77s (± 0.37%)1.76s (± 0.42%)-0.01s (- 0.68%)1.74s1.77s
Bind Time0.65s (± 0.92%)0.65s (± 0.92%)-0.00s (- 0.00%)0.63s0.66s
Check Time11.28s (± 1.07%)11.20s (± 0.67%)-0.08s (- 0.69%)11.09s11.43s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time13.69s (± 0.88%)13.61s (± 0.58%)-0.09s (- 0.65%)13.50s13.85s
Angular - node (v8.9.0, x64)
Memory used323,133k (± 0.02%)322,654k (± 0.02%)-478k (- 0.15%)322,561k322,774k
Parse Time2.13s (± 0.41%)2.11s (± 0.61%)-0.02s (- 0.85%)2.08s2.14s
Bind Time0.94s (± 0.85%)0.94s (± 1.14%)+0.00s (+ 0.32%)0.92s0.97s
Check Time5.50s (± 0.97%)5.44s (± 1.75%)-0.06s (- 1.13%)5.25s5.61s
Emit Time6.32s (± 0.91%)6.33s (± 1.32%)+0.01s (+ 0.19%)6.21s6.59s
Total Time14.88s (± 0.44%)14.83s (± 0.56%)-0.06s (- 0.39%)14.66s14.99s
Monaco - node (v8.9.0, x64)
Memory used325,844k (± 0.02%)325,803k (± 0.02%)-41k (- 0.01%)325,693k325,954k
Parse Time1.55s (± 0.37%)1.55s (± 0.40%)-0.00s (- 0.13%)1.54s1.57s
Bind Time0.94s (± 2.14%)0.91s (± 1.15%)-0.02s (- 2.35%)0.89s0.94s
Check Time5.43s (± 0.74%)5.42s (± 0.67%)-0.01s (- 0.15%)5.33s5.51s
Emit Time3.48s (± 0.86%)3.52s (± 0.62%)+0.03s (+ 0.98%)3.45s3.55s
Total Time11.40s (± 0.48%)11.40s (± 0.42%)+0.00s (+ 0.00%)11.25s11.48s
TFS - node (v8.9.0, x64)
Memory used291,814k (± 0.02%)291,841k (± 0.01%)+27k (+ 0.01%)291,725k291,961k
Parse Time1.26s (± 0.74%)1.26s (± 0.38%)+0.00s (+ 0.08%)1.25s1.27s
Bind Time0.76s (± 0.59%)0.76s (± 0.96%)-0.00s (- 0.00%)0.74s0.77s
Check Time5.14s (± 1.38%)4.97s (± 1.19%)🟩-0.17s (- 3.31%)4.88s5.16s
Emit Time3.20s (± 2.47%)3.32s (± 2.37%)+0.12s (+ 3.72%)3.11s3.46s
Total Time10.36s (± 0.34%)10.31s (± 0.53%)-0.05s (- 0.48%)10.20s10.45s
material-ui - node (v8.9.0, x64)
Memory used452,144k (± 0.01%)451,882k (± 0.01%)-261k (- 0.06%)451,803k452,035k
Parse Time2.13s (± 0.58%)2.13s (± 0.54%)+0.00s (+ 0.19%)2.10s2.15s
Bind Time0.83s (± 1.08%)0.82s (± 0.70%)-0.00s (- 0.36%)0.81s0.84s
Check Time16.75s (± 0.88%)16.79s (± 1.25%)+0.04s (+ 0.23%)16.38s17.27s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time19.71s (± 0.71%)19.74s (± 1.07%)+0.03s (+ 0.17%)19.32s20.23s
Angular - node (v8.9.0, x86)
Memory used186,102k (± 0.03%)185,870k (± 0.02%)-231k (- 0.12%)185,793k185,964k
Parse Time2.07s (± 1.13%)2.05s (± 0.36%)-0.02s (- 1.06%)2.04s2.07s
Bind Time1.09s (± 0.51%)1.08s (± 0.51%)-0.01s (- 0.55%)1.07s1.09s
Check Time5.02s (± 0.61%)5.04s (± 0.43%)+0.02s (+ 0.36%)4.99s5.08s
Emit Time6.14s (± 1.25%)6.09s (± 0.66%)-0.06s (- 0.93%)5.98s6.18s
Total Time14.32s (± 0.66%)14.26s (± 0.31%)-0.06s (- 0.43%)14.17s14.35s
Monaco - node (v8.9.0, x86)
Memory used185,618k (± 0.02%)185,643k (± 0.02%)+25k (+ 0.01%)185,593k185,721k
Parse Time1.61s (± 0.77%)1.60s (± 0.62%)-0.01s (- 0.62%)1.58s1.62s
Bind Time0.78s (± 0.99%)0.78s (± 0.77%)-0.01s (- 0.77%)0.76s0.79s
Check Time5.46s (± 0.44%)5.47s (± 0.76%)+0.01s (+ 0.11%)5.40s5.58s
Emit Time2.89s (± 0.68%)2.89s (± 0.97%)+0.01s (+ 0.21%)2.85s2.96s
Total Time10.73s (± 0.26%)10.73s (± 0.48%)-0.00s (- 0.03%)10.63s10.82s
TFS - node (v8.9.0, x86)
Memory used167,174k (± 0.01%)167,146k (± 0.01%)-28k (- 0.02%)167,086k167,207k
Parse Time1.30s (± 0.53%)1.29s (± 0.51%)-0.01s (- 0.39%)1.28s1.31s
Bind Time0.72s (± 1.23%)0.72s (± 1.01%)-0.01s (- 0.83%)0.70s0.74s
Check Time4.71s (± 0.53%)4.70s (± 0.72%)-0.01s (- 0.13%)4.64s4.79s
Emit Time3.07s (± 3.39%)3.03s (± 1.85%)-0.04s (- 1.21%)2.93s3.21s
Total Time9.79s (± 0.99%)9.74s (± 0.79%)-0.06s (- 0.57%)9.64s9.96s
material-ui - node (v8.9.0, x86)
Memory used256,117k (± 0.02%)255,969k (± 0.02%)-148k (- 0.06%)255,874k256,134k
Parse Time2.20s (± 0.59%)2.18s (± 0.65%)-0.01s (- 0.64%)2.16s2.21s
Bind Time0.70s (± 1.57%)0.69s (± 0.86%)-0.01s (- 1.14%)0.68s0.71s
Check Time15.32s (± 1.08%)15.26s (± 0.64%)-0.06s (- 0.38%)15.03s15.46s
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)0.00s ( NaN%)0.00s0.00s
Total Time18.22s (± 0.96%)18.14s (± 0.55%)-0.07s (- 0.40%)17.91s18.36s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
BenchmarkNameIterations
Current3831110
Baselinemaster10

@ahejlsberg

Copy link
Copy Markdown
MemberAuthor

Tests all look good (nothing but preexisting conditions).

@weswigham

Wesley Wigham (weswigham) commented May 4, 2020

Copy link
Copy Markdown
Member

Does this obsolete #33434 ? #30593?

@jack-williams

Copy link
Copy Markdown
Collaborator

Wesley Wigham (@weswigham) I think it pretty much replaces the second: this is so much simpler. I don’t believe this is quite as general as the other PR aims to be, but I went through all the duplicates on the original issue and this fixes them.

I think my only comment here is whether it’s worth filter the declared type for non-object types. I’m not sure this approach works if the declared type is something like object | undefined.

The first PR you link was some tidying I haven’t updated yet (sorry). I don’t think it’s changed by this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discriminate union inference inconsistent behavior with if/else checks Unknown types narrowed to a union won't narrow further

5 participants

@ahejlsberg@typescript-bot@weswigham@jack-williams@RyanCavanaugh