Flow-related work on the path to RN v0.64 upgrade, part 2.

[chrisbobbe]

This addresses the portion of #3452 (which we should do before the RN v0.64 upgrade, #4426) in our own code, while leaving 36 sites in the libdefs, to be treated in another PR.

Done by temporarily adding implicit-inexact-object=warn to our .flowconfig and fixing all the warnings it flags, as Greg recommends in #3452 (comment).

My focus has been to satisfy the lint rule kind of quickly, using exact (over explicitly inexact) when it's convenient and obviously correct. It's quite possible that more of these could have been made exact—but I don't expect this to cause any type-checking regressions, as explicitly inexact is equivalent to implicitly inexact to the type-checker.

Please give special attention to the last two commits—the rehydrate payload type was looking pretty incorrect, and I think I've improved it. I found that that improvement was possible after making GlobalState readonly, which I do in the second-to-last commit; see discussion where I ask if this is OK.

[gnprice]

Thanks @chrisbobbe ! Generally this looks great. A handful of comments below.

In the interest of simplifying iteration, I'm going to go ahead and merge the 35/39 commits that don't appear in these comments 🙂

[gnprice]

This lets us remove several suppressions at one site -- probably not
because they don't apply anymore, but because Flow doesn't bother to
check for them once it sees the most obvious problem.

😁

It makes sense for GlobalState itself to be read-only, but for result here we really want the read-write version of it -- we're doing these mutations (which would be perfectly well-typed, if only we could lead Flow to use the fact that it's the same key twice), and then we'll return the result and it's fine that the caller promises not to mutate the object further.

One hack for expressing that would be like this:

-  const result: $Shape<GlobalState> = {};
+  const result: $Shape<$Diff<GlobalState, {||}>> = {};

Doesn't super matter here, though it would make the explanation in the comment make a bit more sense. Could be more important somewhere else.

[gnprice]

Hmm, maybe the right answer is that there should be a ReadWrite<T> defined in our generics.js that encodes that trick. Then we'd just use that here.

The $Rest<Foo, { ... }> trick that comes up a lot in exampleData, to make all the properties optional, is another one that'd be good to give that treatment to. (Came to mind because there were a bunch of instances of it in the first commit of this PR.) Maybe named Shape? GoodShape? It's basically a fixed version of $Shape, which is unsound (https://flow.org/en/docs/types/utilities/#toc-shape .)

[chrisbobbe]

Hmm, maybe the right answer is that there should be a ReadWrite<T> defined in our generics.js that encodes that trick. Then we'd just use that here.

That sounds good.

My first thought was that maybe we could implement ReadWrite<T> using $ObjMap instead of $Diff:

• $ObjMap is singled out as having the behavior we want, in the doc, while $Diff is not:

  Additionally, other utility types, such as $ObjMap<T>, may strip any read/write annotations [...]

  (Hm, though I guess it says "may", so it's not for certain.)

• The bug in $Diff, which we'd be relying on for ReadWrite, is documented at $Diff<A, B> causes property variance in A to be lost in resulting type facebook/flow#6225, and is potentially on its way to being fixed. If it got fixed, our ReadWrite would break.

• Someone commented on that Flow issue, linking to a PR, $ObjMap property modifiers facebook/flow#7643, which suggests that $ObjMap might be on its way to growing a nice feature that would help us implement ReadWrite quite transparently, with $ObjMap, with just a small code change.

Sadly I haven't gotten $ObjMap to work as it is currently:

type ReadWrite<T> = $ObjMap<T, <V>(V) => V>;

When I try to use this, Flow still complains with the cannot-write code; you can see this happening on the latest Flow version (alongside an implementation with $Diff that does work) in this repro (let me know if the link doesn't work).

[chrisbobbe]

So I think I'll go with $Diff for now.

[chrisbobbe]

The $Rest<Foo, { ... }> trick that comes up a lot in exampleData, to make all the properties optional, is another one that'd be good to give that treatment to. (Came to mind because there were a bunch of instances of it in the first commit of this PR.) Maybe named Shape? GoodShape? It's basically a fixed version of $Shape, which is unsound (https://flow.org/en/docs/types/utilities/#toc-shape .)

Hmm, yeah, good idea. I see from f7354c1 that $ReadOnly<$Shape<…>> is reportedly sound, while just $Shape is not. Do you think that's right, and do you think that may sensibly remove the need for a Shape, GoodShape, etc.? I think it's a general goal to mark more things $ReadOnly than we do currently.

[gnprice]

I see from f7354c1 that $ReadOnly<$Shape<…>> is reportedly sound, while just $Shape is not.

Hmm, it does say that, but it's not clear what source or reasoning that's based on. In particular that's not mentioned in the upstream docs that point out $Shape is unsound.

Just from a quick look at the example from there in the docs and playing with that in the Flow playground, I think in fact it's not true. Here's the example, with $ReadOnly added:

type Person = $ReadOnly<{
  age: number,
  name: string,
}>;
type PersonDetails = $ReadOnly<$Shape<Person>>;

const personShape: PersonDetails = {age: 28};
(personShape: Person);

That actually does give an error… but only at the {age: 28} literal. Which seems like its own bug -- that line ought to be fine.

And in fact if you adapt the example so it doesn't trip that bug, Flow unsoundly accepts it:

(personShape: PersonDetails): Person => personShape;

(Playground link here.)

So while we do want most of our object types to be $ReadOnly, and that might alleviate some of the symptoms of the unsoundness of $Shape, I don't think it really solves it.

[gnprice]

Can simplify this a bit with plain $ObjMap, and dropping the K parameter.

Hmm, also I think the parameters need to be parameter types of that function type. So like <K, V>(K, V) => V | null.

One reason that's the API is that it actually lets you put more structure in the parameter types -- effectively destructuring them -- like in this example in the docs:

type ExtractReturnObjectType = <K, V>(K, () => V) => { k: K, v: V };

declare function run<O: Object>(o: O): $ObjMapi<O, ExtractReturnObjectType>;

[gnprice]

Hmm, also I think the parameters need to be parameter types of that function type. So like <K, V>(K, V) => V | null.

Ah, yup. As is, in the spot in redux-persist-migrate/index.js where we make one of these, we can put nonsense like { ...migratedState, accounts: 123 } there and with this type Flow doesn't complain. 🙂

Then once this is fixed, Flow does find an error in one of the handful of places we actually consume this type:

  const { payload } = action;
  const haveApiKey = !!(payload && payload.accounts && hasAuth(payload));

saying hasAuth can't accept that payload because all the properties could be null.

A fixme would be pretty reasonable there. But then that actually would make this type lead to less type-checking coverage -- once there's a fixme on that hasAuth(payload), Flow wouldn't complain if that payload.accounts && check went away.

[chrisbobbe]

How about unsoundly casting payload (just where it's passed to hasAuth) to GlobalState, and ignoring specifically that cast and nothing else?

[gnprice]

I think that still doesn't quite get what I want; but I'll push a commit at the end of the branch that revises it further.

In the end I think it winds up better than the code was before -- in particular it really wasn't great for this line over in src/actionTypes.js to have the job of encoding some assumptions that sessionReducer.js is making. We do want something like this about specifically accounts, but we can make it local right next to the code that it's really about.

[gnprice]

Merged these 35 commits:

3fff26a exampleData: Convert object types to exact, or explicitly inexact.
5ef5162 actionTypes: Make recipients and sender exact on EventTypingCommon.
923446d types: Pass exact object as second argument of $Diff wherever we use it.
b3bc0cf webview js types: Make some object types exact.
acc2f13 MessageList types: Mark some object types as explicitly inexact.
d7e94f0 rewriteHTML tests types: Mark an object type as exact.
393826f processAlertWords types: Make an object type exact.
870c20a zulipVersion types: Make an object type exact.
e04db22 types: Make Props exact in a few places we can.
0f9d71f types: Make a few Props definitions explicitly inexact.
08c5f96 SectionSeparatorBetween types: Make some object types explicitly inexact.
6bcd274 UnreadCards types: Make an object type exact.
9b9761a navActions types: Make navigateToRealmInputScreen's args exact.
a5ceaf5 types.js: Make some object types exact.
85a88c7 redux-persist: Make all types in index.js.flow exact or explicitly inexact.
4165606 unreadHelpers: Mark an object explicitly inexact.
d9f2cea StreamList types: Make an object type exact.
8b724c3 actionTypes: Make an object type explicitly inexact.
a3d5872 eventTypes: Make GeneralEvent explicitly inexact.
307dbb9 toggleMobilePushSettings types: Make default export's param type exact.
d87d208 EditStreamCard types: Make initialValues exact.
cabfaf9 reduxTypes [nfc]: Remove NavigationRouteState.
b735bcb reduxTypes: Make TypingState[normalizedRecipients] exact.
27fc2a1 AuthScreen types: Make LinkingEvent explicitly inexact.
98e2da7 messageUpdates types: Make Props.messages[] inexact.
5a06a8b ComposeBox: Make some object types explicitly inexact.
5afe0e0 AppEventHandlers types: Make some object types inexact.
338113d handleOutboundEvents types: Make WebViewOutboundEventError.details exact.
ed1afb6 languages.js types: Make Language exact.
e297dcc messageActionSheet types: Make some object types explicitly inexact.
191c1bd EmojiPickerScreen types: Make an object type exact.
2ba7013 ComposeMenu types: Make an object type explicitly inexact.
65e6bc9 initialDataTypes: Make .realm_authentication_methods explicitly inexact.
1680622 initialDataTypes: Make InitialDataUpdateMessageFlags exact.
2f5fe9c actionTypes: Make subscriptions[] exact on EventSubscriptionRemoveAction.

That leaves these 4:

6060c42 NotSubscribed types: Annotate stream prop after selector's return type.
d4e2ed2 notification types: Make an object type exact.
4445cb2 reduxTypes: Make GlobalState readonly.
dc86ebd reduxTypes: Type the rehydrate payload more accurately.

[chrisbobbe]

In the interest of simplifying iteration, I'm going to go ahead and merge the 35/39 commits that don't appear in these comments 🙂

Good plan; thanks! I'll address those comments and get another revision in soon. 🙂

[chrisbobbe]

Thanks for the review! Revision pushed, and please see my comments above.

[chrisbobbe]

Just rebased past cd29cb9 (I'd forgotten that this old PR also included making GlobalState read-only 🙂).

[gnprice]

Thanks for the revision! Looks good -- merged, with an additional commit on top.

See also replies in threads above: #4520 (comment) and #4520 (comment) .

[chrisbobbe]

Thanks for the review!!