Fix contextual discrimination for omitted members #48448
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Milestone Bug
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at caa081c. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - main..48448
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Oof, this is still reporting a 50% check time regression in |
|
@typescript-bot perf test this |
|
The "problem" is that material-ui has some types that are large unions ~700 types, and at least some have ~100 optional types. It's not clear to me that the performance could be improved any more, so if it's still bad somehow, or is deemed too complex for the handful of edge cases it helps, I'm cool with abandoning in. As an implementation side note, I needed a way to hash subsets of the union type, and I did this by forming a string with space delimited indices. This works, but I'm not sure if there's a better way to achieve this same result. |
The first attempt at fixing this issue caused a large regression on one benchmark. This new PR addresses the regression by caching optional members of union types when they're used for discrimination. This seems to reduce the performance hit at the cost of some extra memory, but it's not clear if the fix is worth this regression.
|
@typescript-bot perf test this |
|
Heya @weswigham, I've started to run the perf test suite on this PR at cd00855. You can monitor the build here. Update: The results are in! |
|
|
@weswigham Here they are:Comparison Report - main..48448
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Well, that's much better, that's for sure. 2-5% worse is a lot better than 30-50% worse. |
|
It was a good thing this was reverted, because the PR is actually unsound, and it was caught from the local serve build. The problem is a few fold, and so I'm writing here to seek guidance. As a general note, I'm unsure how this should function in The original PR sought to add any discriminable property of any optional property of any element of the union type as a discriminator checking for assignability to Either way, this premise of checking for assignability to undefined of optional members is actually flawed. In principle what we actually want to is check for all potential properties in the union type that are missing from the original object for their assignability to missing. Ignoring As I write this, another idea comes to mind, which seems less "correct" but may still be able to achieve the ideal result: In principle this is still meant to help figure out "discriminated unions", so instead of caching all properties, or all optional properties, we could specifically look for optional discriminated unions, e.g. the property is optional on one type and non-optional on any other members. By itself this will still fail on the union of two disjoint optional discriminated unions (which seems unlikely). But instead of checking all discriminators we check them independently for the set of types they discriminate against. Worst case this could be really bad, but I think in most common usages it might not even require an extra check. I'm curious as to general thoughts on these different approaches. I realize this isn't very important, so maybe I'm overthinking this. |
|
After some more experimentation, I think the goal of the PR as stated is out of line with the way typescript handles discriminant unions, and the current handling is sufficient. I'll open up an issue (or search to see if one exists) about better union discrimination. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The first attempt at fixing this issue caused a large regression on one
benchmark. This new PR addresses the regression by caching optional
members of union types when they're used for discrimination. This seems
to reduce the performance hit at the cost of some extra memory, but it's
not clear if the fix is worth this regression.
For context:
first related PR: #41759
original fix: #43633
fixes: #48298
reversion: #48426
I was able to reproduce the regression, but behavior on WSL ended up proving spotty, so in some tests the regression went mostly away (to 3%) on the material-ui docs benchmark. Other times it was closer to 10%. Ideally I'd like to see how this diff performs on all the benchmarks as is. I think further optimization is goin to require deeper caching, and there's an argument that maybe performing "correctly" in this case isn't worth the performance hit.