-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Improve constraints of conditional types applied to constrained type variables #56004
Merged
Merged
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c0ae93f
Fix constraint of conditional type applied to constrained type variable
ahejlsberg 03a45f4
Accept new baselines
ahejlsberg 388daf5
Shrink some error pyramids
ahejlsberg 5082d17
Accept new baselines
ahejlsberg 2f5d9e9
Add tests
ahejlsberg c7613d6
Use reverse assignable check instead of comparable check
ahejlsberg f64e34c
Distribute over unions in reverse assignability check
ahejlsberg 1f06887
Add test
ahejlsberg eb3e4b4
Exclude never types from check and add more comments
ahejlsberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is probably a bit better than the often-incorrect result we give now, so maybe we're just fine with that being that, isn't a check in this style kind of fundamentally flawed? For example, a
type Cond<T extends {a: string} | {b: string}> = T extends {c: string} ? true : false
.T
's constraint isn't related in any way to the check type of{c: string}
, yet the constraint should probably still be the union of both branches, as an instantiation ofT
might satisfy the check type, or might not, while here we're still skipping thetrue
branch type just because{a: string}
isn't assignable to{c: string}
. And comparability wouldn't have been any better. As @andrewbranch said - the only case where we can definitively say only one branch is taken would seem to require checking for an empty intersection between the check-type-constraint and the extends type (and, as you mentioned, ideally tighter empty intersection logic so that has higher accuracy).How different are the results it if we just replace this assignability check with a
intersectTypes(getPermissiveInstantiation(inferredExtendsType), getPermissiveInstantiation(checkType)) !== neverType
? Are the bad changes any worse?