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

Regression: Can't use default value in parameter object destructuring when type is union #2198

Closed
Macil opened this issue Aug 5, 2016 · 8 comments
Assignees
Labels

Comments

@Macil
Copy link
Contributor

Macil commented Aug 5, 2016

This broke in Flow v0.29.0. It worked in previous versions.

/* @flow */
function f({a=5}: {a: number|Function}) {
}
$ ./flow29/flow check
test.js:2
  2: function f({a=5}: {a: number|Function}) {
                 ^ function type. This type is incompatible with
  2: function f({a=5}: {a: number|Function}) {
                 ^ number

test.js:2
  2: function f({a=5}: {a: number|Function}) {
                 ^ number. This type is incompatible with
  2: function f({a=5}: {a: number|Function}) {
                 ^ function type


Found 2 errors
@avikchaudhuri
Copy link
Contributor

@samwgoldman This might be because of BecomeT pinning down things after splitting the union?

@samwgoldman
Copy link
Member

Confirmed. I'm on this.

@ricardogama
Copy link

@samwgoldman Any update on this?

@Augustin82
Copy link

We've hit this problem today. We've encountered several open issues on this (such as #183, in the last few comments).

Any way to circumvent that problem?

@threehams
Copy link

It's not pretty, but you can fall back to what babel compiles to:

const Icon = ({ size }: IconProps) => {
  size = size === undefined ? "sm" : size;

@hlomzik
Copy link

hlomzik commented Jun 15, 2017

@threehams or more es6 way — props: IconProps with later destructuring: const { size = "sm" } = props

@augustinr
Copy link

Any updates on this ?

@zeorin
Copy link

zeorin commented Jan 8, 2019

Seems like this is still a problem.

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
Labels
Projects
None yet
Development

No branches or pull requests

9 participants