Fix object spreading, issues 6108, 7242, 5243, 7210, 7253, 6357, 5253, 4682, 6800, 981, 7048 and more

[villesau]

This fixes bunch of issues with object spreading. Or actually one root cause that had many different symptoms. Now the type is replaced when spread, when before flow made union type out of it, which is not how it should be. In most cases Context.set_prop is just enough: You just want to set the type, and not to do any checks or unions there. Type checks are handled elsewhere so this does not weaken the type safety but makes spreading more sound instead.

To demonstrate the impact, this fixes at least following issues and likely even more that I didn't find or are not obviously related to spreading:

1. Cannot override values when spreading #6108
2. Could not decide which case to select #2892
3. The object spread operator can't be used with union types #1488
4. Object spread properties not working as expected #1511
5. Problem with object spread syntax where the object type has optional values #6751
6. readonly object spread fails: property bar is read-only in object type [1] but writable in object literal [2]. #7242
7. Cannot spread same exact immutable types: null or undefined [1] is incompatible with string [2]. #7210
8. Allow field overrides when spreading types (not spreading values) #7253
9. Spreading to override null produces incorrect inferred object type #6357
10. [BUG]: Object Spread (Runtime) broken in v58 #5253
11. Spreading object breaks optional enum #4682
12. Spread Operator isn't working correctly #5243
13. ignore type of overwritten object spread properties? #6800
14. Object literal with spread property does not correctly overwrite properties #981
15. Flow does not infer type on spread #7048
16. Spread operator inconsistency with optional object properties #7304
17. "Could not decide which case to select" when making copy of union with object spread #7235
18. [bug] union type is not respected when using spread #6974
19. Spreading into an object allows a value of undefined for subsequent properties #6715
20. Spread operator unsoundly marks optional properties as non-optional #6549
21. Spread on union type fails with "Could not decide which case to select" #6342
22. Can't decide between union type cases when using object spread #5154
23. Bug in types inherited from object spread #4338
24. Object spread doesn't seem to behave as expected #4274
25. Flow ignores attribute overwritten with the spread operator (example included) #4045
26. Spreading operator - incorrectly infers the base type. #3342
27. Disjoint enums and spread operator - false incompatibility  #3126
28. Spread operator breaks bounded polymorphism #3061
29. Spread doesn't replace the type of a property that is provided later #2816
30. "This type is incompatible with null" on merging {method: 'POST'} with RequestOptions #2164

The tests follows this naming convention: https://github.com/facebook/flow/blob/master/tests/spread/issue_4152.js

The react_jsx tests actually had wrong expectations, which I changed as well.

For anyone interested in fixing similar issues, here are instructions on how to debug Flow: #4181 (comment)

[villesau]

this pattern match branch was still needed for these to work: https://github.com/facebook/flow/blob/master/tests/destructuring/destructuring.js#L61
https://github.com/facebook/flow/blob/master/tests/object_assign/non_objects.js

[villesau]

https://github.com/facebook/flow/blob/master/tests/destructuring/destructuring.js#L61 does not work without | Some t, to_obj -> branch because this generates OpenT instead of ObjT. Otherwise | Some t, to_obj -> branch could be unnecessary altogether.

[villesau]

not sure if firendly_errors folder is correct folder for these, but https://github.com/facebook/flow/blob/master/tests/friendly_errors/prop-variance.js is perhaps the closest to above tests that I found.

[villesau]

I run this for our codebase and due to this Flow detected dozens of new inconsistent types. In most cases inconsistent maybe types, so plenty of potential runtime failures that has previously been undetected.

So this not only makes Flow more pleasant to use, but also more sound.

[villesau]

@FezVrasta That's unfortunately different issue.

[FezVrasta]

Gotcha! Thanks

[peter-leonov]

Looking forward to fix hundreds of new errors once this gets merged! 😌

[gsklee]

@facebook Could the community get some update on this critical bug? We were promised a really promising, communicative 2019 and got told this issue will be code reviewed soon, but then it's another month of total silence.

It actually makes me feel pretty bad that we have to do something a bit pushy like this to get some progress update, kinda creates a tension between the community and the maintainers. Instead of trying to recruit more devs, I'd say it might actually be more valuable in finding a competent project manager/owner that can help kicking start a healthy communication loop.

[villesau]

This PR is in a bit limbo state at the moment I believe. What I've understood, Flow team is working on totally refactored object model which should also fix spreading issues as well as many other problems with current way of handling objects in Flow. For this reason I don't want to put more effort in this at the moment. Maybe @jbrown215 can shed some light on where the refactoring is going?

[FezVrasta]

@villesau thanks for the update!

May I ask how did you get these info? I would have expected the Flow team to communicate here, after the Medium post they published about the 2019 promises.

Thanks!

[jbrown215]

@gsklee @FezVrasta:

May I ask how did you get these info?

We've been in touch with @villesau on discord. We actually interact with him almost daily there to talk about a variety of issues, and in general we've found the discord to be a source of very good discussions. If you want the more frequent and less formal updates from the Flow team, I recommend joining the discord: https://discord.gg/8ezwRUK

Could the community get some update on this critical bug?

Yes. We are in the middle of a large refactor of our object model. @samwgoldman or I will publish a larger blog post that covers the necessity of the changes we're making. The reason we haven't moved forward on this PR is because of a significant performance regression and because we are still unsure about the correctness.

Fixing object spreads is also my current project (and the vast majority of my day-to-day work). There are a lot more issues than just this one, and I'm aiming to fix all of them. I've been making good progress but I don't want to give an estimate for when it will be released because there are still a few roadblocks that I'm not sure how to get past.

[frontendphil]

@jbrown215 I'm sure there is more to it but I'd still like to point out that releasing some fixes usually is better than releasing no fixes ;) I'm saying this without knowing anything about the implications but sometimes I also need to be reminded of the simple fact that working in small iterations generates more value for end-user than releasing things in large batches.

[jbrown215]

releasing some fixes usually is better than releasing no fixes

I agree! We just aren't positive this is a fix that doesn't introduce other issues, but we also know it introduces a performance issue.

In particular, this fix relies on the constraints flow generates being evaluated in a certain order. There are some cases where that invariant may hold, but it's something we usually assume is false unless a convincing argument is given.

That could lead to something like const x: {foo: number} {...{foo: 'string'}, ...{foo: 3}} to be a type error, which it shouldn't be.

[pkyeck]

So is the buggy object-spreading the reason why this isn't working? Or am I doing something else wrong?

type A = { a: string };
type B = A & { b: string };
type C = A & { b: number };

const testB: B = { a: 'test', b: 'test b' };

function test(obj: B): C {
 return {
   ...obj,
   b: 23,
 }; 
}

test(testB);

    8:  return {
                ^ Cannot return object literal because string [1] is incompatible with number [2] in property `b`.
        References:
        4: type B = A & { b: string };
                             ^ [1]
        5: type C = A & { b: number };
                             ^ [2]

[kangax]

Pretty sure this is it. It also doesn't work with type spreads (which are generally more "predictable" than &'s):

https://flow.org/try/#0C4TwDgpgBAglC8UDeAfKBDAXFAzsATgJYB2A5lCgL4DcAUKJFAEILJoB0nMANFAEbY8RMhRr1w0AMKtUUTux79sxAK4BbPhHyi6tAMYB7YnijAIeJthaIkGbAHIzee7wFRH54P3tQxtAGYqxHrAhEamngAUBnwAVlYAlNjSSLRQ+BDAKvjEyGlQcpwxsdz5SlAATADMpb7UULSUtPRRTsBMCdRAA

[amccloud]

@FezVrasta 👋 heya, did you happen to find the right issue for

https://flow.org/try/#0C4TwDgpgBAglC8UDeVgQM7AFxUwJwEsA7AcygB8oiBXAWwCMI8oBfAbgCgOBjAeyMxQAhjjiIkaTDgBEARmnse-QUIDqBYAAsA8gDcmhACYRRCZJOxRpAJmkAaKADpnQxUA

or

#7298 (comment)

Seems like the type if conflicting with the inferred type of the object? I'm also just trying to provide default options for an argument

[Maggie199]

Any updates here?

[NamPNQ]

Relate issue

const obj = { ...{ hi: () => 1 }, ...{ hi: (x: number) => 'hi' } };
obj.hi(5);

https://flow.org/try/#0MYewdgzgLgBCBGArGBeGBvGA6HmAWAlgFwwAUAlKgHwwCMMAvgDTa4yEmkAeJYArgFt4AUwBOlFDQDkhKY0YBuAFAJEWQqQCs5BUA

[nmote]

This week, @jbrown215 landed some major fixes to spread which will come out in Flow v0.111.0, scheduled for release next week. I haven't checked every associated issue to make sure that they were all fixed, but these changes address most (or all?) of the known issues with spread.

Jordan is working on a blog post with more details.

I think that Jordan's changes accomplish the goals of this PR, so I'm going to close it. @villesau, please let us know if that's not the case.

[nmote]

Update: I went through the list of associated issues and @jbrown215's recent changes address all but one of them.