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

Use TypeVar defaults for Generator and AsyncGenerator #11867

Merged
merged 1 commit into from May 13, 2024

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented May 5, 2024

Make use of TypeVar defaults, so that Generator[int, None, None] and AsyncGenerator[int, None, None] can simply be represented as Generator[int] and AsyncGenerator[int] as recommended in PEP 696: https://peps.python.org/pep-0696/

There might be good reasons to push this back, but mostly wanted to put this out to get the ball rolling, and see if there was any discussion/thoughts around this.

@max-muoto max-muoto changed the title Use default for generator Use TypeVar defaults for Generator and AsyncGenerator May 5, 2024

This comment has been minimized.

@max-muoto max-muoto marked this pull request as ready for review May 5, 2024 22:51

This comment has been minimized.

@max-muoto max-muoto marked this pull request as draft May 5, 2024 23:00
@max-muoto max-muoto marked this pull request as ready for review May 5, 2024 23:01

This comment has been minimized.

@max-muoto
Copy link
Contributor Author

max-muoto commented May 5, 2024

Quick analysis of the new error from mypy primer:

 def _internal_run_as_gen(self) -> Generator:

The main change would be for MyPy users not providing generics, as typing.Generator[Any, Any, Any] will now be typing.Generator[Any, None, None], however, Pyright users shouldn't see effect, assuming they're on stricter configuration settings. This would very much help with verbosity in Pyright type-checked codebases, where it can get verbose to continually to provide None, None. At least for myself, I often just fall back on Iterator to avoid this.

@max-muoto max-muoto changed the title Use TypeVar defaults for Generator and AsyncGenerator Use TypeVar defaults for Generator, AsyncGenerator, and Coroutine May 5, 2024
@max-muoto max-muoto changed the title Use TypeVar defaults for Generator, AsyncGenerator, and Coroutine Use TypeVar defaults for Generator and AsyncGenerator May 5, 2024
@max-muoto max-muoto force-pushed the defaults-for-generator branch 2 times, most recently from 82aeba8 to 0974b0a Compare May 5, 2024 23:21
Copy link
Contributor

github-actions bot commented May 5, 2024

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

ignite (https://github.com/pytorch/ignite)
+ ignite/engine/engine.py:1010: error: No return value expected  [return-value]

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.

Thanks! This looks like a good change to me, but I'll leave it open for a bit, in case another maintainer disagrees.

@Daverball
Copy link
Contributor

Daverball commented May 6, 2024

Unfortunately this will not work at runtime, if you try to omit just one or both of the optional type parameters, I ran into a similar issue when I tried to retain the extra parameter I added for AbstractContextManager/AbstractAsyncContextManager for typing.ContextManager and typing.AsyncContextManager, but this case seems slightly less bad, since it's not adding an additional parameter that can't be specified at runtime, it just means you're not allowed to omit the parameter at runtime, unless you omit all of them.

>>> from typing import Generator
>>> Generator[int]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib64/python3.12/typing.py", line 398, in inner
    return func(*args, **kwds)
           ^^^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.12/typing.py", line 1455, in __getitem__
    _check_generic(self, params, self._nparams)
  File "/usr/lib64/python3.12/typing.py", line 304, in _check_generic
    raise TypeError(f"Too {'many' if alen > elen else 'few'} arguments for {cls};"
TypeError: Too few arguments for typing.Generator; actual 1, expected 3
>>>

It's not great that the type checker is hiding a runtime error this way, but it still might be worth the overall convenience for the subset of type annotations that never need to be evaluated at runtime.


Either way, if this change were to be accepted, we should probably also open pull requests for CPython and typing_extensions to make this version with optional type parameters work at runtime. CPython for 3.13 onwards and typing_extensions for backwards compatibility.

Ideally we can then keep the old parametrization for Python <=3.12 and 3.13 and the typing_extensions version can use the new one.

@JelleZijlstra
Copy link
Member

Added a CPython PR to add defaults to typing.Generator and AsyncGenerator. Note though that the collections.abc versions will already work fine; those don't check their argument counts.

@max-muoto
Copy link
Contributor Author

Added a CPython PR to add defaults to typing.Generator and AsyncGenerator. Note though that the collections.abc versions will already work fine; those don't check their argument counts.

Thanks! In that case, let me make this conditional on Python 3.13

@JelleZijlstra
Copy link
Member

I'm not sure that is right since the collections.abc versions will work regardless of version. We can also put the defaults in typing_extensions but this may lead to more user confusion.

@max-muoto
Copy link
Contributor Author

max-muoto commented May 6, 2024

I'm not sure that is right since the collections.abc versions will work regardless of version. We can also put the defaults in typing_extensions but this may lead to more user confusion.

Sounds good, so we're fine with this not working at runtime with typing.Generator and typing.AsyncGenerator on <= Python 3.12, since they're deprecated?

In that case, is there anything left we need to ship this?

@Daverball
Copy link
Contributor

I'm not sure that is right since the collections.abc versions will work regardless of version. We can also put the defaults in typing_extensions but this may lead to more user confusion.

That's fair, for some reason in the back of my mind Generator and AsyncGenerator were a more recent addition to collections.abc. Technically there's still an issue with 3.8, but with its EOL coming up later this year I guess we can live with that.

@AlexWaygood
Copy link
Member

There's plenty of things relating to type params that work at "typing time" but not at runtime. We try to keep these to a minimum, but it's not always possible. For example, mypy accepts the following:

from warnings import catch_warnings

x: catch_warnings[None]

But it fails at runtime:

Python 3.12.2 (main, Feb 15 2024, 19:30:27) [Clang 15.0.0 (clang-1500.1.0.2.5)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import warnings
>>> warnings.catch_warnings[None]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type 'catch_warnings' is not subscriptable
>>> 

So this definitely wouldn't be unprecedented. Ultimately we can't really distinguish between the real classes in collections.abc and the aliases in typing until #6257 is sorted out.

@max-muoto
Copy link
Contributor Author

Following up here, to see if we see any blockers to merging @JelleZijlstra!

@JelleZijlstra JelleZijlstra merged commit 3195f9f into python:main May 13, 2024
54 checks passed
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

5 participants