[BUG] Documentation suggests that custom type casters do not need to clear exceptions

[anntzer]

Issue description

The documentation at https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html suggests that custom type casters do not need to clear any Python exception that may have been set (the implementation of load basically returning !PyErr_Occurred()). Upon first reading, this seems reasonable as pybind11 may perhaps use that info for better error reporting. However, type casters actually need to clear the exception -- at least to make variant loading work.

I don't know if this should be fixed on the docs side, or on the implementation side.

Reproducible example code

The entire example, except the PYBIND11_MODULE block, is copied verbatim from https://pybind11.readthedocs.io/en/stable/advanced/cast/custom.html.

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

struct inty { long long_value; };

namespace pybind11 { namespace detail {
    template <> struct type_caster<inty> {
    public:
        /**
         * This macro establishes the name 'inty' in
         * function signatures and declares a local variable
         * 'value' of type inty
         */
        PYBIND11_TYPE_CASTER(inty, _("inty"));

        /**
         * Conversion part 1 (Python->C++): convert a PyObject into a inty
         * instance or return false upon failure. The second argument
         * indicates whether implicit conversions should be applied.
         */
        bool load(handle src, bool) {
            /* Extract PyObject from handle */
            PyObject *source = src.ptr();
            /* Try converting into a Python integer value */
            PyObject *tmp = PyNumber_Long(source);
            if (!tmp) {
                return false;
            }
            /* Now try to convert into a C++ int */
            value.long_value = PyLong_AsLong(tmp);
            Py_DECREF(tmp);
            /* Ensure return code was OK (to avoid out-of-range errors etc) */
            return !(value.long_value == -1 && !PyErr_Occurred());
        }

        /**
         * Conversion part 2 (C++ -> Python): convert an inty instance into
         * a Python object. The second and third arguments are used to
         * indicate the return value policy and parent object (for
         * ``return_value_policy::reference_internal``) and are generally
         * ignored by implicit casters.
         */
        static handle cast(inty src, return_value_policy /* policy */, handle /* parent */) {
            return PyLong_FromLong(src.long_value);
        }
    };
}}

PYBIND11_MODULE(python_example, m) {
    m.def("get_index", [](std::variant<inty, std::string> v) { return v.index(); });
}

Calling get_index("a") throws

ValueError: invalid literal for int() with base 10: 'a'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
SystemError: <built-in method get_index of PyCapsule object at 0x7fda858a08d0> returned a result with an error set

Fixing the implementation of load to always clear set exceptions instead makes get_index("a") correctly return 1.

[YannickJadoul]

When calling into the raw C API, you're always expected to check for errors as ... well, you've just circumvented pybind11 and called the Python C API.

So the docs are indeed (very) wrong here, I believe. @bstaletic?

[anntzer]

(Just to be clear, that seems totally reasonable to me, it's just the opposite of what the docs suggested.)

[bstaletic]

Yeah, the SystemError is definitely not an expected error. @anntzer Mind submitting a pull request?

[anntzer]

I'll leave it to others for now, but may revisit later if no one picks this up.