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.

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

control flow analysis incorrectly narrows type to never when type-guarded #45664

Closed
pomack opened this issue Aug 31, 2021 · 7 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@pomack
Copy link

pomack commented Aug 31, 2021

Bug Report

The new control flow analysis incorrectly narrows a type that is type-guarded to fallback to never in the else clause when they should either remove the type-guarded type or stay the original type if not explicitly typed earlier. They should only fallback to never when all types are exhausted (if enum or was an initial set of types).

🔎 Search Terms

control flow analysis
never
type narrowing
type guard

🕗 Version & Regression Information

  • This changed between versions 4.3.5 and 4.4
  • This is still a problem in 4.5.0 nightly as of 2021-08-31

⏯ Playground Link

Playground link for 4.3.5 (no error)

Playground link for 4.4.2 (error)

Playground link for 4.5.0-dev.20210831 (error)

💻 Code

interface Box {
    isSmallBox(): this is SmallBox;
    isLargeBox(): this is LargeBox;
    sayHello(): any;
}

interface SmallBox extends Box {
    sayHello(): string;
}

interface LargeBox extends Box {
    sayHello(): number;
}

const b: SmallBox|LargeBox = {
    isSmallBox() { return false; },
    isLargeBox() { return true; },
    sayHello() { return 0.7734; }
}

const isLargeBox = b.isLargeBox();

if(isLargeBox) {
    b.sayHello();
} else {
    b.sayHello();
}

🙁 Actual behavior

The last b.sayHello() has b as type never, causing an error.

🙂 Expected behavior

The last b.sayHello() should have b as type SmallBox

@MartinJohns
Copy link
Contributor

MartinJohns commented Aug 31, 2021

This is the correct behaviour. Your variable b is not typed SmallBox | LargeBox, it is typed LargeBox. TypeScript correctly sees that the value isLargeBox is always true, and as a result you end up with never in the else-branch.
image

You see the behaviour you expect if you type b as SmallBox | LargeBox, e.g. using a type-assertion: const b = { ...} as SmallBox | LargeBox.

You have the same behaviour in TypeScript 4.3.5 when you replace isLargeBox with b.isLargeBox(). TypeScript 4.3.5 did not have the feature to narrow indirectly via local variables: #44730 - You can opt out from this feature by adding an explicit boolean type annotation to your variable: const isLargeBox: boolean = b.isLargeBox();

@MartinJohns
Copy link
Contributor

That b is narrowed to LargeBox despite your explicit type annotation SmallBox | LargeBox is intentional as well. Assigning a value to a variable narrows the variable to the assigned type. Otherwise code like this would not work:

const value: number | undefined = 123;
value.toString(); // If this were still typed `number | undefined` then we couldn't call the function.
// But TypeScript can see that a number is always assigned and the value can't be `undefined`.

@andrewbranch andrewbranch added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 31, 2021
@andrewbranch
Copy link
Member

TypeScript correctly sees that the value isLargeBox is always true

To clarify, it sees this not because of the return true in the object literal initializer; it sees this because of everything else @MartinJohns explained; that b is immediately narrowed to LargeBox and then it simply looks at this is LargeBox and says “yep, it sure is.”

@ilogico
Copy link

ilogico commented Aug 31, 2021

I don't think TypeScript sees isLargeBox always returns true.
Instead, I think it sees sayHello() return number, making b a LargeBox, which means b has an impossible type in the else statement.

You can validate this by checking that b.sayHello() returns number before any narrowing. But b.isLargeBox() is still described as returning an open value: this is LargeBox. TypeScript doesn't look into the function's implementation to see if it always return true or not.

@pomack
Copy link
Author

pomack commented Sep 1, 2021

Looks like I have the wrong minimal testcase. This is what I intended to do, let me know if I should open a different issue:

interface Box {
    isSmallBox(): this is SmallBox;
    isLargeBox(): this is LargeBox;
    sayHello(): any;
}

interface SmallBox extends Box {
    sayTruth(): boolean;
}

interface LargeBox extends Box {
}

function hello(a: SmallBox|LargeBox, b: SmallBox|LargeBox) {
    const isLargeBox = a.isLargeBox() || b.isLargeBox();
    if(isLargeBox) {
        a.sayHello();
    } else {
        a.sayHello();
    }
}

function hello(a: Box, b: Box) {
    const isLargeBox = a.isLargeBox() || b.isLargeBox();
    if(isLargeBox) {
        a.sayHello();
    } else {
        a.sayHello();
    }
}

In both functions, the second a.sayHello() has a as never.
As a workaround, i can have a type attribute that is different between these, but it wasn't an issue in 4.3 or below.

@MartinJohns
Copy link
Contributor

MartinJohns commented Sep 1, 2021

@andrewbranch This updated example actually looks like a bug of the #44730 feature. a.isLargeBox() || b.isLargeBox() should not narrow a to LargeBox.

Ignore that. The issue is that LargeBox and SmallBox are structurally equivalent. If you add properties to each making them distinguishable you get the behaviour you expect.

@pomack Everything that LargeBox says it provides is present in SmallBox as well. And because TypeScript is structurally typed this essentially means that every SmallBox is also a LargeBox. So if you check for LargeBox you end up with never in the else-branch, because every SmallBox is a LargeBox, and it's not a LargeBox, so it can't be a SmallBox either.

This is why I personally prefer to be explicit using discriminated unions:

interface SmallBox { boxType: 'small' }
interface LargeBox { boxType: 'large' }
type Box = SmallBox | LargeBox

Here you have a dedicated property to explicitly distinguish the different types.

@pomack
Copy link
Author

pomack commented Sep 1, 2021

Thanks @MartinJohns Will add the explicit attribute.

@pomack pomack closed this as completed Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants