-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
[3.14] Address invalid inputs of TypeAliasType #477
[3.14] Address invalid inputs of TypeAliasType #477
Conversation
Also draft for others
- compatibility tests have their own test - skip tests for appropriate python versions
I merged the upstream PR but did not backport it to 3.12 and 3.13, so this should now be ready to go. I'll push some buttons to do so and then review the code. |
src/test_typing_extensions.py
Outdated
for case, msg in invalid_cases: | ||
with self.subTest(type_params=case): | ||
if TYPING_3_12_0 and sys.version_info < (3, 14): | ||
self.skipTest("No backport for 3.12 and 3.13 requires cpython PR #124795") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided not to backport in CPython. This test should be changed so that typing_extensions does provide the backport.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As TypeAliasType
cannot be subclassed, I tried to do a minimal invasive workaround with __new__
to return the typing
variant but this fails the pickling check.
Do you know a workaround or is this __new__
method not something to pursue further?
At the moment the <3.12 backport would pass the test and would work as a full backport for 3.12 & 3.13.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just use the pre-3.12 variant on 3.12 and 3.13.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess with annotationlib (likely?) comming it needs a backport anyway.
src/typing_extensions.py
Outdated
parameters = [] | ||
for type_param in type_params: | ||
if not isinstance(type_param, (TypeVar, TypeVarTuple, ParamSpec)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to accept both the typing and typing_extensions versions of these classes, which are different on some versions. Test this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I saw this too, but realized that _TypeVarLikeMeta
handles isinstance
check. test_type_params_compatibility
test_type_var_compatibility
and the other methods test this.
…' into TypeAliasType/invalid_param_spec
src/test_typing_extensions.py
Outdated
((1,), "Expected a type param, got 1"), | ||
((str,), f"Expected a type param, got {str!r}"), | ||
# Unpack backport behaves like TypeVar in some cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What "some cases" exactly? Ideally this should be accepted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the comment for more clarity. As isinstance(Unpack[Ts], TypeVar) is True
in Python < 3.12 Unpack
would not raise here without the additional check and would be accepted as a type parameter.
for type_param in type_params:
if (not isinstance(type_param, (TypeVar, TypeVarTuple, ParamSpec))
or _is_unpack(type_param)
):
raise TypeError(f"Expected a type param, got {type_param!r}")
I've prepared this draft which contains two changes:
typing_extensions.TypeAliasType
not raising TypeError if a non-tuple is passed compared to thetyping
variant.backport of
gh-124787: Fix
TypeAliasType
and incorrecttype_params
cpython#124795 for <=3.11 (needs to be accepted first)Await merge and change if necessary
Afterwards fix correct python versions. I currently assume the backport is valid in 3.14.
Should backport for 3.12-13 be made? Yes.
It should maybe noted that for 3.12.0-3.12.7 the
typing.TypeAliasType
variant does not contain this stricter backport.I do not think that there is a necessity to do a backport for 3.12 and it will likely do more harm (I did not test it).