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

Improve various signatures that shouldn't be async def, but currently are #7491

Merged
merged 10 commits into from
Mar 19, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 14, 2022

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.aclose
  • AsyncGenerator.asend
  • AsyncGenerator.athrow

typing.AsyncIterator and typing.AsyncGenerator represent 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 on AsyncGenerator at runtime; it is simply inherited from AsyncIterator. I think we should do the same in the stub.
  • AsyncGenerator.aclose is not abstract at runtime, and I don't see any reason why it should be in the stub.
  • _typeshed.SupportsAnext.__anext__ and contextlib._SupportsAclose.aclose were both erroneously made async def functions in Fix several methods that should be async 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 to types.pyi. I think that makes sense, as the classes in types are concrete classes, whereas the classes in typing are abstract interfaces.

This PR adds several entries to the allowlist, but we can take them off again if python/mypy#12343 is merged.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review March 14, 2022 17:00
@AlexWaygood
Copy link
Member Author

AlexWaygood commented Mar 14, 2022

The new boostedblob error is related to this line here, and is because asyncio.create_task is annotated as requiring a coroutine following #6779, and this PR would change the definition of AsyncIterator.__anext__ such that it wouldn't necessarily be guaranteed to return a coroutine anymore.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

Cc. @sobolevn, since this partially reverts your PR 🙂

@graingert
Copy link
Contributor

graingert commented Mar 14, 2022

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
Copy link
Member Author

Would SupportsAnext need fixing? https://github.com/python/typeshed/blob/master/stdlib/_typeshed/__init__.pyi#L36

Good spot. contextlib._SupportsAclose as well.

@AlexWaygood AlexWaygood marked this pull request as draft March 14, 2022 19:05
@graingert
Copy link
Contributor

anext is actually slightly odd as it returns whatever __anext__ returns - eg an Awaitable or even not an awaitable!

Python 3.10.2 (main, Jan 15 2022, 18:02:07) [GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class Foo:
...     def __anext__(self):
...         return "hello"
... 
>>> anext(Foo())
'hello'
>>> 

@graingert
Copy link
Contributor

so the type annotation for builtins.anext should look more like this:

class SupportsAnyAnext(Protocol[_T_co]):
    def __anext__(self) -> _T_co: ...

class SupportsAwaitableAnext(Protcol[_T_co]):
    def __anext__(self) -> Awaitable[_T_co]: ...

@overload
def anext(__i: SupportsAnyAnext[_T]) -> _T: ...
@overload
async def anext(__i: SupportsAwaitableAnext[_T], default: _VT) -> _T | _VT: ...

so that anext doesn't incorrectly promote an AsyncIterable.__next__ Awaitable into a Coroutine

@AlexWaygood
Copy link
Member Author

anext is actually slightly odd as it returns whatever __anext__ returns - eg an Awaitable or even not an awaitable!

Sure, but I'd imagine that if __anext__ returns something that isn't awaitable, it's almost certainly indicative of a programming error. So surely we'd be doing the developer a favour by raising an error if they try to call anext on an object that has an __anext__ method that doesn't return an awaitable?

@graingert
Copy link
Contributor

graingert commented Mar 14, 2022

my point was more about preserving the return type of the .__anext__ call:

_Awaitable_T_co = TypeVar("_Awaitable_T_co", bound=Awaitable, covariant=True)  # bound isn't strictly true - anext supports any return value

class SupportsAnext(Protocol[_Awaitable_T_co]):
    def __anext__(self) -> _Awaitable_T_co: ...

class SupportsAwaitableAnext(Protocol[_T_co]):
    def __anext__(self) -> Awaitable[_T_co]: ...

_Awaitable_T = TypeVar("_Awaitable_T", bound=Awaitable)

@overload
def anext(__i: SupportsAnext[_Awaitable_T]) -> _Awaitable_T: ...
@overload
async def anext(__i: SupportsAwaitableAnext[_T], default: _VT) -> _T | _VT: ...

@AlexWaygood
Copy link
Member Author

my point was more about preserving the return type of the .__anext__ call:

It's frying my brain somewhat, but I think I understand. Thanks for the explanation!

Co-authored-by: graingert <https//@graingert.co.uk>
@AlexWaygood AlexWaygood changed the title Improve typing.AsyncIterator and typing.AsyncGenerator Improve various signatures that shouldn't async def, but currently are Mar 14, 2022
@github-actions

This comment has been minimized.

1 similar comment
@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as ready for review March 14, 2022 23:08
@AlexWaygood
Copy link
Member Author

Better now @graingert?

@AlexWaygood AlexWaygood changed the title Improve various signatures that shouldn't async def, but currently are Improve various signatures that shouldn't be async def, but currently are Mar 15, 2022
stdlib/builtins.pyi Outdated Show resolved Hide resolved
graingert added a commit to graingert/boostedblob that referenced this pull request Mar 15, 2022
@github-actions

This comment has been minimized.

Co-authored-by: Thomas Grainger <tagrain@gmail.com>
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

Thanks for the review @graingert! (I'll leave this open for a while to give other maintainers a chance to review.)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

(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)

@github-actions
Copy link
Contributor

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>]]"

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.

None yet

3 participants