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

Unsoundly loses one branch when another is uninitialized, at top of loop #8526

Closed
gnprice opened this issue Nov 6, 2020 · 2 comments
Closed

Comments

@gnprice
Copy link
Contributor

gnprice commented Nov 6, 2020

Flow version: v0.137.0 (and older versions on flow.org/try, including at least v0.37.0 which is the oldest)

Expected behavior

Should report an error in the following code, which fails at runtime with TypeError:

let thing;
for (const x of [0, 1]) {
  if (thing) {
    thing.nonsense(); // boom! but no error
  }
  thing = { x: 1 };
}

Actual behavior

No error is reported.

On inspection with an IDE, flow type-at-pos, or $Flow$DebugPrint, Flow is apparently seeing the variable's type where it appears at the top of the loop as simply void, when it should be void | {| x: number |}. That'd be enough to cause this issue.

A key element of the repro case is that on entering the loop, the variable is uninitialized. If instead the variable is initialized with undefined -- which should have the exact same behavior at runtime -- then Flow correctly reports an error:

let other = undefined;
for (const x of [0, 1]) {
  if (other) {
    other.nonsense(); // error, correctly
  }
  other = { x: 1 };
}

and inspection shows it correctly sees the variable's type as void | {| x: number |}.

  • Link to Try-Flow: here
@gnprice
Copy link
Contributor Author

gnprice commented Nov 7, 2020

Here's another bit of unsoundness involving uninitialized variables which is sort of an inverse to this one; not sure if it's the same underlying issue.

If instead of using, and then initializing, the variable in a loop body, we do the same thing in a function expression's body (perhaps because we're expressing the same loop using Array.prototype.forEach)... then instead of forgetting that the variable might be already initialized, Flow forgets that it might still not yet be initialized:

let thing;
[0].forEach(() => {
  thing.f(); // boom! but no error
  thing = { f: () => {} };
})

Inspection shows that Flow sees the variable's type as the type that the code further down would initialize it to.

Here again, if the variable is explicitly initialized to undefined, Flow sees both possibilities and correctly reports an error:

let other = undefined;
[0].forEach(() => {
  other.f(); // error, correctly
  other = { f: () => {} };
})

And here's a Try-Flow link for these.

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jan 7, 2021
Kind of puzzling that Flow doesn't complain about this one on its
own!  As ESLint points out in the rule that we have to disable here,
the `= undefined` has no effect on runtime behavior -- it just
states explicitly what the code was already doing anyway.

But once we do make it explicit, Flow starts noticing that the
`undefined` was propagating into boolean expressions and to an
argument passed to `isSameRecipient` -- which also justifies a
check at the top of that function which by its types had looked
dead.  Fix that function's signature, and booleanize the value
when using as a boolean.

This Flow behavior is concerning enough that we should probably
just stop saying `let foo;`, and always explicitly initialize to
`undefined` when that's what we mean.  (The code is potentially
a bit clearer that way anyway.)  We'll do that next.

I've also reported the Flow issue upstream, as:
  facebook/flow#8526
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jan 8, 2021
Kind of puzzling that Flow doesn't complain about this one on its
own!  As ESLint points out in the rule that we have to disable here,
the `= undefined` has no effect on runtime behavior -- it just
states explicitly what the code was already doing anyway.

But once we do make it explicit, Flow starts noticing that the
`undefined` was propagating into boolean expressions and to an
argument passed to `isSameRecipient` -- which also justifies a
check at the top of that function which by its types had looked
dead.  Fix that function's signature, and booleanize the value
when using as a boolean.

This Flow behavior is concerning enough that we should probably
just stop saying `let foo;`, and always explicitly initialize to
`undefined` when that's what we mean.  (The code is potentially
a bit clearer that way anyway.)  We'll do that next.

I've also reported the Flow issue upstream, as:
  facebook/flow#8526
gnprice added a commit to zulip/zulip-mobile that referenced this issue Nov 5, 2021
We've had this practice for a while, since I discovered the Flow bug
  facebook/flow#8526
that makes it necessary.  But a more recent PR review reminded me
that we hadn't documented it in the style guide:
  #4934 (comment)

So do that.
@SamChou19815
Copy link
Contributor

This is now fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants