Consider making the default policy for `const T&` arguments more friendly for non-copyable classes in callbacks

[EricCousineau-TRI]

Follow-up from #1240 (#1200).

We disable copying and moving (not that moving is relevant here) for a set of virtual classes, because we would like to have a compile-time guarantee that we are not slicing classes. This generally has no issue in pybind, until callbacks are used, in which case the const T& instance may not be registered, and thus pybind tries to copy it, and then throws a runtime error.

Potential solutions:

• Specify callback arguments' return-value-policies (per @jagerman's suggestion) when casting a function.
• Modify the type_caster_base::cast(const T&) to check for automatic and if the class is non-copyable and non-movable, default to reference rather than copy.
  • This seems like it might be backwards-incompatible, but possibly not (as copying would be unsuccessful anyways).

My workaround for the time being will be to wrap any callbacks (Python callbacks cast into std::function<...>) that have const T& references to use const T* instead.

[jagerman]

Some food for thought.

Here's an implementation of 2, but I'll say in advance that I don't really like this (because it only works around the issue for this one particular case, rather than solving it more generally):

In functional.h add:

template <typename Arg> inline object functional_arg_cast(Arg &&arg) {
    constexpr return_value_policy rvp = is_copy_constructible<remove_reference_t<Arg>>::value
        ? return_value_policy::automatic_reference
        : return_value_policy::reference;
    return cast(std::forward<Arg>(arg), rvp);
}

then amend the Python function call in the value = [func](... lambda from:

            object retval(func(std::forward<Args>(args)...));

to:

            object retval(func(functional_arg_cast(std::forward<Args>(args))...));

But in terms of addressing the core issue, the main problems (mainly just dumping my thoughts on it here) look to me like this:

We need to be able to pass in an rvp to apply to each function argument in the lambda. This is trickier than it seems, partly because passing just one value won't do: a binding might have multiple std::functions, and each argument of each std::function might be different. So, for example, I need to be able to specify arbitrary set of policies for each of the A, B, C, and D arguments in:

m.def("foo", [](
    std::function<void(const A &, const B &)> f1,
    std::function<void(const C &, D *)> f2) { /* ... */ })

The nicest way I can think about to specify that is through the py::arg object:

m.def("foo", [](
    std::function<void(const A &, const B &)> f1,
    std::function<void(const C &, const D &)> f2) { /* ... */ },
    py::arg().arg_rvp({py::return_value_policy::reference, py::return_value_policy::copy}),
    py::arg().arg_rvp({py::return_value_policy::reference, py::return_value_policy::take_ownership})
);

(of course there's no reason you couldn't also name the argument, e.g. py::arg("f1") or "f1"_a, there, but that doesn't matter to this discussion).

The issue is then how to get that into the std::function type caster during argument loading. One of the things I suggested as part of #864 was to change the type caster loading interface to be something like load(handle h, load_options o) (instead of the current load(handle h, bool convert)). That would provide a nice way to get this setting through: it could just be something in the load_options struct (e.g. a std::vector<py::return_value_policy> arg_rvps; member).

[Edit: fixed bad C++ in py::arg idea]

[EricCousineau-TRI]

Sorry for the late follow-up, but thank you for the follow-up! I may try to incorporate this in our fork.

Do you have an issue tracking this feature already?
I ask because we just ran into another related corner case, related to this issue: trampoline overloads / overrides. See code here:
https://github.com/EricCousineau-TRI/drake/blob/0aa2a8d/bindings/pydrake/systems/framework_py.cc#L129

From the looks of it, PYBIND_OVERLOAD_INT seems like it could be made into a non-macro function, such that you could pass a function (with those loading arguments).
Perhaps that could then be factored into PYBIND_OVERLOAD?

That, or ensure that the user has bound the appropriate method in the base class, and then in PYBIND11_OVERLOAD_INT, it could leverage whatever load_policy / cast_policy (including keep_alive behavior?) from the original defd method?

[EricCousineau-TRI]

See our WrapCallback in pydrake that handles this:

https://github.com/RobotLocomotion/drake/blob/6ee5e9325821277a62bd5cd5456ccf02ca25dab7/bindings/pydrake/common/wrap_pybind.h#L119-L131

See unittests on template metaprogramming stuff:
https://github.com/RobotLocomotion/drake/blob/6ee5e9325821277a62bd5cd5456ccf02ca25dab7/bindings/pydrake/common/test/wrap_function_test.cc