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

Union of unions doesn't work as I expected #582

Closed
jergason opened this issue Jun 28, 2015 · 10 comments
Closed

Union of unions doesn't work as I expected #582

jergason opened this issue Jun 28, 2015 · 10 comments

Comments

@jergason
Copy link
Contributor

I am trying to create a union of union types, and it doesn't work as I expect it to.

/* @flow */

type Foo = 'foo' | 'stuff'
type Bar = 'bar'

type Status = Foo | Bar

type Thingy = {
  id: string;
  status: Status
}

function doStuff(thing: Thingy): Thingy {
  thing.status = 'bar'
  return thing
}

Running flow on this gives the following error:

/Users/jergason/code/flow-test/durp.js:14:18,22: string
This type is incompatible with
/Users/jergason/code/flow-test/durp.js:3:12,26: union type

I expected to be able to create a union out of two union types, but it appears the Status type doesn't pick up values from the Bar type. If I change the code to thing.status = 'foo' or thing.status = 'stuff' it works. If I change the types to:

type Foo = 'foo'
type Bar = 'bar' | 'baz'

then I can assign thing.status = 'bar' or thing.status = 'baz' without problem. It looks like it is only happening when the first type in the union is also a union.

This is on flow 0.12 on Mac OS X.

@samwgoldman
Copy link
Member

Yeah, I can reproduce this too. Here's a slightly simpler test case:

/* @flow */

// no eror
var ok: ('foo' | 'bar') | 'baz' = 'baz'

type FooBar = 'foo' | 'bar'
type Baz = 'baz'
type FooBarBaz = FooBar | Baz

// error
var bad: FooBarBaz = 'baz'

/*
test.js|11 col 22 error|  string
|| This type is incompatible with
test.js|6 col 15 error|  union type
|| 
test.js|7 col 12 error|  string literal baz
|| This type is incompatible with
test.js|6 col 15 error|  union type
|| 
*/

samwgoldman added a commit to samwgoldman/flow that referenced this issue Jun 28, 2015
Union and intersection errors are handled using a speculative match
process, where an error on one side of the match raises an exception,
which is caught by the speculative match code, which then tries the
otehr side of the match.

If, however, there are nested speculative matches in a given flow, the
innermost one would raise an error too early. This can happen if a type
alias is a union containing union types.

To fix, instead of using a boolean to track whether to log the error or
raise, we use an integer representing the "depth" of the speculative
match.

Before, there was some code to "flatten" unions of unions during the
convertion of annotations to types, but this doesn't work for the case
where the unions are themselves type aliases.

Unfortunately, this change affects error messages around union types,
and in my opinion, the new messages are worse and less helpful than they
used to be. Any ideas how to make this change without making error
messages worse?

Fixes facebook#582
@leoasis
Copy link

leoasis commented Jul 29, 2015

Just chiming in here to "vote" for this one to get fixed

@ianobermiller
Copy link
Contributor

Ran into the same thing trying to use a union to represent the different action types in Flux:

type FooActionTypes = {
  type: 'foo/1';
} | {
  type: 'foo/2';
};

type BarActionTypes = {
  type: 'bar/1';
} | {
  type: 'bar/2';
} | {
  type: 'bar/3';
};

type Action =  FooActionTypes | BarActionTypes;

function handle(action: Action) {}

// this call errors with something like "Object literal type incompatible with FooActionTypes union type"
handle({
  type: 'bar/1'
});

handle({
  type: 'foo/1'
});

@niran
Copy link

niran commented Oct 27, 2015

It'd be nice if the docs advised against nesting unions and intersections until this is fixed. It's an easy trap to fall into and it costs a few hours.

@Gozala
Copy link
Contributor

Gozala commented Nov 5, 2015

Is there an ongoing work to fix this that I can follow ?

@paulyoung
Copy link

I just ran into this issue and it appeared to only surface once I split types across files.

@zgao
Copy link

zgao commented Nov 15, 2015

This issue renders flow extremely clunky and essentially unusable for our type system, and we're thinking about switching to TypeScript for this reason alone...

@Gozala
Copy link
Contributor

Gozala commented Nov 16, 2015

Yeah we also started considering typescript as an alternative as we keep hitting this issue over and over, although migration seems like a lot of work so for now we've decided to not type check and hopefully this will be resolved soonish.

@samwgoldman BTW your pull #583 does seem to resolve the issue (at least all occasions we've run into). You did say your fix was pretty hacky on IRC, but do you thing it could still land as temporary fix ? I'm sure users of flow would much rather have a hacky fix for this issue than it being just broken.

@zgao
Copy link

zgao commented Nov 16, 2015

I should comment that we have also started maintaining a separate build of flow with #583, and it works for everything we care about.

@bhosmer
Copy link
Contributor

bhosmer commented Dec 7, 2015

@Gozala et al FYI there are a couple of issues in play here - first has to do with nested unions, second with unions that include externally defined types. Fixes in the works for both. What prevented us from taking @samwgoldman 's nesting fix as-is was (as he mentioned) the regression in error message quality, but we're on it. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants