Flow: Boolean OR operator confusing Flow

[iprignano]

Very easy to reproduce. This fairly simple piece of code:

type ReturnType = {
    key: string,
};

function foo(arr1: string[], arr2: string[]): ?ReturnType {
    const a = arr1 && arr1.length ? arr1[0] : null;
    const b = arr2 && arr2.length ? arr2[0] : null;

    if (a || b) {
        return {
            key: a || b,
        };
    }

    return null;
}

Returns this error:

  Property `key` is incompatible:
    157:             key: a || b,
                                   ^^^^^^^^ null. This type is incompatible with
      9:     key: string,
                   ^^^^^^ string. 

In the REPL:
https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgBKeGArgE4B2AKrgQLxgDeqAkANZ5YBcYAzhnIBLSgHMANKgC+AbnRRSlAMYYhcSmChw4ACgCG5cgEZeA4WIDaAXXFgD5AEynBI0dYCUvAPzEyVWvjMqGAhYErqAnZgjPZGYABk8XaGRgB0MHhiGAAWYF7JxhYADFZgvJSkMDByoWERGGAARtEFDglJ9g7pmaI5ea3FpeWV1ei1QlBg+mAAPjNN7kG1teQkFBosy1tgnDxRc02S22CywaFSZ2yrfhoVVXJSQA

[iprignano]

Another, more contrived, probably related example:

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVUCuA7AxgFwEs5swo44AKAQwC4wB+AZ3wCdDsBzAGjACN6zNh04BKei3ZcwAb1RgwASFYBTfJlakaYAD47+oxmG16DYegHIE1fBdQBfVEA

[mroch]

the second example is definitely easier to reason about, so I think we should deal with that first and see if it fixes the first example.

function foo(a: ?string, b: ?string): string {
    return (a || b) ? (a || b) : 'wat'
}

I think what's going on here is that if (a || b) means that in the consequent, a is either a truthy string, in which case b can be anything (in this case, still a ?string), or a is falsy, in which case b must be a truthy string: a is truthy string | falsy string | null | void == ?string, and b is ?string | truthy string == ?string.

it combines the possible outcomes of the condition and checks the consequent once, where a and b are essentially still ?string.

to do what you'd expect, flow would have to essentially rewrite the code to

  if (a) return a || b;
  else if (!a && b) return a || b;
  else return 'wat';

which then reduces to

  if (a) return a;
  else if (b) return b;
  else return 'wat';

that seems great for this example, but the problem is that it's evaluating the return a || b part twice. the combinatorial explosion from longer and more complex conditions and consequents would, I imagine, make this not scale very well. or it's possible that the simpler values prevent downstream work and this is ok perfwise. we probably need to try it to find out.

I did notice that if you extract the a || b into a variable, it works. that's because Flow then sees a single ?string which it refines to string in the consequent:

function foo(a: ?string, b: ?string): string {
    let c = a || b;
    return c ? c : 'wat'
}

(try flow)

Summary

Phew! Long story short, I agree that this isn't ideal and we should try to fix it. Perf may or may not be a blocking issue. In the meantime, minor tweaks to your code will hopefully provide a workaround.

[iprignano]

@mroch That's very helpful – thanks for the detailed (and swift!) explanation.