Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
EricCousineau-TRI opened this issue Jan 8, 2018 · 3 comments

Comments

@EricCousineau-TRI
Copy link
Collaborator

EricCousineau-TRI commented Jan 8, 2018

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.

@EricCousineau-TRI EricCousineau-TRI changed the title Consider making return_value_policy::copy more friendly for non-copyable non-movable classes in callbacks Consider making the default policy for const T& arguments more friendly for non-copyable non-movable classes in callbacks Jan 8, 2018
@EricCousineau-TRI EricCousineau-TRI changed the title Consider making the default policy for const T& arguments more friendly for non-copyable non-movable classes in callbacks Consider making the default policy for const T& arguments more friendly for non-copyable classes in callbacks Jan 8, 2018
@jagerman
Copy link
Member

jagerman commented Jan 10, 2018

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
Copy link
Collaborator Author

EricCousineau-TRI commented Jan 23, 2018

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
Copy link
Collaborator Author

EricCousineau-TRI commented Mar 29, 2021

zagor added a commit to zagor/pybind11 that referenced this issue Oct 20, 2023
pybind11::cast_error:
     return_value_policy = copy, but the object is non-copyable!

May be the same/related issue as described here:
     pybind#1241

Adapted from:
kefeimo/pybind11@338d615
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants