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

Union passing depends on the order of options #1455

Closed
adrianheine opened this Issue Feb 25, 2016 · 5 comments

Comments

Projects
None yet
5 participants
@adrianheine
Contributor

adrianheine commented Feb 25, 2016

I noticed you have some known issues with unions and are working on them. Here's another quite reduced case that fails:

::::::::::::::
1.js
::::::::::::::
/* @flow */
import type {Foobar} from "./2"

function create(content: ?Foobar | String | Array<String>) {
}

function node(content: ?Foobar | String | Array<String>) {
  create(content)
}
::::::::::::::
2.js
::::::::::::::
/* @flow */

export class Foobar { }
$ flow init
$ flow check
1.js:8
  8:   create(content)
              ^^^^^^^ array type. This type is incompatible with
  4: function create(content: ?Foobar | String | Array<String>) {
                               ^^^^^^ Foobar


Found 1 error

If I move the array to the beginning of the union, it passes. Likewise, if I merge the two files, it passes.

@adrianheine adrianheine changed the title from Union passing depends on the order of option to Union passing depends on the order of options Feb 25, 2016

@mroch

This comment has been minimized.

Show comment
Hide comment
@mroch

mroch Feb 25, 2016

Contributor

so, the way Flow handles unions is to try each branch and choose whichever one succeeds as the resulting type; if none succeed, then it raises an error. the problem is that it currently defines "succeeds" as "doesn't fail." Due to the way Flow infers each file individually and then merges together the results across files, Foobar is still unresolved when it checks each branch, so it "doesn't fail" and thinks content is a Foobar... obviously incorrect. this is why it passes if you merge the two files, since Foobar is known so it doesn't match and goes onto the next branch of the union.

this is something we've been trying to fix for a while and are currently prioritizing (cc @bhosmer).

Contributor

mroch commented Feb 25, 2016

so, the way Flow handles unions is to try each branch and choose whichever one succeeds as the resulting type; if none succeed, then it raises an error. the problem is that it currently defines "succeeds" as "doesn't fail." Due to the way Flow infers each file individually and then merges together the results across files, Foobar is still unresolved when it checks each branch, so it "doesn't fail" and thinks content is a Foobar... obviously incorrect. this is why it passes if you merge the two files, since Foobar is known so it doesn't match and goes onto the next branch of the union.

this is something we've been trying to fix for a while and are currently prioritizing (cc @bhosmer).

@zgao

This comment has been minimized.

Show comment
Hide comment
@zgao

zgao Mar 8, 2016

Just chipping in and confirming that this is a major issue for us as well. We recently refactored our code and replaced a bunch of union fields with classes, and Flow essentially has stopped working for us in the meantime.

zgao commented Mar 8, 2016

Just chipping in and confirming that this is a major issue for us as well. We recently refactored our code and replaced a bunch of union fields with classes, and Flow essentially has stopped working for us in the meantime.

@zgao

This comment has been minimized.

Show comment
Hide comment
@zgao

zgao Mar 26, 2016

@samwgoldman has there been any progress at all in fixing this? Thanks!

zgao commented Mar 26, 2016

@samwgoldman has there been any progress at all in fixing this? Thanks!

@avikchaudhuri

This comment has been minimized.

Show comment
Hide comment
@avikchaudhuri

avikchaudhuri Mar 26, 2016

Contributor

I'm actively working on fixing this problem, expect some news in a few weeks.

Contributor

avikchaudhuri commented Mar 26, 2016

I'm actively working on fixing this problem, expect some news in a few weeks.

@avikchaudhuri

This comment has been minimized.

Show comment
Hide comment
@avikchaudhuri

avikchaudhuri Jun 1, 2016

Contributor

This will be fixed in an upcoming release.

Contributor

avikchaudhuri commented Jun 1, 2016

This will be fixed in an upcoming release.

@ghost ghost closed this in 2df7671 Jun 10, 2016

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment