Suggestion: provide error/warning for always-true type guards / unreachable else blocks

[yortus]

EDIT: the original title was 'Type guards sensitive to type declaration order?', but after some discussion (see comments below) it's clearly not about declaration order, it's really about the behaviour of type guards that always return true because they match all types in the union, leading to unreachable else branches.

---

// CASE 1:
type A = {x;y} // NB: assignable to B
type B = {x}
function isA(arg: A|B): arg is A { return true; }
let ab: A|B;
if (isA(ab)) {
    ab // ab has type A here
}
else {
    ab // ab has type B here
}


// CASE 2: identical to above except order of type declarations reversed
type C = {x}
type D = {x;y} // NB: assignable to C
function isC(arg: C|D): arg is C { return true; }
let cd: C|D;
if (isC(cd)) {
    cd // cd has type C here
}
else {
    cd // cd has type D|C here     <== TYPE NOT NARROWED HERE
}

These two examples both involve a union of two types where one is assignable to the other.

The examples are identical except for the order of declaration of the two types. In the first case, the type is narrowed in both parts of the if statement. In the second case, it's only narrowed in one part of the if statement.

[DanielRosenwasser]

Sorry, I hastily read the issue. I'm not sure why isC removes D in the first place.

[yortus]

My real-life issue with this involves working with a function that has two possible signatures. For simplicity's sake let's say:

type F1 = (x) => any
type F2 = (y, options) => any

I wanted to be able to write a type guard isF2 so I could get narrowing in both branches. But F1 is technically assignable to F2 (even though they are not semantically treated that way), so I get this problem.

[weswigham]

I can explain this! (And why this is expected behavior!)

Read the comments inline:

// CASE 1:
// NB: assignable to B
type A = {x;y}
type B = {x}
function isA(arg: A|B): arg is A { return true; }
let ab: A|B;
if (isA(ab)) {
    ab // ab has type A here
    // This is because type `A` is (obviously) a structural subtype `A`, while `B` is not,
    // meaning `B` is removed from the union and you just have `A`
}
else {
    ab // ab has type B here
    // The compliment of the above - the first branch removed `A`, this branch removes `B`
}


// CASE 2: identical to above except order of type declarations reversed
type C = {x}
// NB: assignable to C
type D = {x;y}
function isC(arg: C|D): arg is C { return true; }
let cd: C|D;
if (isC(cd)) {
    cd // cd has type C here
   // This is a simplification - you could easily still say that `cd` has type `C|D` here,
    // but the type gets collapsed to the common supertype because of the typeguard.
    // Why is the union not narrowed? This is because `D` is a structural subtype of `C`,
    // so it is assumed that `isC` should be true for all `C`s and `D`s!
}
else {
    cd // cd has type D|C here     <== TYPE NOT NARROWED HERE
    // Why is it not narrowed here? Because the result of narrowing for this branch _should_ be
    // `{}` in the absolute sense - this branch should be unreachable if the type guards behave
    // as expected. However, TS follows a "guards should let you do more, not less" philosophy,
    // so they can _never narrow to the empty set_. If this would happen, the original type
    // is used instead.
}

Also, your example isn't "the same except the declaration order is reversed" - in one case your guard is guarding against the subtype, and in the other, the supertype. Which is actually why you can witness this behavior. If your guard was actually against the {x} type both times, you'd see identical behavior.

[yortus]

I understand now that CASE 2 is functioning correctly. My code works if write an isF1 guard function instead of isF2. But I decided to call it isNotF2, since isF1 semantically matches both F1 and F2.

[weswigham]

Long story short, when working with unions of types that are structural subtypes of one another, guard against the most specific types first to actually get narrowing behavior. 👍

[yortus]

@weswigham yep, figured that out now... Just the guard function naming is interesting because you have to guard against the more general type, nor for it (eg isNotSupertype).

EDIT: got myself confused with subtype/supertype with function signatures.

EDIT2: lol, still confused. I think F1 is the subtype and F2 is the supertype, by assignability. But in my code, the syntactic supertype F2 is actually the semantic subtype (ie a special form of the function).

[yortus]

Would it be feasible for tsc to issue a clearer error/warning for CASE 2? i.e., when it sees that the guard always matches everything in the union, so the else block is really unreachable code.

[yortus]

I just wanted to elaborate on my previous confusion about structural subtyping for functions, which led me to write code like CASE 2 without realizing my error. Suppose we have the following API:

type NormalHandler = (request: Request) => Response;
type SpecialHandler = (request: Request, transmogrify: Transmogrifier) => Response;
export function registerHandler(handler: NormalHandler|SpecialHandler) {/*...*/}

I think a lot of people would be fooled by the initially intuitive 'normal' and 'special' labels here. But in terms of structural subtyping, NormalHandler is the more specialized type, and SpecialHandler is the more generalized type.

It's easy given the above to write a guard function like isSpecialHandler, which is effectively useless because it is actually guarding the general type and so never results in narrowing and leads to an unreachable else block. This is exactly like CASE 2 in the original example.

This example would be my real-life rationale for asking that tsc provide an error message when someone writes code like CASE2.

[RyanCavanaugh]

Would it be feasible for tsc to issue a clearer error/warning for CASE 2

The problem is that often times people are writing code like this:

function isActuallyThing(x: any): x is Thing {
  return /* my special way of determining Thingness */
}

function doSomething(x: Thing|string) {
  if (typeof x === 'string') {
    // do something stringy
  } else if (isActuallyThing(x)) {
    // check that JS invocations are still correct
  } else {
    // compute and display meaningful error message
  }
}

We shouldn't error on this code -- it's correct and useful

[yortus]

@RyanCavanaugh something that distinguishes your example from the scenario I'm focusing on is that your example doesn't have a 'useless' type guard anywhere. Your isActuallyThing function narrows from any to Thing. In the CASE 2 example isC doesn't do any narrowing.

This is the scenario where I'm wondering if the compiler could help out with an error or warning, since it seems very likely that the developer did not intend to write a never-narrowing type guard.

My earlier example is a practical case where this really happened to me.

[tinganho]

I think a special casing here would be good. If the union type only consist of two structurally related types and one of them is structurally equal to the type predicate type the type reduction should check against equality(not assignability) to the type predicate type?

If it consist of two or more structurally related types and non of them is equal to the type predicate type it should error, because ordering is quite unpredictable,

[yortus]

Maybe a simpler solution would be to warn directly at the declaration site of a 'useless' custom type guard:

class Animal {walk}
class Dog extends Animal {bark}
class Pug extends Dog {drool}
class Cat extends Animal {meow}
class Animal {walk}
class Dog extends Animal {bark}
class Pug extends Dog {drool}
class Cat extends Animal {meow}

// Current behaviour:
function isAnimal1(x: Animal|Dog|Cat): x is Animal {/***/} // OK
function isAnimal2(x: Animal): x is Animal {/***/}         // OK
function isAnimal3(x: any): x is Animal {/***/}            // OK
function isCat1(x: Animal|Dog|Cat): x is Cat {/***/}       // OK
function isCat2(x: Dog|Cat): x is Cat {/***/}              // OK
function isDog1(x: Animal): x is Dog {/***/}               // OK
function isDog2(x: Dog|Pug): x is Dog {/***/}              // OK
function isDog3(x: Animal|Dog|Pug): x is Dog {/***/}       // OK
function isPug1(x: Animal|Dog|Pug): x is Pug {/***/}       // OK
function isPug2(x: Dog|Cat): x is Pug {/***/}              // OK

// Suggested behaviour:
function isAnimal1(x: Animal|Dog|Cat): x is Animal {/***/} // WARN - no narrowing takes place
function isAnimal2(x: Animal): x is Animal {/***/}         // WARN - no narrowing takes place
function isAnimal3(x: any): x is Animal {/***/}            // OK
function isCat1(x: Animal|Dog|Cat): x is Cat {/***/}       // OK
function isCat2(x: Dog|Cat): x is Cat {/***/}              // OK
function isDog1(x: Animal): x is Dog {/***/}               // OK
function isDog2(x: Dog|Pug): x is Dog {/***/}              // WARN - no narrowing takes place
function isDog3(x: Animal|Dog|Pug): x is Dog {/***/}       // OK
function isPug1(x: Animal|Dog|Pug): x is Pug {/***/}       // OK
function isPug2(x: Dog|Cat): x is Pug {/***/}              // OK

...where 'useless' is defined as the argument type and return type of the custom type guard being mutually assignable.

[tinganho]

function isDog2(x: Dog|Pug): x is Dog {/***/} // WARN - no narrowing takes place

@yortus I think I didn't explain my suggestion so well. My suggestion was to allow the above and have a working solution that could narrow it down to Dog if you pass in Dog|Pug. Since I think many stumble upon this issue.

I think it would be more predictable, since it is a more symmetric solution(referring to OP example you gave). Or what do you think?

[RyanCavanaugh]

function isDog2(x: Dog|Pug): x is Dog {/***/}              // WARN - no narrowing takes place

Isn't the return type x is Dog actually mostly irrelevant to the error the programmer is making? It seems like you're really proposing #3862 -- erroring on Dog|Pug.

In the non-subtype-union case, it's hard to believe that someone wrote

function isAnimal2(x: Animal): x is Animal {/***/} 

accidently and wanted to be stopped from doing so.

[yortus]

I'm just suggesting that perhaps writing function isAnimal2(x: Animal): x is Animal {/***/} should raise an error. In your example you wrote function isActuallyThing(x: any): x is Thing which is much clearer and arguably more correct.

Writing (x: Thing) => x is Thing looks like pure tautology, asserting that a T is a T. If the intent is to say that the input could be really be anything at runtime, but the guard will guarantee it's a Thing, then (x: any) => x is Thing is the correct signature for that.

By the same logic, (x: Animal|Dog) => x is Animal is also a tautology, saying a T is a T. Again, if the input is expected to possibly be something other than an Animal (or its subtypes), then the type guard should allow for that in the annotation.

---

Anyway, coming back to the point of the issue, would it not be useful for the compiler to complain about the following code:

function foo(x: Animal|Dog) {
    if (x instanceof Animal) {
        // Handle Animal...
    }
    else {
        // Handle Dog...
    }
}

It seems pretty obviously incorrect, and indeed won't work, but it compiles just fine. That example might seem contrived but here is an equivalent example, where a complaint from the compiler would be a great help to diagnose what's wrong:

type NormalHandler = (request: Request) => Response;
type SpecialHandler = (request: Request, transmogrify: Transmogrifier) => Response;
function registerHandler(handler: NormalHandler|SpecialHandler) {
    if (isSpecialHandler(handler)) {
        // special handler stuff...
    }
    else {
        // normal handler stuff...
    }
}

With structural subtyping it's not always obvious that one member of the union is a supertype of the others. In this example NormalHandler is a subtype of SpecialHandler, so isSpecialHandler is always true and the else block is unreachable.

[yortus]

@tinganho sorry I missed your second comment.

My suggestion was to allow the above and have a working solution that could narrow it down to Dog if you pass in Dog|Pug.

My point is that no narrowing occurs in this case, because every Pug is also a Dog, so the guard is 'useless'.

[mhegazy]

I do not see the added value here, nor do I see any other place in the system that behave this way. I am inclined to punt on this one. could reconsider if there are new scenarios.

[tinganho]

Shouldn't this at least error on call site?

type C = {x}
type D = {x;y} // NB: assignable to C
function isC(arg: C|D): arg is C { return true; }
let cd: C|D;
if (isC(cd)) {// Error: your type guard is useless.
    cd 
}
else {
    cd 
}

So it can guide you to using @weswigham idea?

Long story short, when working with unions of types that are structural subtypes of one another, guard against the most specific types first to actually get narrowing behavior. 👍