Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-138479: Ensure that__typing_subst__ returns a tuple#138482
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| returnNULL; | ||
| } | ||
| if (unpack) { | ||
| if (!PyTuple_Check(arg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we care abouttuple subclasses here? If so, do we want to test this? Or usePyTuple_CheckExact?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it would be fine to do either. It would technically be a breaking change to disallow tuple subclasses here, but I doubt anyone is actually doing that in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It's an internal undocumented API so I wouldn't feel terrible about disallowing tuple subclasses, but we could go either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's just usePyTuple_Check for the time being. There's no drawback to keeping subclass support here, so we might as well keep it.
| "expected a tuple, not %T",arg); | ||
| Py_DECREF(arg); | ||
| returnNULL; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can't comment there but are we leakingnewargs on line 543 below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like it. Do you want me to fix that in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, might as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
Objects/genericaliasobject.c Outdated
| if (jarg<0) { | ||
| Py_DECREF(item); | ||
| Py_XDECREF(tuple_args); | ||
| Py_DECREF(newargs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It is NULL here.
ZeroIntensitySep 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ah,tuple_extend steals the reference on failure. I added a comment here instead. Thanks for clearing this up.
Uh oh!
There was an error while loading.Please reload this page.
Objects/genericaliasobject.c Outdated
| Py_DECREF(item); | ||
| Py_XDECREF(tuple_args); | ||
| PyErr_Format(PyExc_TypeError, | ||
| "expected a tuple, not %T",arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This error message is not helpful. Please add what method of what type has been called (this was method__typing_subst__ of original argumentPyTuple_GET_ITEM(args, iarg)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done, it's nowexpected __typing_subst__ of %T objects to return a tuple, not %T. Is that good?
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Also fix incorrect DECREF.
Lib/test/test_typing.py Outdated
| evil=EvilTypeVar() | ||
| typetype_alias[*_]=0 | ||
| withself.assertRaises(TypeError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you please test an error message? No need to test the full match, just some unique pattern to ensure that this is an expected TypeError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
vstinner left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. The change does fix the crash.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ZeroIntensity commentedSep 11, 2025
@serhiy-storchaka Would you like to finish up your review? If not, I'll merge this. |
serhiy-storchaka left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. 👍
1da989b intopython:mainUh oh!
There was an error while loading.Please reload this page.
Thanks@ZeroIntensity for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…onGH-138482)Raise an exception if __typing_subst__ returns a non-tuple object.---------(cherry picked from commit1da989b)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sorry,@ZeroIntensity, I could not cleanly backport this to |
GH-138784 is a backport of this pull request to the3.14 branch. |
GH-138786 is a backport of this pull request to the3.13 branch. |
…onGH-138482)Raise an exception if __typing_subst__ returns a non-tuple object.---------Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>(cherry picked from commit1da989b)
pythonGH-138482)Raise an exception if __typing_subst__ returns a non-tuple object.---------(cherry picked from commit1da989b)Co-authored-by: Peter Bierma <zintensitydev@gmail.com>Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Raise an exception if
__typing_subst__returns a non-tuple object.The test case is a little messy because it's based on fuzzer-generated code, so I can come up with my own repro if people aren't happy with using it.