Skip to content

PyErr_SetObject() behavior is strange and not as documented. #101578

Description

@markshannon

Briefly:
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:

Activity

  1. markshannon commented on Feb 5, 2023

    @markshannon
    MemberAuthor

    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))
    
  2. rhettinger commented on Feb 5, 2023

    @rhettinger
    Contributor

    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);
    }
  3. markshannon commented on Feb 6, 2023

    @markshannon
    MemberAuthor

    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.

  4. added a commit that references this issue on Feb 8, 2023
  5. added a commit that references this issue on Feb 9, 2023
  6. erlend-aasland commented on Feb 14, 2023

    @erlend-aasland
    Contributor

    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 reference
    • PyErr_SetRaisedException should not steal a reference
  7. 44 remaining items

  8. added a commit that references this issue on Dec 21, 2023
  9. added a commit that references this issue on Dec 31, 2023
  10. added a commit that references this issue on Dec 31, 2023
  11. added a commit that references this issue on Dec 31, 2023
  12. added a commit that references this issue on Jan 22, 2024
  13. added a commit that references this issue on Feb 11, 2024
  14. markshannon commented on Aug 15, 2024

    @markshannon
    MemberAuthor

    I think this is all done. Thanks @iritkatriel and @erlend-aasland

  15. added 2 commits that reference this issue on Sep 1, 2024
  16. added a commit that references this issue on Sep 2, 2024
  17. added 2 commits that reference this issue on Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions