Error handling design

[emmatyping]

Rather than returning *mut PyObject which may be null, it'd be nice to have an ergonomic API to raise exceptions when need in Rust functions called from Python, and have this encoded in a Result type. The C API documentation for exception handling is located here: https://docs.python.org/3/c-api/exceptions.html

@youknowone wrote up a proof of concept of an error handling model that seems pretty interesting in #1

I like the idea of using a zero sized type (ZST) to mark that an exception has been raised, I think the API may need updating to build on top of what we decide for the basic object abstraction and thread state APIs, but I think it is a great starting point.

[davidhewitt]

I like the idea of using a zero sized type (ZST) to mark that an exception has been raised,

Yes, PyO3 currently lacks such an abstraction though I want to add one. Our PyErr is extremely bloated and needs simplifying / reworking.

One downside of the ZST is that there are many C APIs which do not expect to be called with an exception set. I cannot categorically say if they are unsound or will raise SystemError if there is an exception set.

Does this mean the ZST should clear the error when it is dropped? Probably, so the ZST may need the 'py lifetime so it can safely call PyErr_Clear. Probably the 'py lifetime makes sense on the ZST anyway as it is strongly related to the current thread state.

But even in that case, code like this is still "problematic":

let result = function_possibly_with_zst_error();  // might be `Err(e)`
do_other_thing_which_calls_c_api();  // might call C API with exception set

or

match function_possibly_with_zst_error() {
    Ok(x) => /* do whatever */,
    Err(e) => do_other_thing_which_calls_c_api(); // e is set, so calling C API with exception set
}

As long as calling C API without an exception set is sound, this is fine, just annoying to get SystemError back. It would be nicer if C APIs didn't care and chained exceptions like Python code, but I don't have the full history of that decision.

[davidhewitt]

Also something to consider is that usually we want to unconditionally propagate BaseException and only handle Exception values. PyO3 has not solved this yet. e.g. PyO3/pyo3#5457 (comment)

[youknowone]

The ZST is more like a wrapper interface of CPython error rather than a nice error handling API in Rust.

Implementing Drop for ZST to call PyErr_Clear sounds very confusing. Users will accidently call extra PyErr_Clear.

Probably adding custom unwrap-like method will be better.

function_possibly_with_zst_error().clear_err();  // ()
do_other_thing_which_calls_c_api();  // might call C API with exception set

match function_possibly_with_zst_error().ok_or_clear_err() {
    Some(x) => /* do whatever */,
    None => do_other_thing_which_calls_c_api(); // e is set, so calling C API with exception set
}

[davidhewitt]

Probably adding custom unwrap-like method will be better.

If the result type is not Result<(), ZstError> and instead some custom enum, having methods to unwrap or clear the error, fetch the error, etc is probably a good idea.

Having some methods could also encourage decisions about what to do with BaseException as per my possible concern above.

[encukou]

If the result type is not Result<(), ZstError> and instead some custom enum

That would means you can't use ? to handle Python exceptions, right? That would be sad.

One downside of the ZST is that there are many C APIs which do not expect to be called with an exception set.

That's all C APIs not documented otherwise (except ones with incomplete docs of course).
Some currently won't break in practice, but don't rely on that.

It would be nicer if C APIs didn't care and chained exceptions like Python code, but I don't have the full history of that decision.

It would; it's an idea for ... well, the next complete redesign of the C API.
Perhaps the Rust wrappers can do this automatically? And in cases where it e.g. hurts performance, we might add the appropriate C functions now.

As for history -- well, this predates exception chaining.

[encukou]

Also see #6: in the C API, the error indicator (PyErr_Occurred) is part of the thread state. Code that skips error C handling risks “poisoning” the thread state -- i.e. causing possible bad/undefined behaviour in some future API call. Typical C-style unsoundness, right?
If it's not contained in the wrappers, the current error indicator would probably need to be tracked on the thread state, possibly turning that into a Result-style type that tracks whether you can use it.