[nightly] [regression] Control flow analysis for destructured unions gets invalidated by later assignment

[MichaelMitchell-at]

🔎 Search Terms

control flow assignment destructured discriminated union

🕗 Version & Regression Information

• This changed between versions 5.3.3 and 5.4.0 nightly

⏯ Playground Link

https://tsplay.dev/mpzrgw

💻 Code

function test({
    other,
    success,
    value,
}: {other: unknown} & (
    | { success: true; value: number }
    | { success: false; value?: number }
)): number {
    if (success) {
        return value;
    //  ^^^^^^^^^^^^^ Type 'number | undefined' is not assignable to type 'number'.
    }

    other = 4; // comment me out and the type error goes away
    return 4;
}

🙁 Actual behavior

Type error because value is not narrowed by success check.

🙂 Expected behavior

No type error because narrowing is correctly applied

Additional information about the issue

Note that this seems to be an extended version of a bug that I discovered has existed since TS 4.6 when control flow analysis for destructured unions was introduced!

function test({
    success,
    value,
}:
    | { success: true; value: number }
    | { success: false; value?: number }): number {
    if (success) {
        return value;
    //  ^^^^^^^^^^^^^ Type 'number | undefined' is not assignable to type 'number'.
    }

    value = 4; // comment me out and the type error goes away
    return 4;
}

[fatcerberus]

This is a form of indirect narrowing, which requires the variable(s) involved to be const. Parameters are mutable, but as a special case can be treated as const only if they are never reassigned anywhere in the function. You should see the same difference in behavior if you do the destructuring inside the function via let vs. const.

[ahejlsberg]

The new behavior (the error caused by assignment to other) looks to be caused by #56313.

[Andarist]

As the author of #56313 , I think that this one worked by accident in the past. Just like @fatcerberus mentioned - if this is supposed to work then I think it will take more than a super simple fix. Those parameters now are treated as mutable (since you reassigned some of them) so the fix would involve fixing this (TS playground):

function test(
  stuff: { other: unknown } & (
    | { success: true; value: number }
    | { success: false; value?: number }
  ),
): number {
  let { other, success, value } = stuff;
  if (success) {
    return value;
  }

  // other = 4; // it should work with `let` with or without this line
  return 4;
}

I don't think there is a big appetite for fixing this and I stand by my PR - I think its impact is net positive even if it broke this case here.

[MichaelMitchell-at]

I can't disagree that the previous behavior only worked unintentionally, but Hyrum's law and all that. @Andarist I don't think your PR needs to reverted. I just think it's a good time to revisit the trade-offs in the pre-existing behavior, mostly in terms of explicability rather than correctness. While the explanations given make sense from the perspective of how the checker happens to be implemented, innocent-looking fragments of code like these are difficult to explain and off-putting to unsophisticated users.

A: why doesn't TS narrow this?
B: because those variables are mutable and could potentially change
A: but they clearly haven't changed
B: yeah, but TS doesn't try to perform arbitrary flow analysis because it quickly becomes intractable and will degrade checker performance
A: it doesn't have to perform arbitrary analysis (though doesn't it kinda imply it does?), can't it just look at a variable and narrow it if it definitely hasn't been reassigned prior to that point?
B: narrowing happens at the point when the conditional happens
A: can't it narrow it and then invalidate that narrowing at the first assignment?
B: what if an assignment happens in a later conditional expression? should the variable be narrowed if it may or may not have been assigned to?
A: obviously no. I'm done with this. I'm switching to Rust
B: good luck with that. you'll run back when you realize you have to learn about non-lexical lifetimes and wait years too for improvements to arrive
A: 🤬
(ok I'm done entertaining myself)

[fatcerberus]

@MichaelMitchell-at Years of exchanges like that happening are how we ultimately get PRs like #56908 so don't give up hope just yet 😉¹

Footnotes

1. ...although then other people are unhappy because this leads to special cases piling up, making the rules complicated and difficult to explain (see exceptions in the linked PR vis-a-vis compound statements)... and there's no formal spec... can't win 'em all I suppose. ↩

[ahejlsberg]

We now only narrow dependent destructured parameters when there are no assignments to any of those parameters. I agree with @Andarist here, analysis to determine that a particular parameter isn't depended upon by other parameters would be complex and of marginal value. So we'll call this a design limitation.