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
merged 2 commits into from Jun 21, 2018

Conversation

Projects
None yet
2 participants
@Michael0x2a
Copy link
Collaborator

Michael0x2a commented Jun 20, 2018

Resolves #5250

This makes the "multiple overload matches due to Any" even more strict: we now return a non-Any type only if all of the return types are the same.

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

This makes the "multiple overload matches due to Any" even more strict:
we now return a non-Any type only if all of the return types are the
same.

@Michael0x2a Michael0x2a force-pushed the Michael0x2a:make-overlap-match-due-to-any-checks-more-strict branch from d0bee85 to 1455530 Jun 20, 2018

@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 20, 2018

As an addendum: my initial solution was to convert the is_subtype calls into is_proper_subtype, but I think that's just equivalent to checking for equality.

@@ -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.

@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.

@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.

@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.

@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.

@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.

@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.

@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.

Make overlaps due to Any revert to using erased types if possible
This change also modifies how mypy erases callables. Previously,
callables of type 'Callable[[A, B, ...], R]' were erased to
'Callable[[], None]'.

This change will now make the erasure be 'Callable[..., Any]', largely
on the grounds that it seems more useful.
@Michael0x2a

This comment has been minimized.

Copy link
Collaborator Author

Michael0x2a commented Jun 21, 2018

Another addendum: while I was reviewing the erasetypes module, I found that erasing a callable always produced Callable[[], None]. This wasn't really sure why that was, so I decided to change it so callables erase to Callable[..., Any] mostly because I felt it would be a more useful result when working with overloads (see testOverloadWithOverlappingItemsAndAnyArgument16).

I'm happy to change it back if you think that's best though -- the type erasure code has stood largely untouched for 6 years, and I'm a little worried that changing it now might unexpectedly break something.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Looks good to me!

@ilevkivskyi ilevkivskyi merged commit a75fa88 into python:master Jun 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Michael0x2a Michael0x2a deleted the Michael0x2a:make-overlap-match-due-to-any-checks-more-strict branch Jun 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.