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

More precisely typehint asyncio.gather up to 5 params #1550

Merged
merged 1 commit into from
Aug 29, 2017
Merged

More precisely typehint asyncio.gather up to 5 params #1550

merged 1 commit into from
Aug 29, 2017

Conversation

LiraNuna
Copy link
Contributor

@LiraNuna LiraNuna commented Aug 16, 2017

Since mypy/typing lacks a mechanism to describe variadic templates (See python/typing#193), a temporary solution was proposed to at least support some common cases of 1-5 arguments.

This PR mimics the current hack implemented for zip:

@overload
def zip(iter1: Iterable[_T1]) -> Iterator[Tuple[_T1]]: ...
@overload
def zip(iter1: Iterable[_T1], iter2: Iterable[_T2]) -> Iterator[Tuple[_T1, _T2]]: ...
@overload
def zip(iter1: Iterable[_T1], iter2: Iterable[_T2],
iter3: Iterable[_T3]) -> Iterator[Tuple[_T1, _T2, _T3]]: ...
@overload
def zip(iter1: Iterable[_T1], iter2: Iterable[_T2], iter3: Iterable[_T3],
iter4: Iterable[_T4]) -> Iterator[Tuple[_T1, _T2,
_T3, _T4]]: ...
@overload
def zip(iter1: Iterable[_T1], iter2: Iterable[_T2], iter3: Iterable[_T3],
iter4: Iterable[_T4], iter5: Iterable[_T5]) -> Iterator[Tuple[_T1, _T2,
_T3, _T4, _T5]]: ...
@overload
def zip(iter1: Iterable[Any], iter2: Iterable[Any], iter3: Iterable[Any],
iter4: Iterable[Any], iter5: Iterable[Any], iter6: Iterable[Any],
*iterables: Iterable[Any]) -> Iterator[Tuple[Any, ...]]: ...

@@ -8,6 +8,11 @@ from .futures import Future
__all__: List[str]

_T = TypeVar('_T')
_T1 = TypeVar('_T')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be named _T1

def gather(*coros_or_futures: _FutureT[_TAny],
loop: AbstractEventLoop = ..., return_exceptions: bool = False) -> Future[List[_TAny]]: ...
@overload
def gather(coro_or_future1: _FutureT[_T1],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add *, after the last coro_or_future to indicate that the remaining arguments are keyword-only.

loop: AbstractEventLoop = ..., return_exceptions: bool = False) -> Future[List[_TAny]]: ...
@overload
def gather(coro_or_future1: _FutureT[_T1],
loop: AbstractEventLoop = ..., return_exceptions: bool = False) -> Future[Tuple[_T1]]: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it actually always returns a List. Unfortunately, we can't specify how many elements a List contains in the type system.

Maybe it's acceptable to lie here and pretend that it returns a tuple, but https://docs.python.org/3/library/asyncio-task.html#asyncio.gather does explicitly say that it returns "a list".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last overload could use -> List[Any] instead. I'm not sure what issues specifying the incorrect type hint here would imply for code that explicitly relies on that.

I'm opened to thoughts about how to better approach this or completely abandon this and just accept life with cast

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somebody could be doing

lst = await asyncio.gather(some_coro(), some_other_coro())
lst.append(some_value)

I don't think I would write code like that, but somebody might. We have to decide whether the advantage of getting more precise types in the common case outweighs the risk of false positive type errors on code like this.

@LiraNuna
Copy link
Contributor Author

Tests are failing with error: Overloaded function signatures 4 and 5 overlap with incompatible return types. I'm not sure how to address this because I don't understand the overlaps.

@JelleZijlstra
Copy link
Member

You need to add a coro_or_future6 to the base case. They overlap because *coros_or_futures might be empty, in which case the base case is equivalent to the overload with five specified coro_or_future arguments.

@JelleZijlstra JelleZijlstra merged commit c5f1e90 into python:master Aug 29, 2017
@LiraNuna LiraNuna deleted the asyncio_gather_type_hack branch August 29, 2017 05:13
@carljm
Copy link
Member

carljm commented Oct 3, 2017

This PR is oddly missing the 3-argument case...

@gvanrossum
Copy link
Member

An astute observation!

@LiraNuna
Copy link
Contributor Author

LiraNuna commented Oct 7, 2017

@carljm Thank you for identifying and taking care of my mistake with the followup PR!

@gvanrossum
Copy link
Member

gvanrossum commented Oct 7, 2017 via email

@bsolomon1124
Copy link
Contributor

bsolomon1124 commented Sep 18, 2019

Hi all, one question a few years later about this discussion. @LiraNuna had mentioned:

The last overload could use -> List[Any] instead. I'm not sure what issues specifying the incorrect type hint here would imply for code that explicitly relies on that.

Is there a reason this was decided against as a final overload?

@overload
def gather(
    coro_or_future1: _FutureT[Any],
    *,
    loop: AbstractEventLoop = ...,
    return_exceptions: bool = False
) -> Future[List[Any]]: ...

Related: issue #3243

@JelleZijlstra
Copy link
Member

It does make sense to use List for the variadic overload, yes. Sorry for dropping that idea when I reviewed this PR.

@bsolomon1124
Copy link
Contributor

bsolomon1124 commented Sep 20, 2019

@JelleZijlstra I can submit a PR, but I'm unclear on something. The return would be Future[List[Any]]: ..., but what would the *aws themselves look like? (From signature gather(*aws, loop=None, return_exceptions=False)). Would that be:

@overload                                                                       
def gather(
	*aws: _FutureT[Any],
	*,
	loop: AbstractEventLoop = ...,
	return_exceptions: bool = False
) -> Future[List[Any]]: ...

Or would this call for a handful more overloads with different counts of _FutureT[Any] as the parameters?

@JelleZijlstra
Copy link
Member

I think you can just change the last current overload, which was as follows as of this PR:

@overload
def gather(coro_or_future1: _FutureT[Any], coro_or_future2: _FutureT[Any], coro_or_future3: _FutureT[Any],
           coro_or_future4: _FutureT[Any], coro_or_future5: _FutureT[Any], coro_or_future6: _FutureT[Any],
           *coros_or_futures: _FutureT[Any],
           loop: AbstractEventLoop = ..., return_exceptions: bool = False) -> Future[Tuple[Any, ...]]: ...

And change the return type to use a List instead of a Tuple. The coro_or_future 1 through 6 are needed to distinguish this overload from the ones before it.

We could of course add more overloads with a fixed number of arguments if we feel a need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants