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

Wrong overload resolution with Any union item in argument #5249

Closed
JukkaL opened this issue Jun 20, 2018 · 7 comments
Closed

Wrong overload resolution with Any union item in argument #5249

JukkaL opened this issue Jun 20, 2018 · 7 comments
Assignees
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-0-high topic-overloads topic-union-types

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Jun 20, 2018

This fragment derived from production code causes a false positive:

def f(): pass  # No annotation
t = False
t &= f() and True  # Incompatible types in assignment 
                   # (expression has type "int", variable has type "bool")

Here's a self-contained repro:

from typing import overload, Union, Any

class C:
    def f(self, other: C) -> C: ...

class D(C):
    @overload  # type: ignore
    def f(self, other: D) -> D: ...
    @overload
    def f(self, other: C) -> C: ...
    def f(self, other): ...

x: D
y: Union[D, Any]
reveal_type(x.f(y))  # C

I'd expect the inferred type for x.f(y) to be Union[D, Any] (or maybe D, though it's slightly less correct arguably).

@JukkaL JukkaL added bug mypy got something wrong priority-0-high topic-union-types false-positive mypy gave an error on correct code topic-overloads labels Jun 20, 2018
@ilevkivskyi
Copy link
Member

I think we should switch to a new union math algorithm, I have some ideas how to make it fast enough to replace the current one (that is apparently to dangerous). I will give it a try now.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 20, 2018

Another real-world example where things go wrong:

from typing import Union, Any, List
i: Union[int, Any]
a: List[int]
reveal_type(a[i])  # Union[int, List[int]]

@JelleZijlstra
Copy link
Member

I think that one is actually correct: if i is a slice object, it will return List[int].

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

JukkaL commented Jun 20, 2018

List[int] is inconsistent with how Any works with overloads generally -- if there is ambiguity due to Any types in overload resolution, we tend to fall back to Any to avoid false positives. False negatives are preferred over false positives when Any types are involved. Here is a simple analogy:

from typing import Union, Any
class A:
    x: str
class B:
    x: int
a: Union[A, Any]
reveal_type(a.x)  # Union[str, Any]

We don't infer Union[str, int] as the type of a.x even though int is a plausible type for a.x -- since it could well be something else. Similarly, the precise type of i could actually be just int (though we can't tell for certain), and assuming that it can be a slice would be wrong.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Jun 21, 2018

The PR I am working on will produce (after some tweaks that I didn't push yet) Union[int, Any] for this example:

from typing import Union, Any, List
i: Union[int, Any]
a: List[int]
reveal_type(a[i]) # Union[int, Any]

@JukkaL does this make sense to you?

@ilevkivskyi
Copy link
Member

(Also I think Union[D, Any] in the original example is best.)

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 21, 2018

Yes, I think thatUnion[int, Any] is the most correct type for a[i], and similarly Union[D, Any] for the other example.

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
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-0-high topic-overloads topic-union-types
Projects
None yet
Development

No branches or pull requests

3 participants