-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-138479: Ensure that __typing_subst__
returns a tuple
#138482
Conversation
return NULL; | ||
} | ||
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 about tuple
subclasses here? If so, do we want to test this? Or use PyTuple_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 use PyTuple_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); | ||
return NULL; | ||
} |
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 leaking newargs
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.
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.
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 argument PyTuple_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 now expected __typing_subst__ of %T objects to return a tuple, not %T
. Is that good?
Co-authored-by: Serhiy Storchaka <[email protected]>
Also fix incorrect DECREF.
Lib/test/test_typing.py
Outdated
|
||
evil = EvilTypeVar() | ||
type type_alias[*_] = 0 | ||
with self.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.
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.
@serhiy-storchaka Would you like to finish up your review? If not, I'll merge this. |
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. 👍
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 commit 1da989b) Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>
Sorry, @ZeroIntensity, I could not cleanly backport this to
|
GH-138784 is a backport of this pull request to the 3.14 branch. |
GH-138786 is a backport of this pull request to the 3.13 branch. |
…onGH-138482) Raise an exception if __typing_subst__ returns a non-tuple object. --------- Co-authored-by: Serhiy Storchaka <[email protected]> (cherry picked from commit 1da989b)
pythonGH-138482) Raise an exception if __typing_subst__ returns a non-tuple object. --------- (cherry picked from commit 1da989b) Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>
…138482) (#138784) Co-authored-by: Peter Bierma <[email protected]> Co-authored-by: Serhiy Storchaka <[email protected]>
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.