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

Order of type declarations affects union behavior #815

Closed
Gozala opened this Issue Sep 17, 2015 · 7 comments

Comments

Projects
None yet
5 participants
@Gozala
Contributor

Gozala commented Sep 17, 2015

Here is the code to reproduce this issue with 0.15.0 installed as flow-bin from npm:

/* @flow */

type VirtualElement = Thunk|VirtualNode;
type Child = VirtualElement;
type Children = Array<Child>;


class Thunk {}
class VirtualNode {
  children: Child|Children;
  constructor(type, children/*:Children*/) {
    this.children = children.length === 1 ? children[0] :
                    children;
  }
}

I see following errors

/Users/gozala/Projects/reflex/src/core.js: Library type error:
/Users/gozala/Projects/reflex/src/core.js: inconsistent use of library definitions
/Users/gozala/Projects/reflex/src/core.js:5:17,28: array type
This type is incompatible with
/Users/gozala/Projects/reflex/src/core.js:3:23,39: union type

Found 1 error
@Gozala

This comment has been minimized.

Show comment
Hide comment
@Gozala

Gozala Sep 17, 2015

Contributor

Turns out inlining types joined into Child union resolves the issue, here is a code that does not fail:

/* @flow */

type VirtualElement = Thunk|VirtualNode;
type Child = VirtualElement;
type Children = Array<Child>;


class Thunk {}
class VirtualNode {
  children: Children|Thunk|VirtualNode;
  constructor(type, children/*:Children*/) {
    this.children = children.length === 1 ? children[0] :
                    children;
  }
}
Contributor

Gozala commented Sep 17, 2015

Turns out inlining types joined into Child union resolves the issue, here is a code that does not fail:

/* @flow */

type VirtualElement = Thunk|VirtualNode;
type Child = VirtualElement;
type Children = Array<Child>;


class Thunk {}
class VirtualNode {
  children: Children|Thunk|VirtualNode;
  constructor(type, children/*:Children*/) {
    this.children = children.length === 1 ? children[0] :
                    children;
  }
}
@samwgoldman

This comment has been minimized.

Show comment
Hide comment
@samwgoldman

samwgoldman Sep 17, 2015

Member

Sounds like #582. Linking the issues together so we can check/close them all when a fix arrives.

Member

samwgoldman commented Sep 17, 2015

Sounds like #582. Linking the issues together so we can check/close them all when a fix arrives.

@samwgoldman

This comment has been minimized.

Show comment
Hide comment
@samwgoldman

samwgoldman Dec 21, 2015

Member

This is still failing after the fix for #582.

Member

samwgoldman commented Dec 21, 2015

This is still failing after the fix for #582.

@samwgoldman

This comment has been minimized.

Show comment
Hide comment
@samwgoldman

samwgoldman Jan 27, 2016

Member

OK... so I was able to come up with a simple repro here, and it looks like a bug. @bhosmer, you'll probably like this one.

Fails:

/* @flow */
type T = A|B;
class U {};
declare var children: U;
(children: T|U);
class A {};
class B {};
test.js:5
  5: (children: T|U);
      ^^^^^^^^ U. This type is incompatible with
  5: (children: T|U);
                ^ union: A | B

Passes: (I just moved A and B higher in the source)

/* @flow */
type T = A|B;
class U {};
declare var children: U;
class A {};
class B {};
(children: T|U);

Note this is totally non specific to classes. You can change A, B, and U to whatever (number, string, boolean) and get the same error.

Member

samwgoldman commented Jan 27, 2016

OK... so I was able to come up with a simple repro here, and it looks like a bug. @bhosmer, you'll probably like this one.

Fails:

/* @flow */
type T = A|B;
class U {};
declare var children: U;
(children: T|U);
class A {};
class B {};
test.js:5
  5: (children: T|U);
      ^^^^^^^^ U. This type is incompatible with
  5: (children: T|U);
                ^ union: A | B

Passes: (I just moved A and B higher in the source)

/* @flow */
type T = A|B;
class U {};
declare var children: U;
class A {};
class B {};
(children: T|U);

Note this is totally non specific to classes. You can change A, B, and U to whatever (number, string, boolean) and get the same error.

@samwgoldman samwgoldman added the bug label Jan 27, 2016

@samwgoldman samwgoldman changed the title from Child|Array<Child> style types seem to cause type incompatibility to Order of type declarations affects union behavior Jan 27, 2016

@chrisui

This comment has been minimized.

Show comment
Hide comment
@chrisui

chrisui Feb 8, 2016

To save me creating a new issue I'm wondering if the error I'm encountering is caused by the issue seen here?

// decl
declare class Fixture {
  title: string;
  component: any;
  props?: Object;
  children?: Array<Object|string>|string; // this might just be bad syntax but breaks even with Object|string pulled out into a new type
  expect?: {[key:string]: Array<Function>} // see ps. note about this =D
}
// usage
export const a: Fixture = {
  title: 'title',
  component: 'component',
  children: 'Test Child'
};
decl/Fixture.js:9
  9:   children?: Array<Object|string>|string;
                  ^^^^^^^^^^^^^^^^^^^^ array type. This type is incompatible with
 17:   children: 'Test Child'
                 ^^^^^^^^^^^^ string

Ps. Unrelated but is {[key:string]: Array<Function>} a valid type? I'm trying to describe a map with values of the type Array<Function>.

chrisui commented Feb 8, 2016

To save me creating a new issue I'm wondering if the error I'm encountering is caused by the issue seen here?

// decl
declare class Fixture {
  title: string;
  component: any;
  props?: Object;
  children?: Array<Object|string>|string; // this might just be bad syntax but breaks even with Object|string pulled out into a new type
  expect?: {[key:string]: Array<Function>} // see ps. note about this =D
}
// usage
export const a: Fixture = {
  title: 'title',
  component: 'component',
  children: 'Test Child'
};
decl/Fixture.js:9
  9:   children?: Array<Object|string>|string;
                  ^^^^^^^^^^^^^^^^^^^^ array type. This type is incompatible with
 17:   children: 'Test Child'
                 ^^^^^^^^^^^^ string

Ps. Unrelated but is {[key:string]: Array<Function>} a valid type? I'm trying to describe a map with values of the type Array<Function>.

@avikchaudhuri

This comment has been minimized.

Show comment
Hide comment
@avikchaudhuri

avikchaudhuri Jun 1, 2016

Contributor

@Gozala, @samwgoldman: Both of your examples will be fixed in an upcoming release.

@chrisui: you seem to be exploiting an unrelated bug in your example, which has been fixed in an unrelated way: objects cannot be typed as class instances.

Contributor

avikchaudhuri commented Jun 1, 2016

@Gozala, @samwgoldman: Both of your examples will be fixed in an upcoming release.

@chrisui: you seem to be exploiting an unrelated bug in your example, which has been fixed in an unrelated way: objects cannot be typed as class instances.

@avikchaudhuri

This comment has been minimized.

Show comment
Hide comment
@avikchaudhuri

avikchaudhuri Jun 9, 2016

Contributor

@chrisui see also #1922, where I tried to fix your follow up example but ran into another bug

Contributor

avikchaudhuri commented Jun 9, 2016

@chrisui see also #1922, where I tried to fix your follow up example but ran into another bug

@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