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.

Sign up for GitHub

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

Jump to bottom

Require AsyncGenerator for asynccontextmanager #8698

Closed
wants to merge 1 commit into from
Closed

Require AsyncGenerator for asynccontextmanager #8698

wants to merge 1 commit into from

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

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

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
Open
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