Skip to content

One more bug with union math logic #5243

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

Closed
ilevkivskyi opened this issue Jun 19, 2018 · 3 comments
Closed

One more bug with union math logic #5243

ilevkivskyi opened this issue Jun 19, 2018 · 3 comments

Comments

@ilevkivskyi
Copy link
Member

Well, I have found another bug with union math in overloads, this time false negative:

from typing import Union, overload

@overload
def f(x: int, y: int) -> int: ...
@overload
def f(x: object, y: str) -> str: ...
def f(x):
    pass

x: Union[int, str]
y: Union[int, str]
f(x, y) # passes, but should fail because it never matches (str, int)

The problem is that make_simplified_union is used. On one hand this allows us to do unioning in more scenarios, on other hand it is dangerous for union counting in unioned args. I am not sure what to do. @Michael0x2a could you take care of this? (This is however not urgent, since this is a false negative.)

@Michael0x2a
Copy link
Collaborator

Will do -- I'll take a look a little later today.

@ilevkivskyi ilevkivskyi self-assigned this Jun 20, 2018
@ilevkivskyi
Copy link
Member Author

Sorry, for overtaking, but I went ahead and now have a possible fix for this in a form of a change to a union math logic, so I am un-assigning you here, but I hope you will review my PR that I will open soon :-)

@Michael0x2a
Copy link
Collaborator

No worries :)

I haven't actually started writing any code for this, so this works out perfectly.

ilevkivskyi added a commit that referenced this issue Jul 3, 2018
Fixes #5243
Fixes #5249

Some comments:
* I went ahead with a slow but very simple recursive algorithm that treats all various complex cases correctly. On one hand it can be exponential, but on the other hand, the complexity will be bad _only_ if user abuses lots of unions
* I use a hack caused by the fact that currently most function inference functions pass argument _expressions_ instead of types, I left a TODO to use a more unified approach similar to multiassign_from_union
* It may look like there are many changes in tests, but actually there are not, the differences are because:
  - Error messages now show the _first potentially matching_ overload (which is OK I think)
  - Order of items in many unions turned to the opposite, apparently union `__repr__` is unstable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants