Flow: Spread operator unsoundly marks optional properties as non-optional

Created on 29 Jun 2018  Â·  7Comments  Â·  Source: facebook/flow

The following code typechecks, but raises a runtime error:

// @flow
type O = {|+a?: string, +b?: number|};
const x: O = {a: "bye"};
const y = {...x};
y.b.toFixed();

h/t @decentralion

soundness spread bug

Most helpful comment

Flow built from master issues the expected error

All 7 comments

This yields coerce:

// @flow
function coerce<T, U>(t: T): U {
  const x: {|+t: T, +not?: true|} = {t};
  const y = {...x};
  if (!y.not) {
    return y.t;
  }
  throw new Error("Unreachable.");
}
const twelve: number = coerce("twelve");

this pr fixes this issue too: https://github.com/facebook/flow/pull/7298

@wchargin While we're waiting for #7298 to be merged, have you found a workaround?

This is the simple case I'm trying to fix (Try Flow):

declare var maybeHasA: ?{a: string};
({...maybeHasA}: {a: string});

While we're waiting for #7298 to be merged, have you found a
workaround?

I don’t believe so, sorry. Candidly, my workaround has been to write my
code more and more like OCaml as opposed to “popular” JavaScript: avoid
things like intersections, spreads, and destructor types ($ObjMap or
$Keys) as much as possible, and stick to discriminated unions of
read-only exact objects everywhere. No fancy tricks. It’s still not
perfect, but it means that I have a somewhat faithful mental model of
what Flow’s doing.

Flow has a lot of cool/powerful features. Many of them work reasonably
well independently. Few of them work well together. If you use them in
the same codebase, eventually you _will_ run into mind-boggling
incompleteness errors and/or determine that a whole section of your
codebase is basically being typed as any. So I just stick to sums and
products and go on my way.

It’s not all bad, though. The OCaml-style code is usually more
understandable in addition to having better typing. Algebraic types
really get you a long way.

That's what I figured; thanks!

I just stick to sums and products

Yeah, this is more or less the style I've adopted myself. I ran into this particular issue while attempting to generate random test inputs in a type-safe manner, which involves a bit more annoying dynamism than usual.

Flow has a lot of cool/powerful features. Many of them work reasonably
well independently. Few of them work well together.

Alack, so true...

It might very well be that the PR will not get merged. The Flow team is working on totally revamped object handling which should also tackle this issue. It might make my PR obsolete.

Flow built from master issues the expected error

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ghost picture ghost  Â·  3Comments

ctrlplusb picture ctrlplusb  Â·  3Comments

cubika picture cubika  Â·  3Comments

davidpelaez picture davidpelaez  Â·  3Comments

bennoleslie picture bennoleslie  Â·  3Comments