explicit type narrowing is not working

[fieteboerner]

Bug Report

🔎 Search Terms

explicit type narrowing type narrowing by value

🕗 Version & Regression Information

• This is a crash
• This changed between versions ______ and _______
• This is the behavior in every version I tried, and I reviewed the FAQ for entries about _________
• I was unable to test this on prior versions because _______

⏯ Playground Link

Playground link with relevant code

💻 Code

type ApiResponse = {
    success: true,
    data: string
}

type ApiError = {
    isError: true,
    error: Error
}

// Not longer working
const request = async (): Promise<ApiResponse> => {
    const response: ApiResponse | ApiError = await { success: true, data: 'hello' } as any;
    if (response && typeof response === 'object' && 'isError' in response && response.isError) {
        response
        throw response.error
    }

    return response
}

// Working without the truthiness check inside the condition
const request2 = async (): Promise<ApiResponse> => {
    const response: ApiResponse | ApiError = await { success: true, data: 'hello' } as any;
    if (response && typeof response === 'object' && 'isError' in response) {
        response
        throw response.error
    }

    return response
}

🙁 Actual behavior

TS is complaining in line 19 about unassignable types becauce it thinks the responce can be an ApiError but it technically cant because of the if block and throw statement. The interesting thing is, if I omit the response.isError truthiness check, the code is working. Obviously typescript is thinking about of isError can be a boolean, but by definition it always can have the value true.

🙂 Expected behavior

In this case I expect, typescript can narrow down the type in the if statement to ApiResponse and knows after the if statement the response can only be of type ApiResponse.

[Andarist]

I'm not sure how helpful this is... but the visualized control flow graph of this function looks like this:

Branch ┬ False (response) ───────────────────────────────────────────────────────────────────────────────────────────────────────────┬ Assignment (response: ApiResponse | ApiError = { } as any) ─ Start ((): ApiResponse => {
    const response: ApiResponse | ApiError = { } as any;
    if (response && typeof response === 'object' && 'isError' in response && response.isError) {
        response
        throw response.error
    }

    response

    return response
})  
       ├ False (typeof response === 'object') ─────────────────────────────────────────────────────────────────────┬ True (response) ╯                                                                
       ├ False ('isError' in response) ──────────────────────────────────────┬ True (typeof response === 'object') ╯                                                                                  
       ╰ False (response.isError) ───────────── True ('isError' in response) ╯

[Andarist]

If we analyze what happens with the response identifier after this if statement we can learn that this node has 4 antecedents. They roughly represent this:

[
	{ node: "response", flags: FlowFlags.FalseCondition | FlowFlags.Referenced },
	{ node: "typeof response === 'object'", flags: FlowFlags.FalseCondition | FlowFlags.Referenced },
	{ node: "'isError' in response", flags: FlowFlags.FalseCondition | FlowFlags.Referenced },
	{ node: "response.isError", flags: FlowFlags.FalseCondition | FlowFlags.Referenced },
]

All of those are just parts of that condition in the if statement. So far it looks OK as all of those have FlowFlags.FalseCondition. For each antecedent a possible type is computed, unique types are gathered and a union of those is created. The iteration through antecedents happens here: https://github.dev/microsoft/TypeScript/blob/85d405a1d74c0730a9d8d6307b26e8d6f3f75421/src/compiler/checker.ts#L25066-L25091

The problem is that each antecedent checks its own antecedent (here). So for the "final" response.isError that's isError' in response and the type of that antecedent is ApiError as its own flow node has FlowFlags.TrueCondition.

This type is being narrowed down with the assumeTrue (aka (flow.flags & FlowFlags.TrueCondition) !== 0) that relates to that "final" response.isError. However, this can't be narrowed by truthiness (here) because this property access is not discriminant and thus the input type is simply returned: https://github.dev/microsoft/TypeScript/blob/85d405a1d74c0730a9d8d6307b26e8d6f3f75421/src/compiler/checker.ts#L25318-L25322

All of that makes this ApiError be returned to that loop that iterates through our 4 antecedents and it's pushed to those unique types there. So the overall type for the response identifier outside of this if statement effectively stays not narrowed down at all and this causes the reported error.

I'm not familiar with this part of the code at all and it was the first time I've looked into how flow nodes are consumed and how antecedents are created. It feels to me though that perhaps the issue is that response has 4 separate antecedents in the first place. If the whole binary expression used in the if statement would be treated as a single antecedent then maybe this issue wouldn't happen at all.

I've tried adjusting some things in the narrowTypeByTruthiness but nothing had satisfactory results. Then I tried to somehow backtrack the context through parent nodes etc but that seemed counter-productive as I was trying to derive the information about this being a part of some binary expression and that's complicated and error-prone.

[ahejlsberg]

The core issue here is that we only do discriminant narrowing of union types where (a) at least two variants declare a discriminant property with the same name and different types, and (b) at least one of the discriminant property declarations specify a unit type. In the example, neither success nor isError is considered a discriminant property, so no narrowing occurs as a result of the response.isError test. Thus, we consider response to have type ApiError in both the true and the false brach, and therefore it looks to CFA as if response could be ApiError after the if statement.

Here's a simplified example:

function test(response: ApiResponse | ApiError) {
    if ('isError' in response) {
        if (response.isError) {
            throw 0;
        }
        else {
            response;  // ApiError, but ideally should be never
        }
    }
    else {
        response;  // ApiResponse
    }
    response;  // ApiResponse | ApiError, but ideally should be ApiResponse
}

Above, the response variable ideally should have been narrowed to never in the false branch of the innermost if. And, since the false branch is the only one that completes normally, response should have just been ApiResponse at the bottom of the function.

I'm going to look at dropping requirement (a) above. It would make this example work, but also potentially create more work for the control flow analyzer.