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 type passing depends on order of type definition #1663

Closed
willclarktech opened this Issue Apr 14, 2016 · 6 comments

Comments

Projects
None yet
5 participants
@willclarktech

willclarktech commented Apr 14, 2016

I'm using flow-bin 0.23.0 and getting an order effect I don't understand with a union type:

// @flow

type OptionAnswer = {
    kind           : 'OptionAnswer',
    currentAnswer? : {},
}

type TextAnswer = {
    kind           : 'TextAnswer',
    currentAnswer? : string,
}

type Answer
    = TextAnswer
    | OptionAnswer

////////////////////////////////////////////

// fine
const exampleOptionAnswer: OptionAnswer = {
    kind: 'OptionAnswer',
}

// fine
const exampleTextAnswer: TextAnswer = {
    kind: 'TextAnswer',
}

const exampleAnswer1: Answer = {
    kind: 'OptionAnswer',
}

const exampleAnswer2: Answer = {
    kind: 'TextAnswer',
}

This gives the following error:

29: const exampleAnswer1: Answer = {
                                    ^ object literal. This type is incompatible with
 29: const exampleAnswer1: Answer = {
                           ^^^^^^ union: TextAnswer | OptionAnswer
  Member 1:
   14:     = TextAnswer
             ^^^^^^^^^^ TextAnswer
  Error:
   30:     kind: 'OptionAnswer',
                 ^^^^^^^^^^^^^^ string. Expected string literal `TextAnswer`, got `OptionAnswer` instead
    9:     kind           : 'TextAnswer',
                            ^^^^^^^^^^^^ string literal `TextAnswer`
  Member 2:
   15:     | OptionAnswer
             ^^^^^^^^^^^^ OptionAnswer
  Error:
   10:     currentAnswer? : string,
                            ^^^^^^ string. This type is incompatible with
    5:     currentAnswer? : {},
                            ^^ object type

If I switch the union type definition to type Answer = OptionAnswer | TextAnswer I get this instead:

33: const exampleAnswer2: Answer = {
                                    ^ object literal. This type is incompatible with
 33: const exampleAnswer2: Answer = {
                           ^^^^^^ union: OptionAnswer | TextAnswer
  Member 1:
   14:     = OptionAnswer
             ^^^^^^^^^^^^ OptionAnswer
  Error:
   34:     kind: 'TextAnswer',
                 ^^^^^^^^^^^^ string. Expected string literal `OptionAnswer`, got `TextAnswer` instead
    4:     kind           : 'OptionAnswer',
                            ^^^^^^^^^^^^^^ string literal `OptionAnswer`
  Member 2:
   15:     | TextAnswer
             ^^^^^^^^^^ TextAnswer
  Error:
    5:     currentAnswer? : {},
                            ^^ object type. This type is incompatible with
   10:     currentAnswer? : string,
                            ^^^^^^ string

I'm pretty new to flow so maybe I'm doing something stupid, but it looks like the literal from the first disjunct is somehow infecting the object that is compared to the second disjunct?

@willclarktech

This comment has been minimized.

Show comment
Hide comment
@willclarktech

willclarktech Apr 14, 2016

Possibly related to #815, #582, #1455, #1194.

willclarktech commented Apr 14, 2016

Possibly related to #815, #582, #1455, #1194.

@mroch

This comment has been minimized.

Show comment
Hide comment
@mroch

mroch Apr 14, 2016

Contributor

it looks like the literal from the first disjunct is somehow infecting the object that is compared to the second disjunct?

I think that's correct, and it's a bug. When an object flows to another object, optional props on the first object define "shadow" props on the second object. So in this case, exampleAnswer1 is being compared against TextAnswer first, which (incorrectly) adds a shadow prop for currentAnswer: string to exampleAnswer1, which then causes an error when it tests against OptionAnswer because string is not compatible with {}.

cc @bhosmer, @avikchaudhuri

Contributor

mroch commented Apr 14, 2016

it looks like the literal from the first disjunct is somehow infecting the object that is compared to the second disjunct?

I think that's correct, and it's a bug. When an object flows to another object, optional props on the first object define "shadow" props on the second object. So in this case, exampleAnswer1 is being compared against TextAnswer first, which (incorrectly) adds a shadow prop for currentAnswer: string to exampleAnswer1, which then causes an error when it tests against OptionAnswer because string is not compatible with {}.

cc @bhosmer, @avikchaudhuri

@bhosmer

This comment has been minimized.

Show comment
Hide comment
@bhosmer

bhosmer Apr 14, 2016

Contributor

@mroch Sounds right. It's yet another mutable effect - completely orthogonal to tvar effects - that we'll need to isolate (under any scheme for handling unions). Interestingly the simplest version of the multiverse approach involves deep-copying type terms, so it'll naturally fix this problem, but of course deep copying is also potentially a complete dealbreaker perf-wise, and avoiding it might spider the TC code with hooks in a really unpleasant way. We'll see...

Contributor

bhosmer commented Apr 14, 2016

@mroch Sounds right. It's yet another mutable effect - completely orthogonal to tvar effects - that we'll need to isolate (under any scheme for handling unions). Interestingly the simplest version of the multiverse approach involves deep-copying type terms, so it'll naturally fix this problem, but of course deep copying is also potentially a complete dealbreaker perf-wise, and avoiding it might spider the TC code with hooks in a really unpleasant way. We'll see...

@willclarktech

This comment has been minimized.

Show comment
Hide comment
@willclarktech

willclarktech Apr 15, 2016

It also applies when I specify a required property with an optional value in the type definition (rather than an optional property with a required value) and give a null value to the objects like this:

type TextAnswer = {
    kind          : 'TextAnswer',
    currentAnswer : ?string,
}
const exampleAnswer2: Answer = {
    kind         : 'TextAnswer',
    currentAnswer: null,
}

willclarktech commented Apr 15, 2016

It also applies when I specify a required property with an optional value in the type definition (rather than an optional property with a required value) and give a null value to the objects like this:

type TextAnswer = {
    kind          : 'TextAnswer',
    currentAnswer : ?string,
}
const exampleAnswer2: Answer = {
    kind         : 'TextAnswer',
    currentAnswer: null,
}
@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.

@jfrolich

This comment has been minimized.

Show comment
Hide comment
@jfrolich

jfrolich Jun 2, 2016

Having the same issue with a redux action union type:

export type Action =
    { type: 'FETCH_CONTENT_MESSAGES', payload: { contentItemId: number } }
  | { type: 'SIGN_IN', payload: Object, promise: PromiseType }
  | { type: 'SIGN_OUT' }
  | { type: 'JOIN_CHANNEL', payload: Object }
  | { type: 'VALIDATE_NEW_ORGANIZATION', payload: Object, promise: PromiseType }
  | { type: 'UPDATE_PROFILE', payload: Object, promise: PromiseType }
  | { type: 'RESET_PASSWORD', payload: { token: string, password: string, acceptInvite: string }}
  | { type: 'VERIFY_PASSWORD_RESET_TOKEN', payload: string }
  | { type: 'INITIALIZE_SESSION', payload: { authToken: string } }
  | { type: 'VALIDATE_NEW_USER', payload: Object, promise: PromiseType }
  | { type: 'CREATE_USER', payload: Object, promise: PromiseType }
  | { type: 'UPDATE_CONTENT_ITEM', payload: Object, promise: PromiseType }
  | { type: 'SUBSCRIBE_TO_CONTENT_ITEM', payload: { id: number } }
  | { type: 'FAVORITE_CONTENT_ITEM', payload: { id: number } }
  | { type: 'UNFAVORITE_CONTENT_ITEM', payload: { id: number } }
  | { type: 'UNSUBSCRIBE_FROM_CONTENT_ITEM', payload: { id: number } }
  | { type: 'SEND_CONTENT_MESSAGE', payload: { contentItemId: number, message: string } }
  | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
  | { type: 'SET_MESSAGE', payload: string }

With the incorrect flow response:

  Member 18:
   25:   | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See: web/static/js/actions/types.js:25
  Error:
   15:   | { type: 'VERIFY_PASSWORD_RESET_TOKEN', payload: string }
                                                           ^^^^^^ string. This type is incompatible with. See: web/static/js/actions/types.js:15
   25:   | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
                                                   ^^^^^^ object type. See: web/static/js/actions/types.js:25

If I move CREATE_ORGANIZATION more upward in the union declaration it passes.

Is this the same bug, and if so, any projection on when this bug will be fixed?

jfrolich commented Jun 2, 2016

Having the same issue with a redux action union type:

export type Action =
    { type: 'FETCH_CONTENT_MESSAGES', payload: { contentItemId: number } }
  | { type: 'SIGN_IN', payload: Object, promise: PromiseType }
  | { type: 'SIGN_OUT' }
  | { type: 'JOIN_CHANNEL', payload: Object }
  | { type: 'VALIDATE_NEW_ORGANIZATION', payload: Object, promise: PromiseType }
  | { type: 'UPDATE_PROFILE', payload: Object, promise: PromiseType }
  | { type: 'RESET_PASSWORD', payload: { token: string, password: string, acceptInvite: string }}
  | { type: 'VERIFY_PASSWORD_RESET_TOKEN', payload: string }
  | { type: 'INITIALIZE_SESSION', payload: { authToken: string } }
  | { type: 'VALIDATE_NEW_USER', payload: Object, promise: PromiseType }
  | { type: 'CREATE_USER', payload: Object, promise: PromiseType }
  | { type: 'UPDATE_CONTENT_ITEM', payload: Object, promise: PromiseType }
  | { type: 'SUBSCRIBE_TO_CONTENT_ITEM', payload: { id: number } }
  | { type: 'FAVORITE_CONTENT_ITEM', payload: { id: number } }
  | { type: 'UNFAVORITE_CONTENT_ITEM', payload: { id: number } }
  | { type: 'UNSUBSCRIBE_FROM_CONTENT_ITEM', payload: { id: number } }
  | { type: 'SEND_CONTENT_MESSAGE', payload: { contentItemId: number, message: string } }
  | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
  | { type: 'SET_MESSAGE', payload: string }

With the incorrect flow response:

  Member 18:
   25:   | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See: web/static/js/actions/types.js:25
  Error:
   15:   | { type: 'VERIFY_PASSWORD_RESET_TOKEN', payload: string }
                                                           ^^^^^^ string. This type is incompatible with. See: web/static/js/actions/types.js:15
   25:   | { type: 'CREATE_ORGANIZATION', payload: Object, promise: PromiseType }
                                                   ^^^^^^ object type. See: web/static/js/actions/types.js:25

If I move CREATE_ORGANIZATION more upward in the union declaration it passes.

Is this the same bug, and if so, any projection on when this bug will be fixed?

@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