Flow: [BUG]: Object Spread (Runtime) broken in v58

Created on 3 Nov 2017 · 12Comments · Source: facebook/flow

Referencing #5243 it would appear that staritng with v58 object spread no longer works properly.

Update: Ok some of this is copied ( #1511 ) -- now noticed that it is reported elsewhere and has been an issue. Other bugs mask the issue. Detailed reporting of the situation follows

On another reason - it doesnt work on older versions when using the Try features either. .. .but it is flawless on my older versions of flow locally.

I made a new issue as I wanted to simplify the reproduction of the issue as much as possible:

It seems like it could be due to this change:

Detect match failures with disjoint unions. Example:
type Foo = { type: 'A', ... } | { type: 'B', ... }`
function match(foo: Foo) {
  switch (foo.type) {
    case 'C': ... // dead branch, now error! (was allowed previously)
    ...
  }
}

This seems like it is going to break 90%+ of projects. You have made object spreading pretty much impossible to use with this (i know i would never upgrade past 57.3 and/or remove Flow if this is the new behavior).

Basically what I see happening is that it is losing the type value when using spread immediately, even though it actually does know the type.

Object Spread Fails 100% of Time Example

/* @flow */

type One = {
  foo: string,
  bar: boolean | string,
};

type Two = {
  foo: string,
  bar: boolean,
};

function convertObjectBugged<O: One>(obj: O): Two {
  const res = {
    ...obj,
    bar: true,
  };
  return res;
}

When we look at the inferred value, we can see that it is actually appropriately understanding the type:

However, when we then look at the return value itself then it lost the inferred type and just sees the spread:

Now this makes sense that this is because of the change noted... but this is absolutely horrible and will make every developer hate Flow -- you are basically saying "any use of object spread will break your app and we no longer support it"

You are now requiring that we check every property on every single object manually... this needs to be a different type to "opt-in" to this behavior for SOME types... like

strict type MyType = ...

Object Spread Fails even when Refined Example

Also - it actually looks like its just completely broken - I am pretty surprised your tests did not catch this:

type One = {
  foo: string,
  bar: boolean | string,
};

type Two = {
  foo: string,
  bar: boolean,
};

function convertObjectBugged(obj: One): Two {
  const res = {
    ...obj,
    bar: true,
  };
  if (typeof res.foo === 'string' && typeof res.bar === 'boolean') {
    return res;
  }
  // also fails
  // if (typeof res.bar !== 'boolean') {
  //   throw new TypeError('Not a Boolean');
  // }
  // if (typeof res.foo !== 'string') {
  //   throw new TypeError('Not a String');
  // }
  throw new TypeError('Failed');
}

spread bug

Source

bradennapier

👍3

All 12 comments

I get the desire to make it more and more type safe, this is going a bit extreme but since it is the only way to be absolutely certain statically it does make sense to enforce (perhaps provide a way to configure this per file or something).

In this case though it is obviously a fail because it can 100% guarantee that the "bar" key is in fact a boolean:

Simple Infer Failure Example

type One = {
  foo: string,
  bar: boolean | string,
};

type Two = {
  foo: string,
  bar: boolean,
};

function convertObjectBugged(obj: One): Two {
  const res = {
    ...obj,
    bar: true,
  };
  return {
    foo: res.foo,
    bar: res.bar,
  };
}

It does work if I then refine the value from there but it should definitely not be necessary:

bradennapier on 3 Nov 2017

Update:

So it appears this has been a problem now since the start. Flow fails if you EVER use an object spread anywhere

How is this not a top priority? Stop adding new features and make the simple basic stuff work first.

1511

What is weird is that I apparently never ran into this issue on this specific project because if you use the following config:

module.system.node.resolve_dirname=app
module.system.node.resolve_dirname=node_modules

then Flow simply never reports any errors at any point and reports no errors to indicate it isnt working.

Try Example

If you select new obj and look at the bottom it shows the proper type but still fails.

As for the config thing...

if I do not have the resolve_dirname

add in resolve_dirname=app

module.system.node.resolve_dirname=app
module.system.node.resolve_dirname=node_modules

Actually - adding resolve_dirname=app just seem to make every error go away so its likely the other one I had seen reported that i had forgotten about until now

add in resolve_dirname=./app

.... and if I do relative paths instead...

module.system.node.resolve_dirname=./app
module.system.node.resolve_dirname=node_modules

bradennapier on 3 Nov 2017

So it appears this has been a problem now since the start. Flow fails if you EVER use an object spread anywhere

That's a bit of an overstatement. Object spread works perfectly fine as long as there are no conflicting types, which holds for the majority of use cases.

If your types do conflict then the workaround is relatively easy:

type One = {
  foo: string,
  bar: boolean | string,
};

type Two = {
  foo: string,
  bar: boolean,
};

function convertObjectBugged(obj: One): Two {
  type OneWithoutBar = $Diff<One, { bar : any }>;
  const res = {
    ...(obj : OneWithoutBar),
    bar: true,
  };
  return res;
}

Obviously this is not ideal, but it's workable.

TiddoLangerak on 3 Nov 2017

I fail to see though how this is related to disjoint unions. Can you elaborate?

TiddoLangerak on 3 Nov 2017

Yeah I realized after that it wasn't having to do with disjoint unions.

The problem was that since using the module dir_name feature eats all errors, the repo I created for testing was an older project that was still setup that way and so anything I tested on there appeared to work perfect (it showed it was inferring the correct types, showed perfect coverage, and no errors).

Once I removed the module dir_name config the errors showed up on the older versions as well. I believe I commented that but it got long so I understand it prob got lost in translation.

The post did end up making it seem like a bigger deal than it is, I agree. Largely due to the confusion with the module dir_name bug masking problems and making it seem like it was a much bigger problem then it was.

However, I didn't remove it because it does still contain the most detailed run-through of the situation. I removed any tests that were a result of the dir_name bug.

bradennapier on 3 Nov 2017

Any updates on this? I am using @TiddoLangerak's workaround, but it should not be necessary.

A minimal version of the issue:

/* @flow */
const a: { x: string } = { x: 'x' }
const b: { x: number } = { ...a, x: 2 }

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoAxnAdgZwC5gCGAXGAN5gAep+ATgJZYDmYAvmALzlWkDklvNhmz4wAI1IVqYLAFcAtmICmtNp24A6LYQA0PMACYhQA

draperunner on 23 May 2018

@TiddoLangerak 's workaround doesn't work for exact types

type One = {|
  foo: string,
  bar: boolean | string,
|};

type Two = {|
  foo: string,
  bar: boolean,
|};

function convertObjectBugged(obj: One): Two {
  type OneWithoutBar = $Diff<One, { bar : any }>;
  const res = {
    ...(obj : OneWithoutBar),
    bar: true,
  };
  return res;
}

revyh on 27 Jul 2018

@revyh: You can force the type to be inexact:

// @flow
type One = {| foo: string, bar: boolean | string |};
type Two = {| foo: string, bar: boolean |};

function convertObjectWorkaround(obj: One): Two {
  type OneWithoutBar = { ...$Diff<One, { bar: any }> };
  const res = {
    ...(obj: OneWithoutBar), // OK
    bar: true
  };
  return res;
}

(edit: [Flow Try] link)

wchargin on 27 Sep 2018

OK, I think I start to understand why flow is acting that way and why it's not really a bug (and thus not likely too change…)
This is, I think, the same "issue" as:

const a: number | string = 3;
const b: number = a; // error

In this case, the type of a is number | string and even if a happens to always be a number (because it's a const and thus cannot change), flow does not refine the type for free. When trying to assign a (a number | string) to b (a number), flow complains since the types are indeed incompatible.

While this looks weird for humans (a is obviously a number), this makes sense in the typing world where values are forgotten and only types are kept. flow doesn't know the value of a, only the type we've provided. And there is a type mismatch with b.

And trying to "solve" the issue in this simple case would lead to crazy errors in other cases. Typically is the value of a is the result of a complicated computation, then even if a is constant, flow has no way to figure out the refined type (since it is not going to make that computation). Of course, we could argue that in such an obvious case, flow should do automatic type refinement, but then we could also ask why a has a union type annotation (and the answer is usually "this is a MNWE" which in turns answers the other question that in real life examples, either flow cannot statically refine the union, or the union is not needed…)
That is, if your real life case is const a: number | string = 3, then why on Earth are you using a union type annotation for something that isn't a union? You've told flow a could possibly maybe be a string, don't complain if flow reminds you that "hey, beware, a could be a string"…
And if your real life case is const a: number | string = someComplicatedComputation(), then flow has now way to figure out which of the cases of the union is correct.

Now, with the spread operator, the same thing happens, I guess. When we write:

const res = { ...obj

At that point, res is { foo: obj.foo, bar: obj.bar } and the type of obj.bar is (correctly) a union, so the type of res.bar is (correctly) a union. Assigning res.bar later (whether as part as the assignment as in the examples above, or with an extra res.bar = true does not change its type. It's still a union. And so, res cannot be of type Two since its bar field is and has always been a union.

On the other hand, when using the OneWithoutBar trick, after

const res = { ...(obj: oneWithoutBar)

we've removed bar from the type of obj. So the type of res, at that point, is only { foo: string }. Next, we manually add a bar field with no type annotation and flow types it with the smaller possible type. In this case boolean which makes res compatible with type Two and flow is happy.

So, my opinion so far is that:

This is NOT related to the spread operator.
This is NOT a bug.
And that doesn't make me happy that I cannot complain to somebody else and ask them to "fix" this :-(

Why does this seem related to spread operator? (as many other bug reports/discussions suggest)
Why does this seem to happen especially with disjoint unions? (that's also how I ended up here)

My guess is that the spread operator is making a lot of implicit assignments which are just copies and so the typing looks obvious to humans but isn't once you forget the values. And the same object can be used sometimes for "copy everything, change field foo" and sometimes for "copy everything, change field bar" (reducers, typically, do that). And most of the time, you do not want to change the field the union is decided on. And have very good reason to type that field as an union. But then, flow forgets the value, only remember that the type is a union, cannot auto-refine without knowing the value and/or cannot decide which case of the disjoint union is correct afterwards.

What is a good workaround or flow-friendly way to handle these cases?
I don't know yet… I'll come back if I find something working for me…

Jym77 on 9 Nov 2018

👍1

So, my precise use case was resolved by introducing a few more types and isn't really relevant here.

I've found a workaround that is OK for similar patterns and might be useful to some. It does not solve the original problem here (the OneWithoutBar trick does, and I've found another trick that also solves it), but it does solve similar problems.

// @flow
type FooA = { foo: "a", bar: number };
type FooB = { foo: "b", bar: number };
type Foo = FooA | FooB;

Showing that the issue is about unions, not spread:

const foo: Foo => Foo = (x) => ({foo: x.foo, bar: 3});

This does not work. Flow knows that the type of x.foo is either "a" or "b" but cannot refine it and thus can't find which branch of the union is OK for the result.

Ugly workaround:

const bar: Foo => Foo = (x) => {
  switch (x.foo) {
    case "a":
      return ({...x, bar: 3}: FooA);
    case "b":
      return ({...x, bar: 3}: FooB);
    default:
      return x;
  }
};

Since flow can't find the branch of the union, let's do the job for it. Way too lengthy to be of any real use.

Better workaround:

const baz: FooA => FooA | FooB => FooB = (x) => ({...x, bar: 3});

This is a way better type annotation. It let flow knows that if I send in a FooA, I'll get back a FooA and not a FooB. The Foo => Foo type annotation was imprecise. Oh, and spread operator is not broken at all, by the way…

Of course, this does not solve the original issue:

const cast: FooA => FooB = (x) => ({...x, foo: "b"});

Here, the spread does copy the (type of) x.foo in the new object and that type is "a" which is incompatible with value "b" so flow (correctly) rejects. To handle that, we need either to tell flow to forget the type of x.foo (the OneWithoutBar trick above) or to do a copy that does not copy that field:

const cast2: FooA => FooB = (x) => {
 const { foo, ...rest } = x;
 return {...rest, foo: "b" };
}

Note that the spread operator in object construction does copy ALL the fields, even the ones your are changing immediately after. And thus flow does copy ALL the types and correctly detects mismatch when trying to update the field to an incompatible value. The spread+update syntax is equivalent to:

const cast3: FooA => FooB = (x) => {
 let res = x;
 res.foo ="b";
 return res;
}

This is exactly what the first cast does. And flow sets the type of res.foo to "a" and thus rejects the next assignment.

On the other and, the rest operator (spread in object destruction) does take out the fields you don't want and then typing work. The OneWithoutBar trick is a type annotation to tell flow to not focus on that field. The ...rest trick tell Javascript to totally drop that field (and thus, flow also drops it). I'm not expert enough to try and decide which one has better performances, …

Jym77 on 9 Nov 2018

👍1

This might fix the issue: https://github.com/facebook/flow/pull/7298