Skip to content

Fix narrowing of intersection with function type #47483

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 21, 2022
Merged

Fix narrowing of intersection with function type #47483

merged 2 commits into from
Jan 21, 2022

Conversation

gabritto
Copy link
Member

Fixes #45801.

Second attempt at a fix, first is in #47282 (along with some explanation of the issue).

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Jan 17, 2022
@DanielRosenwasser
Copy link
Member

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at e17ca56. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at e17ca56. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at e17ca56. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 21, 2022 •
edited
Loading

Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at e17ca56. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

@DanielRosenwasser
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..47483

Metric main 47483 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 355,936k (± 0.02%) 355,977k (± 0.02%) +41k (+ 0.01%) 355,827k 356,134k
Parse Time 1.97s (± 0.48%) 1.98s (± 0.48%) +0.01s (+ 0.56%) 1.96s 1.99s
Bind Time 0.86s (± 1.02%) 0.87s (± 1.20%) +0.00s (+ 0.58%) 0.85s 0.90s
Check Time 5.57s (± 0.52%) 5.59s (± 0.44%) +0.02s (+ 0.38%) 5.53s 5.62s
Emit Time 5.93s (± 0.58%) 5.92s (± 0.59%) -0.01s (- 0.15%) 5.84s 6.00s
Total Time 14.32s (± 0.48%) 14.35s (± 0.33%) +0.03s (+ 0.17%) 14.23s 14.43s
Compiler-Unions - node (v10.16.3, x64)
Memory used 204,215k (± 0.02%) 204,224k (± 0.03%) +9k (+ 0.00%) 204,095k 204,323k
Parse Time 0.79s (± 1.11%) 0.79s (± 0.66%) -0.00s (- 0.25%) 0.78s 0.80s
Bind Time 0.53s (± 1.32%) 0.53s (± 1.32%) +0.00s (+ 0.38%) 0.52s 0.55s
Check Time 7.86s (± 0.46%) 7.91s (± 0.42%) +0.04s (+ 0.56%) 7.84s 7.98s
Emit Time 2.46s (± 0.56%) 2.47s (± 0.78%) +0.01s (+ 0.45%) 2.42s 2.51s
Total Time 11.65s (± 0.37%) 11.70s (± 0.35%) +0.05s (+ 0.45%) 11.59s 11.78s
Monaco - node (v10.16.3, x64)
Memory used 342,654k (± 0.01%) 342,561k (± 0.02%) -93k (- 0.03%) 342,351k 342,656k
Parse Time 1.49s (± 0.41%) 1.50s (± 0.62%) +0.01s (+ 0.47%) 1.48s 1.52s
Bind Time 0.76s (± 0.45%) 0.76s (± 0.68%) +0.00s (+ 0.26%) 0.75s 0.77s
Check Time 5.55s (± 0.74%) 5.56s (± 1.03%) +0.01s (+ 0.11%) 5.45s 5.69s
Emit Time 3.20s (± 1.01%) 3.24s (± 1.09%) +0.03s (+ 1.00%) 3.17s 3.35s
Total Time 11.00s (± 0.41%) 11.05s (± 0.64%) +0.05s (+ 0.46%) 10.92s 11.22s
TFS - node (v10.16.3, x64)
Memory used 305,702k (± 0.02%) 305,698k (± 0.04%) -5k (- 0.00%) 305,460k 306,045k
Parse Time 1.20s (± 0.77%) 1.21s (± 0.96%) +0.01s (+ 0.42%) 1.18s 1.24s
Bind Time 0.71s (± 0.96%) 0.72s (± 0.47%) +0.01s (+ 1.12%) 0.72s 0.73s
Check Time 5.04s (± 0.74%) 5.06s (± 0.49%) +0.02s (+ 0.46%) 5.02s 5.12s
Emit Time 3.38s (± 0.91%) 3.39s (± 1.18%) +0.01s (+ 0.30%) 3.28s 3.46s
Total Time 10.34s (± 0.45%) 10.38s (± 0.53%) +0.05s (+ 0.46%) 10.23s 10.50s
material-ui - node (v10.16.3, x64)
Memory used 471,728k (± 0.01%) 471,707k (± 0.02%) -21k (- 0.00%) 471,515k 471,885k
Parse Time 1.78s (± 0.53%) 1.78s (± 0.42%) +0.00s (+ 0.06%) 1.77s 1.80s
Bind Time 0.67s (± 1.43%) 0.67s (± 0.90%) 0.00s ( 0.00%) 0.66s 0.68s
Check Time 14.25s (± 0.52%) 14.36s (± 0.52%) +0.11s (+ 0.79%) 14.19s 14.50s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 16.70s (± 0.46%) 16.81s (± 0.47%) +0.11s (+ 0.67%) 16.62s 16.94s
xstate - node (v10.16.3, x64)
Memory used 577,149k (± 1.84%) 570,027k (± 0.02%) -7,122k (- 1.23%) 569,711k 570,240k
Parse Time 2.56s (± 0.37%) 2.57s (± 0.48%) +0.01s (+ 0.27%) 2.54s 2.59s
Bind Time 1.01s (± 0.36%) 1.02s (± 0.66%) +0.01s (+ 0.99%) 1.01s 1.04s
Check Time 1.51s (± 0.63%) 1.51s (± 0.51%) +0.00s (+ 0.27%) 1.49s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.14s (± 0.30%) 5.16s (± 0.38%) +0.02s (+ 0.47%) 5.11s 5.21s
Angular - node (v12.1.0, x64)
Memory used 333,741k (± 0.02%) 333,722k (± 0.02%) -20k (- 0.01%) 333,630k 333,899k
Parse Time 1.95s (± 0.65%) 1.97s (± 0.59%) +0.02s (+ 1.03%) 1.95s 2.00s
Bind Time 0.84s (± 0.59%) 0.85s (± 0.79%) +0.00s (+ 0.24%) 0.83s 0.86s
Check Time 5.38s (± 0.41%) 5.38s (± 0.61%) -0.01s (- 0.17%) 5.31s 5.45s
Emit Time 6.16s (± 0.56%) 6.22s (± 0.34%) +0.05s (+ 0.86%) 6.16s 6.26s
Total Time 14.35s (± 0.28%) 14.41s (± 0.23%) +0.06s (+ 0.43%) 14.36s 14.49s
Compiler-Unions - node (v12.1.0, x64)
Memory used 191,739k (± 0.07%) 191,749k (± 0.02%) +10k (+ 0.01%) 191,676k 191,854k
Parse Time 0.78s (± 0.79%) 0.78s (± 0.74%) +0.00s (+ 0.13%) 0.77s 0.79s
Bind Time 0.53s (± 0.63%) 0.54s (± 1.08%) +0.01s (+ 1.32%) 0.53s 0.55s
Check Time 7.31s (± 0.56%) 7.40s (± 0.56%) +0.09s (+ 1.27%) 7.33s 7.54s
Emit Time 2.49s (± 0.99%) 2.49s (± 0.82%) +0.00s (+ 0.04%) 2.45s 2.54s
Total Time 11.11s (± 0.44%) 11.22s (± 0.52%) +0.11s (+ 0.95%) 11.12s 11.40s
Monaco - node (v12.1.0, x64)
Memory used 325,619k (± 0.03%) 325,565k (± 0.02%) -54k (- 0.02%) 325,454k 325,712k
Parse Time 1.48s (± 0.46%) 1.49s (± 0.90%) +0.01s (+ 0.75%) 1.44s 1.51s
Bind Time 0.74s (± 0.63%) 0.74s (± 1.08%) +0.00s (+ 0.27%) 0.73s 0.76s
Check Time 5.40s (± 0.48%) 5.44s (± 0.44%) +0.03s (+ 0.59%) 5.37s 5.48s
Emit Time 3.22s (± 0.52%) 3.24s (± 0.69%) +0.02s (+ 0.78%) 3.18s 3.29s
Total Time 10.83s (± 0.36%) 10.91s (± 0.35%) +0.07s (+ 0.66%) 10.82s 11.01s
TFS - node (v12.1.0, x64)
Memory used 290,345k (± 0.02%) 290,333k (± 0.02%) -12k (- 0.00%) 290,230k 290,444k
Parse Time 1.21s (± 0.71%) 1.22s (± 0.68%) +0.01s (+ 0.99%) 1.20s 1.24s
Bind Time 0.70s (± 0.71%) 0.70s (± 1.06%) +0.01s (+ 1.01%) 0.69s 0.72s
Check Time 4.99s (± 0.54%) 4.99s (± 0.39%) +0.00s (+ 0.10%) 4.94s 5.02s
Emit Time 3.42s (± 0.68%) 3.48s (± 0.90%) +0.05s (+ 1.55%) 3.40s 3.54s
Total Time 10.32s (± 0.29%) 10.39s (± 0.40%) +0.08s (+ 0.74%) 10.30s 10.48s
material-ui - node (v12.1.0, x64)
Memory used 450,445k (± 0.01%) 450,343k (± 0.05%) -102k (- 0.02%) 449,413k 450,546k
Parse Time 1.78s (± 0.37%) 1.79s (± 0.98%) +0.01s (+ 0.67%) 1.77s 1.84s
Bind Time 0.64s (± 0.53%) 0.65s (± 0.80%) +0.01s (+ 1.24%) 0.64s 0.66s
Check Time 12.81s (± 0.81%) 12.87s (± 0.55%) +0.06s (+ 0.44%) 12.71s 13.02s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.24s (± 0.68%) 15.31s (± 0.53%) +0.08s (+ 0.50%) 15.14s 15.51s
xstate - node (v12.1.0, x64)
Memory used 539,525k (± 1.44%) 539,516k (± 1.43%) -9k (- 0.00%) 535,911k 570,641k
Parse Time 2.49s (± 0.64%) 2.51s (± 0.45%) +0.02s (+ 0.76%) 2.49s 2.54s
Bind Time 1.05s (± 0.67%) 1.06s (± 1.06%) +0.00s (+ 0.38%) 1.03s 1.08s
Check Time 1.44s (± 0.89%) 1.44s (± 0.65%) -0.00s (- 0.21%) 1.41s 1.45s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.06s (± 0.42%) 5.07s (± 0.50%) +0.02s (+ 0.32%) 5.00s 5.12s
Angular - node (v14.15.1, x64)
Memory used 332,065k (± 0.00%) 332,074k (± 0.00%) +9k (+ 0.00%) 332,049k 332,098k
Parse Time 1.96s (± 0.56%) 1.96s (± 0.82%) +0.00s (+ 0.05%) 1.93s 2.00s
Bind Time 0.88s (± 0.88%) 0.88s (± 0.80%) +0.00s (+ 0.00%) 0.87s 0.90s
Check Time 5.43s (± 0.48%) 5.42s (± 0.44%) -0.00s (- 0.06%) 5.39s 5.48s
Emit Time 6.22s (± 0.67%) 6.23s (± 1.44%) +0.02s (+ 0.27%) 6.12s 6.51s
Total Time 14.48s (± 0.43%) 14.50s (± 0.70%) +0.02s (+ 0.11%) 14.36s 14.83s
Compiler-Unions - node (v14.15.1, x64)
Memory used 193,280k (± 0.37%) 192,236k (± 0.59%) -1,044k (- 0.54%) 190,377k 193,672k
Parse Time 0.81s (± 0.71%) 0.81s (± 0.49%) -0.00s (- 0.12%) 0.80s 0.82s
Bind Time 0.56s (± 0.60%) 0.56s (± 0.84%) 0.00s ( 0.00%) 0.55s 0.57s
Check Time 7.46s (± 0.66%) 7.50s (± 0.62%) +0.04s (+ 0.59%) 7.42s 7.64s
Emit Time 2.47s (± 0.34%) 2.48s (± 0.50%) +0.02s (+ 0.69%) 2.45s 2.51s
Total Time 11.29s (± 0.46%) 11.36s (± 0.47%) +0.06s (+ 0.54%) 11.26s 11.51s
Monaco - node (v14.15.1, x64)
Memory used 324,443k (± 0.00%) 324,438k (± 0.00%) -4k (- 0.00%) 324,411k 324,476k
Parse Time 1.51s (± 0.51%) 1.52s (± 0.86%) +0.01s (+ 0.66%) 1.49s 1.55s
Bind Time 0.77s (± 0.75%) 0.77s (± 0.43%) -0.00s (- 0.39%) 0.76s 0.78s
Check Time 5.36s (± 0.85%) 5.38s (± 0.30%) +0.03s (+ 0.49%) 5.35s 5.41s
Emit Time 3.28s (± 0.75%) 3.27s (± 0.93%) -0.01s (- 0.24%) 3.21s 3.32s
Total Time 10.92s (± 0.56%) 10.94s (± 0.37%) +0.02s (+ 0.22%) 10.86s 11.01s
TFS - node (v14.15.1, x64)
Memory used 289,220k (± 0.00%) 289,223k (± 0.01%) +3k (+ 0.00%) 289,177k 289,285k
Parse Time 1.24s (± 0.90%) 1.23s (± 0.72%) -0.00s (- 0.16%) 1.22s 1.25s
Bind Time 0.75s (± 1.19%) 0.75s (± 0.49%) -0.00s (- 0.00%) 0.74s 0.75s
Check Time 4.97s (± 0.46%) 4.99s (± 0.68%) +0.02s (+ 0.38%) 4.89s 5.06s
Emit Time 3.55s (± 0.72%) 3.57s (± 0.92%) +0.02s (+ 0.68%) 3.52s 3.66s
Total Time 10.50s (± 0.49%) 10.54s (± 0.52%) +0.04s (+ 0.42%) 10.41s 10.66s
material-ui - node (v14.15.1, x64)
Memory used 448,666k (± 0.00%) 448,539k (± 0.06%) -128k (- 0.03%) 447,440k 448,706k
Parse Time 1.84s (± 0.67%) 1.84s (± 0.57%) +0.01s (+ 0.27%) 1.82s 1.86s
Bind Time 0.68s (± 0.54%) 0.69s (± 0.58%) +0.00s (+ 0.58%) 0.68s 0.70s
Check Time 12.88s (± 0.68%) 12.98s (± 0.42%) +0.10s (+ 0.78%) 12.84s 13.07s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.40s (± 0.56%) 15.51s (± 0.36%) +0.11s (+ 0.71%) 15.37s 15.62s
xstate - node (v14.15.1, x64)
Memory used 533,709k (± 0.00%) 533,728k (± 0.00%) +19k (+ 0.00%) 533,679k 533,788k
Parse Time 2.55s (± 0.40%) 2.57s (± 0.49%) +0.03s (+ 1.02%) 2.54s 2.60s
Bind Time 1.17s (± 0.91%) 1.17s (± 0.66%) +0.00s (+ 0.34%) 1.15s 1.19s
Check Time 1.48s (± 0.57%) 1.50s (± 0.47%) +0.02s (+ 1.22%) 1.48s 1.51s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.27s (± 0.25%) 5.32s (± 0.25%) +0.05s (+ 0.85%) 5.28s 5.34s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory7 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)
Benchmark Name Iterations
Current 47483 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot typescript-bot mentioned this pull request Jan 21, 2022
Open
@typescript-bot
Copy link
Collaborator

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

@gabritto gabritto merged commit 3cbc8d2 into main Jan 21, 2022
@gabritto gabritto deleted the gabritto/issue45801pt2 branch January 21, 2022 20:05
@ahejlsberg
Copy link
Member

I'm curious why you've included TypeFacts.TypeofEQObject and TypeFacts.TypeofNEObject in TypeFacts.OrFactsMask? I'd be inclined to think an intersection is an object only when every constituent is an object. Meaning the flags should be and-ed, not or-ed. For example:

function test(x: number & { _foo: string }) {
    if (typeof x === 'object') {
        x;  // Used to narrow to never, now doesn't
    }
}

It's fairly common to have tagged primitives like that, but they shouldn't appear to be objects.

@gabritto gabritto mentioned this pull request Jan 24, 2022
Merged
@gabritto
Copy link
Member Author

I think TypeFacts.TypeofEQObject was a mistake on my part, given that behavior. But TypeofNEObject still makes sense to me to be an or mask because of the interaction between objects and functions:

function f1(x: F & { foo: number }) {
    if (typeof x !== "object") {
        x; // Narrows to `never` if `TypeFacts.TypeofNEObject` is an `and` mask
    }
}

However, there's also a mistake on the ignoreObjects behavior: we should return the AndMask instead of falling back and returning All, since All is no longer the identity element for computing type facts of intersections.

Made the fixes here: #47583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanCavanaugh RyanCavanaugh RyanCavanaugh approved these changes

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

@weswigham weswigham Awaiting requested review from weswigham

Assignees

@gabritto gabritto

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
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
5 participants
@gabritto @DanielRosenwasser @typescript-bot @ahejlsberg @RyanCavanaugh