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

Fix ParamSpec ellipsis default for <3.10 #279

Merged
merged 6 commits into from
Sep 6, 2023

Conversation

Gobot1234
Copy link
Contributor

No description provided.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Could you add a test?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

CHANGELOG entry/docs? :-)

If this fixes an issue you hit, other people might have hit it too, so they probably deserve to know about the fix!

@JelleZijlstra
Copy link
Member

The change itself looks good, but please add a changelog/docs entry as Alex requested. For reference, PEP 696 explicitly allows this: https://peps.python.org/pep-0696/#paramspec-defaults

@Gobot1234
Copy link
Contributor Author

Oh weird, it doesn't seem to have pushed the changes

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.

Looks good but @AlexWaygood please take a look too.

I'll likely cut a new release once the PEP 727 (Doc()) changes are in.

doc/index.rst Outdated Show resolved Hide resolved
Comment on lines +1280 to +1283
if isinstance(type_param, ParamSpec) and default is ...: # ... not valid <3.11
type_param.__default__ = default
else:
type_param.__default__ = typing._type_check(default, "Default must be a type")
Copy link
Member

@AlexWaygood AlexWaygood Sep 6, 2023

Choose a reason for hiding this comment

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

Aha, so the issue is that typing._type_check changed in Python 3.11 so that it would accept an ellipsis, but the function didn't allow it on previous versions of CPython

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, other than my point about the docs.

Another possible fix could be to vendor the current version of _type_check from the CPython main branch, since we use that function quite a lot in typing_extensions, and there might have been other changes to _type_check in recent versions of CPython that are causing bugs we don't know about yet in typing_extensions. It would be a more invasive change, though; I think this fix is fine for now

doc/index.rst Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@Gobot1234
Copy link
Contributor Author

Looks reasonable to me, other than my point about the docs.

Another possible fix could be to vendor the current version of _type_check from the CPython main branch, since we use that function quite a lot in typing_extensions, and there might have been other changes to _type_check in recent versions of CPython that are causing bugs we don't know about yet in typing_extensions. It would be a more invasive change, though; I think this fix is fine for now

I think its this change FWIW python/cpython@870b22b

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 6, 2023

I think its this change FWIW python/cpython@870b22b

Riiight... so we probably can't use typing_extensions.Annotated to wrap typing_extensions.ParamSpecArgs on lower versions of Python... probably not the biggest problem in the world... :)

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

thanks, lgtm!

@JelleZijlstra JelleZijlstra merged commit 13c9484 into python:main Sep 6, 2023
@Gobot1234 Gobot1234 deleted the patch-2 branch September 6, 2023 18:37
@AlexWaygood
Copy link
Member

I think its this change FWIW python/cpython@870b22b

Riiight... so we probably can't use typing_extensions.Annotated to wrap typing_extensions.ParamSpecArgs on lower versions of Python... probably not the biggest problem in the world... :)

lol, confirmed:

Python 3.9.12 (main, Apr  4 2022, 05:22:27) [MSC v.1916 64 bit (AMD64)] :: Anaconda, Inc. on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing_extensions import *
>>> P = ParamSpec("P")
>>> Annotated[P.args, 'foo']
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\.conda\envs\py39\lib\typing.py", line 277, in inner
    return func(*args, **kwds)
  File "C:\Users\alexw\.conda\envs\py39\lib\typing.py", line 1340, in __class_getitem__
    origin = _type_check(params[0], msg, allow_special_forms=True)
  File "C:\Users\alexw\.conda\envs\py39\lib\typing.py", line 166, in _type_check
    raise TypeError(f"{msg} Got {arg!r:.100}.")
TypeError: Annotated[t, ...]: t must be a type. Got P.args.

Works fine with typing on later Python versions:

Python 3.11.2 (tags/v3.11.2:878ead1, Feb  7 2023, 16:38:35) [MSC v.1934 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing import *
>>> P = ParamSpec("P")
>>> Annotated[P.args, 'foo']
typing.Annotated[P.args, 'foo']

@JelleZijlstra
Copy link
Member

We probably should vendor _type_check unconditionally. It's not a particularly likely thing to run into, but it is technically a bug in typing-extensions. Do you want to make a PR?

@AlexWaygood
Copy link
Member

Sure

@AlexWaygood
Copy link
Member

AlexWaygood commented Sep 6, 2023

We probably should vendor _type_check unconditionally.

hmm... seems tricky to just copy over the py311+ implementation of _type_convert without also backporting _type_convert() from py311+ (_type_convert is called by _type_check()), and that in turn requires backporting ForwardRef as it exists on py311+ (ForwardRef is instantiated inside _type_convert(), and is instantiated using new arguments that were only added to ForwardRef.__init__ in py311).

@JelleZijlstra
Copy link
Member

Thanks for investigating! That feels like it might be more trouble than a purely hypothetical use case is worth. Adding our own ForwardRef would potentially break users who do isinstance(..., typing.ForwardRef). (They should technically be checking for typing_extensions.ForwardRef anyway, but we don't have to break them unnecessarily.)

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.

3 participants