pybind / pybind11

Seamless operability between C++11 and Python
https://pybind11.readthedocs.io/
Other
15.72k stars 2.11k forks source link

[BUG] `PYBIND11_OVERRIDE` macros do not allow changing the default `return_value_property` #2516

Open YannickJadoul opened 4 years ago

YannickJadoul commented 4 years ago

Issue description

The PYBIND11_OVERRIDE macros don't allow for the return_value_policy to be specified. It is always object_api<T>::operator ()'s default, automatic_reference. This results in problems when trying to nót copy a C++ object that doesn't have a Python object associated yet.

@davidelbaze bumped into this issue on Gitter, and I debugged and tracked the solution down to this issue.

Reproducible example code

@davidelbaze's original code:

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

namespace py = pybind11;

PYBIND11_MAKE_OPAQUE(std::vector<uint8_t>);

PYBIND11_MODULE(example, m) {
  py::bind_vector<std::vector<uint8_t>>(m, "Buffer", py::buffer_protocol()).def("ptr", [](std::vector<uint8_t>&v) { return (size_t) &v; });

using T = std::vector<uint8_t> &;
    class Muter {
     public:
      virtual void Mutate(T s) {
        s[0] = s[0] * 2;
      }
      size_t ptr() { return (size_t) this; }
      virtual ~Muter() = default;
    };

    class PyMuter : public Muter {
     public:
      using Muter::Muter;
      void Mutate(T s) override {
        // auto tmp = py::cast(s, py::return_value_policy::reference);  // This line/hack makes things work
         PYBIND11_OVERLOAD(void, /* Return type */
                          Muter,   /* Parent class */
                          Mutate,  /* Name of function in C++ (must match Python name) */
                          s        /* Argument(s) */
        );
      }
    };

    py::class_<Muter, PyMuter>(m, "Muter").def(py::init<>()).def("Mutate", &Muter::Mutate).def("ptr", &Muter::ptr);
    m.def(
        "test",
        [](Muter &ref) {
          std::vector<uint8_t> v{1,2,3,4,5};
          ref.Mutate(v);
          return v;
        });
}
import example

class CustomMuter(example.Muter):
    def Mutate(self, v):
        print(type(v))
        v[0]*=20

b=CustomMuter()
example.test(b)

So, it seems like we 1. need to add a warning to the docs about the default return_value_policy, but it would also be nice to avoid a way to specify the return_value_policy. Better ideas than adding 4 more PYBIND11_OVERRIDE macro variants are welcome. I can create the PR, obviously, but I'd first like to discuss how to best solve this.

virtuald commented 4 years ago

A mildly related change that I've ran into -- having virtual functions that have different python signatures from the C++ signature. Take for example:

getDescription(std::ostream &os) override;

In this case, I want the python API to be:

getDescription() -> str

But still be interoperable from C++. I have some modifications of the OVERRIDE macros in robotpy-build if there's interest in tackling this problem upstream.

YannickJadoul commented 4 years ago

@virtuald For that, we do have py::get_override, I guess, to roll your own? But yes, true, it's related. How horrible macros are, is also related :-/