Improve documentation of Python and C++ exceptions

[jbarlow83]

The main change is to treat error_already_set as a separate category
of exception that arises in different circumstances and needs to be
handled differently. The asymmetry between Python and C++ exceptions
is further emphasized.

[YannickJadoul]

Looks pretty nice to me. A few minor suggestions, but I'll leave it up to your judgement whether to implement or ignore them!

Thanks for your work! :-)

[YannickJadoul]

Maybe we can keep this and just add a note and a reference that this is different from the other exceptions?

[jbarlow83]

I've changed the section heading so we're now specifically taking about "C++ to Python" translation, so I think it's best to not mention here it all and treat it as a separate topic.

[jbarlow83]

I added a second table that I think fulfills my pedagogical goals.

[YannickJadoul]

Right, OK. I just thought it would be nice to have an overview of all exceptions, but I see your point. Well, the second table isn't highly necessary to me, then, but up to you!

[wjakob]

LGTM, thank you for the thorough writeup.

[bstaletic]

At first I really wasn't sure if this part of documentation needs fixing. This pull request does make things clearer.

[rwgk]

Thanks a lot for these great clarifications! I only have one minor suggestion/question. I think this PR is good to merge as-is. Potential follow-up work, if any (like adding an example), could be in a separate PR.

[rwgk]

A useful approach is to convert them to Python exceptions and then discard them as unraisable using the unraisablehook.

Is that a correct understanding? Do we have an example or unit test we could point to?

[jbarlow83]

#2372 adds a relevant example.

We could use Python's "discard as unraisable" API as a way of dealing with a C++ exception, but I'm not sure if we'd prescribe that. That could look like this

    void ~cpp_to_py() {
        try {
            std::vector<int> myvector(10);
            myvector.at(20) = 10; // throws std::out_of_range...
        } catch (std::exception &e) {
            PyErr_SetString(PyExc_IndexError, "C++ exception converted to Python");
            py::error_already_set eas;
            eas.discard_as_unraisable(__func__);
        }
    }

...but I'd stop well short of prescribing that as the official solution. It makes some sense to use the API since it's there and capable of reporting the issue. I think it would depend on the nature of the code we're binding.

The intermediate case is where we have a py::builtin_exception or user defined py::exception<T>, i.e. one of exceptions we translate to Python. I suppose it makes a lot more sense to use discard as unraisable there, because we were trying to raise a Python exception but we can't. Unfortunately, those, these two are not subclasses of each other, so there's no way to generically catch all exception translators, that I know of... but maybe we want something like:

    void ~cpp_to_py() {
        try {
            throw py::value_error("");
        } catch (py::exception<T> &e) {
            e(); // operator() sets the Python error
            py::error_already_set eas;
            eas.discard_as_unraisable(__func__);
        } catch (py::builtin_exception &e) {
            e.set_error(); // Yes the API is different
            py::error_already_set eas;
            eas.discard_as_unraisable(__func__);
        }
    }

[rwgk]

I see, thanks for the explanation! TBH I didn't make the connection from the "discard as unraisable" text and error_already_set::discard_as_unraisable, but only because in the commenting view I'm looking at right now the code isn't visible (I'm still getting up to speed with the GH tools). When looking at the whole file it's clear. My suggestion then would simply become some slight rewording:

A useful approach is to convert them to Python exceptions and then discard them as unraisable as shown above.

But also what you have already is great.

[YannickJadoul]

If this needs to be fixed, could we somehow (in a separate PR, I think this is good to go in as is) create py::discard_as_unraisable, which would call the exception translators and then call py::error_already_set::discard_as_unraisable?

The exception translation can be refactored out; I once did so for a PR (that got rejected, but not for that refactoring): https://github.com/pybind/pybind11/pull/1651/files#diff-f006a8533314068e1531966fc4927175R291-R318)

Apart from that, I'm not sure, but does py::exception<T> ever get thrown? I thought this was just the Python object, but that either the C++ exception gets thrown or py::error_already_set (but it never gets translated back to py::exception<T>, right?)

[jbarlow83]

I kept it simple and left the code at suggesting the user should make their own decision on unraisable C++ exceptions for now.

What you're proposing makes sense to me as a simpler way of doing handling these cases, although I'm not sure how much it needs to be fixed. Refactoring exception translation makes sense to me - pretty complex chunk of code where it is.

[YannickJadoul]

No rush on this indeed. I'm just not totally a fan of the three lines

            e.set_error(); // Yes the API is different
            py::error_already_set eas;
            eas.discard_as_unraisable(__func__);

but it works, and it's more than good enough! :-)

[YannickJadoul]

Whenever you're ready, give a shout, and I'll push "merge", @jbarlow83 ;-)

[jbarlow83]

Good to go

[YannickJadoul]

No reason to wait for CI, then! :-)
Thanks a lot for the hard work, @jbarlow83!