bpo-43224: Implement substitution of unpacked TypeVarTuple in C#31828
Uh oh!
There was an error while loading. Please reload this page.
Conversation
serhiy-storchaka requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners
JelleZijlstra commented Mar 11, 2022
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. |
JelleZijlstra left a comment
JelleZijlstra left a comment 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){ | ||
| returnPyErr_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.
| classG(Generic[Unpack[Ts]]): pass | ||
| forAinG, Tuple: | ||
| forAinG, 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.
bedevere-bot commented Mar 12, 2022
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! |
serhiy-storchaka commented Mar 12, 2022
I have made the requested changes; please review again. |
bedevere-bot commented Mar 12, 2022
Thanks for making the requested changes! @JelleZijlstra: please review the changes made to this pull request. |
mrahtz commented Mar 12, 2022
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. |
mrahtz commented Mar 13, 2022
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. |
serhiy-storchaka commented Mar 13, 2022
I do not understand what tests are in question. The change you referred removes tests, not adds new tests. |
mrahtz commented Mar 13, 2022
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 bedevere-bot commented Apr 29, 2022
🤖 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 6 participants
https://bugs.python.org/issue43224