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

Make overload checks more strict when there are multiple 'Any's #5254

Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+53 −8
Diff settings

Always

Just for now

@@ -1314,12 +1314,9 @@ def infer_overload_return_type(self,
return None
elif any_causes_overload_ambiguity(matches, return_types, arg_types, arg_kinds, arg_names):
# An argument of type or containing the type 'Any' caused ambiguity.
if all(is_subtype(ret_type, return_types[-1]) and
is_subtype(return_types[-1], ret_type)
for ret_type in return_types[:-1]):
# The last match is mutually compatible with all previous ones, so it's safe
# to return that inferred type.
return return_types[-1], inferred_types[-1]
if all_same_types(return_types):
# The return types are all the same, so it's safe to return that instead of 'Any'
return return_types[0], inferred_types[0]
else:
# We give up and return 'Any'.
return self.check_call(callee=AnyType(TypeOfAny.special_form),
@@ -1400,8 +1400,8 @@ a: Any
b: List[Any]
c: List[str]
d: List[int]
reveal_type(f(a)) # E: Revealed type is 'builtins.list[Any]'
reveal_type(f(b)) # E: Revealed type is 'builtins.list[Any]'

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 20, 2018

Collaborator

TBH, I don't like this change. I think the original issue only appears when one of the returns is (an explicit or inferred) Any. In this case List[Any] is perfectly safe. So a better fix would be to check if there are Anys among returns and return Any in this case. Otherwise keep the current logic.

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Jun 20, 2018

Author Collaborator

I'm not so sure anymore if that'll necessarily be safe in all cases though. For example:

from typing import Any, overload, TypeVar, Generic

T = TypeVar('T')

class Wrapper(Generic[T]): pass

class A(Generic[T]):
    @overload
    def f(self, x: int) -> Wrapper[T]: ...
    @overload
    def f(self, x: slice) -> Wrapper[A[T]]: ...
    def f(self, x): ...

i: Any
a: A[Any]
reveal_type(a.f(i))  # Wrapper[A[Any]], but should be Wrapper[Any] or Any

It's basically the same example from #5250, except I added a Wrapper class around the return types and we get the same sort of problem, except with no top-level Any.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 20, 2018

Collaborator

OK, I see, but on the contrary there is another problem with current logic (and can be even worse with proposed):

class B: ...
class C(B): ... # a subclass

@overload
def f(x: C) -> C: ...
@overload
def f(x: B) -> B: ...

x: Any
f(x)  # currently and with this change - Any, but B is totally OK

I think a solution that will solve both problem is to return the least precise type of all return (if such a type exists). This is of course a bit larger change, but I think it is a well defined problem. And this PR is not urgent (the current bug only produces very few errors). What do you think?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 20, 2018

Collaborator

Addition: if you prefer it is totally OK to just use heuristics to find if there is the least precise type among all matching returns, if we can't find such, just return Any. I am fine if we will just cover the above case and the test case I mentioned. (I could imagine a careful treatment of all possibilities may be tedious).

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Jun 20, 2018

Author Collaborator

Wouldn't that algorithm cause a regression with #5232 though? (e.g. suppose we replace B with 'unicode' and C with 'bytes' in your test case)

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 20, 2018

Collaborator

Sigh. Yes, this would be problematic. But at least can we "save" the test case? For example we allow a fallback option if "erased" types (with all args replace with Any) are the same for all matching returns, then we return the erased type?

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Jun 20, 2018

Author Collaborator

Hmm, I think that would probably work. (Or more precisely, I can't think of a way that would break yet, lol)

I'll implementing that change in a bit.

reveal_type(f(a)) # E: Revealed type is 'Any'
reveal_type(f(b)) # E: Revealed type is 'Any'
reveal_type(f(c)) # E: Revealed type is 'builtins.list[Any]'
reveal_type(f(d)) # E: Revealed type is 'builtins.list[builtins.int]'

@@ -1443,6 +1443,54 @@ reveal_type(f(**a)) # E: Revealed type is 'Any'

[builtins fixtures/dict.pyi]

[case testOverloadWithOverlappingItemsAndAnyArgument12]
from typing import overload, Any

@overload
def f(x: int) -> Any: ...
@overload
def f(x: str) -> str: ...
def f(x): pass

a: Any
reveal_type(f(a)) # E: Revealed type is 'Any'

[case testOverloadWithOverlappingItemsAndAnyArgument13]
from typing import Any, overload, TypeVar, Generic

class slice: pass

T = TypeVar('T')
class A(Generic[T]):
@overload
def f(self, x: int) -> T: ...
@overload
def f(self, x: slice) -> A[T]: ...
def f(self, x): ...

i: Any
a: A[Any]
reveal_type(a.f(i)) # E: Revealed type is 'Any'

[case testOverloadWithOverlappingItemsAndAnyArgument14]
from typing import overload, Any, Union

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

@overload
def g(x: int) -> Union[str, int]: ...
@overload
def g(x: str) -> Union[int, str]: ...
def g(x): pass

a: Any
reveal_type(f(a)) # E: Revealed type is 'builtins.str'
reveal_type(g(a)) # E: Revealed type is 'Union[builtins.str, builtins.int]'

[case testOverloadOnOverloadWithType]
from typing import Any, Type, TypeVar, overload
from mod import MyInt
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.