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))

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);
}

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.

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

The remark "(If you don’t understand this, don’t use this function. I warned you.)" comes directly from the documentation for PyErr_Restore and PyErr_Fetch which this API is meant to replace, as does the reference count behavior.

PyErr_GetRaisedException should return a new reference

How does that differ from the current behavior?

AFAICS, PyErr_GetRaisedException doesn't return a borrowed reference. The caller owns the return value.
It's just PyErr_SetRaisedException stealing its argument.

AFAICS, PyErr_GetRaisedException doesn't return a borrowed reference. The caller owns the return value.
It's just PyErr_SetRaisedException stealing its argument.

Huh? In that case, I'm reading this completely wrong:

Anyway, I still think it smells to introduce a new API that steals its argument. There should be no performance hit in making the caller instead of the callee having to decref that argument.

The remark "(If you don’t understand this, don’t use this function. I warned you.)" comes directly from the documentation for PyErr_Restore and PyErr_Fetch which this API is meant to replace, as does the reference count behavior.

If we introduce a new API, why copy the IMO inadequate docs and the smelly ref count behaviour? It's a new API; we can choose to follow the convention; we can choose to document it properly.

I'm not saying we shouldn't document it better, just don't blame me for the docs 🙂

I'm not saying we shouldn't document it better, just don't blame me for the docs 🙂

Sorry, if it came out that way; that was not my intent! Let's just amend the docs; I can propose a patch 🙂

(Regarding the reference counting, I'd still like to see the public APIs following the established conventions.)

(Re-opening till we've patched the docs)

We've fixed the docs; I'm fine with closing this now.

OOI, why are all the version labels applied?

OOI, why are all the version labels applied?

because the behavior of PyErr_SetObject() doesn't match the documentation in all those versions.
I suspect it has been wrong forever.

The fix for 3.12+ is a better API. For older versions, the docs need fixing.

So let's keep this issue open until we've patched the docs even more.

The fix for 3.12+ is a better API.

Furthermore, I still consider that the set-function should not steal a reference to its argument. A better API is a consistent API.

Before we can close this, we need...:

• What's New entry
• Tests for the get/set args C APIs
• Amend PyErr_SetObject docs for 3.12 through 3.10. (I don't think we can justify this as important enough to backport to 3.9 and 3.8. Backporting to 3.7 is definitely out of the question.)

@markshannon, after looking through the implementation again, I agree that this is indeed a better API. I'm very sorry for the noise I've created in trying to push for a change in semantics for PyErr_SetRaisedException.

Regarding #101578 (comment): I'll add some more tests soon, and I'll also draft a proposal for the PyErr_SetObject doc changes.

One more thing: did you deliberately not apply the Py_DEPRECATED macro to the deprecated APIs, @markshannon?

One more thing: did you deliberately not apply the Py_DEPRECATED macro to the deprecated APIs

Let's be careful with loud deprecations. The macros aren't particularly dangerous, so pushing users to rewrite their code wouldn't help much: this edge case most likely doesn't affect them, and if it does they “just” get the wrong exception. And they aren't much of a burden to keep in CPython.
IMO, a docs note is appropriate in this case. There's no plan to remove them.

There's no plan to remove them.

Sure; I don't think anyone is planning to do that.

So if the plan is to deprecate PyErr_SetObject, why change its semantics? The change in semantics may still trip folks up, which is not so nice -- they may need three versions of their calling code, one for the old PyErr_SetObject semantics (3.11 and before), one for the 3.12 semantics, and one using the new API. (PyErr_SetObject is in the stable ABI so they may want to use that in 3.12 in some configurations.)

There is no plan to change the semantics of PyErr_SetObject(). We plan to deprecate it.

We do seem to have to changed its semantics accidentally, as described in #102594, but the semantics in that case made no sense and was setting up a trap for whatever code tried to normalize the exception.

From offline conversation with Irit it looks like it's better to change the behavior of PyErr_SetObject (but in an acceptable way) without deprecating it. @markshannon We should discuss this at the Wednesday meeting.

Why not deprecate it?