Uh oh!
There was an error while loading. Please reload this page.
gh-133548: fix handling of empty and 1-item tuples for sys.exit - #135789
gh-133548: fix handling of empty and 1-item tuples for sys.exit#135789picnixz wants to merge 4 commits into
sys.exit#135789Conversation
bedevere-bot
commented
Jun 21, 2025
🤖 New build scheduled with the buildbot fleet by @picnixz for commit ae37e95 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135789%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
bcc2970 to
6b65314Compare| } | ||
| else { | ||
| PyErr_SetObject(PyExc_SystemExit, status); | ||
| } |
There was a problem hiding this comment.
Isn't this a problem with how PyErr_SetObject creates the exception? Shouldn't it be fixed there?
There was a problem hiding this comment.
I think it's a bit unfortunate because that's how _PyErr_CreateException handles tuples:
staticPyObject*_PyErr_CreateException(PyObject*exception_type, PyObject*value)
{
PyObject*exc;
if (value==NULL||value==Py_None) {
exc=_PyObject_CallNoArgs(exception_type);
}
elseif (PyTuple_Check(value)) {
exc=PyObject_Call(exception_type, value, NULL);
}
else {
exc=PyObject_CallOneArg(exception_type, value);
}
if (exc!=NULL&& !PyExceptionInstance_Check(exc)) {
PyErr_Format(PyExc_TypeError,
"calling %R should have returned an instance of ""BaseException, not %s",
exception_type, Py_TYPE(exc)->tp_name);
Py_CLEAR(exc);
}
returnexc;
}Unless you want me to change _PyErr_CreateException to prevent the fast paths, it could be annoying for downstream users.
There was a problem hiding this comment.
Changing PyErr_SetObject() is a much larger change, it should be carefully prepared since it would affect many C extensions.
Uh oh!
There was an error while loading. Please reload this page.
| if (PyTuple_Check(status)) { | ||
| /* Make sure that tuples are not flattened during normalization. */ | ||
| /* Make sure that tuples are not flattened during normalization | ||
| * due to the fast path for tuples in _PyErr_CreateException(). */ |
There was a problem hiding this comment.
It's not a "fast path" if removing it changes behaviour. We need to understand whether _PyErr_CreateException has a bug or not.
This PR is stale because it has been open for 30 days with no activity. |
vstinner
left a comment
There was a problem hiding this comment.
I like this approach but I have remarks on the implementation.
| * Several error messages incorrectly using the term "argument" have been corrected. | ||
| (Contributed by Stan Ulbrych in :gh:`133382`.) | ||
| * :func:`sys.exit` and :exc:`SystemExit` now correctly handle empty and 1-item |
There was a problem hiding this comment.
There is no need to mention SystemExit, the PR only changes sys.exit().
| * due to the fast path for tuples in _PyErr_CreateException(). */ | ||
| PyObject *exc = PyObject_CallOneArg(PyExc_SystemExit, status); | ||
| PyErr_SetObject(PyExc_SystemExit, exc); | ||
| Py_DECREF(exc); |
There was a problem hiding this comment.
exc can be NULL. You can use:
PyObject*exc=PyObject_CallOneArg(PyExc_SystemExit, status);
if (exc!=NULL) {
PyErr_SetObject(PyExc_SystemExit, exc);
Py_DECREF(exc);
}I don't think that we need a special path if PyTuple_Check(). Use the same code path for any status.
| } | ||
| else { | ||
| PyErr_SetObject(PyExc_SystemExit, status); | ||
| } |
There was a problem hiding this comment.
Changing PyErr_SetObject() is a much larger change, it should be carefully prepared since it would affect many C extensions.
cc @markshannon@iritkatriel
sys.exitunpacks its argument if it is a 0- or 1-element tuple #133548