Flow doesn't recognize type possibly widened by mix of env merging and a function call

[samwgoldman]

Given the following:

/* @flow */

function example(b: bool): number {
  var x = 0;
  function f() { x = "" }
  if (b) { f(); }
  return x;
}

I expected an error, something like:

test.js:7:10,10: string
This type is incompatible with
test.js:3:42,45: number

Got: Found 0 errors

I did pull the thread here, and I am quite amazed how smart Flow is. Here's what I understand:

1. Before we start processing the if { ... } statement, we grab the current env and store it away. Let's call this env0. We copy that env and swap in the copy. That copy is env1 and is the environment for the consequent block.
2. In the consequent block, the (FunT,CallT) flow will set scope entries' specific = general—and we can see an expected failure if we remove the if {...} wrapping around the the f() call. I added some printf debugging in Flow_js.havoc_ctx and I can observe x's specific type changing from Num(0) to Root (rank = 0, unresolved = { lower = [NumT(0),StrT("")]; }).
3. There's no alternate block on the if statement, but we copy env0 again. Let's call that env2, which is equal to env0/uninteresting here.
4. Then, after the if { ... } statement is processed, we merge env0 with env1 and env2. We can see in env1 that x's specific type is number | string, but x is not in the changeset, so we never merge the entries from the two envs. Thus, we "revert" back to the env where x is just NumT.

So, it seems that the problem is that x is not in the changeset when we merge the envs. I suspect this is because the changeset that does include x is thrown away when we call Env_js.pop_env after processing the function declaration for f.

If my hypothesis is correct, the fix for this one is going to be pretty involved. FWIW, I came to this test case trying to pick apart this logic and understand it, which I need to do in order to implement let/const. We run into a few similar issues when we add lex scopes, and I am still working out how to manage changesets. Seems like we should figure out this issue and let/const together.

Definitely need some feedback on this one. Thanks :)

[samwgoldman]

Oh, perhaps it's as simple as installing x into the "correct" changeset—that is, not the top one, but the one that corresponds to the scope where the binding is declared.

(Edit: it doesn't help—we explicitly ignore changeset updates outside of the if stmt)

let add_change_var name =
  let rec loop acc = function
    | scope::scopes, changeset::changesets ->
        (match Scope.get_entry name scope with
        | Some _ ->
            let (vars, refs) = changeset in
            let vars = SSet.add name vars in
            List.rev_append acc ((vars, refs)::changesets);
        | None ->
            loop (changeset::acc) (scopes, changesets))
    | _ -> assert_false "uh oh"
  in
  changesets := loop [] (!scopes, !changesets)

[bhosmer]

Yup, you nailed it. The problem is that the havocing that happens in havoc_ctx is lost when envs are swapped around.

The solution is nontrivial but reasonably contained - and it's a step towards dealing properly with read/write sets in functions (so that we can blow away only the variables that a closure actually writes to, among other things). I did a little work on this over the weekend, but could describe it to you in a couple sentences if you wanted to take a crack at it. Otherwise I can probably get it landed by mid-week. Either way, thanks for bumping the issue!

[samwgoldman]

I don't have the bandwidth to take on more stuff, but I'm glad to hear that my analysis was correct. Looking forward to reading your fix. :)