gh-124445: Allow specializing generic ParamSpec aliases #124512
Conversation
|
Just trying to debug this weird error now: Unfortunately seems to only happen when running the tests, not when I run the test case as a script :/ |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
|
Ready for another round :) |
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
Objects/genericaliasobject.c
Outdated
| Py_ssize_t len = nargs; | ||
| PyObject *parameters = PyTuple_New(len); | ||
| if (parameters == NULL) | ||
| return NULL; | ||
| Py_ssize_t iparam = 0; | ||
| for (Py_ssize_t iarg = 0; iarg < nargs; iarg++) { | ||
| PyObject *t = PyTuple_GET_ITEM(args, iarg); | ||
| PyObject *t = is_args_tuple ? PyTuple_GET_ITEM(args, iarg) : PyList_GET_ITEM(args, iarg); |
There was a problem hiding this comment.
This is still unsafe for lists, since below you invoke code that might call back into arbitrary Python code that mutates the list. I think for safety, you need to either convert the list into a tuple or use an iteration API.
There was a problem hiding this comment.
Damn Python getting in the way of my C code 😅 I'll use the tuple workaround..
There was a problem hiding this comment.
Ok I think that should be fixed. I now convert the arguments to a tuple before processing them. Btw, do you have any opinion on this? #124512 (comment)
There was a problem hiding this comment.
I don't entirely agree with Daraan; I have it on my todo list to respond to the Discuss thread they opened.
There was a problem hiding this comment.
Hi Jelle, just wondering if you've had the time to ponder this issue? What should we do about this PR?
There was a problem hiding this comment.
Looking at this again I'm not sure what I was thinking of; I think Daraan's post was mostly right.
There was a problem hiding this comment.
(replying to your discourse answer): I think making generic aliases hashable by converting lists to tuples is a good idea but I'd prefer to implement it in a separate PR (@Daraan already made an issue for it) since it affects both the C (GenericAlias) and Python (_GenericAlias) implementations.
If that's fine by you, then this PR is ready for a final check :)
|
|
||
| // Recursively substitute params in lists/tuples. | ||
| if (PyTuple_Check(arg) || PyList_Check(arg)) { | ||
| PyObject *subargs = _Py_subs_parameters(self, arg, parameters, item); |
There was a problem hiding this comment.
Passing parameters as is here seems wrong; what if we only need part of the parameters? _Py_subs_parameters will error if the count doesn't match. The test case I suggested should help verify this.
There was a problem hiding this comment.
Actually with some local testing it seems this does work correctly. More tests would be useful though.
There was a problem hiding this comment.
This actually uncovered an issue where nested args tuples would be converted to lists, which should be now fixed.
Also added more tests if you feel like having another look :)
| @@ -0,0 +1 @@ | |||
| Allow specializing Generic ParamSpec aliases | |||
There was a problem hiding this comment.
This isn't very clear. Something like "Fix specialization of generic aliases that are generic over a :class:typing.ParamSpec and have been specialized with a nested type variable."
There was a problem hiding this comment.
Thanks! That's much clearer 🙂
I think I more or less understood how this is supposed to work, but please let me know if I missed something :)
Also, if you think I should add more tests.