function<Type*(void)> will attempt to copy Type on return

[gratzdhg]

Firstly, thank you for this wonderful library. As I was using it I found some unexpected/undesired behaviour. Wrapped function objects from std::functional will copy the result even when the result is of type pointer.
Here is my sample code to reproduce the issue.

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

namespace py = pybind11;

struct Test {
    Test() {};
    Test(const Test &obj) = delete;
    int i;
};
Test* fullFunction()
{
    return new Test();
}
std::function<Test*(void)> func = &fullFunction;

PYBIND11_MODULE(example, m) {
    // optional module docstring
    m.attr("full") = py::cpp_function(func);
    m.attr("func") = func;
    py::class_<Test>(m,"Test")
        .def_readwrite("i",&Test::i);
}

import example
example.full()
example.func()

example.func() will result in an error: return_value_policy = copy, but the object is non-copyable!

It's my understanding that both full & func should return a Test* and that the underlying Test will not be copied however when the pybind11/functional wrapper handles it, it will attempt to copy Test*. I believe this occurs because a function object is an object and not a pointer which causes copy to be the default behaviour that is detected. Sorry if I have misunderstood anything.

[henryiii]

The problem seems to lie in

pybind11/include/pybind11/functional.h

Lines 72 to 75 in 37de2da

	if (result)
	return cpp_function(*result, policy).release();
	else
	return cpp_function(std::forward<Func>(f_), policy).release();

where it inherits the parent call policy, which is copy since the function object is not a pointer. This is the correct parent call policy, but inheriting it is probably incorrect. Removing the explicit call policies here fixes this problem.

[gratzdhg]

thanks, my solution was to replace those lines with

        auto res_policy = policy;
        res_policy = std::is_pointer<Return>::value ? return_value_policy::take_ownership : return_value_policy::copy;

        auto result = f_.template target<function_type>();
        if (result)
            return cpp_function(*result, res_policy).release();
        else
            return cpp_function(std::forward<Func>(f_), res_policy).release();

but I'm not confidant that this would always be correct

[henryiii]

Simply removing the explicit policies seems to also pass all tests, so I'm assuming this is either also correct or not tested. Waiting for someone with a bit more experience with PyBind11 to weigh in. I think something like you've written above probably is a reasonable solution?

[dean0x7d]

I'm not sure about that fix. It may not be covered by the tests, but the forwarded policy is probably useful for higher-order functions, e.g. m.def("foo", &foo, some_policy) where foo() returns a std::function<...> and so some_policy doesn't apply to foo itself but is forwarded to the returned function. @wjakob Was this the intent behind the std::function caster?

In general, this line:

m.attr("func") = func;

can be replaced with any of the following lines to get the desired effect:

// alt 1
m.def("func", func);
// alt 2
m.attr("func") = py::cpp_function(func);
// alt 3
m.attr("func") = py::cast(func, py::return_value_policy::take_ownership);

[henryiii]

m.attr("func") = &func;

Might also work.

It would still be nice to cover the use case for forwarded function in a test if possible.

[henryiii]

PS: Would a better solution be to keep the "auto" return policy, instead of immediately replacing it with copy or ownership,

pybind11/include/pybind11/cast.h

Lines 1607 to 1610 in 37de2da

	if (policy == return_value_policy::automatic)
	policy = std::is_pointer<T>::value ? return_value_policy::take_ownership : return_value_policy::copy;
	else if (policy == return_value_policy::automatic_reference)
	policy = std::is_pointer<T>::value ? return_value_policy::reference : return_value_policy::copy;

And then handling automatic further down in the code?

[wjakob]

This looks like a regression to me, this used to work way back (e.g. in v1.2).

The intent of this feature was that a function returning a function doesn't need a return value policy (not a class_<> type), and that this flag could be forwarded to be used as the return value policy of the returned function so that the user has a way of customizing this behavior, e.g.

m.def("return_a_function", &function_returning_a_std_function, return_value_policy::take_ownership);

I'm not sure if the following ever worked, but theoretically it should also be possible to use py::cast<> to do this in other situations, e.g.

std::function<..> f = ...;
py::function func = py::cast(f, return_value_policy::take_ownership);

The pybind11 documentation is really spotty here, I don't think we even document the return value policy argument of py::cast.

[dean0x7d]

The intent of this feature was that a function returning a function doesn't need a return value policy (not a class_<> type), and that this flag could be forwarded to be used as the return value policy of the returned function so that the user has a way of customizing this behavior

As far as I understand, this forwarding behavior is exactly how it works now. The only issue is that

m.attr("func") = func; // implicit `py::cast()`

ends up behaving like

m.attr("func") = py::cpp_function(func, py::return_value_policy::copy);

instead of the expected

m.attr("func") = py::cpp_function(func); // implicit `py::return_value_policy::automatic`

The cause seems to be, as @henryiii pointed out, that py::cast() intercepts the policy and changes it to copy early on, so the std::function caster gets that instead of the original automatic.

[wjakob]

That's true -- however, it is even more broken than that since forwarding the policy in bound functions as in

m.def("return_a_function", &function_returning_a_std_function, return_value_policy::take_ownership);

also doesn't work anymore.

[dean0x7d]

Ah, right, the move policy override for rvalues. That part could be resolved quickly by specializing return_value_policy_override for std::function. The py::cast() overrides are a bit trickier in this context.