err: add PyErr::take

[davidhewitt]

This is a resurrection of #1715.

The PRs I have seen in downstream projects to update PyO3 to 0.15 often have a number of changes around PyErr::fetch which add additional panics and would benefit from what we called api_call_failed.

This PR changes PyErr::fetch to just return PyErr again, and adds PyErr::take which returns Option<PyErr>. I'm not convinced take is the perfect name for this function, but I can't think of anything better. (I briefly considered fetch_if_set, which is potentially clearer, although not exactly perfect either.)

[davidhewitt]

@mejrs continuing from #1949 (comment)

I think the most common usage of PyErr::fetch can also be seen in PyO3/rust-numpy#212

let result = unsafe { do_some_c_ffi(); }
if result == ERROR_VALUE {
    return Err(PyErr::fetch(py));
}

Signature of fetch(Python) -> Result<(), PyErr> is interesting, but I think doesn't fit well with that pattern above. For example, if there is no error set then

PyErr::fetch(py)?;

would just continue the program, but if the C FFI return value indicated there is an error then there's something more deeply wrong.

https://doc.rust-lang.org/std/net/struct.TcpStream.html#method.take_error - this is interesting. This makes me think the proposal to add take as per this PR is reasonably appropriate?

https://docs.rs/async-std/1.10.0/async_std/io/struct.Error.html#method.last_os_error - this is also interesting and I was considering it too. If we wanted a last_error method, then I would rename what is called PyErr::fetch in this PR to PyErr::last_error, and rename PyErr::take from this PR to PyErr::fetch. The only thing is that PyErr::last_error would also clear the last error state, which didn't quite sit right with me and why I went the take direction instead.

What do you think is best?

[mejrs]

IMO, this is about "what makes it easy for users to do the right thing".

I don't like having both of these:

fetch(py) -> PyErr;
take(py) -> Option<PyErr>

To me, take suggests that it could be missing and that that case should be handled. If there's another function that does (or seems to do) the exact same thing but can't fail that seems weird¹, and suggests the function does something "clever" to paper over that something went wrong.

if the C FFI return value indicated there is an error then there's something more deeply wrong.

If some function returns an error sentinel value but no exception is set, that's considered a bug, right?

I'd rather have only fetch(py) -> Option<PyErr>, and have the documentation tell users that they can return SystemError in case of a None (like what api_call_failed does), or unwrap if it indicates a bug.

Footnotes

1. Consider std's Path::exists and Path::try_exists apis for example. To me, the second's existence seems to suggest there's something weird, wrong or unexpected with the former. ↩

[davidhewitt]

Agreed on all of those points, and that was my original motivation to suggest fetch() -> Option<PyErr>.

If some function returns an error sentinel value but no exception is set, that's considered a bug, right?

Yes, and there was some debate about what to do in this case in this thread. In summary, we felt that the appropriate thing to do was to panic in debug mode and return SystemError in release mode. This was a judgement call and may turn out not to be "right".

IMO, this is about "what makes it easy for users to do the right thing".

Agreed, and that's why I'd like us to expose the functionality of api_call_failed for downstream users to also use. However, I didn't love the name api_call_failed, which is why I renamed it to fetch in this PR.

I was considering an alternative arrangement to fetch + take which was something like this:

fetch(py) -> Option<PyErr>;
last_error(py) -> PyErr    // or api_call_failed(py) -> PyErr

The advantage of exposing last_error or api_call_failed for users is that they then will have exactly the same bug-handling as we'll have in PyO3.

I wasn't entirely sure I liked either name last_error or api_call_failed, and so wasn't sure going for that design justified the breaking change to fetch for all users. Still, I could also support that design. Would that be preferable to you? I definitely prefer fetch returning Option, if we can come up with a supporting design which we're happy with.

[mejrs]

I was considering an alternative arrangement to fetch + take which was something like this:

I think I like that design, where it panics in debug mode and returns SystemError in release.

I didn't love the name api_call_failed

I agree.

What about fetch_or_systemerror?

[davidhewitt]

Hmm, fetch_or_systemerror is quite clear for the release behaviour, which is nice, however doesn't capture the debug panic.

What about fetch_last_error? To me that reads that it's similar to fetch, but that the user is expecting an error to have occurred.

[davidhewitt]

Actually, I like that last one enough to open a PR - #1962

[mejrs]

We could also mirror std's path api and have:

fetch(py) -> PyErr;
try_fetch(py) -> Option<PyErr>;

The main advantage of that is that it's not a breaking change.

[davidhewitt]

Avoiding the breaking change is definitely desirable if we're not totally sold on the alternative being better, for sure.

I considered try_fetch() the other day although all prior art I can find of try_ functions return Result not Option.

try_fetch(py) -> Result<PyErr, PyErr> (i.e. -> PyResult<PyErr>) could potentially work? We can return the PySystemError as the Err variant. Following the panic-on-debug behaviour I would make fetch essentially equivalent to try_fetch(py).unwrap(), and on release builds I would make fetch return the PyErr from whichever variant.

[davidhewitt]

Ok, to summarise where we're at: there are two main designs we have discussed:

1. Continue with breaking change, create the following pair of APIs:

fetch(py) -> Option<PyErr>;
fetch_last_error(py) -> PyErr;

2. Avoid breaking fetch, and add a new Option- or Result-based API.

fetch(py) -> PyErr;

take(py) -> Option<PyErr>;
// OR
try_fetch(py) -> PyResult<PyErr>;

---

I'm personally leaning towards design 1. fetch isn't so widely used that a breaking change is a huge problem. I like that fetch(py) -> Option<PyErr> correctly reflects the fact that it can't fail but may return nothing. fetch_last_error is similar to io::Error::last_os_error which is also used to interact with errors after C-style return values.

If we were to go for design 2, I think that having try_fetch do all the work of creating the SystemError is a little unfortunate, because frequently only whether an exception exists is relevant, and the SystemError will immediately be thrown away. E.g. this example which originally led to the take suggestion, as well as some of the changes in this PR. So I think I prefer take as proposed in this PR than try_fetch, and overall prefer design 1.

What's everyone else's preference? In the interest of making the release happen, I'd like to merge #1962 soon, perhaps tomorrow, if you folks are also happy with that.

[mejrs]

I don't love any of them, but I think I prefer:

fetch(py) -> PyErr;

take(py) -> Option<PyErr>;

We should add some documentation to both of these that say something along the lines of "if you receive some sort of error indicator and you call this function but it finds there's no exception set, that's a bug", and on fetch: "if you want to handle that case yourself, use take and handle None".

[davidhewitt]

@birkenfeld @messense - if you've got any preference towards design 1 or 2, would be really helpful to hear your inclination to help make a decision.

(+ @indygreg too - I think I have seen you use fetch in PyOxidizer; if you are willing to share any opinion on whether you like design 1 or 2 that would be great. We'll release 0.15 as soon as a decision on whether to revert or proceed with breaking fetch is made!)

[messense]

I like adding take(py) -> Option<PyErr>;, it's similar to Option::take.

[birkenfeld]

Yeah, fetch and take is fine with me.

[davidhewitt]

Thanks all, sounds like this PR reverting the breakage to fetch is the way to go. I've force-pushed some documentation improvements to this PR and will proceed to merge it. 👍