Excess property checks for discriminated unions #16363
Conversation
This uses the same code as #14006, which improves error messages for discriminated unions.
|
@ahejlsberg mind taking a look. |
|
Awesome! Thanks for looking into this 👍 |
| // check excess properties against discriminant type only, not the entire union | ||
| return hasExcessProperties(source, discriminantType, reportErrors); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure it's that simple. What if multiple types in the target have a matching discriminant? I think there's an implicit assumption in your code that each target constituent has a unique discriminant, which may not be the case.
There was a problem hiding this comment.
This is similar to the problem with #14006, which added findMatchingDiscriminantType, although there the aim was to improve an existing error, so it was still improvement to return one of multiple possible types with matching discriminants.
I think a fix here would be to make findMatchingDiscriminantType not return anything if multiple types match. However, I'm not sure what type shows this behaviour. Is it something like { tag: "A", x } | { tag: "A", y } ?
There was a problem hiding this comment.
{ tag: "A", x } | { tag: "A", y } already works with the current PR:
type AAA = { tag: "A", x: number } | { tag: "A", y: string };
let aaa: AAA;
aaa = { tag: "A", x: 12 }; // ok
aaa = { tag: "A", y: "Hi" }; // ok
aaa = { tag: "A", x: 12, extra: "extra" } // errorSo I'm not sure how to construct a non-unique discriminant example that fails.
There was a problem hiding this comment.
Never mind, I oversimplified the discriminated union. The second example with y does indeed fail, but only if the type is type AABC = { tag: "A", x: number } | { tag: "A", y: string } | { tag: "B", z: boolean } | { tag: "C" }
|
@ahejlsberg I forgot about this in between vacations, but I think it's ready to go. Do you want to take another look? |
|
Thanks for the warning @weswigham. Updated. |
|
After re-reading the code, it looked kind of slow to me. So I ran a performance test. The complete results are below. In summary, compile time got faster for Angular and the compiler (Unions variant). It was also faster for TFS, but not more than the reported margin of error [1]. Compile time was about the same for Monaco. More speedup came from checking than elsewhere. The only reason I can think of for check time getting faster is excess property checking find lots of new errors and causing
[1] I'm not really sure what the margin of error is, though. Is it the first standard deviation or something? I'll have to ask @rbuckton when I see him. |
|
@mhegazy and I think that the reason may be improved narrowing resulting from early failures from hasExcessProperties. |
Excess property checks for discriminated unions. Uses
findMatchingDiscriminantjust like #14006 does, which improves error messages for discriminated unions.Fixes #12745