facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.85k forks source link

Property access error when using union of intersections as function argument #2969

Closed EugeneZ closed 5 months ago

EugeneZ commented 7 years ago

Flow REPL example

There are so many union/intersection issues on GitHub, but none of them looked like this one except a closed issue #1759, figured that might be outdated by now.

Basically this code fails:

type Animal = {
  id: number
};

type Dog = Animal & {
  breed: string
};

type Cat = Animal & {
  food: string
};

function readAnimals(animal: Cat | Dog) {
  if (animal && animal.breed) {
  }
}

But this works:

type Dog = {
  id: number,
  breed: string
};

type Cat = {
  id: number,
  food: string
};

function readAnimals(animal: Cat | Dog) {
  if (animal.breed) {
  }
}

The only difference is that the Animal type has been copied-and-pasted into the Dog and Cat types. This makes it harder to write DRY types.

For reference, the error in the first example:

14:   if (animal && animal.breed) {
                    ^ property `breed`. Property cannot be accessed on any member of intersection type
14:   if (animal && animal.breed) {
                    ^ intersection
gcanti commented 7 years ago

As a workaround I usually write a disjoint union

type Dog = Animal & {
  tag: 'Dog',
  breed: string
};

type Cat = Animal & {
  tag: 'Cat',
  food: string
};

function readAnimals(animal: Cat | Dog) {
  if (animal.tag === 'Dog') {
    animal.breed // ok
  }
}
EugeneZ commented 7 years ago

@gcanti Thanks. Yes, this workaround fixes the problem for me, and its what I have resorted to, but I am forced to mutate data that I'd rather not mutate. I shouldn't have to do it to get it to typecheck correctly.

vkurchatkin commented 7 years ago

Similar question http://stackoverflow.com/questions/40874530/idiomatic-way-to-access-properties-of-union-type

levrik commented 7 years ago

Encountered this problem too today. This really needs to get fixed. Is a blocker for us expanding our Flow coverage.

Skurkify commented 7 years ago

My team would appreciate a fix for this as well.

vkurchatkin commented 7 years ago

This behaviour is unsound either way, so I recommend just to use any

Skurkify commented 7 years ago

Our use case is similar to the animal example posted above, except we have around ~10 common properties I don't want to duplicate across the eight types that compose our disjoint union. This was an effort on my part to keep the code DRY. I consider it a reasonable use case for intersections. Is there a more preferred way to keep types DRY?

vkurchatkin commented 7 years ago

It's not specific to intersections, it actually shouldn't work even without them

EugeneZ commented 7 years ago

The issue I am reporting is unique to intersections, I believe.

I think you might be confusing it for another issue (the one you linked on StackOverflow). That or I am the one that is confused.

For clarification, here are some more examples. They're similar to the one above, but now I'm using typeof to be extra specific. This changes the behavior of the bug, but it still disappears if you remove the intersection. Now, instead of displaying an error during the test itself, the test does not error (good) but when I attempt to access a property of one of the members of the intersection, I don't get a Flow error, even though I should!

Broken, even though I'm being very specific with my type tests

It works when I remove the intersection!

vkurchatkin commented 7 years ago

What I mean is that the bug is not that it doesn't work with intersection, it's that it works without them.

c10b10 commented 7 years ago

@vkurchatkin What do you mean? Why shouldn't you be able to access a property of an object that has a type composed through an intersection?

vkurchatkin commented 7 years ago

@c10b10 you shouldn't be able to access a property on union type, if it's not present on all bracnhes

marcusdarmstrong commented 7 years ago

It's not that we/I want to access a property that's on one branch (that's obviously unsound), it's that I want to be able to do a type refinement via a test on that property.

Edit: My simplified test case: https://flowtype.org/try/#0PQKgBAAgZgNg9gdzCYAoALgTwA4FMwAiYAvGAN5gBGAhgF4BcYAzugE4CWAdgOZgC+Abgw58AYRLkwUOHEYsOPfmABkhIVjxgAQhIo1Wctl158Va4ZoCCEnQB8wooagDGcTizDVG10hWmywACIAC1wYeECAGio6RkCEOFYYABNA-iF2KDAACmoAOn8ASnJUAEh8-yE+VAt8AFFdGPRDBRN1ETAAMUaaAFsW43TasABxCQb7TqFXd3QwbkYx3ya44PY0wRrMnO48mnRisjLd-aqgA

vkurchatkin commented 7 years ago

That's basically as unsound. You can't my assumptions about a property if it's not a part of type. So, checking it is not enough for refinement.

marcusdarmstrong commented 7 years ago

I'm not making any assumptions about a property in that example, so I'm afraid I don't follow... Could you provide an example of the way in which this inference is unsafe?

leonkenneth commented 6 years ago

Sorry for bumping up an old thread, but it looks like we still observe the same behavior as of today:

  1. @EugeneZ's example (slightly modified to explicitly check for undefined) is still raising an error https://flow.org/try/#0C4TwDgpgBAggdgSwLYEMA2UC8UDeAoKKBAEwC4o4BXJAIwgCc8BfAbjz1EigBEB7AcyyxEqDADJcBKDXoQIZKAGdg9BHH7M2HcNADCKYEPjJ0UCfkIAzXrwXLV6ze0uU4AY2AJecKLJTFjUUUAChQRdHJ9QwAfHgEASklCBEsoYM4IXktQ8LQAOhk5YkTMUqgAcldiCEs1eXLEiygmZiA

  2. Its non-raising counterpart https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVAXAngBwKZgAicA5mALxgDeqYYAlgCYBcYAdgK4C2ARngE4AaWmB788eFmADOGfvTYlUAXwDc6bPjABhAIYYK1EU1adeA4XShw4U2fMUr1qKBzYBjDPThsw43YwAgmz0XLow0gAUuiFhMKx6BgA+RKQAlEZ09FBg0bHhAHRiEowZNHR07j7ScDB4BfAkkQBEAJIYAOTSYLpgjKQAhM1p6nTKYHgRBOUVVWw1dQ2kLQAK4hjYMhziDJ3dve76w6NgyipAA

If I'm following your thoughts correctly @vkurchatkin, it's not @EugeneZ's example failing that is a bug, but rather the second example not failing that is in itself a bug.

However I too have trouble understanding why this is unsound. Since by definition we are checking whether this property is defined first, I would assume it's not the same as accessing it.

Assuming the following code:

function handleCatOrDog(catOrDog) {
    if (typeof(catOrDog.breed) !== 'undefined') {
        handleDog(catOrDog);
    } else {
        handleCat(catOrDog);
    }
}

How would someone type it without modifying catOrDog's code to include a type discriminator? If it came from an external API for example?

leonkenneth commented 6 years ago

Apologies for not having re-read https://flow.org/en/docs/types/unions/#toc-disjoint-unions-with-exact-types before, that explains why using non-exact types will not allow for refinements.

Along with https://github.com/facebook/flow/issues/2626#issuecomment-267449133, I actually came up with something close to what I was searching for.

Leaving it here in case it's useful to other people stumbling upon this thread: https://flow.org/try/#0PTAEAEDMBsHsHcBQAXAngBwKagIIDsBLAWwENpQBeUAbwB9FRQCATALlDwFciAjTAJ0S0AvgG5EKDNgAisAOaUa9Rj36ZMbUAGdk-AnjkAaBqAB05-MTJCxEtFlABhEskV0TkWLE069B44zmppak0DbiiJCceADGyASweKBqJMzOyAAUMS7s6QCUNCYxiVqw0JimcHIZAOQAFpjQcKDZyDV54sISUbHxicmYqbLVzPLswwXURSVlFVW1dQSgo3Ltnd3RcQlJKcwhZFoZJIShuS6gtKAThYwEkKBHJ2Smnt6TJoy76Y9W0HkmwlAjS02DuD2Ov1MqnUzEmoBAoAAKostKAAAZ3NHaOqwTjQZigdD8WA8Eg8aCoDiwVx8DgwjQAfg+AyG8h+oX+jC6wiAA

goodmind commented 5 years ago

Workaround with spreads

type Animal = {|
  id: number
|};

type Dog = {|
  ...Animal,
  breed: string
|};

type Cat = {|
  ...Animal,
  food: string
|};

function readAnimals(animal: Cat | Dog) {
  if (animal && animal.breed) {
  }
}

Try Flow

SamChou19815 commented 5 months ago

This is intended for inexact objects, because otherwise you might access some property that Flow doesn't know its existence, but incorrectly get void instead