gh-91162: Fix substitution of unpacked tuples in generic aliases #92335
Conversation
serhiy-storchaka
requested review from
gvanrossum,
Fidget-Spinner and
JelleZijlstra
as code owners
|
cc @mrahtz |
| @@ -664,23 +658,29 @@ class C(Generic[T1, T2]): pass | |||
| ('generic[T1, T2]', '[int, str]', 'generic[int, str]'), | |||
| ('generic[T1, T2]', '[int, str, bool]', 'TypeError'), | |||
| ('generic[T1, T2]', '[*tuple_type[int]]', 'TypeError'), | |||
| ('generic[T1, T2]', '[*tuple_type[int, str]]', 'TypeError'), | |||
| ('generic[T1, T2]', '[*tuple_type[int, str]]', 'generic[int, str]'), | |||
There was a problem hiding this comment.
Based on the tentative spec we'd decided on at #91162 (comment), even though this is valid and easy to evaluate, we should also disallow it so that the rule stays simple: "If the generic accepts a fixed number of arguments, it can't take any unpacked type arguments."
Ditto various cases below.
There was a problem hiding this comment.
I think the spec should be changed. There is no problem to unpack a tuple of fixed length into the corresponding number of type variables. I think that it is simpler to allow this than add a special exception.
There was a problem hiding this comment.
@pradeep90 Do you want to argue the case here? I agreed with you in #32341 (comment) tbh mainly because it didn't seem a huge deal and I didn't want to drag out the discussion there, but I actually agree with Serhiy here that it seems simpler to just allow it. Is there context that Serhiy and I are missing here?
As a starting point, I think the rule here could be something like: "You can only use an unpacked arbitrary-length type (such as an unpacked TypeVarTuple or an unpacked arbitrary-length tuple) in the arguments list if the generic accepts an arbitrary number of type parameters". That seems a simple enough rule to me - it disallows tuple[T][*tuple[int, ...]] and tuple[T][*Ts] while allowing tuple[T][*tuple[int]]. Or are there edge cases it wouldn't cover?
There was a problem hiding this comment.
As a starting point, I think the rule here could be something like: "You can only use an unpacked arbitrary-length type (such as an unpacked TypeVarTuple or an unpacked arbitrary-length tuple) in the arguments list if the generic accepts an arbitrary number of type parameters". That seems a simple enough rule to me
I disagree that it's a simple enough rule. Given an already-complex PEP, this seems like yet another arcane rule for users to keep in mind. The only benefit it gives us is that we allow the fairly useless example of generic[T1, T2][*tuple[int, str]]. Contrast that to the simpler rule that "*<anything> is only for variadic types". That way, users don't have to ever be surprised by weird syntax such as list[T][*tuple[int]].
I don't want to block runtime behavior on this. The runtime can always be more flexible and accept arbitrary cases. Type checkers can forbid this during type checking to keep things simple for users.
In short, for the runtime implementation, I'm ok with whatever is simpler for you and Serhiy. I disagree with changing the spec we agreed on for the sake of this.
There was a problem hiding this comment.
Oh, yeah, to be clear - this conversation is totally just about the spec for what we allow at runtime.
If you're happy with the runtime being flexible on this, then so am I. You can go ahead and resolve this, Serhiy.
Lib/test/test_typing.py
Outdated
| @@ -603,22 +603,16 @@ class C(Generic[T]): pass | |||
| ('generic[T]', '[int]', 'generic[int]'), | |||
| ('generic[T]', '[int, str]', 'TypeError'), | |||
| ('generic[T]', '[tuple_type[int, ...]]', 'generic[tuple_type[int, ...]]'), | |||
| ('generic[T]', '[*tuple_type[int]]', 'generic[int]'), | |||
| # Should raise TypeError: a) according to the tentative spec, | |||
There was a problem hiding this comment.
We can get rid of the comment here and a few lines below now that these cases do indeed raise a TypeError.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
|
GH-92484 is a backport of this pull request to the 3.11 branch. |
| !(PyTuple_GET_SIZE(subargs) && | ||
| PyTuple_GET_ITEM(subargs, PyTuple_GET_SIZE(subargs)-1) == Py_Ellipsis)) | ||
| { | ||
| if (PyList_SetSlice(newargs, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, subargs) < 0) { |
There was a problem hiding this comment.
Noob question: how does setting it to PY_SSIZE_T_MAX work? I'm surprised it's not something like low=i, high=i+PyTuple_GET_SIZE(subargs) ? I think I missed something.
There was a problem hiding this comment.
It works like:
a = [1, 2, 3]
a[sys.maxsize:sys.maxsize] = [4, 5]There was a problem hiding this comment.
I knew the Python equivalent but my mind is blown. I didn't know that was possible in Python! I'd always assumed list.extend was the only way to do it. Thanks for teaching me something new today Serhiy.
Last question: Why not use _PyList_Extend. Or is it just a personal preference which to use?
There was a problem hiding this comment.
_PyList_Extend is not convenient. It returns Py_None, for which you should call Py_DECREF. Compare:
if (PyList_SetSlice(newargs, PY_SSIZE_T_MAX, PY_SSIZE_T_MAX, subargs) < 0) {
// error
}and
PyObject *tmp = _PyList_Extend(newargs, subargs);
if (tmp == NULL) {
// error
}
else {
Py_DECREF(tmp);
}Using PyList_SetSlice() for list.extend() or list.clear() is idiomatic.
pythonGH-92335) (cherry picked from commit 9d25db9) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…es (GH-92335) (#92484) * gh-91162: Fix substitution of unpacked tuples in generic aliases (GH-92335) (cherry picked from commit 9d25db9) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> * Regenerate ABI file Co-authored-by: Serhiy Storchaka <storchaka@gmail.com> Co-authored-by: Pablo Galindo <pablogsal@gmail.com>
No description provided.