instanceof incorrectly guards else block

[tetrogem]

Bug Report

When using instanceof, the else block will remove the type of the class that was guarded from the type's union, even though the variable could have been assigned to a different object with the same interface.

🔎 Search Terms

instanceof guard if else interface class never

🕗 Version & Regression Information

Noticed on TypeScript 5.1.3
Still occurs on Nightly

This is the behavior in every version I tried, and I reviewed the FAQ for entries about common bugs that aren't bugs

⏯ Playground Link

Playground example

💻 Code

class Car {
    drive(): void {
        console.log("Car drive");
    }
}

class Bus {
    drive(): void {
        console.log("Bus drive");
    }
}

function driveCar(car: Car): void {
    if(car instanceof Car) {
        console.log("Hello");
        car.drive();
    } else {
        car.drive(); // 'car' is 'never' here, although this is ran at runtime
    }
}

driveCar(new Bus());

🙁 Actual behavior

In this example, an instance of Bus is passed to a function that takes a value with the interface of Car. Then, an instanceof guard is done to check if car is an instance of the class Car. Because of this, the else block infers car as never, and errors when attempting to call the drive() method. Although, Bus both follows the interface of Car and fails instanceof Car, leading to inconsistent behavior between the TS Compiler and JS, where car actually has a value at runtime, and should not be inferred as a never type.

🙂 Expected behavior

Ideally, the fix for this should be that instanceof guards do not remove types in else blocks, so within it car should continue to be typed as a Car.

[MartinJohns]

It's a design limitation. Unless your classes have a private member they're treated structurally, and structurally both your classes are the same.

[tetrogem]

The problem here though is the the Car in car: Car is a type/interface, whereas the Car in car instanceof Car is a value/class. Doing car instanceof Car does not guarantee that the value of car in the else block will not still be of type Car, just that it is not an instance of the Car class or a class that extends Car.
This already works properly for interfaces/types that aren't the same as the class being checked, so I definitely think this would be possible to fix and should be fixed as it doesn't reflect how the JavaScript it compiles to will run.

[MartinJohns]

Doing car instanceof Car will also not ensure that it's not a car in the else branch, because a matching object literal is assignable to Car, it doesn't have to be an instance of the class.

Again, it's a design limitation. It's best practice to never have fully public classes.

[tetrogem]

Yes, this is exactly why it shouldn't remove the Car type from it in the else block, because it could be an object literal with the same interface. The argument is of the structural type Car, not the nominal type Car, which is why instanceof shouldn't remove it as a possibility in the else block.

E.g.

const bus: Car | null = new Bus(); // this is valid, Bus and Car have the same interface
if(bus instanceof Car) {
    bus; // type: Car (since null would never pass this guard, it makes sense to remove it from the type)
} else {
    bus; // type: Car | null (Another type that implements Car could not be an instanceof Car, so it
         // should be typed as so.  Currently this would just be typed null, which doesn't
         // reflect the runtime behavior where it can be an instance of Bus in this case as well)
}

This is already how this logic works for interface types, I see no reason as to why this can't be changed to follow that logic as well.

[MartinJohns]

This will cause issues on the opposite side of the spectrum. 🤷‍♂️
I can already see the issues coming up like "I checked for class A, why is it still in the else branch?"

Generally it's assumed you either use structurally typed classes or instanceof, not both.

[tetrogem]

Yeah I definitely get what you mean by it possibly being confusing for new learners -- I don't think that a feature being slightly confusing (but still making logical sense once you know that the type Car is an interface that doesn't only accept Car objects) should stop it from being implemented though when the alternative is to just have it not reflect what's actually happening at runtime.

Worst-case scenario this could be put behind a compiler flag, but I definitely think it should be addressed.

[jcalz]

Duplicate #33481

[fatcerberus]

In particular #33481 (comment)

This is an intentional trade-off. Analysis of real code showed that people either use instanceof, or use structural-class matching, but not both.

[RyanCavanaugh]

Aside, this used to be the behavior and everyone hated it.

[typescript-bot]

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.