Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.9k
Improve various signatures that shouldn't be async def, but currently are#7491
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
AlexWaygood commented Mar 14, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AlexWaygood commented Mar 14, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
AlexWaygood commented Mar 14, 2022
Cc. @sobolevn, since this partially reverts your PR 🙂 |
graingert commented Mar 14, 2022 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Would SupportsAnext need fixing? https://github.com/python/typeshed/blob/master/stdlib/_typeshed/__init__.pyi#L36 currently it requires a coroutine - but any Awaitable is supported (it gets wrapped with anext_awaitable with a default) |
AlexWaygood commented Mar 14, 2022
Good spot. |
graingert commented Mar 14, 2022
anext is actually slightly odd as it returns whatever |
graingert commented Mar 14, 2022
so the type annotation for classSupportsAnyAnext(Protocol[_T_co]): def__anext__(self) ->_T_co: ... classSupportsAwaitableAnext(Protcol[_T_co]): def__anext__(self) ->Awaitable[_T_co]: ... @overloaddefanext(__i: SupportsAnyAnext[_T]) ->_T: ... @overloadasyncdefanext(__i: SupportsAwaitableAnext[_T], default: _VT) ->_T|_VT: ...so that anext doesn't incorrectly promote an |
AlexWaygood commented Mar 14, 2022
Sure, but I'd imagine that if |
graingert commented Mar 14, 2022 • edited by AlexWaygood
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by AlexWaygood
Uh oh!
There was an error while loading. Please reload this page.
my point was more about preserving the return type of the _Awaitable_T_co=TypeVar("_Awaitable_T_co", bound=Awaitable, covariant=True) # bound isn't strictly true - anext supports any return valueclassSupportsAnext(Protocol[_Awaitable_T_co]): def__anext__(self) ->_Awaitable_T_co: ... classSupportsAwaitableAnext(Protocol[_T_co]): def__anext__(self) ->Awaitable[_T_co]: ... _Awaitable_T=TypeVar("_Awaitable_T", bound=Awaitable) @overloaddefanext(__i: SupportsAnext[_Awaitable_T]) ->_Awaitable_T: ... @overloadasyncdefanext(__i: SupportsAwaitableAnext[_T], default: _VT) ->_T|_VT: ... |
AlexWaygood commented Mar 14, 2022
It's frying my brain somewhat, but I think I understand. Thanks for the explanation! |
Co-authored-by: graingert <https//@graingert.co.uk>
2a19f1b to 80acd91Comparetyping.AsyncIterator and typing.AsyncGeneratorasync def, but currently are This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
AlexWaygood commented Mar 14, 2022
Better now @graingert? |
async def, but currently areasync def, but currently areUh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Thomas Grainger <[email protected]>
This comment has been minimized.
This comment has been minimized.
AlexWaygood commented Mar 15, 2022
Thanks for the review @graingert! (I'll leave this open for a while to give other maintainers a chance to review.) |
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
AlexWaygood commented Mar 17, 2022
(Planning to merge this in a day or so, unless any other maintainers want to review, since it's sort of blocking python/mypy#12321) |
Diff from mypy_primer, showing the effect of this PR on open source code: tornado (https://github.com/tornadoweb/tornado) - tornado/gen.py:429: error: Incompatible return value type (got "WaitIterator", expected "AsyncIterator[Any]")- tornado/gen.py:429: note: Following member(s) of "WaitIterator" have conflicts:- tornado/gen.py:429: note: Expected:- tornado/gen.py:429: note: def __anext__(self) -> Coroutine[Any, Any, Any]- tornado/gen.py:429: note: Got:- tornado/gen.py:429: note: def __anext__(self) -> Future[Any] boostedblob (https://github.com/hauntsaninja/boostedblob) + boostedblob/boost.py:547: error: Need type annotation for "task"+ boostedblob/boost.py:547: error: Argument 1 to "create_task" has incompatible type "Awaitable[T]"; expected "Union[Generator[Any, None, <nothing>], Coroutine[Any, Any, <nothing>]]" |
As discussed in #7475, the following methods are all "async def" functions at runtime, but should not be "async def" functions in the stub:
AsyncIterator.__anext__AyncGenerator.__anext__AsyncGenerator.acloseAsyncGenerator.asendAsyncGenerator.athrowtyping.AsyncIteratorandtyping.AsyncGeneratorrepresent abstract interfaces rather than concrete classes, and PEP 525, which introduced the asynchronous iteration protocol, explicitly states that it is fine for these methods to return any awaitable. The current annotations, which state that these methods have to return coroutines, are therefore too restrictive, and cause false positives.In addition to these changes:
AsyncGenerator.__anext__is not abstract at runtime, and I don't see any reason why it should be in the stub either.AsyncGenerator.__aiter__is also not abstract at runtime, and not even defined onAsyncGeneratorat runtime; it is simply inherited fromAsyncIterator. I think we should do the same in the stub.AsyncGenerator.acloseis not abstract at runtime, and I don't see any reason why it should be in the stub._typeshed.SupportsAnext.__anext__andcontextlib._SupportsAclose.aclosewere both erroneously madeasync deffunctions in Fix several methods that should beasync def, but aren't #7107; this PR also reverts the changes that PR made to those classes.This PR reverts the changes #7105 made to
typing.pyi, but not the changes #7105 made totypes.pyi. I think that makes sense, as the classes intypesare concrete classes, whereas the classes intypingare abstract interfaces.This PR adds several entries to the allowlist, but we can take them off again if python/mypy#12343 is merged.