Fix narrowing union with union type predicate

[andrewbranch]

Fixes #31156

Previously, narrowing any union type started by filtering constituents of the original union out:

let a: number | string;
if (typeof a === 'number') {
  a; // 'number' - one of the original of 'number | string'
}

If any constituents matched, they were returned immediately.

Sometimes the candidate is more specific than the type you started with, so you can’t get anywhere by filtering from the original:

let a: number | string;
declare function isEight(n: any): n is 8
if (isEight(a)) {
  a; // '8' - neither 'number' nor 'string' is a subtype of '8', so it fell back to an intersection
}

This worked because filtering down from number | string failed, and the intersection 8 & (number | string) was used as a fallback, which reduces to 8—a correct match.

But, if the candidate had a subtype constituent like 8 and also had something that caused filtering down from number | string to succeed, then we’d never see 8 in the resulting union:

let a: number | string;
declare function isEightOrString(n: any): n is 8 | string
if (isEight(a)) {
  a; // 'string' - WRONG! One of the original from 'number | string', and we stopped here!
}

To fix, when looking through target constituents, we see if any candidate constituents are subtypes of the current target constituent, and if so, replace that target constituent with the subtype candidate constituents. This ensures we don’t stop finding matches too early, and also that we pick the narrowest matches possible.

[weswigham]

Why not define it recursively?:

if (candidate.flags & TypeFlags.Union) {
  return mapType(candidate, t => getNarrowedType(type, t, assumeTrue, isRelated);
}

[andrewbranch]

That's what I tried first, but the intersection logic at the end screws up the computation for object types. If two constituents are entirely not related, we want to drop them out of the union, not include their intersection.

[weswigham]

Wait, why?

let a: {x: number} | { y: string };
declare function isEightOrString(n: any): n is {x: 8} | {x: string}
if (isEightOrString(a)) {
  a; // Should totally be `({ x: number; } & { x: 8; }) | ({ x: number; } & { x: string; }) | ({ y: string; } & { x: 8; }) | ({ y: string; } & { x: string; })` even though `{x: string}` and `{ y: string }` are unrelated (and yes, the first one of those could supertype reduce to just `{ x: 8; }`)
}

[andrewbranch]

Yeah... I see that's technically correct, but we have several baselines that expect unrelated constituents to be discarded. From partiallyDiscriminatedUnions.ts:

class Square { kind: "square"; }
class Circle { kind: "circle"; }

type Shape = Circle | Square;
type Shapes = Shape | Array<Shape>;

declare function isShape(s : Shapes): s is Shape;

if (isShape(s)) {
    s; // Before: Shape
       // With recursive `getNarrowedType()`: Square | Circle | (Square & Shape[]) | (Circle & Shape[])
    if (s.kind === "circle") {
        s; // Before: Circle.
           // With recursive `getNarrowedType()`: Circle | (Circle & Shape[])
    }
}

And there's nothing technically incorrect about that, but it's definitively grosser, and we've been rolling with the current behavior for 3 years and nobody has complained.

[weswigham]

The above example I wrote behaves as I described today, though (you can put it in the playground and it yields exactly that type). And the "unrelated variants are discarded" shtick is mostly limited solely to discriminable variants (excepting in narrowing, which we know is unsafe) - types whose member types guarantee that membership to one or the other is mutually exclusive.

[andrewbranch]

And the "unrelated variants are discarded" shtick is mostly limited solely to discriminable variants

What do you think about the Shape example then? Technically Circle and Shape[] aren't discriminable because I can define an object that conforms to both, but in practice it seems pretty unlikely.

There are a few other baselines that fail in the same vein, and it's even more painful when the result is an intersection between a primitive literal type and some type of object:

type S = "a" | "b";
type T = S[] | S;

function isS(t: T): t is S {
    return t === "a" || t === "b";
}

function f(foo: T) {
    if (isS(foo)) {
        return foo; // Before: S
        // After: "a" | "b" | ("a" & S[]) | ("b" & S[])
    }
}

I can suspend my disbelief that I might have an object that is simultaneously a Circle and Shape[], but "a" & S[]? 🤨

It feels like there must be a case for using heuristics to simplify the results when a user is explicitly narrowing something. Otherwise, this function would just return the intersection no matter what, wouldn't it?

[weswigham]

Otherwise, this function would just return the intersection no matter what, wouldn't it?

Intersections don't actually do supertrype reduction, so we'd still be testing to see if one side is the supertype of another and simplifying to the supertype.

But pretty much, yeah.

but "a" & S[]

Counterexample, we have branded literals:

type Brand<TString extends string, TBrand extends object> = TString & TBrand; // Have seen this type in the wild

type PathBrand = { __path: void; };
type Path<T extends string = string> = Brand<T, PathBrand>;

declare function isPathOrEmpty(x: any): x is PathBrand | ""; // checks if string is path-y or empty

function f(foo: "/usr/home" | "/tmp") { // literals used directly instead of branded versions
    if (isPathOrEmpty(foo)) {
        return foo; // Should definitely be `("/usr/home" & PathBrand) | ("/tmp" & PathBrand)`
        // But `PathBrand` is "totally unrelated" to "/tmp" 
    }
}

[andrewbranch]

Sure, of course you're still right. It just feels bad 😄

I'm not sure what I would really propose here. I think most of these results appear really unintuitive, but I guess they don’t stop you from using the variable the way you would before.

And it catches an actual bug bug. Wow.

[andrewbranch]

I've updated to this approach. You can see the handful of baselines change that look like these examples.

[andrewbranch]

So this has always been unsafe, and just now gets caught 😅

[andrewbranch]

This change actually created a compilation error in TypeScript’s source itself:

Before

After

Both hasType and hasInitializer narrow to a union, so previously, the latter simply filtered the first. Now, it's a union of 300 intersections 🙃

We should probably see how much real-world code breaks with this. @weswigham isn't there a way to trigger a test like that?

(Pushed the wrong button 👇 )

[weswigham]

@weswigham isn't there a way to trigger a test like that?

@typescript-bot test this
@typescript-bot run dt

[typescript-bot]

Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 9733060. You can monitor the build here. It should now contribute to this PR's status checks.

[typescript-bot]

Heya @weswigham, I've started to run the extended test suite on this PR at 9733060. You can monitor the build here. It should now contribute to this PR's status checks.

[RyanCavanaugh]

RWC looks like it has some unrelated diffs that are making it difficult to assess impact there.

At a minimum we should delay this fix by a release since we're trying to have 3.5 have as few breaking changes as possible.

To the general point of what constitutes a correct narrowing and what doesn't, in general we disavow the existence of some arbitrary T & U for any T and U that don't have a pre-existing relationship (e.g. given Cat | Mortgage, the possibility of a Cat & Mortgage is assumed to be impossible-ish even in the absence of an explicit discriminant).

It feels like there should be a more targeted fix for the original problem reported, but maybe not.

[weswigham]

To the general point of what constitutes a correct narrowing and what doesn't, in general we disavow the existence of some arbitrary T & U for any T and U that don't have a pre-existing relationship (e.g. given Cat | Mortgage, the possibility of a Cat & Mortgage is assumed to be impossible-ish even in the absence of an explicit discriminant).

I don't think this is true - we have a set number of scenarios and operators where we intentionally pretend that's the case, like instanceof and in narrowing, but in general point out when this could be the case since it finds real errors in the absence of an intent otherwise.

[RyanCavanaugh]

I don't think this is true - we have a set number of scenarios and operators where we intentionally pretend that's the case

We're certainly inconsistent and can behave either way depending on context, but there are core scenarios like cat === mortgage which are disallowed even though cat might be an alias to a Cat & Mortgage.

[weswigham]

We're certainly inconsistent and can behave either way depending on context, but there are core scenarios like cat === mortgage which are disallowed even though cat might be an alias to a Cat & Mortgage.

That scenario uses comparability to determine "correctness" which is even more relaxed than assignability, though - even a single property of overlap will make that check allowable (while the types will won't be assignable in either direction). It's a different trade-off.

[andrewbranch]

We discussed this in a design meeting and decided there’s not currently a solution that’s worth the disruption it will bring. A hypothetical future where we can stop treating “branded primitives” as they exist today (e.g. string & { __kind: 'path' }) as possible value-containing types will help somewhat, and we also discussed this as an indicator that people often think about their object types as being closed, when in fact the type system doesn’t represent this at all (aside from excess property checking on fresh object literals).

I may look into doing the correct thing when all constituents of both the original type and the predicate type are primitives, since intersections of those are more intuitive and fall out when empty. On the other hand, I’m not sure if we want that kind of logical branching—maybe it’s better just to keep it in its current slightly wrong but consistent state. At any rate, this PR is out of scope for that kind of change.