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

Destructuring union types with string literals does not work in function parameters #6408

Closed
kristiehowboutdat opened this issue Jun 1, 2018 · 6 comments

Comments

@kristiehowboutdat
Copy link

Issue

Flow throws cryptic error messages when destructuring a union type that contains a string literal inside function parameters. However, if you destructure with the same fields inside the function body, no errors are thrown.

Reproduced using Flow 0.73.0: Try Flow

type MyType = { __typename: 'A' }  | { __typename: 'B' };

// Destructuring in function body - works
const works = (t: MyType) => {
    const { __typename } = t;
    if (__typename === 'A') {
    	console.log('A');
    }
}

// Destructuring in function params - does NOT work
const doesntWork = ({ __typename }: MyType) => {
    if (__typename === 'A') {
    	console.log('A');
    }
}

Errors

12: const doesntWork = ({ __typename }: MyType) => {
                          ^ string literal `B` [1] is incompatible with string literal `A` [2].
References:
1: type MyType = { __typename: 'A' }  | { __typename: 'B' };
                                                      ^ [1]
1: type MyType = { __typename: 'A' }  | { __typename: 'B' };
                               ^ [2]

Expected

Behavior inside the function and inside the function parameters is the same.

Actual

Behavior inside the function parameters throws errors, while doing the exact same destructuring is fine inside the function body.

Related Tickets

#5461
#5745

@mrkev
Copy link
Contributor

mrkev commented Jun 4, 2018

Yeah, this is one worth looking into. Thanks for the report and finding the related tickets 👍

@mikoz93
Copy link

mikoz93 commented Jun 9, 2018

It seems to work fine when you provide a default value to the destructured property, but that defeats the purpose... Try Flow

@kristiehowboutdat
Copy link
Author

Interesting, it "works" (no errors) with a default value that's not a valid string literal based on the union type:
Try Flow

@mrkev
Copy link
Contributor

mrkev commented Jun 12, 2018

Hmmm interesting. Welp, looks like a general problem with destructuring in params then.

@HairyRabbit
Copy link
Contributor

any update here ?
:)

@HairyRabbit
Copy link
Contributor

The code looks also not work,

type Props = {
  value: boolean | string
}

function test({ value = true }: Props) {}

show errors:

5: function test({ value = true }: Props) {}
                   ^ boolean [1] is incompatible with string [2].
References:
2:   value: boolean | string
            ^ [1]
2:   value: boolean | string                      
                      ^ [2]


5: function test({ value = true }: Props) {}
                           ^ boolean [1] is incompatible with string [2].
References:
5: function test({ value = true }: Props) {}
                           ^ [1]
2:   value: boolean | string                      
                      ^ [2]

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

5 participants