Fix #24193: Allow nullable discriminants

[jack-williams]

Fixes #24193

This is technically a breaking change because it enables excessing property checking for certain kinds of unions (with null discriminant), when the check was previously disabled.

Example:

type Foo = { a: null; b: string } | { a: string, c: number };
const x: Foo = { a: null, b: "foo", c: 4};

Before: No error
Now:

Type '{ a: null; c: number; b: string; }' is not assignable to type 'Foo'.
  Object literal may only specify known properties, and 'c' does not exist in type '{ a: null; b: string; }'. [2322]

[jack-williams]

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.

[ahejlsberg]

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 mixedDiscriminantTypes (see here). There is only one minor difference in the test baselines and one minor difference in the RWC tests, so I think it is workable.

[jack-williams]

@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:

• it must be a union containing at least one unit type and no instatiable types

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 T should be comparable to a unit type unless the base constraint of T is definitely not comparable.

Not sure how to proceed, but here is a summary:

• I like your solution better but we should make sure to not filter union members that could be instantiated to a matching type. The simple fix would be rejecting a union with an instantiable type.
• The more complete solution seems to be changing the filtering done by narrowTypeByDiscriminant, either by changing the filter, or changing the comparable relation. I'm starting to get abit out of my depth at this point, so I defer to your judgment (and anyone else's on this for that matter).
• I'm happy to proceed however: whether you just want to want to merge your branch and drop this, or pull changes into this.

[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
    
  }
}

[ahejlsberg]

I think we need an additional constraint that says the union does not contain an instantiable type.

Good point. I think that would be a reasonable rule to add.

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.

Yes, that's the root of the problem. We only allow comparing type parameters to undefined, null, and compatible type parameters. Since an unconstrained type parameter could be anything, it would technically be correct to allow it to be compared to anything. Yet, the current rules are very helpful in preventing inadvertent mixing of apples and bananas in expressions, and for that reason I think they're worth keeping.

I will put up a PR later today.

[jack-williams]

Sounds good! I'll close this issue up then.

[jack-williams]

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
}

[ahejlsberg]

@jack-williams PR is up in #27695.