Flow: Flow loses track of type in some branches

[omninonsense]

As the title says, it seems like Flow loses type information in certain cases. Here is a made-up example (link):

~~~js
// @flow

type Health = {|
total : number,
current : number
|}

type Status = {|
body : Health,

// Some things don't have a weapon
weapon : ?Health
|}

type Props = {|
status : ?Status
//...
|}

export default ({status} : Props) => {
if (!status)
return '???'

if (!status.body)
// Should error here:
return Health: ${status.weapon.current}/${status.weapon.total}

return Health: (${status.weapon ? status.weapon.current : 0}/${status.weapon ? status.weapon.total : 0}) ${status.body.current}/${status.body.total}
}
~~~

The "user code" bug is on line 25, (it should check for !status.weapon, but that's besides the point here, I think).

The flow bug is on line 26; flow should highlight this code as an error, but it doesn't. I am not sure if there is any reason for refinement invalidation to be happening here. If there is, could someone explain why it happens?

Furthermore, the result of type-at-pos for status.weapon.current/total. status.weapon, and status on line 26 is all empty. I understand why status.weapon.current and status.weapon.total are marked as empty, but I don't understand why status and status.weapon are. :confused:

Changing the code to this (link):

~~~js
// @flow

type Health = {|
total : number,
current : number
|}

type Status = {|
body : Health,

// Some things don't have a weapon
weapon : ?Health
|}

type Props = {|
status : ?Status
//...
|}

export default ({status} : Props) => {
if (!status)
return '???'

const { body, weapon } = status

if (!body)
return Health: ${weapon.current}/${weapon.total}

return Health: (${weapon ? weapon.current : 0}/${weapon ? weapon.total : 0}) ${body.current}/${body.total}
}
~~~

Unpacking the values seems to be a workaround for this, and now Flow will generate the errors on line 27, which is as it should be.

[wchargin]

Flow is correct here. Your code is sound: as long as the input parameter
really is a Props, the function will always return a string without
raising an error, so Flow raises no error.

(it should check for !status.weapon, but that's besides the point
here, I think).

Nope, this is not beside the point; it’s actually critical.

The issue boils down to the fact that status is refined to empty
within the conditional. If status is a Status, then status.body
must be an object, so !status.body can _never_ be true. The fact that
status is refined to empty within the conditional indicates that
that conditional is not reachable.

Once status is empty, so is any attribute on status. You could
have written return status.wat; and it would have typechecked—again,
this is okay because the branch is unreachable. So status.weapon has
type empty, and so does status.weapon.current. Because empty is a
subtype of every type, this can be safely upcast to string for the
string interpolation.

(You are correct that there is no refinement invalidation at play.)

Does this make sense?

[anisjonischkeit]

Is it possible to have flow tell you that if (!status.body) will never be truthy and so the code inside will never execute?

[wchargin]

Is it possible to have flow tell you that if (!status.body) will
never be truthy and so the code inside will never execute?

Yes: you can write const z: false = !status.body;, which typechecks
even outside the conditional.

There is the separate question of whether there exists a [lint rule]
that will flag any unreachable branches that do not contain an explicit
empty-assertion*. There is not such a lint rule, but I would support
the addition of one, provided that the details can be worked out in a
sensible way.

* This exception is needed because writing things like

switch (x.type) {
  case "FOO":
    // do things
    break;
  case "BAR":
    // do stuff
    break;
  default:
    throw new Error((x.type: empty));
}

is common and good, and should not raise an error even though the
default branch is unreachable.

An “explicit empty assertion” could also be construed to include
invoking a function whose parameter is declared to have type empty,
for patterns like throw fail(x.type) where fail: (empty) => Error.

[omninonsense]

@wchargin that explanation makes perfect sense; I feel a bit silly for missing that now.

I agree that it would be nice if there was a lint rule for these types of errors. :)

Thank you so much for the detailed explanation, it helped clear things up! :smile:

[wchargin]

You're quite welcome; glad to have helped.