Strict null check flow analysis broken by unrelated branch logic

[donaldpipowitch]

TypeScript Version: 3.8.0-dev.20200104

Search Terms:

flow, branch, unrelated

Code

type PendingRequest<T, E> = {
  pending: true;
  response: T | null;
  error: E | null;
};

type IdleRequest<T, E> = {
  pending: false;
  response: T | null;
  error: E | null;
};

type MyRequest<T, E> = PendingRequest<T, E> | IdleRequest<T, E>;

const request: MyRequest<string, {}> = { pending: false, response: 'hello', error: null };

function someFlow() {
    if (!request.response) return;
    console.log(request.pending);
    return request.response.toUpperCase();
}

function someFlow2() {
    if (!request.response) return;
    console.log(request.pending ? 'pending' : 'nothing');
    return request.response.toUpperCase();
}

Expected behavior:

someFlow2 works just like someFlow.

Actual behavior:

someFlow2 contains an unhandled null case.

Playground Link:

Click me!

Related Issues:

#9998 is about trade-offs in flow analysis, but I'm not sure if this case is a bug or known tradeoff. At least for me it was unexpected.

[RyanCavanaugh]

Simplified

type PendingRequest = {
  pending: true;
  response: string | null;
}

type IdleRequest = {
  pending: false;
  response: string | null;
}

declare const request: PendingRequest | IdleRequest;

function someFlow() {
  if (request.response) {
    if (request.pending) { } // <-- induces error in following line
    request.response.toUpperCase();
  }
}

[ahejlsberg]

This is a known design limitation. In a property access obj.foo, when foo is a discriminant property we reset all control flow types of properties of obj in the following code (i.e. we revert to the declared types) because they may have different types in different variants. In this particular example the pending property is a discriminant property so we reset the control flow type of response in the following code. Ideally we'd "replay" all of the control flow narrowing proofs for the newly selected variants, but we're not really equipped to do that.

[donaldpipowitch]

Ideally we'd "replay" all of the control flow narrowing proofs for the newly selected variants, but we're not really equipped to do that.

Is that out-of-scope (so this issue should be closed) or "just" very hard to do and it will take a while (so the issue can stay open to get notifications on the progress).

Thank you very much for looking into that.

[RyanCavanaugh]

We'd have to totally rethink CFA, so there are no immediate plans in this area.

[ahejlsberg]

So, turns out we don't really need this restriction after all. See #36114.

[donaldpipowitch]

You're my hero of the week @ahejlsberg. Thank you so much for such a quick response.

Also super interesting to see that this was fixed by removing an old workaround.