Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Errors when using destructuring and union types #2220

Closed
abalhier opened this issue Aug 9, 2016 · 9 comments
Closed

Errors when using destructuring and union types #2220

abalhier opened this issue Aug 9, 2016 · 9 comments

Comments

@abalhier
Copy link

abalhier commented Aug 9, 2016

Hello !

First thank you for your awesome work on this project ! 👍

On our project we encountered a bug with flow's handling of destructuring. It seems like it does not deduce the attributes available from the property on which destructuring is applied. This bug can be circumvented by using a generic clone function which does the destructuring :

function clone<T>(obj: T) : T {
    return {...obj,};
}

There is quiet the same issue when creating an object from a $Shape of its type and filling the object with default values for missing properties. Using an Object instead of a Shape makes the error disappear.

I pushed an example here : https://github.com/abalhier/flowPlayground/blob/master/destructuring.js
Or you can see errors directly here on flow website.

It seems to happen in flow 0.27+. I didn't try with older versions.

@gabelevi
Copy link
Contributor

There seems to be an unfortunate interaction between the new union type algorithms and spreads. It's not clear where to annotate to clear the ambiguity. Small repro:

type Foo = { type: 'A'; } | { type: 'B'; };

function copy(a: Foo) : Foo {
    return {...a};
}

@avikchaudhuri - mind taking a look?

@avikchaudhuri
Copy link
Contributor

@gabelevi Sure, it's on my queue already! (This problem was noted in the new union type implementation...currently spreads have orthogonal bugs in their implementation that interact especially badly with unions).

@Augustin82
Copy link

Hi all,
we're still encountering the problem.
Any news on this?

@eloytoro
Copy link

Still happening on master branch

@nmn nmn added the bug label Mar 13, 2017
@thymikee
Copy link

Is this on a roadmap? It would be super nice to use destructured action unions in Redux reducers:

export type Action = Update | Remove;
export default function reducer(state: State = initialState, action: Action) {
  const { type, payload } = action;
  switch (type) {
    case 'UPDATE':
      return {
        ...state,
        [payload.field]: payload.value,
      };
    case 'REMOVE':
      return {
        ...state,
        array: state.array.filter(item => item.code !== payload),
      };
  }
}

@eloytoro
Copy link

eloytoro commented Aug 28, 2017 via email

@thymikee
Copy link

@eloytoro thanks, but I would like to do it, like the docs say: https://flow.org/en/docs/react/redux/#toc-typing-redux-reducers

@eloytoro
Copy link

eloytoro commented Aug 28, 2017 via email

facebook-github-bot pushed a commit that referenced this issue Jul 15, 2019
Summary:
Bindings introduced by destructuring a type annotation should behave as
annotations themselves. That is, `p` in `var {p}: {p: T}` should constrain any
subsequent write to be a subtype of `T`.

This logic works today by wrapping the binding in an AnnotT using BecomeT
internally, which works for most cases. However, since BecomeT performs
unification, it is necessary that its lower bound be resolve once and exactly
once.

The once-and-exactly-once invariant is broken by union-like types when combined
with the destructuring operation. Consider the following example:

```
var {p}: {p:string}|{p:number} = ...
```

When evaluating this variable declaration, we emit the following constraints:

1. {p:string}|{p:number} -> GetPropT ('p', T)
2. T -> BecomeT T'
3. P = T'

Constraint (1) is processed by splitting the lower bound, which turns into two
separate constraints `string -> T` and `number -> T`. Since `T` resolves twice,
the `BecomeT` constraint misbehaves and we see a spurious error that `number` is
not compatible with `string`.

This diff deals with the above by handling union-like types specially in the
`DestructuringT` use type.

Fixes #183
Fixes #2198
Fixes #2220
Fixes #4077
Fixes #4270
Fixes #5461
Fixes #5745
Fixes #6408

Reviewed By: panagosg7

Differential Revision: D15457398

fbshipit-source-id: 22c9aba1e1df475c73b36a92bdf7ff5cf57504a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants