Flow: Can't use a default value from a destructuring assignment as a computed property since 0.111

[mwiencek]

Flow version: 0.111.1

Expected behavior

/* @flow */

type Args = {|
  required: string,
  optional?: string,
|};

function func({ required, optional = 'default' }: Args) {
  // This should be allowed, since `optional` should always be a string?
  ({ [optional]: null });
}

func({ required: '' });

Actual behavior

    9:   ({ [optional]: null }); // errors, but worked in 0.110
             ^ Cannot use `optional` [1] as a computed property. Computed properties may only be primitive literal values, but default value [1] is a union. Can you add a literal type annotation to `optional` [1]? See https://flow.org/en/docs/types/literals/ for more information on literal types.
        References:
        8: function func({ required, optional = 'default' }: Args) {
                                     ^ [1]

This seems wrong, since optional has a string default if left undefined. This worked in 0.110.

Additionally, casting it to a string works, as in { [(optional: string)]: null }, which I wouldn't expect if optional was actually a union.

• Link to Try-Flow: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgCCATgOYDOYAvGAN4A+qYYxeAjgK4CWrAJgFxhyGYlwB2pADRMwcHBi5wxAQxgB+QcNETp9AL4BudFA5iAxgqVgT5gBS0W7bn0mz5ilTGpgA5LzxQyhwwGD5geoIkFACUdDL2YADacpaeALqCYsFeetEGYMDAYHjExHDE5K4ARhwYYAjlANZ4vGDiYAAMAHQAjD0d8Q6Jtikeqpoi4qTRGWBZMDl5BUVmysJTYBhwQpMS9U3kqHrGpmYJrJw8LYI+YblGQA

[frontendphil]

The default assignment only works when optional is undefined. When you pass { optional: null } it would not default to 'default' but actually be null which breaks your type definition.

[jbrown215]

The union here is string | 'default'. Flow unsoundly handles computed props that are typed as string, which is why the error goes away if you type it like that.

How do you intend to use the object with the computed prop? maybe there's a better way to type it

[mwiencek]

@frontendphil Flow doesn't allow passing { optional: null }, though — if the property is present it must be a string. (It's true that passing null at run-time wouldn't use the default, but that's why I'm using Flow. :))

[mwiencek]

@jbrown215

The union here is string | 'default'. Flow unsoundly handles computed props that are typed as string, which is why the error goes away if you type it like that.

Ah, that makes sense. I thought it was string | void. So the destructuring assignment and default value aspects are mostly unrelated.

How do you intend to use the object with the computed prop? maybe there's a better way to type it

I have a paginator component that takes a pageVar prop, which is the query parameter that represents the page number in the URL (which varies for some reason).

Something like this (simplifying): https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBLAtgBzgJwBdkwBDAZzACUBTUgY2KnzizAHJ87H2BudQgE8cNMAAUWOSgF4wAb1RgwAahykA5jQBcYAHYBXLACMa+ADSKVazQDVS+APw7yhfBl3qLS5VwCOAVTdnV3dPVABfflRgYDAAFQALDEouQn18XUp0jABlEI8wBAxCBLA1fFIsSixTTQATMHdCOEbCADpUOpp6GHtRADd7MGyAdWKEnQAKS2y8tw9g+bClcsryHTkAbRclgF0dLAwADxo6szA2y-CLAEowaQA+MB3QqPo4TOIxDXdSZvx7mBJnIyhoaOdrDQ7ADZOxIexzn5AhgwOEdBI4FI7o8gZYADykMAJLhQaRyUbjSZItznLaQ6H7UGaFRgACM4Ru4QeliUADkaEdiJD8cBSNybvwgA

[mwiencek]

The other example of where I ran into this issue is in a bit of code where I'm parsing HTML attributes, and want to return an object with the attribute name as the key: https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVUCuA7AxgFwEs5swAHAQwCcBnAUwAl8BbGAQX3yoApqBzGgEoAXGADeAbRpdC2PgF1R0qrL4AaMADptAX3GowYGHXxhsFZnSUy5YAD5nMMGGAC8YUGFxwAJnTD4ABYUppS0dDQBgf4hMgBGmPj+5pbIwGAA5BkA3OiGhFBg3Cn+rmWOzoJRVIiZcRQ+YLEqCUlmFnQ5eWAFRSVu5Rm4MBQ0NBlVYgaG7anuQyNjAHIdXYY63b3FHQPzgVR0UBP6Mx7pNHCp8HyEuGDRB9Mb097Y0mAAbhQwmKUe4N4-FEQuRqPRIkEYpwWol-F8fv4UJkutMDvhMFRSJISopPt9fjpchsgA

I don't mind adding a {[(name: string)]: value} annotation here or in the previous example, but it seemed to be an odd new requirement — I've already annotated name and the function return value indexer with string, and in any case, the string literals are strings too.

[jbrown215]

Annotating with string just hits unsound behavior in Flow, so that's not the real solution. I wrote up a blog post about the new spread changes here, and it includes some recommended fixes for computed props.

[mwiencek]

Thanks @jbrown215. I read that, and "annotate the object with optional properties and then explicitly set the property afterwards" helps (though I can't make the object exact in that case).

Not sure if there's a coherent issue here that you're not already aware of, so it's probably best to close this and manage things with the recommended fixes.

[lukeapage]

@jbrown215 It seems like an excessive fix

"annotate the object with optional properties and then explicitly set the property afterwards"

I have a few new errors where most of the time I need to make the code more verbose in order to make flow happy - yet surely typewise, this fix ends up with exactly the same typings as flow would have inferred?

Personally I would have much preferred setting an arbitrary limit for unions used as keys than making me rewrite my JS into something that does the same thing. It seems like it prevents most uses of computed properties, unless the key can be any string.

Further

Annotating with string just hits unsound behavior in Flow

Is there a plan to make this an error at some point? I don't get any warning that I could be using any key.

Heres a breakdown of one of my cases, with the 3 alternatives open to me. I think I prefer suppressing the flow error out of all of them.

/* @flow */

const a: 'a' = 'a';
const b: 'b' = 'b';

type U = typeof a | typeof b;

type O = { [U]: bool };

function set(param: U) {
  setState({
    [param]: true,
  });
}

function set2(param: U) {
  const o: O = {};
  o[param] = true;
  setState(o);
}

function set3(param: string) {
  setState({
    [param]: true,
  });
}

function setState(o: O) {
}

[jbrown215]

@lukeapage Happy to discuss this, since I know it's a pretty disruptive new rule.

The most precise thing to infer for this is a union of object types, each of which contains one member from the union. That's what Flow would naturally infer.

The next most precise things we can infer are

1. An indexer {[Union]: T} or
2. an object type with all optional properties (as suggested above).

There are cases where I'd prefer 1.:

const keys: Array<Union> = [...];
const obj = keys.reduce((k, prev) => {...prev, [k]: 3});

and places where I'd prefer 2.:

type Props = {a?: number, b?: number};
const key: 'a' | 'b' = ...;
const props = {[key]: 3};

Personally I would have much preferred setting an arbitrary limit for unions used as keys than making me rewrite my JS into something that does the same thing. It seems like it prevents most uses of computed properties, unless the key can be any string.

That's a fair third option to consider. I tend to not like these kinds of solutions personally. I hate when I add that last line to a long list and all of a sudden need to completely re-write all of the code. But I am open to having my mind changed!

Is there a plan to make this an error at some point? I don't get any warning that I could be using any key.

Long term ideas, but no concrete plans yet.

[lukeapage]

I see.. that makes sense, sadly.

I hadn’t grasped that the infer was n different types with 1 property on each and essentially there’s no way to cast the object before creation when using computed properties.

[jbrown215]

I see.. that makes sense, sadly.

I'm actually really sad about this too. I was also able to construct cases where you could get into a position where Flow would emit several million unique error messages. All you need are two large enums with no overlap and then you'd get Property 'A' is not in object 1 for every message in the cross product of the two enums. Flow would crash on really big ones.

This was definitely a conservative way to handle the initial release, but I didn't want people in OSS to have to try to debug those crashes either.

Right now, I'm just letting the feedback trickle in and trying to internalize it all. I'm hoping that we can find a _good_ solution here.

Another advantage to being conservative right now is that we are in a position to make any changes necessary to the inference here to make it work! It's really easy to make Flow more complete, but making Flow more sound is a ton of pain.

[zcabjro]

@jbrown215 Do you have any thoughts about cases where we definitely want to infer an indexer {[Union]: T}?

For example:

type Enum = 'A' | 'B' | 'C';
type S = { [Enum]: number };

const reduce = (s: S, e: Enum, n: number): S => ({ ...s, [e]: n });

Flow will currently error because of the computed property e. The rationale stated in your blog post seems to be a performance concern when having to type-check the resulting union of object types.

We can make it work (if not for https://github.com/facebook/flow/issues/8178):

const reduce = (s: S, e: Enum, n: number): S => {
  const update: S = {};
  update[e] = n;
  // $FlowFixMe: https://github.com/facebook/flow/issues/8178
  return { ...s, ...update };
};

But rewriting our code like this is far from ideal given the frequency of this pattern in reducers etc. It also seems to betray that the performance concern does not apply when inferring an indexer, so I am curious to know if there is some (future?) way we can hint this to flow to circumvent the error.