Conditional Prop Incorrectly Required When Using Spreads/Desctructures

[Rascle]

Description

We have a function used across several components which expects an exact object with all conditional props.

type SpreadType = {|
  foo?: string,
  bar?: string,
|};

function useRest(props: SpreadType) {|
  console.log(props);
|}

In a component where the function is used, we import this type and spread it into the component's props' type. This will allow overwriting the original props, because we don't want SpreadType to block any prop typing. (e.g. foo is now required and a boolean).

type AllProps = {|
  ...SpreadType,
  foo: boolean,
  baz: boolean
|};

To pass the component's props to the function, you must destructure and remove props not expected by the function, or props that you have overwritten.

function test(all: AllProps) {
  const { foo, baz, ...rest } = all;
  useRest(rest);
}

Expected Result

We expect the test function to allow us to pass rest as an argument because the type is only {|bar?: string|}.

Actual Result

Even though rest is correctly typed as {|bar?: string|}, flow expects the overwritten property foo to be required in the object functions argument.

Workaround

The only solution I found was to reassign the object by spreading the remaining properties into a new object.

I tried several other ways to address this, including explicitly typing the rest properties after assignment, but that didn't work. The is also no difference in typing the objects as exact or inexact.

Try Flow

/* @flow */

type SpreadType = {|
  foo?: string,
  bar?: string,
|};

type AllProps = {|
  ...SpreadType,
  foo: boolean,
  baz: boolean
|};

function useRest(props: SpreadType) {
  console.log(props);
}

function test(all: AllProps) {
  const { foo, baz, ...rest } = all;
  // (rest: $Rest<All, {|foo: boolean, baz: boolean|}>) // this has no effect.

  // fails because useRest is expecting foo
  useRest(rest);

  // passes
  useRest({...rest});

  // passes if you decide to use inexact types for All and SpreadType
  useRest(Object.assign({}, rest));

  // passes
  useRest({ foo: 'true', bar: 'string'});
}

test({foo: true, baz: true})

Error

              ^ Cannot call `useRest` with `rest` bound to `props` because property `foo` is missing in `rest` [1] but exists in `SpreadType` [2].
References:
19:   const { foo, baz, **...rest** } = all;
                        ^ [1]
14: function useRest(props: **SpreadType**) {
                            ^ [2]```

[wchargin]

This is the clearest presentation of a complex issue that I’ve seen in
a while. Thanks: you made it easy to understand the problem.

Flow is correct here.

You note that rest is correctly typed as {|bar?: string|}. In
particular, rest does not have a property foo.

But a valid implementation of useRest with the same type signature
could be:

function useRest(props: SpreadType) {
  console.log(props);
  props.foo = "hello";
}

If you were allowed to call useRest(rest), then after the call rest
would no longer ascribe to the type {|bar?: string|}, because it would
have an extra property foo.

This explains why the shallow copy ({...rest}) fixes the problem: the
original variable is no longer in danger of being mutated.

You can fix this by explicitly declaring that useRest does not mutate
its argument:

function useRest(props: $ReadOnly<SpreadType>) {
  console.log(props);
}

[joshduck]

Thanks for the explanation. That makes sense, but leads to other questions:

Why does Flow understand the shallow copy is safe to mutate (or safe from mutation?) but not the original rest1 variable? If I make a shallow copy called rest3 then Flow expands the type to be compatible with the argument for useRest, but doesn't try and do that with rest1. That seems inconsistent.

Furthermore, if I shallow copy rest1 to rest2, but don't pass to the function, then Flow seems to incorrectly type it, and doesn't catch the invalid access of the bar property.

function test(all: AllProps) {
  const { foo, baz, ...rest1 } = all;
  
  rest1;                     // Flow types this as {| bar?: string |} (Expected)
  useRest(rest1);            // Error (Expected)
  rest1.bar.toUpperCase();   // Error (Expected)
  
  // An error in Flow? When expanding an object with optional 'bar' key, Flow makes the 
  // type required, and misses the type error.
  const rest2 = { ...rest1 } // Flow types this as  {| bar: string |} (Unexpected)
  rest2.bar.toUpperCase();   // OK (Unexpected)
  
  // This is unexpected. Flow seems to realize that it can broaden the type
  // of rest3 to match the input type of `useRest`. This seems to be sound. 
  // But why does this only happen here, and not when rest is originally extracted 
  // on the first line of this function?
  const rest3 = { ...rest1 } // Flow types this as {| bar?: string, foo?: string |}
  useRest(rest3);            // OK (...but why)
  rest3.bar.toUpperCase();   // Error (Expected)
  rest3.foo;                 // Typed as void | string
}

[wchargin]

Why does Flow understand the shallow copy is safe to mutate (or safe
from mutation?) but not the original rest1 variable?

It’s not that the shallow copy is “safe” to mutate; it’s that it’s at a
different type. When you copy a value at type {|bar?: string|}, you
get a value at type {|bar?: string, foo?: string|}—or with any other
optional properties that you want. You can verify this independently of
the rest of your example.

As for why Flow understands it: it’s just part of the semantics for the
object-spread construction {...rest}; that’s what the typing rules
say (and the typing rules are written that way because it’s as
permissive as possible while still being sound).

(Let me know if this is unclear…)

If I make a shallow copy called rest3 then Flow expands the type to
be compatible with the argument for useRest, but doesn't try and do
that with rest1. That seems inconsistent.

I agree; this is inconsistent. The $Rest type should have the same
semantics as the spread construction as described above.

Furthermore, if I shallow copy rest1 to rest2, but don't pass to
the function, then Flow seems to incorrectly type it, and doesn't
catch the invalid access of the bar property.

You’re correct: this is definitely a bug in Flow. I reported it as #6549
a while ago, and it hasn’t been addressed.

[joshduck]

Thanks for the detailed explanation! Much appreciated.

[Rascle]

Thanks for the explanations. It's clearer now. I think I was (and may still be) a bit confused by the inconsistencies in how rest was being typed.

To clarify: there is an inconsistency in how ...rest is typed in these different cases. There is also a bug with flow incorrectly typing the shallow copy of rest, rest2, as {|bar: string|} when not passing it to useRest. So, should we expect flow to extend the type of rest1 in the original assignment to be {|bar?: string, foo?: string|}, and should that be valid to pass into useRest expecting SpreadType?

Addendum: I'm using the $ReadOnly utility in the function expecting the spread type as a working solution.

[SamChou19815]

Now all examples work except for Object.assign. In that case, we recommend spread