Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix narrowing union with union type predicate #31206

Closed
wants to merge 4 commits into from
Closed

Fix narrowing union with union type predicate #31206

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e08ce19
Fix narrowing union with union type predicate
andrewbranch May 2, 2019
d6e795a
Don’t throw out unrelated constituents
andrewbranch May 3, 2019
e32849d
Revert accidental indentation change
andrewbranch May 3, 2019
9733060
Fix TypeScript compilation with new changes
andrewbranch May 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16841,15 +16841,22 @@ namespace ts {
return getNarrowedType(type, targetType, assumeTrue, isTypeDerivedFrom);
}

function getNarrowedType(type: Type, candidate: Type, assumeTrue: boolean, isRelated: (source: Type, target: Type) => boolean) {
function getNarrowedType(type: Type, candidate: Type, assumeTrue: boolean, isRelated: (source: Type, target: Type) => boolean): Type {
if (!assumeTrue) {
return filterType(type, t => !isRelated(t, candidate));
}
// If the current type is a union type, remove all constituents that couldn't be instances of
// the candidate type. If one or more constituents remain, return a union of those.
if (type.flags & TypeFlags.Union) {
const assignableType = filterType(type, t => isRelated(t, candidate));
if (!(assignableType.flags & TypeFlags.Never)) {
const assignableType = candidate.flags & TypeFlags.Union
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define it recursively?:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@weswigham weswigham May 3, 2019 •
edited by andrewbranch
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }`)
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@weswigham weswigham May 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// If both the target and the candidate are union types, for each target constituent:
// - if the constituent is a subtype of the candidate, toss it in
// - if any candidate constituents are a subtype of the target constituent, toss ’em in
// - return a union of everything we tossed in.
? mapType(type, t => getNarrowedType(candidate, t, assumeTrue, isRelated))
// If the target type is a union type and the candidate is not, remove all
// constituents that couldn't be instances of the candidate type. If one or
// more constituents remain, return a union of those.
: filterType(type, t => isRelated(t, candidate));
if (assignableType && !(assignableType.flags & TypeFlags.Never)) {
return assignableType;
}
}
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/partiallyDiscriminantedUnions.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ function fail(s: Shapes) {
if (s.kind === "circle") {
>s.kind === "circle" : boolean
>s.kind : "square" | "circle"
>s : Shape
>s : Square | Circle | (Square & Shape[]) | (Circle & Shape[])
>kind : "square" | "circle"
>"circle" : "circle"

let c: Circle = s;
>c : Circle
>s : Circle
>s : Circle | (Circle & Shape[])
}
}
}
2 changes: 1 addition & 1 deletion tests/baselines/reference/stringLiteralCheckedInIf02.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ function f(foo: T) {
>foo : T

return foo;
>foo : S
>foo : "a" | "b" | ("a" & S[]) | ("b" & S[])
}
else {
return foo[0];
Expand Down
68 changes: 68 additions & 0 deletions tests/baselines/reference/typeGuardNarrowsEmptyStringOrUndefinedUnion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
//// [typeGuardNarrowsEmptyStringOrUndefinedUnion.ts]
declare let a: string | undefined;

declare function isEmptyStringOrUndefined(a: string | undefined): a is "" | undefined;
if (isEmptyStringOrUndefined(a)) {
a; // "" | undefined
}

declare function isEmptyStringOrFoo(a: any): a is "" | "foo";
if (isEmptyStringOrFoo(a)) {
a; // "" | "foo"
}

declare function isNumberOrBoolean(a: any): a is number | boolean;
if (isNumberOrBoolean(a)) {
a; // never
}

declare let b: "" | undefined;

declare function isStringOrUndefined(b: any): b is string | undefined;
if (isStringOrUndefined(b)) {
b; // "" | undefined
}

if (isNumberOrBoolean(b)) {
b; // never
}

type A = { a: unknown };
type B = { b: unknown };
declare let c: { a: string } | { z: number };

declare function isAorB(c: any): c is A | B;
if (isAorB(c)) {
c; // { a: string }
}

declare let d: A | B;

declare function hasStringPropertyAOrIsBOrUndefined(d: any): d is { a: string } | B | undefined;
if (hasStringPropertyAOrIsBOrUndefined(d)) {
d; // { a: string } | B
}


//// [typeGuardNarrowsEmptyStringOrUndefinedUnion.js]
if (isEmptyStringOrUndefined(a)) {
a; // "" | undefined
}
if (isEmptyStringOrFoo(a)) {
a; // "" | "foo"
}
if (isNumberOrBoolean(a)) {
a; // never
}
if (isStringOrUndefined(b)) {
b; // "" | undefined
}
if (isNumberOrBoolean(b)) {
b; // never
}
if (isAorB(c)) {
c; // { a: string }
}
if (hasStringPropertyAOrIsBOrUndefined(d)) {
d; // { a: string } | B
}
115 changes: 115 additions & 0 deletions tests/baselines/reference/typeGuardNarrowsEmptyStringOrUndefinedUnion.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardNarrowsEmptyStringOrUndefinedUnion.ts ===
declare let a: string | undefined;
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))

declare function isEmptyStringOrUndefined(a: string | undefined): a is "" | undefined;
>isEmptyStringOrUndefined : Symbol(isEmptyStringOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 34))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 2, 42))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 2, 42))

if (isEmptyStringOrUndefined(a)) {
>isEmptyStringOrUndefined : Symbol(isEmptyStringOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 34))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))

a; // "" | undefined
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))
}

declare function isEmptyStringOrFoo(a: any): a is "" | "foo";
>isEmptyStringOrFoo : Symbol(isEmptyStringOrFoo, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 5, 1))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 7, 36))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 7, 36))

if (isEmptyStringOrFoo(a)) {
>isEmptyStringOrFoo : Symbol(isEmptyStringOrFoo, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 5, 1))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))

a; // "" | "foo"
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))
}

declare function isNumberOrBoolean(a: any): a is number | boolean;
>isNumberOrBoolean : Symbol(isNumberOrBoolean, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 10, 1))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 12, 35))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 12, 35))

if (isNumberOrBoolean(a)) {
>isNumberOrBoolean : Symbol(isNumberOrBoolean, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 10, 1))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))

a; // never
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 0, 11))
}

declare let b: "" | undefined;
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 11))

declare function isStringOrUndefined(b: any): b is string | undefined;
>isStringOrUndefined : Symbol(isStringOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 30))
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 19, 37))
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 19, 37))

if (isStringOrUndefined(b)) {
>isStringOrUndefined : Symbol(isStringOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 30))
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 11))

b; // "" | undefined
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 11))
}

if (isNumberOrBoolean(b)) {
>isNumberOrBoolean : Symbol(isNumberOrBoolean, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 10, 1))
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 11))

b; // never
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 17, 11))
}

type A = { a: unknown };
>A : Symbol(A, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 26, 1))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 28, 10))

type B = { b: unknown };
>B : Symbol(B, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 28, 24))
>b : Symbol(b, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 29, 10))

declare let c: { a: string } | { z: number };
>c : Symbol(c, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 11))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 16))
>z : Symbol(z, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 32))

declare function isAorB(c: any): c is A | B;
>isAorB : Symbol(isAorB, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 45))
>c : Symbol(c, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 32, 24))
>c : Symbol(c, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 32, 24))
>A : Symbol(A, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 26, 1))
>B : Symbol(B, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 28, 24))

if (isAorB(c)) {
>isAorB : Symbol(isAorB, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 45))
>c : Symbol(c, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 11))

c; // { a: string }
>c : Symbol(c, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 30, 11))
}

declare let d: A | B;
>d : Symbol(d, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 37, 11))
>A : Symbol(A, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 26, 1))
>B : Symbol(B, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 28, 24))

declare function hasStringPropertyAOrIsBOrUndefined(d: any): d is { a: string } | B | undefined;
>hasStringPropertyAOrIsBOrUndefined : Symbol(hasStringPropertyAOrIsBOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 37, 21))
>d : Symbol(d, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 39, 52))
>d : Symbol(d, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 39, 52))
>a : Symbol(a, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 39, 67))
>B : Symbol(B, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 28, 24))

if (hasStringPropertyAOrIsBOrUndefined(d)) {
>hasStringPropertyAOrIsBOrUndefined : Symbol(hasStringPropertyAOrIsBOrUndefined, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 37, 21))
>d : Symbol(d, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 37, 11))

d; // { a: string } | B
>d : Symbol(d, Decl(typeGuardNarrowsEmptyStringOrUndefinedUnion.ts, 37, 11))
}

111 changes: 111 additions & 0 deletions tests/baselines/reference/typeGuardNarrowsEmptyStringOrUndefinedUnion.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
=== tests/cases/conformance/expressions/typeGuards/typeGuardNarrowsEmptyStringOrUndefinedUnion.ts ===
declare let a: string | undefined;
>a : string | undefined

declare function isEmptyStringOrUndefined(a: string | undefined): a is "" | undefined;
>isEmptyStringOrUndefined : (a: string | undefined) => a is "" | undefined
>a : string | undefined

if (isEmptyStringOrUndefined(a)) {
>isEmptyStringOrUndefined(a) : boolean
>isEmptyStringOrUndefined : (a: string | undefined) => a is "" | undefined
>a : string | undefined

a; // "" | undefined
>a : "" | undefined
}

declare function isEmptyStringOrFoo(a: any): a is "" | "foo";
>isEmptyStringOrFoo : (a: any) => a is "" | "foo"
>a : any

if (isEmptyStringOrFoo(a)) {
>isEmptyStringOrFoo(a) : boolean
>isEmptyStringOrFoo : (a: any) => a is "" | "foo"
>a : string | undefined

a; // "" | "foo"
>a : "" | "foo"
}

declare function isNumberOrBoolean(a: any): a is number | boolean;
>isNumberOrBoolean : (a: any) => a is number | boolean
>a : any

if (isNumberOrBoolean(a)) {
>isNumberOrBoolean(a) : boolean
>isNumberOrBoolean : (a: any) => a is number | boolean
>a : string | undefined

a; // never
>a : never
}

declare let b: "" | undefined;
>b : "" | undefined

declare function isStringOrUndefined(b: any): b is string | undefined;
>isStringOrUndefined : (b: any) => b is string | undefined
>b : any

if (isStringOrUndefined(b)) {
>isStringOrUndefined(b) : boolean
>isStringOrUndefined : (b: any) => b is string | undefined
>b : "" | undefined

b; // "" | undefined
>b : "" | undefined
}

if (isNumberOrBoolean(b)) {
>isNumberOrBoolean(b) : boolean
>isNumberOrBoolean : (a: any) => a is number | boolean
>b : "" | undefined

b; // never
>b : never
}

type A = { a: unknown };
>A : A
>a : unknown

type B = { b: unknown };
>B : B
>b : unknown

declare let c: { a: string } | { z: number };
>c : { a: string; } | { z: number; }
>a : string
>z : number

declare function isAorB(c: any): c is A | B;
>isAorB : (c: any) => c is A | B
>c : any

if (isAorB(c)) {
>isAorB(c) : boolean
>isAorB : (c: any) => c is A | B
>c : { a: string; } | { z: number; }

c; // { a: string }
>c : { a: string; }
}

declare let d: A | B;
>d : A | B

declare function hasStringPropertyAOrIsBOrUndefined(d: any): d is { a: string } | B | undefined;
>hasStringPropertyAOrIsBOrUndefined : (d: any) => d is B | { a: string; } | undefined
>d : any
>a : string

if (hasStringPropertyAOrIsBOrUndefined(d)) {
>hasStringPropertyAOrIsBOrUndefined(d) : boolean
>hasStringPropertyAOrIsBOrUndefined : (d: any) => d is B | { a: string; } | undefined
>d : A | B

d; // { a: string } | B
>d : B | { a: string; }
}

Loading