Disallow partial matches for discriminant properties when generating error messages #37589
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
| @@ -37852,13 +37856,13 @@ namespace ts { | |||
| } | |||
|
|
|||
| // Keep this up-to-date with the same logic within `getApparentTypeOfContextualType`, since they should behave similarly | |||
| function findMatchingDiscriminantType(source: Type, target: Type, isRelatedTo: (source: Type, target: Type) => Ternary) { | |||
| function findMatchingDiscriminantType(source: Type, target: Type, isRelatedTo: (source: Type, target: Type) => Ternary, skipPartial?: boolean) { | |||
There was a problem hiding this comment.
When do you not want to skipPartial discriminants?
There was a problem hiding this comment.
So we report excess property errors on something like
type Group = {a: 1, b: 1, field: string, data: string}; | {a: 2, field: string} | {b: 2, data: string};
const x: Group = {a: 1, b: 1, field: "ok", data: "ok", excess: "not ok"};a and b are both partial discriminants, but taken together, can discriminate to the first member. We have a test expecting this.
sandersn
approved these changes
Apr 1, 2020
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.
Fixes #37506
In the example given,
datais marked as a discriminable property, because for the members it exists on, it can discriminate between them; so, taken with another (potentially partial) discriminant, it may discriminate the whole union (we have tests to that effect). Unfortunately, allowing such partial discriminants when choosing error targets makes us pick some suboptimal error messages; so in this PR, we disallow "partial" union properties from being enumerated as possible discriminant properties when fetching a discriminated union member for error elaboration purposes.