Improve discriminated union error messages #14006
Conversation
Assignability errors for discriminated unions now check the value of the
discriminant to decide which member of the union to check for
assignability.
Previously, assignability didn't know about discriminated unions and
would check every member, issuing errors for the last member of the
union if assignability failed.
For example:
```ts
type Square = { kind: "sq", size: number }
type Rectangle = { kind: "rt", x: number, y: number }
type Circle = { kind: "cr", radius: number }
type Shape =
| Square
| Rectangle
| Circle;
let shape: Shape = {
kind: "sq",
x: 12,
y: 13,
}
```
`typeRelatedToSomeType` now checks whether each property in the source
type is a discriminant. It finds `kind` and proceeds to look for the
type in the target union that has `kind: "sq"`. If it finds it, which it
does in this example (`Square`), then it checks only assignbility to
`Square`.
The result is that the error now says that property 'size' is missing in
type `{ kind: "sq", x: number, y: number }` instead of saying that that
"sq" is not assignable to type "cr" like it did before.
Fixes #10867
|
@ahejlsberg do you mind taking a look too? |
src/compiler/checker.ts
Outdated
| @@ -7771,6 +7771,11 @@ namespace ts { | |||
| if (target.flags & TypeFlags.Union && containsType(targetTypes, source)) { | |||
| return Ternary.True; | |||
| } | |||
| const discriminantType = findMatchingDiscriminantType(source, target); | |||
| if (discriminantType) { | |||
| return isRelatedTo(source, discriminantType, reportErrors); | |||
There was a problem hiding this comment.
I'm not sure this approach is correct. The target might contain multiple types with overlapping discriminants (e.g. { kind: 'a' | 'b', ... } | { kind: 'a' | 'c', ... } and you need to try all of them. I think it would be better to keep the existing loop for all cases and then look for a matching discriminant only as you are about to report an error.
There was a problem hiding this comment.
How would the recursive call of isRelatedTo know that it was comparing a discriminated union member to a discriminant? I guess reportErrors could take on a third value that means "only report errors if you can find a matching discriminant", but that seems like using a general-purpose flag for a special case.
There was a problem hiding this comment.
I would run the entire loop with false for the reportErrors parameter. Then, if nothing matches, find the first constituent with a matching discriminant and call isRelatedTo with error reporting, or call isRelatedTo on the last one if no discriminants match.
src/compiler/checker.ts
Outdated
| if (related) { | ||
| return related; | ||
| } | ||
| } | ||
| const discriminantType = findMatchingDiscriminantType(source, target); | ||
| isRelatedTo(source, discriminantType || targetTypes[targetTypes.length - 1], reportErrors); |
There was a problem hiding this comment.
Enclose the two lines above in if (reportErrors) { ... } and pass true as the reportErrors argument.
This uses the same code as #14006, which improves error messages for discriminated unions.
Assignability errors for discriminated unions now check the value of the discriminant to decide which member of the union to check for assignability.
Previously, assignability didn't know about discriminated unions and would check every member, issuing errors for the last member of the union if assignability failed.
For example:
typeRelatedToSomeTypenow checks whether each property in the source type is a discriminant. It findskindand proceeds to look for the type in the target union that haskind: "sq". If it finds it, which it does in this example (Square), then it checks only assignbility toSquare.The result is that the error now says that property 'size' is missing in type
{ kind: "sq", x: number, y: number }instead of saying that that "sq" is not assignable to type "cr" like it did before.Fixes #10867