Skip to content

refine asyncio call_exception_handler context type #6981

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

Closed

Conversation

graingert
Copy link
Contributor

No description provided.

@JelleZijlstra
Copy link
Member

I don't think we're ready to use PEP 655 in typeshed yet, unfortunately. #6301.

@github-actions

This comment has been minimized.

@sobolevn
Copy link
Member

@JelleZijlstra looks like we can use intermediate total=False inheritance hack for this case.

@srittau srittau added the status: deferred Issue or PR deferred until some precondition is fixed label Feb 2, 2022
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Let's see how much I can do in review suggestions

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Oops, I made pytype unhappy:

stdlib/asyncio/__init__.pyi (3.9): ParseError: 'total' allowed as classdef kwarg only for TypedDict subclasses

But I _Context is a TypedDict subclass. I filed google/pytype#1122.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

+1 for more TypedDicts. A few things I noticed below.

class _BaseContext(TypedDict):
message: str

class _Context(_BaseContext, total=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking forward to PEP 655.

default_exception_handler() also adds a handle_trackback field to the context:

https://github.com/python/cpython/blob/0a222db2bca63070f429c0e613707da1bdfaf0e0/Lib/asyncio/base_events.py#L1732-L1733

Copy link
Collaborator

Choose a reason for hiding this comment

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

_Context = dict[str, Any]

class _BaseContext(TypedDict):
message: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really required? At least default_exception_handler() uses .get() to access it:

https://github.com/python/cpython/blob/0a222db2bca63070f429c0e613707da1bdfaf0e0/Lib/asyncio/base_events.py#L1719

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

This is quite disruptive and I don't think we should merge. Because this type doesn't exist at runtime, I think it means in practice that any time you're not passing a dict literal, you'll get a type error, with no good way to refer to the type.

@AlexWaygood AlexWaygood removed the status: deferred Issue or PR deferred until some precondition is fixed label Mar 20, 2022
@AlexWaygood
Copy link
Member

This no longer breaks pytype, so I've removed the "deferred" label. We now just need to decide whether it's a good idea or not.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

anyio (https://github.com/agronholm/anyio)
+ src/anyio/_backends/_asyncio.py:1910: error: Argument 1 to "set_exception_handler" of "AbstractEventLoop" has incompatible type "Callable[[AbstractEventLoop, Dict[str, Any]], None]"; expected "Optional[Callable[[AbstractEventLoop, _Context], Any]]"  [arg-type]

aiohttp (https://github.com/aio-libs/aiohttp)
+ aiohttp/connector.py:108: error: Argument 1 to "call_exception_handler" of "AbstractEventLoop" has incompatible type "Dict[str, object]"; expected "_Context"  [arg-type]
+ aiohttp/connector.py:258: error: Argument 1 to "call_exception_handler" of "AbstractEventLoop" has incompatible type "Dict[str, object]"; expected "_Context"  [arg-type]
+ aiohttp/client.py:306: error: Argument 1 to "call_exception_handler" of "AbstractEventLoop" has incompatible type "Dict[str, object]"; expected "_Context"  [arg-type]
+ aiohttp/web.py:455: error: Incompatible types (expression has type "Optional[BaseException]", TypedDict item "exception" has type "BaseException")  [typeddict-item]

core (https://github.com/home-assistant/core)
+ homeassistant/runner.py:67: error: Argument 1 to "set_exception_handler" of "AbstractEventLoop" has incompatible type "Callable[[Any, Dict[str, Any]], None]"; expected "Optional[Callable[[AbstractEventLoop, _Context], Any]]"  [arg-type]
+ homeassistant/runner.py:156: error: Incompatible types (expression has type "Optional[BaseException]", TypedDict item "exception" has type "BaseException")  [typeddict-item]

boostedblob (https://github.com/hauntsaninja/boostedblob)
+ boostedblob/globals.py:231: error: Argument 1 to "default_exception_handler" of "AbstractEventLoop" has incompatible type "Dict[str, Any]"; expected "_Context"
+ boostedblob/globals.py:233: error: Argument 1 to "set_exception_handler" of "AbstractEventLoop" has incompatible type "Callable[[AbstractEventLoop, Dict[str, Any]], None]"; expected "Optional[Callable[[AbstractEventLoop, _Context], Any]]"

aioredis (https://github.com/aio-libs/aioredis)
+ aioredis/client.py:1076: error: Argument 1 to "call_exception_handler" of "AbstractEventLoop" has incompatible type "Dict[str, object]"; expected "_Context"  [arg-type]

python-chess (https://github.com/niklasf/python-chess)
+ chess/engine.py:1261: error: Incompatible types (expression has type "Optional[SubprocessTransport]", TypedDict item "transport" has type "BaseTransport")

dragonchain (https://github.com/dragonchain/dragonchain)
+ dragonchain/broadcast_processor/broadcast_processor.py:367:42: error: Argument 1 to "set_exception_handler" of "AbstractEventLoop" has incompatible type "Callable[[AbstractEventLoop, Dict[Any, Any]], None]"; expected "Optional[Callable[[AbstractEventLoop, _Context], Any]]"
+ dragonchain/contract_invoker/contract_invoker.py:137:42: error: Argument 1 to "set_exception_handler" of "AbstractEventLoop" has incompatible type "Callable[[AbstractEventLoop, Dict[Any, Any]], None]"; expected "Optional[Callable[[AbstractEventLoop, _Context], Any]]"

@AlexWaygood
Copy link
Member

I'm closing this PR: as @hauntsaninja says, this would be quite disruptive, and it would be complicated trying to access this TypedDict at runtime. I don't think we have enough consensus to make such a disruptive change; @srittau's review from February also has not been addressed.

The asyncio maintainers don't seem averse to typing, however: some of the recent additions have been quite heavily typed. Perhaps a better way forward would be to file an issue over at CPython asking for this TypedDict to be added to the runtime. Then users would have an easy way of accessing the TypedDict at runtime, and so the concerns about disruption wouldn't be nearly so great.

@AlexWaygood AlexWaygood closed this Jun 6, 2022
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.

6 participants