Skip to content

fix(46909):Fix typePredicate in UnionType #47172

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
Closed
wants to merge 1 commit into from
Closed

fix(46909):Fix typePredicate in UnionType #47172

wants to merge 1 commit into from

Conversation

islandryu
Copy link
Contributor

Fixes #46909

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Dec 16, 2021
@islandryu
Copy link
Contributor Author

I'm getting an error in privateNameInInExpression.ts, but it didn't happen locally.

@islandryu islandryu changed the title Fix typePredicate in UnionType (fix:46909):Fix typePredicate in UnionType Jan 1, 2022
@islandryu islandryu changed the title (fix:46909):Fix typePredicate in UnionType fix(46909):Fix typePredicate in UnionType Jan 1, 2022
weswigham
weswigham reviewed Feb 7, 2022
View reviewed changes
@islandryu islandryu force-pushed the fix/typepredicate branch 2 times, most recently from c068103 to 3f0423d Compare February 10, 2022 02:18
islandryu
islandryu commented Feb 10, 2022
View reviewed changes
src/compiler/checker.ts
Comment on lines +24743 to +24745
if (!isTypeSubtypeOf(unionAssignableType, assignableType)) {
assignableType = unionAssignableType;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This branch is included because even if the types of assignableType and unionAssignableType are the same, the symbols change.
If there is a more appropriate function than isTypeSubtypeOf, I would like to use it.

@islandryu islandryu requested a review from weswigham February 10, 2022 04:03
@weswigham
Copy link
Member

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

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

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

Heya @weswigham, I've started to run the tarball bundle task on this PR at 465c0ba. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

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

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

Heya @weswigham, I've started to run the diff-based community code test suite on this PR at 465c0ba. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 5, 2022 •
edited
Loading

Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/121319/artifacts?artifactName=tgz&fileId=D0955D121F17A9B6C2305384BC2F0E1C4AEA7409E78CF7BAF264F010C27A922C02&fileName=/typescript-4.7.0-insiders.20220305.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/[email protected]".;

@typescript-bot
Copy link
Collaborator

@weswigham
Great news! no new errors were found between main..refs/pull/47172/merge

@typescript-bot
Copy link
Collaborator

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

Here they are:

Comparison Report - main..47172

Metric main 47172 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 356,980k (± 0.02%) 356,935k (± 0.01%) -46k (- 0.01%) 356,859k 357,025k
Parse Time 2.04s (± 0.20%) 2.06s (± 0.93%) +0.02s (+ 1.08%) 2.03s 2.13s
Bind Time 0.86s (± 0.75%) 0.86s (± 0.46%) -0.00s (- 0.12%) 0.85s 0.87s
Check Time 5.72s (± 0.49%) 5.74s (± 0.62%) +0.01s (+ 0.23%) 5.70s 5.87s
Emit Time 5.96s (± 0.87%) 5.96s (± 1.00%) +0.00s (+ 0.03%) 5.88s 6.17s
Total Time 14.59s (± 0.46%) 14.63s (± 0.45%) +0.04s (+ 0.26%) 14.52s 14.85s
Compiler-Unions - node (v10.16.3, x64)
Memory used 205,694k (± 0.02%) 208,596k (± 0.03%) +2,902k (+ 1.41%) 208,448k 208,787k
Parse Time 0.85s (± 0.43%) 0.84s (± 0.81%) -0.00s (- 0.24%) 0.83s 0.86s
Bind Time 0.52s (± 0.71%) 0.52s (± 0.72%) -0.00s (- 0.19%) 0.51s 0.52s
Check Time 7.84s (± 0.43%) 8.41s (± 0.64%) +0.56s (+ 7.16%) 8.28s 8.57s
Emit Time 2.52s (± 0.63%) 2.48s (± 0.89%) -0.03s (- 1.35%) 2.44s 2.53s
Total Time 11.72s (± 0.39%) 12.25s (± 0.57%) +0.53s (+ 4.48%) 12.08s 12.46s
Monaco - node (v10.16.3, x64)
Memory used 343,460k (± 0.01%) 343,394k (± 0.02%) -66k (- 0.02%) 343,264k 343,518k
Parse Time 1.56s (± 0.36%) 1.56s (± 0.82%) +0.00s (+ 0.32%) 1.52s 1.59s
Bind Time 0.76s (± 0.77%) 0.75s (± 0.49%) -0.00s (- 0.26%) 0.75s 0.76s
Check Time 5.69s (± 0.26%) 5.70s (± 0.32%) +0.00s (+ 0.07%) 5.65s 5.74s
Emit Time 3.23s (± 0.55%) 3.23s (± 0.50%) +0.00s (+ 0.09%) 3.21s 3.28s
Total Time 11.24s (± 0.18%) 11.24s (± 0.18%) +0.01s (+ 0.07%) 11.19s 11.28s
TFS - node (v10.16.3, x64)
Memory used 305,243k (± 0.01%) 305,186k (± 0.03%) -57k (- 0.02%) 305,054k 305,385k
Parse Time 1.27s (± 0.44%) 1.27s (± 0.37%) -0.00s (- 0.16%) 1.26s 1.28s
Bind Time 0.71s (± 0.81%) 0.71s (± 0.52%) +0.00s (+ 0.70%) 0.71s 0.72s
Check Time 5.17s (± 0.39%) 5.17s (± 0.64%) -0.00s (- 0.06%) 5.08s 5.23s
Emit Time 3.41s (± 1.09%) 3.42s (± 1.17%) +0.01s (+ 0.23%) 3.33s 3.51s
Total Time 10.56s (± 0.36%) 10.58s (± 0.59%) +0.01s (+ 0.12%) 10.41s 10.69s
material-ui - node (v10.16.3, x64)
Memory used 475,730k (± 0.01%) 475,771k (± 0.01%) +41k (+ 0.01%) 475,657k 475,911k
Parse Time 1.84s (± 2.44%) 1.81s (± 0.52%) -0.03s (- 1.63%) 1.79s 1.83s
Bind Time 0.69s (± 2.42%) 0.67s (± 1.49%) -0.02s (- 2.46%) 0.64s 0.69s
Check Time 22.85s (± 1.88%) 23.03s (± 1.12%) +0.18s (+ 0.78%) 22.53s 23.60s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 25.39s (± 1.86%) 25.52s (± 1.04%) +0.13s (+ 0.52%) 25.01s 26.10s
xstate - node (v10.16.3, x64)
Memory used 576,890k (± 1.30%) 576,888k (± 1.31%) -2k (- 0.00%) 573,220k 607,370k
Parse Time 2.58s (± 0.39%) 2.58s (± 0.47%) -0.00s (- 0.08%) 2.54s 2.60s
Bind Time 1.00s (± 0.50%) 1.00s (± 0.65%) -0.00s (- 0.30%) 0.99s 1.01s
Check Time 1.50s (± 0.50%) 1.51s (± 0.60%) +0.00s (+ 0.33%) 1.48s 1.52s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.15s (± 0.27%) 5.15s (± 0.40%) -0.00s (- 0.06%) 5.08s 5.18s
Angular - node (v12.1.0, x64)
Memory used 334,682k (± 0.07%) 334,773k (± 0.02%) +92k (+ 0.03%) 334,663k 334,900k
Parse Time 2.04s (± 0.59%) 2.07s (± 0.74%) +0.03s (+ 1.57%) 2.04s 2.10s
Bind Time 0.83s (± 1.33%) 0.84s (± 1.03%) +0.01s (+ 0.96%) 0.83s 0.87s
Check Time 5.55s (± 0.56%) 5.57s (± 0.85%) +0.02s (+ 0.34%) 5.47s 5.67s
Emit Time 6.20s (± 0.72%) 6.26s (± 0.95%) +0.06s (+ 1.03%) 6.17s 6.44s
Total Time 14.62s (± 0.44%) 14.74s (± 0.59%) +0.12s (+ 0.83%) 14.58s 14.91s
Compiler-Unions - node (v12.1.0, x64)
Memory used 193,028k (± 0.13%) 195,700k (± 0.14%) +2,672k (+ 1.38%) 195,185k 196,150k
Parse Time 0.83s (± 0.82%) 0.84s (± 0.80%) +0.00s (+ 0.24%) 0.83s 0.86s
Bind Time 0.53s (± 0.56%) 0.53s (± 0.56%) +0.00s (+ 0.76%) 0.53s 0.54s
Check Time 7.33s (± 0.51%) 7.89s (± 0.66%) +0.56s (+ 7.68%) 7.75s 8.03s
Emit Time 2.55s (± 0.98%) 2.54s (± 1.82%) -0.01s (- 0.31%) 2.40s 2.63s
Total Time 11.24s (± 0.33%) 11.80s (± 0.43%) +0.56s (+ 4.99%) 11.68s 11.90s
Monaco - node (v12.1.0, x64)
Memory used 326,458k (± 0.02%) 326,436k (± 0.06%) -23k (- 0.01%) 325,751k 326,805k
Parse Time 1.53s (± 0.84%) 1.54s (± 0.83%) +0.01s (+ 0.59%) 1.51s 1.57s
Bind Time 0.75s (± 1.28%) 0.74s (± 0.78%) -0.01s (- 0.67%) 0.73s 0.76s
Check Time 5.54s (± 0.49%) 5.57s (± 0.52%) +0.02s (+ 0.41%) 5.49s 5.63s
Emit Time 3.24s (± 0.65%) 3.29s (± 1.05%) +0.05s (+ 1.57%) 3.23s 3.39s
Total Time 11.06s (± 0.34%) 11.14s (± 0.52%) +0.08s (+ 0.69%) 10.99s 11.31s
TFS - node (v12.1.0, x64)
Memory used 289,839k (± 0.03%) 289,905k (± 0.02%) +66k (+ 0.02%) 289,834k 290,022k
Parse Time 1.30s (± 1.43%) 1.30s (± 0.95%) -0.00s (- 0.08%) 1.27s 1.32s
Bind Time 0.71s (± 0.67%) 0.71s (± 0.81%) +0.00s (+ 0.42%) 0.70s 0.73s
Check Time 5.11s (± 0.50%) 5.15s (± 0.43%) +0.04s (+ 0.80%) 5.09s 5.21s
Emit Time 3.46s (± 0.84%) 3.48s (± 0.92%) +0.02s (+ 0.69%) 3.43s 3.56s
Total Time 10.57s (± 0.36%) 10.64s (± 0.49%) +0.07s (+ 0.65%) 10.52s 10.74s
material-ui - node (v12.1.0, x64)
Memory used 454,537k (± 0.08%) 454,785k (± 0.01%) +248k (+ 0.05%) 454,690k 454,857k
Parse Time 1.79s (± 2.02%) 1.81s (± 0.47%) +0.02s (+ 1.17%) 1.80s 1.83s
Bind Time 0.64s (± 1.90%) 0.65s (± 0.69%) +0.01s (+ 1.89%) 0.64s 0.66s
Check Time 20.02s (± 1.04%) 20.30s (± 0.86%) +0.28s (+ 1.40%) 20.04s 20.66s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.45s (± 0.93%) 22.76s (± 0.77%) +0.31s (+ 1.39%) 22.49s 23.13s
xstate - node (v12.1.0, x64)
Memory used 542,634k (± 1.37%) 539,321k (± 0.02%) -3,313k (- 0.61%) 538,976k 539,476k
Parse Time 2.53s (± 0.63%) 2.54s (± 0.64%) +0.00s (+ 0.16%) 2.51s 2.58s
Bind Time 1.03s (± 0.58%) 1.04s (± 0.35%) +0.01s (+ 0.87%) 1.04s 1.05s
Check Time 1.47s (± 0.65%) 1.47s (± 0.55%) +0.00s (+ 0.34%) 1.45s 1.49s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.10s (± 0.28%) 5.12s (± 0.42%) +0.02s (+ 0.31%) 5.07s 5.17s
Angular - node (v14.15.1, x64)
Memory used 332,989k (± 0.01%) 332,998k (± 0.01%) +9k (+ 0.00%) 332,947k 333,040k
Parse Time 2.02s (± 0.48%) 2.03s (± 0.95%) +0.01s (+ 0.59%) 2.01s 2.09s
Bind Time 0.87s (± 0.66%) 0.88s (± 0.96%) +0.01s (+ 0.69%) 0.87s 0.91s
Check Time 5.57s (± 0.46%) 5.59s (± 0.27%) +0.02s (+ 0.34%) 5.56s 5.63s
Emit Time 6.28s (± 0.76%) 6.31s (± 0.58%) +0.03s (+ 0.49%) 6.23s 6.40s
Total Time 14.74s (± 0.45%) 14.81s (± 0.30%) +0.06s (+ 0.41%) 14.71s 14.87s
Compiler-Unions - node (v14.15.1, x64)
Memory used 193,467k (± 0.65%) 196,934k (± 0.55%) +3,467k (+ 1.79%) 194,656k 197,927k
Parse Time 0.85s (± 0.39%) 0.85s (± 0.56%) +0.00s (+ 0.35%) 0.84s 0.86s
Bind Time 0.55s (± 0.62%) 0.56s (± 0.61%) +0.00s (+ 0.72%) 0.55s 0.56s
Check Time 7.40s (± 0.64%) 7.99s (± 0.68%) +0.60s (+ 8.06%) 7.88s 8.08s
Emit Time 2.49s (± 0.70%) 2.49s (± 0.66%) +0.00s (+ 0.08%) 2.45s 2.52s
Total Time 11.29s (± 0.56%) 11.89s (± 0.49%) +0.60s (+ 5.35%) 11.78s 12.00s
Monaco - node (v14.15.1, x64)
Memory used 325,214k (± 0.00%) 325,224k (± 0.00%) +9k (+ 0.00%) 325,180k 325,241k
Parse Time 1.56s (± 0.57%) 1.56s (± 0.38%) 0.00s ( 0.00%) 1.55s 1.58s
Bind Time 0.77s (± 0.43%) 0.77s (± 0.84%) +0.00s (+ 0.26%) 0.76s 0.79s
Check Time 5.44s (± 0.35%) 5.47s (± 0.36%) +0.03s (+ 0.48%) 5.42s 5.50s
Emit Time 3.29s (± 0.71%) 3.30s (± 0.88%) +0.01s (+ 0.30%) 3.24s 3.36s
Total Time 11.07s (± 0.38%) 11.11s (± 0.31%) +0.04s (+ 0.35%) 11.04s 11.17s
TFS - node (v14.15.1, x64)
Memory used 288,784k (± 0.01%) 288,771k (± 0.01%) -13k (- 0.00%) 288,742k 288,802k
Parse Time 1.31s (± 1.19%) 1.31s (± 0.78%) +0.00s (+ 0.08%) 1.29s 1.33s
Bind Time 0.73s (± 0.79%) 0.74s (± 1.20%) +0.01s (+ 0.96%) 0.73s 0.77s
Check Time 5.10s (± 0.32%) 5.11s (± 0.40%) +0.01s (+ 0.18%) 5.04s 5.14s
Emit Time 3.49s (± 1.56%) 3.44s (± 2.23%) -0.05s (- 1.35%) 3.35s 3.66s
Total Time 10.62s (± 0.57%) 10.59s (± 0.83%) -0.03s (- 0.26%) 10.46s 10.81s
material-ui - node (v14.15.1, x64)
Memory used 452,927k (± 0.07%) 452,770k (± 0.09%) -158k (- 0.03%) 451,845k 453,217k
Parse Time 1.85s (± 0.57%) 1.86s (± 0.67%) +0.01s (+ 0.32%) 1.84s 1.89s
Bind Time 0.70s (± 0.83%) 0.71s (± 0.95%) +0.00s (+ 0.57%) 0.70s 0.73s
Check Time 20.23s (± 0.55%) 20.53s (± 1.41%) +0.30s (+ 1.48%) 20.08s 21.03s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 22.79s (± 0.52%) 23.10s (± 1.22%) +0.31s (+ 1.36%) 22.65s 23.59s
xstate - node (v14.15.1, x64)
Memory used 537,105k (± 0.01%) 537,109k (± 0.01%) +4k (+ 0.00%) 537,046k 537,167k
Parse Time 2.58s (± 0.36%) 2.59s (± 0.30%) +0.01s (+ 0.27%) 2.57s 2.60s
Bind Time 1.15s (± 0.87%) 1.15s (± 0.81%) +0.00s (+ 0.44%) 1.14s 1.18s
Check Time 1.50s (± 0.64%) 1.51s (± 0.72%) +0.01s (+ 0.73%) 1.49s 1.54s
Emit Time 0.07s (± 0.00%) 0.07s (± 3.14%) +0.00s (+ 1.43%) 0.07s 0.08s
Total Time 5.30s (± 0.36%) 5.32s (± 0.31%) +0.02s (+ 0.34%) 5.30s 5.37s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory5 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 47172 10
Baseline main 10

Developer Information:

Download Benchmark

@weswigham
Copy link
Member

Yowch, this is an 8% perf hit in a union + type guard heavy codebase. Any way we can mitigate that? That big a diff is something we've blocked bigger issues on.

weswigham
weswigham requested changes Mar 5, 2022
View reviewed changes
Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

Unfortunately, I think the perf's gonna need to improve in the compiler-unions suite before we can take this fix. We may need to get creative to keep the perf cost down.

@sandersn
Copy link
Member

sandersn commented Jun 1, 2022

@islandryu
Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs.

@islandryu
Copy link
Contributor Author

@sandersn
Sorry, I can't seem to take the time to take this task, so I'll CLOSE it.

@islandryu islandryu closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weswigham weswigham weswigham requested changes

@gabritto gabritto Awaiting requested review from gabritto

Assignees

@weswigham weswigham

Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type predicate can sometimes result in different type than type discrimination
4 participants
@islandryu @weswigham @typescript-bot @sandersn