Skip to content

Require AsyncGenerator for asynccontextmanager#8698

Closed
hauntsaninja wants to merge 1 commit intopython:masterfrom
hauntsaninja:asynccontextmanager
Closed

Require AsyncGenerator for asynccontextmanager#8698
hauntsaninja wants to merge 1 commit intopython:masterfrom
hauntsaninja:asynccontextmanager

Conversation

@hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Sep 6, 2022 •
edited
Loading

I'm against merging #7430 because:

  • Generator is not an ergonomic type
  • It's very widely breaking
  • Safety could easily be enforced by a linter that ensures use of yield from x instead of return x in an @contextmanager

To expand on that a little:

  • I care about the typing system being accessible. Generator with its three type vars is not a friendly type. I see enough confusion about Iterable, Iterator, Generator as it is.
  • Maintaining a high signal to noise / effort ratio is an important part of making typing accessible. Breaking changes that seem arbitrary to most users reinforce existing negative impressions of typing that hurt the ecosystem as a whole.
  • In all the years of typing, this has come up basically never (reported twice by asottile, none of the outlinks from the issue contain the bug, issue itself is not popular), so I think this is 99.99% noise. Between typeshed and mypy and gitter and discord and work, I've seen a lot of typing issue reports.

But I'm willing to admit that the considerations are slightly different for asynccontextmanager:

  • async is less widely used than sync, so this is less widely breaking (let's at least see primer)
  • async is typically used by more experienced Python users, so there's already an accessibility floor here
  • There is no equivalent to yield from in async that a linter could enforce, so this genuinely can only be solved in type checkers
  • The issue that led to require Generator for contextmanager #7430 was with asynccontextmanager
  • We've gotten away with some pedantic narrowings of async types before

This is just an experiment, I'm not sure whether I'm actually in favour of this change, but I'm a lot more willing to consider it. Note that I'm not open to slippery slope consistency arguments here; if a constraint is that we do both async and sync or neither, then I'm firmly in the neither camp.

@hauntsaninja
Require AsyncGenerator for asynccontextmanager
cd13dd0 
I'm against merging python#7430 because:
- Generator is not an ergonomic type
- It's very widely breaking
- Safety could easily be enforced by a linter that ensures use of `yield
  from` instead of `return` in an @contextmanager

To expand on that a little:
- I care about the typing system being accessible. Generator with its
  three type vars is not a friendly type. I see enough confusion
  about Iterable, Iterator, Generator as it is.
- Maintaining a high signal to noise / effort ratio is an important
  part of making typing accessible. Breaking changes that seem
  arbitrary to a casual user reinforce existing negative impressions
  of typing that hurt the ecosystem as a whole.
- In all the years of typing, this has come up basically never
  (reported twice by asottile, none of the outlinks from the issue
  contain the bug, issue itself is not popular), so I think this is
  99.99% noise. Between typeshed and mypy, I've seen a lot of typing
  issue reports.

But I'm willing to admit that the considerations are slightly different
for asynccontextmanager:
- async is less widely used than sync, so maybe this is less
  widely breaking (let's at least see primer)
- async is typically used by more experienced Python users, so there's
  already an accessibility floor here
- There is no equivalent to `yield from` in async that a linter could
  enforce, so this genuinely can only be solved in type checkers
- The issue that led to python#7430 was with asynccontextmanager
- We've gotten away with some pedantic narrowings of async types before

This is just an experiment, I'm not sure whether I'm actually in favour
of this change, but I'm a lot more willing to consider it. Note that I'm
not open to slippery slope consistency arguments here; if a constraint
is that we do both async and sync or neither, then I'm firmly in the
neither camp.
@github-actions
Copy link
Contributor

github-actions bot commented Sep 6, 2022

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

kopf (https://github.com/nolar/kopf)
+ kopf/_core/actions/execution.py:193: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[], AsyncIterator[None]]"; expected "Callable[[], AsyncGenerator[<nothing>, Any]]"
+ kopf/_cogs/clients/watching.py:71: error: Need type annotation for "operator_pause_waiter"
+ kopf/_cogs/clients/watching.py:89: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[NamedArg(Resource, 'resource'), NamedArg(Optional[NamespaceName], 'namespace'), DefaultNamedArg(Optional[ToggleSet], 'operator_paused')], AsyncIterator[Future[Any]]]"; expected "Callable[[NamedArg(Resource, 'resource'), NamedArg(Optional[NamespaceName], 'namespace'), DefaultNamedArg(Optional[ToggleSet], 'operator_paused')], AsyncGenerator[<nothing>, Any]]"
+ kopf/_core/reactor/subhandling.py:17: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[], AsyncIterator[None]]"; expected "Callable[[], AsyncGenerator[<nothing>, Any]]"

core (https://github.com/home-assistant/core)
+ homeassistant/components/fjaraskupan/__init__.py:105: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[Coordinator], AsyncIterator[Any]]"; expected "Callable[[Coordinator], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ homeassistant/components/fjaraskupan/number.py:61: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/light.py:50: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/light.py:60: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/fan.py:88: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/fan.py:109: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/fan.py:126: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/fjaraskupan/fan.py:133: error: Need type annotation for "device"  [var-annotated]
+ homeassistant/components/amcrest/__init__.py:208: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AmcrestChecker, VarArg(Any), KwArg(Any)], AsyncIterator[Any]]"; expected "Callable[[AmcrestChecker, VarArg(Any), KwArg(Any)], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ homeassistant/components/amcrest/__init__.py:217: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AmcrestChecker], AsyncIterator[None]]"; expected "Callable[[AmcrestChecker], AsyncGenerator[<nothing>, Any]]"  [arg-type]

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/client.py:124: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[Any], AbstractContextManager[None]]"; expected "Callable[[Any], AsyncIterator[<nothing>]]"
+ src/prefect/client.py:124: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[Any], AbstractContextManager[None]]"; expected "Callable[[Any], AsyncGenerator[<nothing>, Any]]"
+ src/prefect/task_runners.py:142: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[T], AsyncIterator[T]]"; expected "Callable[[T], AsyncGenerator[<nothing>, Any]]"
+ src/prefect/engine.py:488: error: Need type annotation for "task_runner"

psycopg (https://github.com/psycopg/psycopg)
+ psycopg/psycopg/cursor_async.py:205: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AsyncCursor[Row], Union[str, bytes, SQL, Composed], Union[Sequence[Any], Mapping[str, Any], None], DefaultNamedArg(Optional[AsyncWriter], 'writer')], AsyncIterator[AsyncCopy]]"; expected "Callable[[AsyncCursor[Row], Union[str, bytes, SQL, Composed], Union[Sequence[Any], Mapping[str, Any], None], DefaultNamedArg(Optional[AsyncWriter], 'writer')], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ psycopg/psycopg/connection_async.py:296: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AsyncConnection[Row], Optional[str], bool], AsyncIterator[AsyncTransaction]]"; expected "Callable[[AsyncConnection[Row], Optional[str], bool], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ psycopg/psycopg/connection_async.py:327: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AsyncConnection[Row]], AsyncIterator[AsyncPipeline]]"; expected "Callable[[AsyncConnection[Row]], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ psycopg_pool/psycopg_pool/pool_async.py:84: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[AsyncConnectionPool, Optional[float]], AsyncIterator[AsyncConnection[Any]]]"; expected "Callable[[AsyncConnectionPool, Optional[float]], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ tests/test_pipeline_async.py:40: error: Need type annotation for "p"  [var-annotated]
+ tests/test_pipeline_async.py:42: error: "None" has no attribute "status"  [attr-defined]
+ tests/test_pipeline_async.py:43: error: "None" has no attribute "status"  [attr-defined]
+ tests/test_pipeline_async.py:48: error: Need type annotation for "p1"  [var-annotated]
+ tests/test_pipeline_async.py:49: error: Need type annotation for "p2"  [var-annotated]
+ tests/pool/test_pool_async.py:48: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:56: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:63: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:66: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:69: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:83: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:87: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:153: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:168: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:171: error: Unused "type: ignore" comment
+ tests/pool/test_pool_async.py:176: error: Unused "type: ignore" comment
+ tests/pool/test_pool_async.py:182: error: Unused "type: ignore" comment
+ tests/pool/test_pool_async.py:230: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:253: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:275: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:294: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:354: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:382: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:410: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:436: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:440: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:457: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:481: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:501: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:531: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_pool_async.py:590: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:601: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:683: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:723: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:746: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:784: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:815: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:822: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:859: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:881: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:901: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:950: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:962: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:983: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:991: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:1031: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_pool_async.py:1049: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:49: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:55: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:62: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:65: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_null_pool_async.py:68: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:109: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:123: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:126: error: Unused "type: ignore" comment
+ tests/pool/test_null_pool_async.py:131: error: Unused "type: ignore" comment
+ tests/pool/test_null_pool_async.py:137: error: Unused "type: ignore" comment
+ tests/pool/test_null_pool_async.py:188: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:195: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:223: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:228: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:254: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:259: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:291: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:351: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:379: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:406: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:432: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:436: error: Need type annotation for "conn2"  [var-annotated]
+ tests/pool/test_null_pool_async.py:448: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:481: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:510: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:538: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:618: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:629: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:711: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:721: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:744: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:765: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:805: error: Need type annotation for "conn"  [var-annotated]
+ tests/pool/test_null_pool_async.py:823: error: Need type annotation for "conn"  [var-annotated]

boostedblob (https://github.com/hauntsaninja/boostedblob)
+ boostedblob/google_auth.py:68: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/google_auth.py:78: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:151: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:188: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:220: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:242: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:293: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:344: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:369: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:384: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/azure_auth.py:491: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/globals.py:169: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[], AsyncIterator[None]]"; expected "Callable[[], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ boostedblob/request.py:38: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[Request], AsyncIterator[ClientResponse]]"; expected "Callable[[Request], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ boostedblob/request.py:59: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/request.py:100: error: Argument 1 to "asynccontextmanager" has incompatible type "Callable[[Request], AsyncIterator[ClientResponse]]"; expected "Callable[[Request], AsyncGenerator[<nothing>, Any]]"  [arg-type]
+ boostedblob/request.py:157: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/request.py:286: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/path.py:352: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/path.py:367: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/path.py:464: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/path.py:475: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/write.py:325: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/write.py:359: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/write.py:423: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/write.py:457: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/copying.py:250: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/copying.py:273: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/copying.py:340: error: Need type annotation for "resp"  [var-annotated]
+ boostedblob/_recover.py:85: error: Need type annotation for "resp"  [var-annotated]

@hauntsaninja hauntsaninja mentioned this pull request Sep 6, 2022
Closed
@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 11, 2022

FWIW I'm pretty neutral on this:

  • Unlike #7430, I think the mypy_primer fallout is acceptable
  • AsyncGenerator doesn't seem to me to be as horrifyingly ugly a type as Generator (only two type parameters instead of three)
  • As you say, there's no async version of yield from, so the workarounds for @contextmanager don't apply for @asynccontextmanager

All that said:

  • For all the months that PR was open, I still don't feel like I've seen a complete repro of the actual problem that led to #7430 being opened
  • I'm still not persuaded that the problem that led to #7430 being opened is all that common
  • This would still be at least somewhat disruptive
  • AsyncGenerator is still uglier as a type annotation than AsyncIterator
  • It would be slightly weird to do one thing for the sync version but another thing for the async version

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Sep 11, 2022

Okay, no one seems bullish on this and no one from the original PR has shown up in favour, so I'm happy to let status quo win. I remain potentially open to this change.

@srittau
Copy link
Collaborator

srittau commented Sep 12, 2022

FWIW, I'm in favor of every step in the right direction. And the sooner we make a change, the less problems we cause. I still think not biting the bullet in #7430 was a major mistake.

@Sachaa-Thanasius Sachaa-Thanasius mentioned this pull request Jun 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hauntsaninja @AlexWaygood @srittau