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 Concatenate and Generic with ParamSpec substitution #489

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

Daraan
Copy link
Contributor

@Daraan Daraan commented Oct 23, 2024

this PR is is a follow up on #491 and addresses multiple locations that case problems for different situations and versions; see comments below.

@Daraan
Copy link
Contributor Author

Daraan commented Oct 23, 2024

The code could be slimmed down in some cases if we introduce __typing_subst__ and __typing_prepare_subst__, was there a decision against introducing them?

@AlexWaygood
Copy link
Member

The code could be slimmed down in some cases if we introduce __typing_subst__ and __typing_prepare_subst__, was there a decision against introducing them?

Not that I'm aware of. But I think functions in typing.py and elsewhere don't pay attention to these hooks on Python less than 3.11(?), so it might be less effective to add them than you might think. (I'm not confident in this opinion, definitely try it out if you think it might work!)

@@ -3171,6 +3309,13 @@ def _collect_type_vars(types, typevar_types=None):
tvars.append(t)
if _should_collect_from_parameters(t):
tvars.extend([t for t in t.__parameters__ if t not in tvars])
elif isinstance(t, tuple):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a copied up from _collect_paramters needed to detect the parameters in the cases G5, G6, H9, G10

Comment on lines 3149 to 3159
if (inspect.isclass(cls) and issubclass(cls, typing.Generic)
and any(isinstance(t, ParamSpec) for t in cls.__parameters__)
):
# should actually modify parameters but is immutable
if (
len(cls.__parameters__) == 1
and parameters
and not _is_param_expr(parameters[0])
):
assert isinstance(cls.__parameters__[0], ParamSpec)
return
Copy link
Contributor Author

@Daraan Daraan Oct 23, 2024

Choose a reason for hiding this comment

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

See PR #491 for this base.

Note changes here are the combined checks from _prepare_paramspec_params(cls, params) and Generic.__class_getitem__ that would normally not execute _check_generic in 3.10+

Comment on lines +2644 to +2645
def _unpack_args(*args):
newargs = []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed for 3.8-3.10 to unpack Unpack during substitution, e.g.:

C2 = Concatenate[str, P]
U1 = Unpack[Tuple[int, str]]
self.assertEqual(C2[U1], (str, int, str))


# This is an invalid parameter expression but useful for testing correct subsitution
G10 = klass[int, Concatenate[str, P]]
with self.subTest("Check invalid form substitution"):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a valid parameter expression, it is nice for debugging though, should keep or remove it?

@Daraan Daraan marked this pull request as ready for review October 29, 2024 14:58
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.

Generic with ParamSpec and Concatenate substitutions behave incorrectly
2 participants