-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Bad inference for asyncio.gather with *args #5124
Comments
The reveal_type(asyncio.sleep(1)) # Future[<nothing>] see #3032 The actual issue is better seen here, reveal_type(asyncio.gather(*[asyncio.sleep(1, 1.0), asyncio.sleep(1, 2.0)])) # Future[Tuple[builtins.float*]] A type is correct, but single item tuple is inferred instead of variable length one. I think the problem is that the first overload matches the star signature, because they are overlapping. There already a TODO item to prohibit such unsafe overlaps in #5119, second bullet, but I am not sure what should we actually write in typeshed in such cases. Maybe we need to re-think the second bullet. @Michael0x2a your opinion? |
(To be clear this is a regression on master; it didn't happen in 0.590 or 0.600.) |
This is (kind of) expected. It was a necessary compromise, we decided to split overload overhaul in few PRs to avoid single mega-PR. |
I think this is a case of a previously silent bug becoming more visible. If you try running @ilevkivskyi's example on mypy 0.600, the revealed type will be just But my PR removes the "let's have overloads sometimes give up and return Any" behavior which, unfortunately, ends up breaking some code. I think the easiest way to fix this would be to flip the order in which the overloads are defined so that the alternative with the long list of arguments and the return type of The only catch is that when I tried making that change, mypy ended up inferring a return type of I think this might actually be yet another unrelated bug: I get the same issue when I try doing this:
(That said, I suspect this sort of pattern might end up being mildly confusing: you typically want the signature with the widest types to come last so you avoid shadowing another signature, but you want the signatures with the largest number of arguments to come first so that |
Also, something I can also do is write a script to search to search typescript to search for long overloads/overloads with a steadily increasing number of arguments -- basically, search for cases where people are trying to use overloads to hack around the lack of "variable type vars" and manually check to make sure they're all ok. |
This is a bug in In the original example it is however a single element tuple (mypy gives "Index out of range" error), so indeed the first overload is matched. We just had a discussion with @JukkaL and agreed the best way to proceed is to modify the rule "in ambiguous cases first overload wins" to "in ambiguous cases type checker are free to determine best match; if a checker sees more than one equally best match, the first one wins." This has two upsides:
@Michael0x2a what do you think? If you agree, could you please update the checklist in #5119 |
@ilevkivskyi -- I'd be ok with leaving the behavior of overloads undefined when the ambiguity is due to things like multiple inheritance. I'm a bit more hesitant about allowing the behavior to be undefined in other cases, though. In particular, I think it's worth pinning down what exactly ought to happen when you do I'm not sure what exactly the spec should look like though. One idea I had was that if a call could pass in a potentially infinite number of arguments (e.g. when doing If there are no matches left (e.g. none of the alternative signatures contain an As a minor aside, I think the way we handle empty containers I think is already undefined. Basically, when we do Mypy currently will pick the first matching overload and use that to infer the correct type if the empty container is inline; if we try assigning the empty list to a variable it mandates a type annotation. I could see other type checkers being more strict in the case of the former -- they could maybe infer some sort of union type or maybe require an explicit cast or something. |
OK, makes sense. Although the overloads like in this issue may be less important when we will have variadic type variables, the calls like this may still happen quite often. The problem however is that the behaviour of star args is not well specified even for non-overloaded calls. A better spec may be also part of the upcoming shape types PEP (the "numpy" one). With the new PEP each collection will be given not only item type, but also shape (generalisation of size for 1D collections). The latter can be also Coming back to overloads, current behaviour for non-overloaded calls is mostly class B(A): ...
@overload
def k(x: int) -> B: ...
@overload
def k(x: Any) -> A: ... # note _explicit_ Any overload
a: Any
k(a) # Currently this is Any, but it is safe to infer 'A' I think. Second, we can continue this to containers: @overload
def h(x: List[int]) -> List[int]: ...
@overload
def h(x: List[Any]) -> List[Any]: ... # Again, explicit Any
ll: List[Any]
h(ll) # I think this should be List[Any], not Any as currently Now coming back to star args. I think they should behave as @Michael0x2a what do you think? Sorry for potentially fuzzy logic, I hope you get my idea, I really need to run now. |
@ilevkivskyi -- sorry for the delayed response; I didn't see this until now.
|
This is normal :-) I mean I was thinking a lot about them recently in context of numeric stack support, so now I see analogies everywhere.
This sounds right.
Yes, exactly. |
This pull request implements the changes discussed in python#5124. Specifically... 1. When two overload alternatives match due to Any, we return the last matching return type if it's a supertype of all of the previous ones. If it's not a supertype, we give up and return 'Any' as before. 2. If a user calls an overload with a starred expression, we try matching alternatives with a starred arg or kwarg first, even if those alternatives do not appear first in the list. If none of the starred alternatives are a valid match, we fall back to checking the other remaining alternatives in order.
This pull request implements the changes discussed in #5124. Specifically... 1. When two overload alternatives match due to Any, we return the last matching return type if it's a supertype of all of the previous ones. If it's not a supertype, we give up and return 'Any' as before. 2. If a user calls an overload with a starred expression, we try matching alternatives with a starred arg or kwarg first, even if those alternatives do not appear first in the list. If none of the starred alternatives are a valid match, we fall back to checking the other remaining alternatives in order.
@JelleZijlstra -- sorry, I'm a bit late in following up on this, but we merged in #5166 a day or two ago which should hopefully make your code typecheck. When you get the chance, can you check and see if these regressions are fixed in your codebase? |
Yes, all good now. Thanks! |
On the following file:
I expect to get something like
Future[Tuple[Any, ...]]
, but actually with current master I get:(i.e., a single-element tuple).
The asyncio.gather stubs at https://github.com/python/typeshed/blob/master/stdlib/3.4/asyncio/tasks.pyi#L27 look right to me.
I found this in real code that was doing something like
a, b = await asyncio.gather(*[task1, task2])
.cc @Michael0x2a
The text was updated successfully, but these errors were encountered: