Fix #24193: Allow nullable discriminants#27631
Fix #24193: Allow nullable discriminants#27631jack-williams wants to merge 1 commit intomicrosoft:masterfrom
Conversation
| hasDiscriminant = true; | ||
| continue; | ||
| } | ||
| else if (current.flags & (TypeFlags.DisjointDomains | TypeFlags.Object)) { |
There was a problem hiding this comment.
As a signpost for @ahejlsberg, I wasn't sure about this flag combination (TypeFlags.DisjointDomains | TypeFlags.Object). I was mostly concerned about avoiding unions that had a generic parameter that might be instantiated to something nullable, but felt it was safer to be positive about what is accepted, rather than negative about what is not. I also considered whether it would be just sufficient to check the union has at least one nullable and at least one known non-nullable.
|
Let me propose a simpler and more general approach. We currently consider a property of a union type to be a discriminant property if it has a non-uniform type and that type is a union of only unit types. I think we could slightly relax this to say it must be a union containing at least one unit type. This would allow us to discriminate in other cases such as type Result<T> = { error?: undefined, value: T } | { error: Error };
function test(x: Result<number>) {
if (!x.error) {
x.value;
}
else {
x.error.message;
}
}
test({ value: 10 });
test({ error: new Error("boom") });I put up the proposed (very simple) change in a branch named |
|
@ahejlsberg I like your approach better (but to be pedantic your example does work with my fix). For your suggestion I think we need an additional constraint that says the union does not contain an instantiable type. Without so, the wrong narrowings are produced. For example, I think your fix produces the following behaviour: type GenericA<T> = { a: 3; c: string } | { a: T; c: number };
function narrowT<T>(x: GenericA<T>) {
if (x.a === 3) {
// x: { a: 3; c: string }
const aString: string = x.c; // ok, probably shouldn't be
}
}
narrowT({ a: 3, c: 4 });So a proposed alternative is:
However this approach does have one drawback in that it prevents some useful narrowings: type GenericA<T> = { a: 3; c: string } | { a: T; c: number }
function narrowT<T>(x: GenericA<T>) {
if (x.a !== 3) {
// in master (or my change) x: Generic<T>
// the assignment is not ok, but probably should be
// ok, in mixedDiscriminantTypes
const aNumber: number = x.c;
}
}
narrowT({ a: 3, c: 4 });I think maybe the root of the problem is that the filtering done by discriminant narrowing uses the comparable relation, which does not relate type parameters to literals [1]. This filters out types with generic discriminates which shouldn't be filtered unless their base constraint is not comparable. Changing the comparable relation seems really scary, though I would argue a generic Not sure how to proceed, but here is a summary:
[1] I'm not sure of the full details of the comparable relation, I used the behaviour of the following to guide me: function foo<X>(x: X) {
if (x === 3) { // error
}
} |
Good point. I think that would be a reasonable rule to add.
Yes, that's the root of the problem. We only allow comparing type parameters to I will put up a PR later today. |
|
Sounds good! I'll close this issue up then. |
|
Would it be sufficient to only exclude unconstrained type parameters? To answer my own question: no. function foo<X extends {}, Y extends unknown>(x: X, y: Y) {
x === 3; // err
x === true; // err
y === 3; // err
y === true; // err
} |
|
@jack-williams PR is up in #27695. |
Fixes #24193
This is technically a breaking change because it enables excessing property checking for certain kinds of unions (with
nulldiscriminant), when the check was previously disabled.Example:
Before: No error
Now: