GeezerVison

I think the doc should say what happens if you violate the rule. I think in reality it will just create the cycle and if it doesn't bother you then good for you.

I think the doc should say what happens if you violate the rule.

Is undefined behavior, technically as the contract of this function withing CPython is that cycles should not be created. Some parts of the exception machinery can break (or at least they used to) in presence of cycles.

But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.

How is the caller supposed to ensure this? Is there any API for checking it?

There is no API to check this, but to create a cycle with exceptions it needs to be s deliberate action. There is not an easy way to accidentally crate an exception cycle

But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.

I think the movement is in the direction of trying to prevent / avoid the creation of cycles in the first place (as was mentioned in the discussion for this recent ticket, for example). The warning being added here is in line with that.

As per my comment below, the easiest way to violate this requirement is to call the API without setting a new exception first (or clearing the existing one). Buggy code (using my API naming suggestion from above):

PyErr_Fetch(&type, &value, &traceback);
// Exception state is not cleared, no new exception is set
PyErr_RestoreOrChain(type, value, traceback); // Sets exception as its own context

Because the exception might not have been instantiated yet when PyErr_Fetch() is called, there's no easy way for the API to reliably detect this error, so the onus is on the caller to manage the exception state correctly.

Isn't it the other way around - the existing exception becomes the context of the provided exception?

The code calls PyException_SetContext(val2, val); where val is the provided one and val2 is the one that is currently set. PyException_SetContext sets the second argument as the context for the first.

So unless I am missing something (is late here 😅 ) this means that "The provided exception (val) becomes the context of the existing exception (val2)"

Right, I was thinking we are raising the provided exception in the context of the existing one, but that's what SetObject is for. Can we think of a more descriptive name for this function? I confused the direction, and you confused cause/context because "chain" is ambiguous here.

How about PyErr_RestoreContext? IIUC this describes what you use this function for - putting back a fetched exception, as the context of any exception that happened between the fetch and now. (Should something like the previous sentence be in the doc?)

Or possibly PyErr_RestoreAsContext?

In general authors look for "chain" in the docs because that's the action they want to do, so I would really recommend to have "chain" in the name.

That may be the case, but I think we should promote clear language, and "chain" in this context is vague. The doc can contain "chain" in the prose for the benefit of people who search for it (for now). But should it be part of the function name for all eternity?

Is unclear to me. I agree with the ambiguity but what users if the function want is a higher level approach to the python behaviour: an exception is raised when another exception was there and I want to do in C the same thing as Python does.

I'm not a native speaker do I trust your judgement regarding how ambiguous this sounds and that we need a better name so let's search for one.

Maybe we should make a section in the docs about "exception chaining" and offer two APIs: one for cause and another for context

@iritkatriel Maybe you can suggest a more specific name that still retains "chain" as part of it?

The context is an implicitly chained exception (the cause is explicitly chained). There is no need to have both chained and context in the name. If we have only one of them I think it should be context and not chain.

Given the equivalence to PyErr_Restore when no exception is currently set, my public naming suggestion would be PyErr_RestoreOrChain, with the shorthand documentation being "Restores the given exception state, or chains it as the exception context if an exception is already set".

The key point to emphasise is that the function takes the same arguments as PyErr_Restore (retrieved from a prior PyErr_Fetch call), and generally does the same thing, but handles currently set exceptions differently: instead of clearing them, it leaves them in place, and "restores" the given exception as the exception context, rather than as the active exception.

Right, I was thinking we are raising the provided exception in the context of the existing one, but that's what SetObject is for. Can we think of a more descriptive name for this function? I confused the direction, and you confused cause/context because "chain" is ambiguous here.

How about PyErr_RestoreContext? IIUC this describes what you use this function for - putting back a fetched exception, as the context of any exception that happened between the fetch and now. (Should something like the previous sentence be in the doc?)

But when the exception machinery breaks in the presence of a cycle we consider that to be a bug in python.

Could it not steal its arguments?
Functions that don't follow the usual calling convention are harder to reason about; it would be good to treat them as historical exceptions.

It steals the arguments because of what it calls. It had the same calling convention as Pyerr_Restore.

Also, stealing arguments is not an uncommon calling convention.

Stealing references is only common because much of the public API was born as internal APIs, and we often try to avoid redundant refcount adjustments in internal APIs.

It took us a long time to realise that stealing references in public APIs makes things confusing for API clients, and can easily lead to double decref bugs (which are more potentially destructive than the object leaks that can arise from missing decref bugs, since deleting objects early can lead to access violations). So I think the new public API should be a wrapper around the internal API that avoids stealing references.

I changed my mind on this point, as PyErr_Fetch and PyErr_Restore are a matched pair. If we go with my PyErr_RestoreOrChain naming suggestion, then the refcounting semantics of the new API should match those of PyErr_Restore (which is to take back the references that PyErr_Fetch created)

How is the caller supposed to ensure this? Is there any API for checking it?

Stealing references is only common because much of the public API was born as internal APIs, and we often try to avoid redundant refcount adjustments in internal APIs.

It took us a long time to realise that stealing references in public APIs makes things confusing for API clients, and can easily lead to double decref bugs (which are more potentially destructive than the object leaks that can arise from missing decref bugs, since deleting objects early can lead to access violations). So I think the new public API should be a wrapper around the internal API that avoids stealing references.

I changed my mind on this point, as PyErr_Fetch and PyErr_Restore are a matched pair. If we go with my PyErr_RestoreOrChain naming suggestion, then the refcounting semantics of the new API should match those of PyErr_Restore (which is to take back the references that PyErr_Fetch created)

Given the equivalence to PyErr_Restore when no exception is currently set, my public naming suggestion would be PyErr_RestoreOrChain, with the shorthand documentation being "Restores the given exception state, or chains it as the exception context if an exception is already set".

The key point to emphasise is that the function takes the same arguments as PyErr_Restore (retrieved from a prior PyErr_Fetch call), and generally does the same thing, but handles currently set exceptions differently: instead of clearing them, it leaves them in place, and "restores" the given exception as the exception context, rather than as the active exception.

This example is misleading, as if the code doesn't set a new exception, the fetched exception will end up being chained with itself instead of being restored. To safely invoke the new API unconditionally, the code needs to look like this:

    /* Fetch and clear the original exception */
    PyErr_Fetch(&type, &value, &traceback);
    PyErr_Clear();

    /* ... code that might produce other errors ... */

    /* Either restore the original exception, or set it as the context for any new exception */
    PyErr_RestoreOrChain(type, value, traceback);

Skipping the PyErr_Clear() call is only OK if the code definitely sets a new active exception.

As per my comment below, the easiest way to violate this requirement is to call the API without setting a new exception first (or clearing the existing one). Buggy code (using my API naming suggestion from above):

PyErr_Fetch(&type, &value, &traceback);
// Exception state is not cleared, no new exception is set
PyErr_RestoreOrChain(type, value, traceback); // Sets exception as its own context

Because the exception might not have been instantiated yet when PyErr_Fetch() is called, there's no easy way for the API to reliably detect this error, so the onus is on the caller to manage the exception state correctly.