bpo-43224: Implement substitution of unpacked TypeVarTuple in C #31828
Conversation
serhiy-storchaka
requested review from
gvanrossum,
Fidget-Spinner and
JelleZijlstra
as code owners
|
Thanks for working on this! Please don't merge this without approval from the PEP 646 authors or the typing experts; I'm not confident we'll get all the edge cases right. I'd have liked a chance to review #31800 in more detail too. |
There was a problem hiding this comment.
I found a crash; see comment.
In general I'm worried that this approach will be too difficult to get right in all cases. I'd prefer the approach in #31804, which I suggested in #31021 (review).
Objects/genericaliasobject.c
Outdated
| if (varparam < nparams) { | ||
| if (nitems < nparams - 1) { | ||
| return PyErr_Format(PyExc_TypeError, | ||
| "!Too few arguments for %R", |
There was a problem hiding this comment.
| "!Too few arguments for %R", | |
| "Too few arguments for %R", |
Or does ! do something here?
There was a problem hiding this comment.
It was temporary, for debugging.
| @@ -467,7 +467,7 @@ def test_var_substitution(self): | |||
| T2 = TypeVar('T2') | |||
| class G(Generic[Unpack[Ts]]): pass | |||
|
|
|||
| for A in G, Tuple: | |||
| for A in G, Tuple, tuple: | |||
There was a problem hiding this comment.
There need to be more tests for cases where we substitute an unpacked tuple into the TypeVarTuple. I found a crash on your branch:
% ./python.exe
Python 3.11.0a6+ (heads/serhiy-storchaka-typevartuple-subst-c:a35af462a3, Mar 11 2022, 19:02:45) [Clang 13.0.0 (clang-1300.0.29.3)] on darwin
Type "help", "copyright", "credits" or "license" for more information
>>> from typing import Unpack, TypeVarTuple
>>> Ts = TypeVarTuple("Ts")
>>> t = tuple[Ts]
>>> t[int, Unpack[tuple[int, str]], str]
Assertion failed: (iparam != varparam), function _Py_subs_parameters, file genericaliasobject.c, line 382.
zsh: abort ./python.exe
There was a problem hiding this comment.
Good catch. Is not tuple[Ts] illegal?
I have added a runtime error for this case. It is consistent for Tuple, tuple and user generics.
|
When you're done making the requested changes, leave the comment: And if you don't make the requested changes, you will be poked with soft cushions! |
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @JelleZijlstra: please review the changes made to this pull request. |
|
Wow, thanks for implementing this - it hadn't even occurred to me this would be needed! I also feel uneasy, though - as I mentioned in #31800 we'd decided against implementing it this way. I think we should pause work on this until we've decided on exactly which approach we're proceeding with. |
|
Based on your note in #31800, I'm guessing you're going to proceed with this anyway. In that case, I think it would be worth adding some tests to this PR along the lines of b9b1c80#diff-04d29c98076c2d6bb75921ea9becb26a862544d39b71db87b6e354c759b9305dL794. |
|
I do not understand what tests are in question. The change you referred removes tests, not adds new tests. |
|
Right - I'm proposing adding them back, with the changes necessary to also test the code in this PR. |
(Code by Matthew Rahtz) Co-authored-by: Matthew Rahtz <mrahtz@gmail.com>
JelleZijlstra
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 0b9a4d3 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
https://bugs.python.org/issue43224