feat: add detail::type_caster_std_function_specializations

[wangxf123456]

Description

For callback functions that throw Python exceptions, we might want to handle it inside the func_wrapper specializations, instead of throwing the uncaught pybind11::error_already_set exception.

Note that the production code change is a completely straightforward local refactoring, simply moving two types from class scope to namespace scope. The increase in code complexity is very minor, the compile time increase is probably in the measurement noise, and the runtime overhead is expected to be zero.

The exact case we want to support is callback functions with absl::Status and absl::StatusOr return values. If the status it not ok, an exception will be thrown: https://github.com/pybind/pybind11_abseil/blob/429ae15db6c8d6ccac21bf6e1a84e92ab1d2c1a4/pybind11_abseil/status_caster.h#L110. But for callback functions, in some situations we don't want to propagate any exception. Instead, we want to convert the exception to an absl::Status Python object.

Suggested changelog entry:

A ``pybind11::detail::type_caster_std_function_specializations`` feature was added, to support specializations for ``std::function``'s with return types that require custom to-Python conversion behavior (to primary use case is to catch and convert exceptions).

[rwgk]

@lalaland or @Skylion007: Could you please help with an independent review?

[EthanSteinberg]

Is this change actually necessary? Adding additional specializations like this adds quite a bit to the public API and can be quite clunky to use. Tracking down that functions with a certain return type can be registered / deregistered in this certain way seems like it would be tricky in the long-term IMHO. We already have a lot of issues tracking down various type caster registration things.

Can't you get an equivalent result here by making your function take an object as input and have a separate function that casts it so std::function?

CC @rwgk

[wangxf123456]

Thanks for your suggestions! Actually I tried that first. The only problem is that the functions are very similar with the type caster of std::function, with only a few lines change. This PR can avoid unnecessary copy-paste of code.

[EthanSteinberg]

What about a design like this?

I understand there will be some redundancy, but it seems that's still a better option than exposing deep internals in a sorta hard to detect way.

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

#include "pybind11_tests.h"

namespace py = pybind11;

namespace {

struct SpecialReturn {
    int value = 99;
};

} // namespace

template <typename... Args>
SpecialReturn custom_converter(std::function<SpecialReturn(Args... args)> func, Args... args) {
        SpecialReturn  result;
        try {
            result = func(std::forward<Args>(args)...);
        } catch (error_already_set &) {
            result.value += 1;
        }
        result.value += 100;
        return result;
}

TEST_SUBMODULE(type_caster_std_function_specializations, m) {
    py::class_<SpecialReturn>(m, "SpecialReturn")
        .def(py::init<>())
        .def_readwrite("value", &SpecialReturn::value);
    m.def("call_callback_with_special_return",
          [](const std::function<SpecialReturn()> &func) { 
                return custom_converter(func); 
    });
}

[wangxf123456]

Thanks very much for the example. I think this works. I am working on some test cases to make sure this supports our needs.

[EthanSteinberg]

@wangxf123456 Another thing to think about. You can write a wrapper function that might be better in some situations.

Not sure of the exact syntax, but something like the following is probably close

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

#include "pybind11_tests.h"

namespace py = pybind11;

namespace {

struct SpecialReturn {
    int value = 99;
};

} // namespace

template <typename... Args>
SpecialReturn custom_converter(std::function<SpecialReturn(Args... args)> func, Args... args) {
        SpecialReturn  result;
        try {
            result = func(std::forward<Args>(args)...);
        } catch (error_already_set &) {
            result.value += 1;
        }
        result.value += 100;
        return result;
}

template <typename... Args>
std::function<SpecialReturn(Args... args)> SpecialReturn convert_functor(std::function<SpecialReturn(Args... args)> func) {
        return std::bind(custom_converter, func);
}

TEST_SUBMODULE(type_caster_std_function_specializations, m) {
    py::class_<SpecialReturn>(m, "SpecialReturn")
        .def(py::init<>())
        .def_readwrite("value", &SpecialReturn::value);
    m.def("call_callback_with_special_return",
          [](const std::function<SpecialReturn()> &func) { 
              
                return convert_functor(func)(); 
    });
}

[EthanSteinberg]

@wangxf123456 gentle ping to ask if this PR is still necessary or whether my suggestion was sufficient.

Might want to close this PR if this seems to be unnecessary.

[wangxf123456]

Yes your suggestion is sufficient. Thanks again for sharing! This PR should be unnecessary.

[wangxf123456]

Note that we already merged this change in google/pywrapcc a while ago: https://github.com/google/pywrapcc/blob/main/include/pybind11/functional.h#L20. The main reason that we prefer the solution that modifies the type caster of std::function is that we are working on automatically generating pybind11 code. It is quite hard for us to handle nested types automatically, for example, a function that takes std::unordered_map<std::string, std::function<SpecialReturn()>> as parameter. So in general we prefer solutions from bottom-up, instead of solutions from top-down.

[rwgk]

@EthanSteinberg (reviewer in April 2013)

@nevedaren (new owner of Python/C++ bindings at Google)

Updating this PR to reflect reality:

Regarding this comment from May 2023: #4597 (comment)

Yes your suggestion is sufficient. Thanks again for sharing! This PR should be unnecessary.

It unfortunate that this was written, because it was never true. Google has been consistently using this code in production:

• absl::Status: https://github.com/pybind/pybind11_abseil/blob/4222c78a4d3920e11c802371c6e3615a72c2508d/pybind11_abseil/status_caster.h#L123-L167
• absl::StatusOr: https://github.com/pybind/pybind11_abseil/blob/4222c78a4d3920e11c802371c6e3615a72c2508d/pybind11_abseil/statusor_caster.h#L127-L172

Backing out those specializations would have two consequences:

1. We'd first have to somehow pin-point all functions (including callback arguments) that involve absl::Status/StatusOr to-Python conversions, and replace all of them with lambda functions, similar to what was suggested under feat: add detail::type_caster_std_function_specializations #4597 (comment). This is very costly and error prone.

2. In the future, people writing new pybind11 bindings will simply not know that they need to use lambda functions, and just wrap their functions directly. Situations with exceptions tend to not be fully unit tested. At Google scale, it is only a matter of time until there are production breakages, recurringly. Again, this will be very costly. (pybind/pybind11_abseil is OSS, therefore external users may be affected similarly).

Fundamentally from an engineering quality perspective: any "centralized solution" (specializations in two central places in this case) is better than a solution that requires "sprawling provisions" (e.g. the fixes under 1. above, which could easily number in the hundreds).

Contrasting that with these concerns (#4597 (comment)):

Adding additional specializations like this adds quite a bit to the public API

I'd argue that is a very minor concern, compared to the many APIs in the pybind11::detail namespace that are de-facto public already.

and can be quite clunky to use.

True, the specializations (links above) aren't exactly pretty, but compare that with the alternative of cluttering client code with O(hundreds) of lambda functions, and the greatly increased risk of oversights and related production breakages.

I think the cost:benefit ratio is very strongly on the benefit side in this case.

I'm leaving Google in less than a month and it's not really my problem anymore. I believe Google doesn't have a viable alternative to carrying the current adaption of this PR (under #5289) indefinitely. It is plausible that only pybind/pybind11_abseil users will need the specializations, but then again,

• most people will not need to know and will simply never know the feature is even there (good!),
• the compile-time overhead is very likely to be in the measurement noise,
• and there is no runtime overhead AFAIK.

Having the option for the specializations merged upstream will help us all collaborating efficiently. (I already spent hours worth of my time just juggling this patch.)

[rwgk]

I'll try to modernize this PR.

[EthanSteinberg]

@rwgk it sounds like I was incorrect, and the benefit of this PR is worth the complexity cost, and it should probably be added to pybind11

Thanks for providing the additional context and explanation

[rwgk]

Thanks @EthanSteinberg! I'll go ahead with merging this PR. In addition to the other things mentioned, it's really helping me making the job-change related hand-over as smooth as possible.