Skip to content

Type guards have strange narrowing behavior when other guard branches exhaust union constituents #4029

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.

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

Closed
zpdDG4gta8XKpMCd opened this issue Jul 26, 2015 · 13 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@zpdDG4gta8XKpMCd
Copy link

module A {
    export const enum Brand {} 
}
interface A {
    brand: A.Brand;
    a: string;
}
function isA(value: S): value is A {
    return 'a' in <any> value;
}

module B {
    export const enum Brand {}
}
interface B {
    brand: B.Brand;
    b: number;
}
function isB(value: S): value is B {
    return 'b' in <any> value;
}

module C {
    export const enum Brand {}
}
function isC(value: S): value is C {
    return isA(value) && isB(value);
}
interface C {
    brand: C.Brand;
    a: string;
    b: number;  
}
type S = A | B | C;

function test(state: S) {
    if (isA(state)) {
            // works: state is A
    } else if (isB(state)) {
            // works: state is B
    } else if (isC(state)) {
            // works: state is C
    } else {
            // hey, state is A again! how come?
    }
}
@mhegazy
Copy link
Contributor

mhegazy commented Jul 26, 2015

@tinganho wanna take a look?

@mhegazy mhegazy added the Bug A bug in TypeScript label Jul 26, 2015
@tinganho
Copy link
Contributor

@mhegazy I will take a look.

@DanielRosenwasser
Copy link
Member

This is not a type predicate issue, this applies to all type guards:

var x: boolean | string | number;

if (typeof x === "string") {
    // string
}
else if (typeof x === "number") {
    // number
}
else if (typeof x === "boolean") {
    // boolean
}
else if (typeof x === "string") {
    // string   
}
else {
    // ...number?
}

@zpdDG4gta8XKpMCd
Copy link
Author

thanks for narrowing down the problem, interestingly if the last possible case isn't explicitly typeguarded it gets deduced correctly

if (typeof x === "string") {
    // string
}
else if (typeof x === "number") {
    // number
}
//else if (typeof x === "boolean") {
//    // boolean
//}
//else if (typeof x === "string") {
//    // string   
//}
else {
    // boolean as aexpected
}

@tinganho
Copy link
Contributor

The root cause is that it narrows down to 1 type. There is nothing that handles if the narrowing type is the same as narrowed type by an if clause.

if (isA(state)) { // narrowed to A again
} 
else if (isB(state)) { //  narrowed to A
} 
else if (isC(state)) { // narrowed to A | B
} 
else { // type begins with to A | B | C
     state // is A
}

we could probably just add one check for it:

if (isA(state)) { // isA(...) returns A and the narrowing type has the value of A so snap back to A | B | C
} 
else if (isB(state)) { //  narrowed to A
} 
else if (isC(state)) { // narrowed to A | B
} 
else { // type begins with to A | B | C
     state // is A | B | C
}

@zpdDG4gta8XKpMCd
Copy link
Author

I'd expect from practical standpoint since we exhausted all valid cases the only acceptable type for the final else should be void. Am I making sense? Would it violate anything in TS architecture?

if (isA(state)) { // isA(...) returns A and the narrowing type has the value of A so snap back to A | B | C
} 
else if (isB(state)) { //  narrowed to A
} 
else if (isC(state)) { // narrowed to A | B
} 
else {
    // can we have state of "void" in here?
}

@tinganho
Copy link
Contributor

Also instanceof doesn't seem to narrow the type correctly :

class A {
    a: boolean;
}
class B {
    b: boolean;
}
class C {
    c: boolean;
}


let s: A | B | C;
if (s instanceof A) {
    s.a;
}
else if(s instanceof B) {
    s.b;
}
else {
    s;// type is A | B | C, should be just C
}

http://www.typescriptlang.org/Playground#src=class%20A%20%7B%0A%09a%3A%20boolean%3B%0A%7D%0Aclass%20B%20%7B%0A%09b%3A%20boolean%3B%0A%7D%0Aclass%20C%20%7B%0A%09c%3A%20boolean%3B%0A%7D%0A%0A%0Alet%20s%3A%20A%20%7C%20B%20%7C%20C%3B%0Aif%20(s%20instanceof%20A)%20%7B%0A%09s.a%3B%0A%7D%0Aelse%20if(s%20instanceof%20B)%20%7B%0A%09s.b%3B%0A%7D%0Aelse%20%7B%0A%09s.b%3B%0A%7D

@tinganho
Copy link
Contributor

@Aleksey-Bykov it probably makes more sense with void 👍

@zpdDG4gta8XKpMCd
Copy link
Author

hm.. i think there might be something in the specs that clears this out

@RyanCavanaugh
Copy link
Member

@tinganho See #1719 re: instanceof not affecting else blocks

@DanielRosenwasser DanielRosenwasser changed the title union types + type guards look not working in LKG Type guards have strange narrowing behavior when other guard branches exhaust union constituents Jul 27, 2015
@tinganho
Copy link
Contributor

I just submitted a PR to handle void narrowing #4051.

I'm just wondering how to handle the case when dealing with distinct types(not union types).

let x: number;
if (typeof x === "number") {
}
else {
      x// is what? void?
}

Though, I think it would mean walking up the parent for every variable, if we support distinct types as well.

@zpdDG4gta8XKpMCd
Copy link
Author

I'm just wondering how to handle the case when dealing with distinct types(not union types).

TLDR: should be void

Isn't a distinct case just a special case of a union of one? If so, then void in the else block is quite expected. More to that, if you think about it, what else can it be? It's declared to be of a certain type and this is what we just asserted. According to its declaration it cannot be anything else. So the else block is technically unreachable and doesn't make sense, because the clause always holds true as long as the type system is sound. I'd suggested to disallow such if statements altogether due to lack of sense they make.

@sandersn
Copy link
Member

This is fixed in 1.8. When the union is exhausted, the original type is used instead.

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Nov 17, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

6 participants