-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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.
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
Fix #30557 Narrow non-declared unions by discriminant #30593
Fix #30557 Narrow non-declared unions by discriminant #30593
Conversation
13443ae
to
e130cd6
Compare
Short and seemingly straightforward. Hopefully someone with the power to merge this can review it soon. Thank you for tracking this bug down and fixing it! |
@typescript-bot test this |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at e130cd6. You can monitor the build here. It should now contribute to this PR's status checks. |
Perf regression? |
For large unions without a discriminant I was computing the union of the property twice, which I guess is expensive. Not sure if this is enough to fix the problem however. |
@typescript-bot test this new commit |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at e038290. You can monitor the build here. It should now contribute to this PR's status checks. |
Edit: Ah, I think the property cache is getting wrecked because we are no longer always hitting the declared type. I’ll try fix this by first testing the declared type then falling back if it’s not a union. I dont know if anyone with more knowledge than me sees something obvious that I’m missing. One alternative might be to limit the narrowing to cases where the declared type is |
@typescript-bot test this because maybe it works now! |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 61d2eaf. You can monitor the build here. It should now contribute to this PR's status checks. |
Sorry @typescript-bot, I hope I don't let you down Narrator: He did |
Psyched! |
The RWC tests were running at 40m and going, so I don't think my change fixed that. I've changed it to only ever narrow from one type. Before, if the declared type was not a union but narrowed to a big union, then each subsequent narrowing would hit a different property cache. |
@typescript-bot test this |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 4b3a96a. You can monitor the build here. It should now contribute to this PR's status checks. |
Forgot to sync this branch with master; need to rerun |
@typescript-bot test this |
Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 8ed48e6. You can monitor the build here. It should now contribute to this PR's status checks. |
RWC is actually clean @typescript-bot perf test this |
Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 8ed48e6. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@jack-williams Here they are:Comparison Report - master..30593
System
Hosts
Scenarios
|
@typescript-bot perf test this |
Heya @jack-williams, I've started to run the perf test suite on this PR at b31b417. You can monitor the build here. It should now contribute to this PR's status checks. |
@typescript-bot perf test this |
Heya @jack-williams, I've started to run the perf test suite on this PR at b31b417. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@jack-williams Here they are:Comparison Report - master..30593
System
Hosts
Scenarios
|
@typescript-bot perf test this |
Heya @jack-williams, I've started to run the perf test suite on this PR at 356b91d. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@jack-williams Here they are:Comparison Report - master..30593
System
Hosts
Scenarios
|
356b91d
to
d798557
Compare
@typescript-bot perf test this |
Heya @jack-williams, I've started to run the perf test suite on this PR at 1925a6c. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@jack-williams Here they are:Comparison Report - master..30593
System
Hosts
Scenarios
|
@typescript-bot perf test this with recent CFA changes |
Heya @jack-williams, I've started to run the perf test suite on this PR at 0a037d0. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@jack-williams Here they are:Comparison Report - master..30593
System
Hosts
Scenarios
|
Fixes #30557