GeezerVison

Based on your comment and the repro, isn't the real issue that we don't combine discriminant narrowing with other generated facts in control flow well?

Wesley Wigham (@weswigham) Well, yes and no. We would do better if we didn't have the design limitation that causes us to reset narrowed types for properties when the shape of an object changes because of a discriminant check, and we might in fact not have this issue. But it would require the ability to "record and replay" preceding narrowing checks, and that would definitely complicate the control flow analyzer. And, either way, there's really no point in considering a and b discriminant properties in the example.

In the given example, sure, but I can't help but feel that someone somewhere has taken a dependency on a property of string | undefined being discriminable since support was added.

Anders Hejlsberg (@ahejlsberg) When you say:

reset narrowed types for properties when the shape of an object changes because of a discriminant check

What does shape mean: changing the apparent properties aggregated across the union type, or something else?

Would it be too difficult (or wrong) to test for this change of shape, rather than classifying a and b as non-discriminable? Narrowing U, where type U = A | B; never changes shape and therefore the narrowing does not need to be reset.

Wesley Wigham (@weswigham) Discriminants are meant to be unit types and we only narrow based on equality and truthiness checks. With a discriminant of string | undefined you'd potentially be able to narrow based on obj.x === undefined, but you wouldn't be able to narrow based on a string value or a typeof check. Just doesn't seem useful, it didn't work before, and I can't imagine anyone taking a dependency on that in the last few weeks.

With a discriminant of string | undefined you'd potentially be able to narrow based on obj.x === undefined, but you wouldn't be able to narrow based on a string value or a typeof check. Just doesn't seem useful, it didn't work before, and I can't imagine anyone taking a dependency on that in the last few weeks.

A typeof-based narrowing works juts fine for string | undefined.

Also 2-3 months != 2 weeks. This has been out for awhile (and has shipped in stable versions of TS).

Wesley Wigham (@weswigham) No, we don't do discriminant based narrowing for typeof checks:

type Foo = { a: string, x: number } | { a: 0, y: number };

function xxx(obj: Foo) {
  if (typeof obj.a === "string") {
    obj.x;  // Error (because we don't narrow here)
  }
  if (obj.a === 0) {
    obj.y;  // Ok (discriminant based narrowing)
  }
}

With the fix in this PR our behavior changes in the following because we no longer consider a a discriminant property:

type Foo = { a: string | undefined, x: number } | { a: string, y: number };

function xxx(obj: Foo) {
  if (obj.a === undefined) {
    obj.x;  // Previously would narrow, now doesn't
  }
}

As I said, I can't imagine anyone having taken a dependency on this in the last few weeks (or months or whatever).

This PR is the right fix for the issue given the design constraints we currently have.

If I understand the issue properly it seems to be a tradeoff between

1. Not adding this fix: breaking code for people using u.a && u.b && f(u.a, u.b); who have not upgraded to 3.2 yet.

2. Adding this fix: breaking code for early adopters narrowing on undefined. There probably are a few, as this example was in the original thread on nullable discriminants.

The number of people affected by (1) seems likely to be larger than (2), and (2) can be fixed by replacing the equality test with a type guard.

Jack Williams (@jack-williams) Yes, that's the tradeoff. But #2 is actually even further qualified: Breaking code for early adopters of discriminant narrowing involving properties that have no single underlying property of a pure unit type or union of unit types.