-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix asyncio.gather
regression
#8271
Conversation
This comment has been minimized.
This comment has been minimized.
Worth a regression test? |
This comment has been minimized.
This comment has been minimized.
Good job you suggested that... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
(The reason why this PR is not a simple reversion of 0740d1c is that the added tests do not pass with a simple revert PR.) |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The test looks good, and helps make changes here much more sane.
That said, it might be nice to get rid of the type ignores. What goes wrong with adding the empty overload at the bottom?
...
@overload
def gather(
__coro_or_future1: _FutureLike[Any],
__coro_or_future2: _FutureLike[Any],
__coro_or_future3: _FutureLike[Any],
__coro_or_future4: _FutureLike[Any],
__coro_or_future5: _FutureLike[Any],
__coro_or_future6: _FutureLike[Any],
*coros_or_futures: _FutureLike[Any],
return_exceptions: bool = ...,
) -> Future[list[Any]]: ...
@overload
def gather(*, return_exceptions: bool = ...) -> Future[tuple[()]]: ...
Edit: minor, but maybe the clearest thing to do would be to move the big overload to the top, since that matches what type checkers are doing when they type check gather(*[...])
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
That's what I tried at first: https://github.com/python/typeshed/pull/8271/files/a345b43e68b56359e4decead1a261e283f8081a4. But the test cases failed: https://github.com/python/typeshed/runs/7299574031?check_suite_focus=true It seems like pyright picks the empty overload for
Meh, not sure I agree that that would improve readability :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure I understand what pyright is doing then, so you may want someone else to review this. Test case looks good though!
I think I understand what pyright's doing (though it did surprise me at first, for sure), and the test cases make me confident that this fixes the regression, so I'm inclined to merge. If somebody figures a way to get all the test cases to pass without the |
Wait, why is pyright doing what it's doing? @erictraut also mentioned moving the empty overload to the bottom as something that's likely expected to work in microsoft/pyright#3685 (comment) Yeah, fine to merge, I was just clarifying what my "approve" of the PR promised :-) |
Adding the empty-tuple overload caused major problems for pyright, and that overload only deals with an unlikely edge case anyway. Get rid of it, and replace the fallback overload with a more general overload.
Fixes #8270.