GeezerVison is showing github.com — `PyErr_SetObject()` behavior is strange and not as documented. · Issue #101578 · python/cpython
PyErr_SetObject() behavior is strange and not as documented. #101578
DescriptionBriefly:
PyErr_SetObject(exc_type, exc_val) does not create a new exception iff isinstance(exc_val, BaseException), but uses exc_val instead.
Callers of PyErr_SetObject() need various workarounds to handle this.
The long version:
Internally CPython handles exceptions as a triple (type, value, traceback), but the language treats exceptions as a single value.
This a legacy of the olden days before proper exceptions.
To handle adding proper exceptions to Python, various error handling functions, specifically _PyErr_SetObject still treat exceptions as triples, with the convention that if the value is an exception, then the exception is already normalized.
One other oddity is that if exc_val is a tuple, it is treated as the * arguments to exc_type when calling it. So, if isinstance(exc_val, BaseException) the desired behavior can be achieved by wrapping exc_val in a one-tuple.
As a consequence, both _PyErr_SetKeyError and _PyGen_SetStopIterationValue are a lot more complex than they should be to workaround this behavior.
We could make PyErr_SetObject act as documented, but that is likely to break C extensions, given how old this behavior is, and that it is relied on throughout CPython.
Code that does the following is common:
exc = new_foo_exception();
PyErr_SetObject(&PyFooException_Type, exc);We could just document the current behavior, but the current behavior is strange.
What I suggest is this:
- Create a new API function, PyErr_SetException(exc)` that takes a single exception object.
- Document
PyErr_SetObject() accurately
- Deprecate the old function
This is an old bug going back to the 2 series.
Linked PRs
Also relevant:
Activitymarkshannon commented on Feb 5, 2023 The only case I can find of strange behavior leaking out is this:
import sys
def wrap_in_sys_exit(ex):
try:
try:
raise ex(-1)
except ex as err:
sys.exit(err)
except BaseException as err:
return err
print(repr(wrap_in_sys_exit(ValueError)))
print(repr(wrap_in_sys_exit(SystemExit)))Which prints
SystemExit(ValueError(-1))
SystemExit(-1)
instead of the expected
SystemExit(ValueError(-1))
SystemExit(SystemExit(-1))
rhettinger commented on Feb 5, 2023 This API goes by to 1997. It is remarkable how simple and clear the code used to be:
void
PyErr_Restore(PyObject *type, PyObject *value, PyObject *traceback)
{
PyThreadState *tstate = PyThreadState_GET();
PyObject *oldtype, *oldvalue, *oldtraceback;
if (traceback != NULL && !PyTraceBack_Check(traceback)) {
/* XXX Should never happen -- fatal error instead? */
Py_DECREF(traceback);
traceback = NULL;
}
/* Save these in locals to safeguard against recursive
invocation through Py_XDECREF */
oldtype = tstate->curexc_type;
oldvalue = tstate->curexc_value;
oldtraceback = tstate->curexc_traceback;
tstate->curexc_type = type;
tstate->curexc_value = value;
tstate->curexc_traceback = traceback;
Py_XDECREF(oldtype);
Py_XDECREF(oldvalue);
Py_XDECREF(oldtraceback);
}
void
PyErr_SetObject(PyObject *exception, PyObject *value)
{
Py_XINCREF(exception);
Py_XINCREF(value);
PyErr_Restore(exception, value, (PyObject *)NULL);
}Reacted by Giampaolo Rodola markshannon commented on Feb 6, 2023 The code may have been simple, but it can leave the VM in a precarious state if the value is not a legal argument for calling the type. erlend-aasland commented on Feb 14, 2023 The new C APIs PyErr_GetRaisedException and PyErr_SetRaisedException are IMO problematic:
- the former returns a borrowed reference
- the latter steals a reference
This goes against our recommendations for adding new public C APIs.
I also find the added documentation inadequate. It is not documented that the former function returns a borrowed reference. The documentation for the latter function is not exactly friendly written, and I'm specifically thinking of the last parenthesised sentence:
exc must be a valid exception. (Violating this rules will cause subtle problems later.) This call consumes a reference to the exc object: you must own a reference to that object before the call and after the call you no longer own that reference. (If you don’t understand this, don’t use this function. I warned you.)
Suggesting to considering reopening this issue until at least the docs have been improved. IMO, we should also fix the newly added C APIs to be consistent in their reference handling, which means:
PyErr_GetRaisedException should return a new referencePyErr_SetRaisedException should not steal a reference added a commit that references this issue on Dec 31, 2023gh-101578: [doc] mention that PyErr_GetRaisedException returns NULL w… markshannon commented on Aug 15, 2024
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|