From 2df39b2ec25fa625950f6aad82852c6235f4a77a Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Wed, 20 Dec 2017 18:04:19 -0500 Subject: [PATCH] Support ownership transfer between C++ and Python with `shared_ptr` and `unique_ptr` for pure C++ instances and single-inheritance instances --- docs/advanced/cast/overview.rst | 3 + docs/advanced/classes.rst | 147 +++++++++- docs/advanced/smart_ptrs.rst | 142 ++++++++-- include/pybind11/attr.h | 5 +- include/pybind11/cast.h | 395 +++++++++++++++++++++++---- include/pybind11/detail/common.h | 101 +++++++ include/pybind11/detail/internals.h | 4 +- include/pybind11/pybind11.h | 394 +++++++++++++++++++++++++- include/pybind11/pytypes.h | 109 ++++++++ tests/CMakeLists.txt | 1 + tests/test_class.py | 30 +- tests/test_methods_and_attributes.py | 12 +- tests/test_multiple_inheritance.cpp | 21 ++ tests/test_multiple_inheritance.py | 27 ++ tests/test_ownership_transfer.cpp | 179 ++++++++++++ tests/test_ownership_transfer.py | 145 ++++++++++ tests/test_smart_ptr.cpp | 50 +++- tests/test_smart_ptr.py | 22 ++ 18 files changed, 1676 insertions(+), 111 deletions(-) create mode 100644 tests/test_ownership_transfer.cpp create mode 100644 tests/test_ownership_transfer.py diff --git a/docs/advanced/cast/overview.rst b/docs/advanced/cast/overview.rst index 2ac7d30097a..8e7c12916a4 100644 --- a/docs/advanced/cast/overview.rst +++ b/docs/advanced/cast/overview.rst @@ -119,6 +119,9 @@ as arguments and return values, refer to the section on binding :ref:`classes`. | ``std::string_view``, | STL C++17 string views | :file:`pybind11/pybind11.h` | | ``std::u16string_view``, etc. | | | +------------------------------------+---------------------------+-------------------------------+ +| ``std::unique_ptr``, | STL (or custom) smart | :file:`pybind11/cast.h` | +| ``std::shared_ptr``, etc. | pointers. | | ++------------------------------------+---------------------------+-------------------------------+ | ``std::pair`` | Pair of two custom types | :file:`pybind11/pybind11.h` | +------------------------------------+---------------------------+-------------------------------+ | ``std::tuple<...>`` | Arbitrary tuple of types | :file:`pybind11/pybind11.h` | diff --git a/docs/advanced/classes.rst b/docs/advanced/classes.rst index 93deeec6215..f4c9209cabe 100644 --- a/docs/advanced/classes.rst +++ b/docs/advanced/classes.rst @@ -65,10 +65,10 @@ helper class that is defined as follows: .. code-block:: cpp - class PyAnimal : public Animal { + class PyAnimal : public py::wrapper { public: /* Inherit the constructors */ - using Animal::Animal; + using py::wrapper::wrapper; /* Trampoline (need one for each virtual function) */ std::string go(int n_times) override { @@ -90,6 +90,8 @@ function* slots, which defines the name of function in Python. This is required when the C++ and Python versions of the function have different names, e.g. ``operator()`` vs ``__call__``. +The base class ``py::wrapper<>`` is optional, but is recommended as it allows us to attach the lifetime of Python objects directly to C++ objects, explained in :ref:`virtual_inheritance_lifetime`. + The binding code also needs a few minor adaptations (highlighted): .. code-block:: cpp @@ -157,7 +159,7 @@ Here is an example: class Dachschund(Dog): def __init__(self, name): - Dog.__init__(self) # Without this, undefind behavior may occur if the C++ portions are referenced. + Dog.__init__(self) # Without this, undefined behavior may occur if the C++ portions are referenced. self.name = name def bark(self): return "yap!" @@ -232,15 +234,15 @@ override the ``name()`` method): .. code-block:: cpp - class PyAnimal : public Animal { + class PyAnimal : public py::wrapper { public: - using Animal::Animal; // Inherit constructors + using py::wrapper::wrapper; // Inherit constructors std::string go(int n_times) override { PYBIND11_OVERLOAD_PURE(std::string, Animal, go, n_times); } std::string name() override { PYBIND11_OVERLOAD(std::string, Animal, name, ); } }; - class PyDog : public Dog { + class PyDog : public py::wrapper { public: - using Dog::Dog; // Inherit constructors + using py::wrapper::wrapper; // Inherit constructors std::string go(int n_times) override { PYBIND11_OVERLOAD_PURE(std::string, Dog, go, n_times); } std::string name() override { PYBIND11_OVERLOAD(std::string, Dog, name, ); } std::string bark() override { PYBIND11_OVERLOAD(std::string, Dog, bark, ); } @@ -260,9 +262,9 @@ declare or override any virtual methods itself: .. code-block:: cpp class Husky : public Dog {}; - class PyHusky : public Husky { + class PyHusky : public py::wrapper { public: - using Husky::Husky; // Inherit constructors + using py::wrapper::wrapper; // Inherit constructors std::string go(int n_times) override { PYBIND11_OVERLOAD_PURE(std::string, Husky, go, n_times); } std::string name() override { PYBIND11_OVERLOAD(std::string, Husky, name, ); } std::string bark() override { PYBIND11_OVERLOAD(std::string, Husky, bark, ); } @@ -270,14 +272,14 @@ declare or override any virtual methods itself: There is, however, a technique that can be used to avoid this duplication (which can be especially helpful for a base class with several virtual -methods). The technique involves using template trampoline classes, as +methods). The technique (the Curiously Recurring Template Pattern) involves using template trampoline classes, as follows: .. code-block:: cpp - template class PyAnimal : public AnimalBase { + template class PyAnimal : public py::wrapper { public: - using AnimalBase::AnimalBase; // Inherit constructors + using py::wrapper::wrapper; // Inherit constructors std::string go(int n_times) override { PYBIND11_OVERLOAD_PURE(std::string, AnimalBase, go, n_times); } std::string name() override { PYBIND11_OVERLOAD(std::string, AnimalBase, name, ); } }; @@ -997,5 +999,122 @@ described trampoline: MSVC 2015 has a compiler bug (fixed in version 2017) which requires a more explicit function binding in the form of - ``.def("foo", static_cast(&Publicist::foo));`` - where ``int (A::*)() const`` is the type of ``A::foo``. +.. ``.def("foo", static_cast(&Publicist::foo));`` +.. where ``int (A::*)() const`` is the type of ``A::foo``. + +.. _virtual_inheritance_lifetime: + +Virtual Inheritance and Lifetime +================================ + +When an instance of a Python subclass of a ``pybind11``-bound C++ class is instantiated, there are effectively two "portions": the C++ portion of the base class's alias instance, and the Python portion (``__dict__``) of the derived class instance. +Generally, the lifetime of an instance of a Python subclass of a ``pybind11``-bound C++ class will note pose an issue as long as the instance is owned in Python - that is, you can call virtual methods from C++ or Python and have the correct behavior. + +However, if this Python-constructed instance is passed to C++ such that there are no other Python references, then C++ must keep the Python portion of the instance alive until either (a) the C++ reference is destroyed via ``delete`` or (b) the object is passed back to Python. ``pybind11`` supports both cases, but **only** when (i) the class inherits from :class:`py::wrapper`, (ii) there is only single-inheritance in the bound C++ classes, and (iii) the holder type for the class is either :class:`std::shared_ptr` (suggested) or :class:`std::unique_ptr` (default). + +.. seealso:: + + :ref:`holders` has more information regaring holders and how general ownership transfer should function. + +When ``pybind11`` detects case (a), it will store a reference to the Python object in :class:`py::wrapper` using :class:`py::object`, such that if the instance is deleted by C++, then it will also release the Python object (via :func:`py::wrapper::~wrapper()`). The wrapper will have a unique reference to the Python object (as any other circumstance would trigger case (b)), so the Python object should be destroyed immediately upon the instance's destruction. +This will be a cyclic reference per Python's memory management, but this is not an issue as the memory is now managed via C++. + +For :class:`std::shared_ptr`, this case is detected by placing a shim :func:`__del__` method on the Python subclass when ``pybind11`` detects an instance being created. This shim will check for case (a), and if it holds, will "resurrect" since it created a new reference using :class:`py::object`. + +For :class:`std::unique_ptr`, this case is detected when calling `py::cast>`, which itself implies ownership transfer. + +.. seealso:: + + See :ref:`unique_ptr_ownership` for information about how ownership can be transferred via a cast or argument involving ``unique_ptr``. + +When ``pybind11`` detects case (b) (e.g. ``py::cast()`` is called to convert a C++ instance to `py::object`) and (a) has previously occurred, such that C++ manages the lifetime of the object, then :class:`py::wrapper` will release the Python reference to allow Python to manage the lifetime of the object. + +.. note:: + + This mechanism will be generally robust against reference cycles in Python as this couples the two "portions"; however, it does **not** protect against reference cycles with :class:`std::shared_ptr`. You should take care and use :class:`std::weak_ref` or raw pointers (with care) when needed. + +.. note:: + + There will a slight difference in destructor order if the complete instance is destroyed in C++ or in Python; however, this difference will only be a difference in ordering in when :func:`py::wrapper::~wrapper()` (and your alias destructor) is called in relation to :func:`__del__` for the subclass. For more information, see the documentation comments for :class:`py::wrapper`. + +For this example, we will build upon the above code for ``Animal`` with alias ``PyAnimal``, and the Python subclass ``Cat``, but will introduce a situation where C++ may have sole ownership: a container. In this case, it will be ``Cage``, which can contain or release an animal. + +.. note:: + + For lifetime, it is important to use a more Python-friendly holder, which in this case would be :class:`std::shared_ptr`, permitting an ease to share ownership. + +.. code-block:: cpp + + class Animal { + public: + virtual ~Animal() { } + virtual std::string go(int n_times) = 0; + }; + + class PyAnimal : public py::wrapper { + public: + /* Inherit the constructors */ + using py::wrapper::wrapper; + std::string go(int n_times) override { + PYBIND11_OVERLOAD_PURE(std::string, Animal, go, n_times); + } + }; + + class Cage { + public: + void add(std::shared_ptr animal) { + animal_ = animal; + } + std::shared_ptr release() { + return std::move(animal_); + } + private: + std::shared_ptr animal_; + }; + +And the following bindings: + +.. code-block:: cpp + + PYBIND11_MODULE(example, m) { + py::class_> animal(m, "Animal"); + animal + .def(py::init<>()) + .def("go", &Animal::go); + + py::class_> cage(m, "Cage") + .def(py::init<>()) + .def("add", &Cage::add) + .def("release", &Cage::release); + } + +With the following Python preface: + +.. code-block:: pycon + + >>> from examples import * + >>> class Cat(Animal): + ... def go(self, n_times): + ... return "meow! " * n_times + ... + >>> cage = Cage() + +Normally, if you keep the object alive in Python, then no additional instrumentation is necessary: + +.. code-block:: pycon + + >>> cat = Cat() + >>> c.add(cat) # This object lives in both Python and C++. + >>> c.release().go(2) + meow! meow! + +However, if you pass an instance that Python later wishes to destroy, without :class:`py::wrapper`, we would get an error that ``go`` is not implented, +as the `Cat` portion would have been destroyed and no longer visible for the trampoline. With the wrapper, ``pybind11`` will intercept this event and keep the Python portion alive: + +.. code-block:: pycon + + >>> c.add(Cat()) + >>> c.release().go(2) + meow! meow! + +Note that both the C++ and Python portion of ``cat`` will be destroyed once ``cage`` is destroyed. diff --git a/docs/advanced/smart_ptrs.rst b/docs/advanced/smart_ptrs.rst index da57748ca58..4ef707e55a8 100644 --- a/docs/advanced/smart_ptrs.rst +++ b/docs/advanced/smart_ptrs.rst @@ -1,5 +1,17 @@ -Smart pointers -############## +.. _holders: + +Smart pointers and holders +########################## + +Holders +======= + +The binding generator for classes, :class:`class_`, can be passed a template +type that denotes a special *holder* type that is used to manage references to +the object. If no such holder type template argument is given, the default for +a type named ``Type`` is ``std::unique_ptr``, which means that the object +is deallocated when Python's reference count goes to zero. It is possible to switch to other types of reference counting wrappers or smart +pointers, which is useful in codebases that rely on them, such as ``std::shared_ptr``, or even a custom type. std::unique_ptr =============== @@ -15,31 +27,100 @@ instances wrapped in C++11 unique pointers, like so m.def("create_example", &create_example); -In other words, there is nothing special that needs to be done. While returning -unique pointers in this way is allowed, it is *illegal* to use them as function -arguments. For instance, the following function signature cannot be processed -by pybind11. +In other words, there is nothing special that needs to be done. + +.. _unique_ptr_ownership: + +Transferring ownership +---------------------- + +It is also possible to pass ``std::unique_ptr`` as a function +argument, or use ``py::cast>(std::move(obj))``. Note that this tells pybind11 to not manage the memory for this object, and delegate that to ``std::unique_ptr``. +For instance, the following function signature can be processed by pybind11: .. code-block:: cpp void do_something_with_example(std::unique_ptr ex) { ... } -The above signature would imply that Python needs to give up ownership of an -object that is passed to this function, which is generally not possible (for -instance, the object might be referenced elsewhere). +The above signature does imply that Python needs to give up ownership of an +object that is passed to this function. There are two ways to do this: + +1. Simply pass the object in. The reference count of the object can be greater than one (non-unique) when passing the object in. **However**, you *must* ensure that the object has only **one** reference when C++ (which owns the C++ object). + + To expand on this, when transferring ownership for ``std::unique_ptr``, this means that Pybind11 no longer owns the reference, which means that if C++ lets the ``std::unique_ptr`` destruct but if there is a dangling reference in Python, then you will encounter undefined behavior. + + Examples situations: + + * The C++ function is terminal (i.e. will destroy the object once it completes). This is generally not an issue unless a Python portion of the object has a non-trivial ``__del__`` method. + * The Python object is passed to a C++ container which only tracks ``std::unique_ptr``. If the container goes out of scope, the Python object reference will be invalid. + + .. note:: + + For polymorphic types that inherit from :class:`py::wrapper`, ``pybind11`` *can* warn about these situations. + You may enable this behavior with ``#define PYBIND11_WARN_DANGLING_UNIQUE_PYREF``. This will print a warning to ``std::err`` if this case is detected. + +2. Pass a Python "move container" (a mutable object that can "release" the reference to the object). This can be a single-item list, or any Python class / instance that has the field ``_is_move_container = True`` and has a ``release()`` function. + + .. note:: + + When using a move container, this expects that the provided object is a **unique** reference, or will throw an error otherwise. This is a little more verbose, but will make debugging *much* easier. + + As an example in C++: + + .. code-block:: cpp + + void terminal_func(std::unique_ptr obj) { + obj.do_something(); // `obj` will be destroyed when the function exits. + } + + // Binding + py::class_ example(m, "Example"); + m.def("terminal_func", &terminal_func); + + In Python, say you would normally do this: + + .. code-block:: pycon + + >>> obj = Example() + >>> terminal_func(obj) + + As mentioned in the comment, you *must* ensure that `obj` is not used past this invocation, as the underlying data has been destroyed. To be more careful, you may "move" the object. The following will throw an error: + + .. code-block:: pycon + + >>> obj = Example() + >>> terminal_func([obj]) + + However, this will work, using a "move" container: + + .. code-block:: pycon + + >>> obj = Example() + >>> obj_move = [obj] + >>> del obj + >>> terminal_func(obj_move) + >>> print(obj_move) # Reference will have been removed. + [None] + + or even: + + .. code-block:: pycon + + >>> terminal_func([Example()]) + + .. note:: + + ``terminal_func(Example())`` also works, but still leaves a dangling reference, which is only a problem if it is polymorphic and has a non-trivial ``__del__`` method. + + .. warning:: + + This reference counting mechanism is **not** robust aganist cyclic references. If you need some sort of cyclic reference, *please* consider using ``weakref.ref`` in Python. std::shared_ptr =============== -The binding generator for classes, :class:`class_`, can be passed a template -type that denotes a special *holder* type that is used to manage references to -the object. If no such holder type template argument is given, the default for -a type named ``Type`` is ``std::unique_ptr``, which means that the object -is deallocated when Python's reference count goes to zero. - -It is possible to switch to other types of reference counting wrappers or smart -pointers, which is useful in codebases that rely on them. For instance, the -following snippet causes ``std::shared_ptr`` to be used instead. +If you have an existing code base with ``std::shared_ptr``, or you wish to enable reference counting in C++ as well, then you may use this type as a holder. +As an example, the following snippet causes ``std::shared_ptr`` to be used instead. .. code-block:: cpp @@ -111,6 +192,12 @@ There are two ways to resolve this issue: class Child : public std::enable_shared_from_this { }; +.. seealso:: + + While ownership transfer is generally not an issue with ``std::shared_ptr``, it becomes an issue when an instance of a Python subclass of a pybind11 class is effectively managed by C++ (e.g. all live references to the object are from C++, and all reference in Python have "died"). + + See :ref:`virtual_inheritance_lifetime` for more information. + .. _smart_pointers: Custom smart pointers @@ -147,7 +234,7 @@ Please take a look at the :ref:`macro_notes` before using this feature. By default, pybind11 assumes that your custom smart pointer has a standard interface, i.e. provides a ``.get()`` member function to access the underlying -raw pointer. If this is not the case, pybind11's ``holder_helper`` must be +raw pointer, and a ``.release()`` member function for move-only holders. If this is not the case, pybind11's ``holder_helper`` must be specialized: .. code-block:: cpp @@ -171,3 +258,20 @@ provides ``.get()`` functionality via ``.getPointer()``. The file :file:`tests/test_smart_ptr.cpp` contains a complete example that demonstrates how to work with custom reference-counting holder types in more detail. + +.. warning:: + + Holder type conversion (see :ref:`smart_ptrs_casting`) and advanced ownership transfer (see :ref:`virtual_inheritance_lifetime`) is **not** supported for custom shared pointer types, due to constraints on dynamic type erasure. + +.. _smart_ptrs_casting: + +Casting smart pointers +====================== + +As shown in the :ref:`conversion_table`, you may cast to any of the available holders (e.g. ``py::cast>(obj)``) that can properly provide access to the underlying holder. + +``pybind11`` will raise an error if there is an incompatible cast. You may of course cast to the exact same holder type. You may also move a ``std::unique_ptr`` into a ``std::shared_ptr``, as this is allowed. **However**, you may not convert a ``std::shared_ptr`` to a ``std::unique_ptr`` as you cannot release an object that is managed by ``std::shared_ptr``. + +Additionally, conversion to ``std::unique_ptr`` is not supported if ``Deleter`` is not ``std::default_deleter``. + +Conversion to a different custom smart pointer is not supported. diff --git a/include/pybind11/attr.h b/include/pybind11/attr.h index dce875a6b99..9c5ab8a9c48 100644 --- a/include/pybind11/attr.h +++ b/include/pybind11/attr.h @@ -221,11 +221,14 @@ struct type_record { void *(*operator_new)(size_t) = ::operator new; /// Function pointer to class_<..>::init_instance - void (*init_instance)(instance *, const void *) = nullptr; + void (*init_instance)(instance *, holder_erased) = nullptr; /// Function pointer to class_<..>::dealloc void (*dealloc)(detail::value_and_holder &) = nullptr; + /// See `type_info::has_cpp_release`. + instance::type_release_info_t release_info; + /// List of base classes of the newly created type list bases; diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 715ec932d95..bbe771bf45a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -156,12 +156,16 @@ inline const std::vector &all_type_info(PyTypeObject *type) * ancestors are pybind11-registered. Throws an exception if there are multiple bases--use * `all_type_info` instead if you want to support multiple bases. */ -PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type) { +PYBIND11_NOINLINE inline detail::type_info* get_type_info(PyTypeObject *type, bool do_throw = true) { auto &bases = all_type_info(type); if (bases.size() == 0) return nullptr; - if (bases.size() > 1) - pybind11_fail("pybind11::detail::get_type_info: type has multiple pybind11-registered bases"); + if (bases.size() > 1) { + if (do_throw) + pybind11_fail("pybind11::detail::get_type_info: type has multiple pybind11-registered bases"); + else + return nullptr; + } return bases.front(); } @@ -229,6 +233,7 @@ struct value_and_holder { template H &holder() const { return reinterpret_cast(vh[1]); } + void* holder_ptr() const { return &vh[1]; } bool holder_constructed() const { return inst->simple_layout ? inst->simple_holder_constructed @@ -479,6 +484,75 @@ inline PyThreadState *get_thread_state_unchecked() { inline void keep_alive_impl(handle nurse, handle patient); inline PyObject *make_new_instance(PyTypeObject *type); +enum class LoadType { + PureCpp, + DerivedCppSinglePySingle, + DerivedCppSinglePyMulti, + DerivedCppMulti, + /// Polymorphic casting or copy-based casting may be necessary. + ConversionNeeded, +}; + +typedef type_info* base_ptr_t; +typedef const std::vector bases_t; + +inline LoadType determine_load_type(handle src, const type_info* typeinfo, + const bases_t** out_bases = nullptr, + base_ptr_t* out_base = nullptr) { + // Null out inputs. + if (out_bases) + *out_bases = nullptr; + if (out_base) + *out_base = nullptr; + PyTypeObject *srctype = Py_TYPE(src.ptr()); + // See `type_caster_generic::load_impl` below for more detail on comments. + + // Case 1: If src is an exact type match for the target type then we can reinterpret_cast + // the instance's value pointer to the target type: + if (srctype == typeinfo->type) { + // TODO(eric.cousineau): Determine if the type is upcast from a type, which is + // still a pure C++ object? + return LoadType::PureCpp; + } + // Case 2: We have a derived class + else if (PyType_IsSubtype(srctype, typeinfo->type)) { + const bases_t& bases = all_type_info(srctype); + if (out_bases) + *out_bases = &bases; // Copy to output for caching. + const bool no_cpp_mi = typeinfo->simple_type; + // Case 2a: the python type is a Python-inherited derived class that inherits from just + // one simple (no MI) pybind11 class, or is an exact match, so the C++ instance is of + // the right type and we can use reinterpret_cast. + // (This is essentially the same as case 2b, but because not using multiple inheritance + // is extremely common, we handle it specially to avoid the loop iterator and type + // pointer lookup overhead) + // TODO(eric.cousineau): This seems to also capture C++-registered classes as well, not just Python-derived + // classes. + if (bases.size() == 1 && (no_cpp_mi || bases.front()->type == typeinfo->type)) { + return LoadType::DerivedCppSinglePySingle; + } + // Case 2b: the python type inherits from multiple C++ bases. Check the bases to see if + // we can find an exact match (or, for a simple C++ type, an inherited match); if so, we + // can safely reinterpret_cast to the relevant pointer. + else if (bases.size() > 1) { + for (auto base : bases) { + if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { + if (out_base) { + *out_base = base; + } + return LoadType::DerivedCppSinglePyMulti; + } + } + } + // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type match + // in the registered bases, above, so try implicit casting (needed for proper C++ casting + // when MI is involved). + return LoadType::DerivedCppMulti; + } else { + return LoadType::ConversionNeeded; + } +} + class type_caster_generic { public: PYBIND11_NOINLINE type_caster_generic(const std::type_info &type_info) @@ -495,7 +569,7 @@ class type_caster_generic { const detail::type_info *tinfo, void *(*copy_constructor)(const void *), void *(*move_constructor)(const void *), - const void *existing_holder = nullptr) { + holder_erased existing_holder = {}) { if (!tinfo) // no type info: error will be set already return handle(); @@ -503,11 +577,64 @@ class type_caster_generic { if (src == nullptr) return none().release(); + const bool take_ownership = policy == return_value_policy::automatic || policy == return_value_policy::take_ownership; + // We only come across `!existing_holder` if we are coming from `cast` and not `cast_holder`. + const bool is_bare_ptr = !existing_holder.ptr() && existing_holder.type_id() == HolderTypeId::Unknown; + auto it_instances = get_internals().registered_instances.equal_range(src); for (auto it_i = it_instances.first; it_i != it_instances.second; ++it_i) { for (auto instance_type : detail::all_type_info(Py_TYPE(it_i->second))) { - if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) - return handle((PyObject *) it_i->second).inc_ref(); + if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) { + instance* const inst = it_i->second; + + bool try_to_reclaim = false; + if (!is_bare_ptr) { + switch (instance_type->release_info.holder_type_id) { + case detail::HolderTypeId::UniquePtr: { + try_to_reclaim = take_ownership; + break; + } + case detail::HolderTypeId::SharedPtr: { + if (take_ownership) { + // Only try to reclaim the object if (a) it is not owned and (b) has no holder. + if (!inst->simple_holder_constructed) { + if (inst->owned) + throw std::runtime_error("Internal error?"); + try_to_reclaim = true; + } + } + break; + } + default: { + // Otherwise, do not try any reclaiming. + break; + } + } + } + if (try_to_reclaim) { + // If this object has already been registered, but we wish to take ownership of it, + // then use the `has_cpp_release` mechanisms to reclaim ownership. + // @note This should be the sole occurrence of this registered object when releasing back. + // @note This code path should not be invoked for pure C++ + + // TODO(eric.cousineau): This may be still be desirable if this is a raw pointer... + // Need to think of a desirable workflow - and if there is possible interop. + if (!existing_holder) { + throw std::runtime_error("Internal error: Should have non-null holder."); + } + // TODO(eric.cousineau): This field may not be necessary if the lowest-level type is valid. + // See `move_only_holder_caster::load_value`. + if (!inst->reclaim_from_cpp) { + throw std::runtime_error("Instance is registered but does not have a registered reclaim method. Internal error?"); + } + return inst->reclaim_from_cpp(inst, existing_holder).release(); + } else { + // TODO(eric.cousineau): Should really check that ownership is consistent. + // e.g. if we say to take ownership of a pointer that is passed, does not have a holder... + // In the end, pybind11 would let ownership slip, and leak memory, possibly violating RAII (if someone is using that...) + return handle((PyObject *) it_i->second).inc_ref(); + } + } } } @@ -559,13 +686,13 @@ class type_caster_generic { throw cast_error("unhandled return_value_policy: should not happen!"); } + // TODO(eric.cousineau): Propagate `holder_erased` through this chain. tinfo->init_instance(wrapper, existing_holder); - return inst.release(); } // Base methods for generic caster; there are overridden in copyable_holder_caster - void load_value(value_and_holder &&v_h) { + void load_value(value_and_holder &&v_h, LoadType) { auto *&vptr = v_h.value_ptr(); // Lazy allocation for unallocated values: if (vptr == nullptr) { @@ -638,49 +765,35 @@ class type_caster_generic { auto &this_ = static_cast(*this); this_.check_holder_compat(); - PyTypeObject *srctype = Py_TYPE(src.ptr()); - - // Case 1: If src is an exact type match for the target type then we can reinterpret_cast - // the instance's value pointer to the target type: - if (srctype == typeinfo->type) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder()); - return true; - } - // Case 2: We have a derived class - else if (PyType_IsSubtype(srctype, typeinfo->type)) { - auto &bases = all_type_info(srctype); - bool no_cpp_mi = typeinfo->simple_type; - - // Case 2a: the python type is a Python-inherited derived class that inherits from just - // one simple (no MI) pybind11 class, or is an exact match, so the C++ instance is of - // the right type and we can use reinterpret_cast. - // (This is essentially the same as case 2b, but because not using multiple inheritance - // is extremely common, we handle it specially to avoid the loop iterator and type - // pointer lookup overhead) - if (bases.size() == 1 && (no_cpp_mi || bases.front()->type == typeinfo->type)) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder()); + const bases_t* bases = nullptr; + base_ptr_t base_py_multi = nullptr; + LoadType load_type = determine_load_type(src, typeinfo, &bases, &base_py_multi); + switch (load_type) { + case LoadType::PureCpp: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(), + load_type); return true; } - // Case 2b: the python type inherits from multiple C++ bases. Check the bases to see if - // we can find an exact match (or, for a simple C++ type, an inherited match); if so, we - // can safely reinterpret_cast to the relevant pointer. - else if (bases.size() > 1) { - for (auto base : bases) { - if (no_cpp_mi ? PyType_IsSubtype(base->type, typeinfo->type) : base->type == typeinfo->type) { - this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(base)); - return true; - } - } + case LoadType::DerivedCppSinglePySingle: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(), + load_type); + return true; } - - // Case 2c: C++ multiple inheritance is involved and we couldn't find an exact type match - // in the registered bases, above, so try implicit casting (needed for proper C++ casting - // when MI is involved). - if (this_.try_implicit_casts(src, convert)) + case LoadType::DerivedCppSinglePyMulti: { + this_.load_value(reinterpret_cast(src.ptr())->get_value_and_holder(base_py_multi), + load_type); return true; + } + case LoadType::DerivedCppMulti: { + if (this_.try_implicit_casts(src, convert)) + return true; + } + case LoadType::ConversionNeeded: { + break; + } } - // Perform an implicit conversion + // If nothing else succeeds, perform an implicit conversion if (convert) { for (auto &converter : typeinfo->implicit_conversions) { auto temp = reinterpret_steal(converter(src.ptr(), typeinfo->type)); @@ -828,8 +941,11 @@ template class type_caster_base : public type_caster_generic { make_copy_constructor(src), make_move_constructor(src)); } - static handle cast_holder(const itype *src, const void *holder) { + static handle cast_holder(const itype *src, holder_erased holder) { auto st = src_and_type(src); + if (!holder) { + throw std::runtime_error("Internal error: Should not have null holder"); + } return type_caster_generic::cast( st.first, return_value_policy::take_ownership, {}, st.second, nullptr, nullptr, holder); @@ -1372,6 +1488,12 @@ struct holder_helper { static auto get(const T &p) -> decltype(p.get()) { return p.get(); } }; +inline const detail::type_info* get_lowest_type(handle src, bool do_throw = true) { + auto* py_type = (PyTypeObject*)src.get_type().ptr(); + return detail::get_type_info(py_type, do_throw); +} + + /// Type caster for holder types like std::shared_ptr, etc. template struct copyable_holder_caster : public type_caster_base { @@ -1384,7 +1506,10 @@ struct copyable_holder_caster : public type_caster_base { using base::typeinfo; using base::value; - bool load(handle src, bool convert) { + handle src; + + bool load(handle src_in, bool convert) { + src = src_in; return base::template load_impl>(src, convert); } @@ -1400,11 +1525,20 @@ struct copyable_holder_caster : public type_caster_base { explicit operator holder_type&() { return holder; } #endif + // Risk increasing the `shared_ptr` ref count temporarily to maintain writeable + // semantics without too much `const_cast<>` ooginess. + static handle cast(holder_type &&src, return_value_policy, handle) { + const auto *ptr = holder_helper::get(src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); + } + static handle cast(const holder_type &src, return_value_policy, handle) { const auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); } + // TODO(eric.cousineau): Define cast_op_type??? + protected: friend class type_caster_generic; void check_holder_compat() { @@ -1412,7 +1546,27 @@ struct copyable_holder_caster : public type_caster_base { throw cast_error("Unable to load a custom holder type from a default-holder instance"); } - bool load_value(value_and_holder &&v_h) { + bool load_value(value_and_holder &&v_h, LoadType load_type) { + bool do_release_to_cpp = false; + const type_info* lowest_type = nullptr; + if (src.ref_count() == 1 && load_type == LoadType::DerivedCppSinglePySingle) { + // Go ahead and release ownership to C++, if able. + auto* py_type = (PyTypeObject*)src.get_type().ptr(); + lowest_type = detail::get_type_info(py_type); + // Double-check that we did not get along C++ inheritance. + // TODO(eric.cousineau): Generalize this to unique_ptr<> case as well. + const bool is_actually_pure_cpp = lowest_type->type == py_type; + if (!is_actually_pure_cpp) { + if (lowest_type->release_info.can_derive_from_wrapper) { + do_release_to_cpp = true; + } else { + std::cerr << "WARNING! Casting to std::shared_ptr<> will cause Python subclass of pybind11 C++ instance to lose its Python portion. " + "Make your base class extend from pybind11::wrapper<> to prevent aliasing." + << std::endl; + } + } + } + if (v_h.holder_constructed()) { value = v_h.value_ptr(); holder = v_h.template holder(); @@ -1425,6 +1579,17 @@ struct copyable_holder_caster : public type_caster_base { "of type '" + type_id() + "''"); #endif } + + // Release *after* we already have + if (do_release_to_cpp) { + assert(v_h.inst->owned); + assert(lowest_type->release_info.release_to_cpp); + // Increase reference count to pass to release mechanism. + object obj = reinterpret_borrow(src); + lowest_type->release_info.release_to_cpp(v_h.inst, &holder, std::move(obj)); + } + + return true; } template ::value, int> = 0> @@ -1447,6 +1612,7 @@ struct copyable_holder_caster : public type_caster_base { holder_type holder; + constexpr static detail::HolderTypeId holder_type_id = detail::get_holder_type_id::value; }; /// Specialize for the common std::shared_ptr, so users don't need to @@ -1454,15 +1620,142 @@ template class type_caster> : public copyable_holder_caster> { }; template -struct move_only_holder_caster { +struct move_only_holder_caster : type_caster_base { + using base = type_caster_base; + static_assert(std::is_base_of>::value, + "Holder classes are only supported for custom types"); + using base::base; + using base::cast; + using base::typeinfo; + using base::value; + static_assert(std::is_base_of, type_caster>::value, "Holder classes are only supported for custom types"); static handle cast(holder_type &&src, return_value_policy, handle) { + // Move `src` so that `holder_helper<>::get()` can call `release` if need be. + // That way, if we mix `holder_type`s, we don't have to worry about `existing_holder` + // from being mistakenly reinterpret_cast'd to `shared_ptr` (#1138). auto *ptr = holder_helper::get(src); - return type_caster_base::cast_holder(ptr, &src); + return type_caster_base::cast_holder(ptr, holder_erased(&src)); + } + + // Disable these? +// explicit operator type*() { return this->value; } +// explicit operator type&() { return *(this->value); } + +// explicit operator holder_type*() { return &holder; } + + // Force rvalue. + template + using cast_op_type = holder_type&&; + + explicit operator holder_type&&() { return std::move(holder); } + + object extract_obj(handle src) { + // See if this is a supported `move` container. + bool is_move_container = false; + object obj = none(); + // TODO(eric.cousineau): See if we might need to safeguard against objects that are + // implicitly convertible from `list`. + // Can try to cast to T* first, and if that fails, assume it's a move container. + if (isinstance(src, (PyObject*)&PyList_Type) && PyList_Size(src.ptr()) == 1) { + // Extract the object from a single-item list, and remove the existing reference so we have exclusive control. + // @note This will break implicit casting when constructing from vectors, but eh, who cares. + // Swap. + list li = src.cast(); + obj = li[0]; + li[0] = none(); + is_move_container = true; + } else if (hasattr(src, "_is_move_container")) { + // Try to extract the value with `release()`. + obj = src.attr("release")(); + is_move_container = true; + } else { + obj = reinterpret_borrow(src); + } + if (is_move_container && obj.ref_count() != 1) { + throw std::runtime_error("Non-unique reference from a move-container, cannot cast to unique_ptr."); + } + return obj; } + + bool load(handle src, bool /*convert*/) { + // Allow loose reference management (if it's just a plain object) or require tighter reference + // management if it's a move container. + object obj = extract_obj(src); + // Do not use `load_impl`, as it's not structured conveniently for `unique_ptr`. + // Specifically, trying to delegate to resolving to conversion. + // return base::template load_impl>(src, convert); + check_holder_compat(); + auto v_h = reinterpret_cast(obj.ptr())->get_value_and_holder(); + LoadType load_type = determine_load_type(obj, typeinfo); + return load_value(std::move(obj), std::move(v_h), load_type); + } + static constexpr auto name = type_caster_base::name; + +protected: + friend class type_caster_generic; + void check_holder_compat() { + if (!typeinfo->default_holder) + throw cast_error("Unable to load a non-default holder type (unique_ptr)"); + } + + bool load_value(object obj_exclusive, value_and_holder &&v_h, LoadType load_type) { + // TODO(eric.cousineau): This should try and find the downcast-lowest + // level (closest to child) `release_to_cpp` method that is derived-releasable + // (which derives from `wrapper`). + // This should resolve general casting, and should also avoid alias + // branch issues: + // Example: `Base` has wrapper `PyBase` which extends `wrapper`. + // `Child` extends `Base`, has its own wrapper `PyChild`, which extends + // `wrapper`. + // Anything deriving from `Child` does not derive from `PyBase`, so we should + // NOT try to release using `PyBase`s mechanism. + // Additionally, if `Child` does not have a wrapper (for whatever reason) and is extended, + // then we still can NOT use `PyBase` since it's not part of the hierachy. + + // Try to get the lowest-hierarchy level of the type. + // This requires that we are single-inheritance at most. + const detail::type_info* lowest_type = nullptr; + switch (load_type) { + case LoadType::PureCpp: { + // We already have the lowest type. + lowest_type = typeinfo; + break; + } + // If the base type is explicitly mentioned, then we can rely on `DerivedCppSinglePySingle` being used. + case LoadType::DerivedCppSinglePySingle: + // However, if it is not, it may be that we have a C++ type inheriting from another C++ type without the inheritance being registered. + // In this case, we delegate by effectively downcasting in Python by finding the lowest-level type. + // @note No `break` here on purpose! + case LoadType::ConversionNeeded: { + // Try to get the lowest-hierarchy (closets to child class) of the type. + // The usage of `get_type_info` implicitly requires single inheritance. + auto* py_type = (PyTypeObject*)obj_exclusive.get_type().ptr(); + lowest_type = detail::get_type_info(py_type); + break; + } + case LoadType::DerivedCppMulti: { + throw std::runtime_error( + "pybind11 does not support avoiding slicing with " + "multiple inheritance"); + } + default: { + throw std::runtime_error("Unsupported load type"); + } + } + if (!lowest_type) + throw std::runtime_error("No valid lowest type. Internal error?"); + auto& release_info = lowest_type->release_info; + if (!release_info.release_to_cpp) + throw std::runtime_error("No release mechanism in lowest type?"); + release_info.release_to_cpp(v_h.inst, &holder, std::move(obj_exclusive)); + return true; + } + + holder_type holder; }; template diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 1da36bf321c..089eac10da3 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -354,6 +354,8 @@ enum class return_value_policy : uint8_t { reference_internal }; +class object; + NAMESPACE_BEGIN(detail) inline static constexpr int log2(size_t n, int k = 0) { return (n <= 1) ? k : log2(n >> 1, k + 1); } @@ -373,6 +375,85 @@ constexpr size_t instance_simple_holder_in_ptrs() { return size_in_ptrs(sizeof(std::shared_ptr)); } +enum class HolderTypeId { + Unknown, + UniquePtr, + SharedPtr, +}; +template +struct get_holder_type_id { + static constexpr HolderTypeId value = HolderTypeId::Unknown; +}; +template +struct get_holder_type_id, void> { + static constexpr HolderTypeId value = HolderTypeId::SharedPtr; +}; +template +struct get_holder_type_id, void> { + // TODO(eric.cousineau): Should this only specialize for `std::default_deleter`? + static constexpr HolderTypeId value = HolderTypeId::UniquePtr; +}; + +class holder_erased { + public: + holder_erased() = default; + holder_erased(const holder_erased&) = default; + holder_erased& operator=(const holder_erased&) = default; + + template + holder_erased(const holder_type* holder) + : ptr_(const_cast(holder)), + type_id_(get_holder_type_id::value), + is_const_(true) {} + + template + holder_erased(holder_type* holder) + : holder_erased(static_cast(holder)) { + is_const_ = false; + } + + holder_erased(const void* ptr, HolderTypeId type_id) + : ptr_(const_cast(ptr)), + type_id_(type_id), + is_const_(true) {} + + holder_erased(void* ptr, HolderTypeId type_id) + : ptr_(ptr), + type_id_(type_id), + is_const_(false) {} + + holder_erased(std::nullptr_t) {} + + void* ptr() const { return ptr_; } + HolderTypeId type_id() const { return type_id_; } + + template + holder_type& mutable_cast() const { + if (is_const_) + throw std::runtime_error("Trying to mutate const reference?"); + return do_cast(); + } + + template + const holder_type& cast() const { + return do_cast(); + } + + operator bool() const { return ptr_; } + private: + template + holder_type& do_cast() const { + if (type_id_ != get_holder_type_id::value) { + throw std::runtime_error("Mismatch on holder type."); + } + return *reinterpret_cast(ptr_); + } + + void* ptr_{}; + HolderTypeId type_id_{HolderTypeId::Unknown}; + bool is_const_{true}; +}; + // Forward declarations struct type_info; struct value_and_holder; @@ -423,6 +504,26 @@ struct instance { /// If true, get_internals().patients has an entry for this object bool has_patients : 1; + typedef void (*release_to_cpp_t)(instance* inst, holder_erased external_holder, object&& obj); + typedef object (*reclaim_from_cpp_t)(instance* inst, holder_erased external_holder); + + struct type_release_info_t { + // Release an instance to C++ for pure C++ instances or Python-derived classes. + release_to_cpp_t release_to_cpp = nullptr; + + // For classes wrapped in `wrapper<>`. See `move_only_holder_caster` for more info. + // Pure / direct C++ objects do not need any fancy releasing mechanisms. They are simply + // unwrapped and passed back. + bool can_derive_from_wrapper = false; + + // The holder that is contained by this class. + HolderTypeId holder_type_id = HolderTypeId::Unknown; + }; + /// If the instance is a Python-derived type that is owned in C++, then this method + /// will permit the instance to be reclaimed back by Python. + // TODO(eric.cousineau): This may not be necessary. See note in `type_caster_generic::cast`. + reclaim_from_cpp_t reclaim_from_cpp = nullptr; + /// Initializes all of the above type/values/holders data (but not the instance values themselves) void allocate_layout(); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 213cbaeb211..f769faffc44 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -91,7 +91,7 @@ struct type_info { const std::type_info *cpptype; size_t type_size, holder_size_in_ptrs; void *(*operator_new)(size_t); - void (*init_instance)(instance *, const void *); + void (*init_instance)(instance *, holder_erased); void (*dealloc)(value_and_holder &v_h); std::vector implicit_conversions; std::vector> implicit_casts; @@ -108,6 +108,8 @@ struct type_info { bool default_holder : 1; /* true if this is a type registered with py::module_local */ bool module_local : 1; + + instance::type_release_info_t release_info; }; /// Tracks the `internals` and `type_info` ABI version independent of the main library version diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index b489bb2493d..ede8959b0f3 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -47,6 +47,9 @@ NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +template +void unused(Args&&...) {} + /// Wraps an arbitrary C++ function/method/lambda function/.. into a callable Python object class cpp_function : public function { public: @@ -70,14 +73,14 @@ class cpp_function : public function { /// Construct a cpp_function from a class method (non-const) template cpp_function(Return (Class::*f)(Arg...), const Extra&... extra) { - initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*) (Class *, Arg...)) nullptr, extra...); } /// Construct a cpp_function from a class method (const) template cpp_function(Return (Class::*f)(Arg...) const, const Extra&... extra) { - initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(args...); }, + initialize([f](const Class *c, Arg... args) -> Return { return (c->*f)(std::forward(args)...); }, (Return (*)(const Class *, Arg ...)) nullptr, extra...); } @@ -899,6 +902,8 @@ class generic_type : public object { tinfo->default_holder = rec.default_holder; tinfo->module_local = rec.module_local; + tinfo->release_info = rec.release_info; + auto &internals = get_internals(); auto tindex = std::type_index(*rec.type); tinfo->direct_conversions = &internals.direct_conversions[tindex]; @@ -1008,6 +1013,149 @@ auto method_adaptor(Return (Class::*pmf)(Args...) const) -> Return (Derived::*)( return pmf; } + + +template +struct wrapper_interface_impl { + static void use_cpp_lifetime(type* cppobj, object&& obj, detail::HolderTypeId holder_type_id) { + auto* tr = dynamic_cast*>(cppobj); + if (tr == nullptr) { + // This has been invoked at too high of a level; should use a + // downcast class's `release_to_cpp` mechanism (if it supports it). + throw std::runtime_error( + "Attempting to release to C++ using pybind11::wrapper<> " + "at too high of a level. Use a class type lower in the hierarchy, such that " + "the Python-derived instance actually is part of the lineage of " + "pybind11::wrapper"); + } + // Let the external holder take ownership, but keep instance registered. + tr->use_cpp_lifetime(std::move(obj), holder_type_id); + } + + static object release_cpp_lifetime(type* cppobj) { + auto* tr = dynamic_cast*>(cppobj); + if (tr == nullptr) { + // This shouldn't happen here... + throw std::runtime_error("Internal error?"); + } + // Return newly created object. + return tr->release_cpp_lifetime(); + } + static wrapper* run(type*, std::false_type) { + return nullptr; + } +}; + +template +struct wrapper_interface_impl { + static void use_cpp_lifetime(type*, object&&, detail::HolderTypeId) { + // This should be captured by runtime flag. + // TODO(eric.cousineau): Runtime flag may not be necessary. + throw std::runtime_error("Internal error?"); + } + static object release_cpp_lifetime(type*) { + throw std::runtime_error("Internal error?"); + } +}; + +template +struct holder_check_impl { + template + static bool check_destruct(...) { + // Noop by default. + return true; + } + template + static bool allow_null_external_holder(const holder_type&) { + return false; + } + template + static bool attempt_holder_transfer(holder_type& holder, detail::holder_erased external_holder_raw) { + // Only called when holder types are different. + unused(holder, external_holder_raw); + throw std::runtime_error("Unable to transfer between holders of different types"); + } + template + static bool accept_holder(detail::holder_erased external_holder_raw, holder_type& holder) { + // Only called when holder types are different. + unused(holder, external_holder_raw); + throw std::runtime_error("Unable to transfer between holders of different types"); + } +}; + +template <> +struct holder_check_impl : public holder_check_impl { + template + static bool check_destruct(detail::instance* inst, detail::holder_erased holder_raw) { + const holder_type& h = holder_raw.cast(); + handle src((PyObject*)inst); + const detail::type_info *lowest_type = get_lowest_type(src, false); + if (!lowest_type) + // We have multiple inheritance, skip. + return true; + auto load_type = detail::determine_load_type(src, lowest_type); + // Check use_count(), assuming that we have an accurate count (no competing threads?) + if (load_type == detail::LoadType::DerivedCppSinglePySingle) { + if (h.use_count() > 1) { + // Increase reference count + const auto& release_info = lowest_type->release_info; + if (release_info.can_derive_from_wrapper) { + // Increase reference count + object obj = reinterpret_borrow(src); + // Release to C++. + holder_type* null_holder = nullptr; + release_info.release_to_cpp(inst, detail::holder_erased(null_holder), std::move(obj)); + return false; + } + } + } + return true; + } + + template + static bool allow_null_external_holder(const holder_type& holder) { + // Called by `release_to_cpp`. + if (holder.use_count() == 1) + // TODO(eric.cousineau): This may not hold true if we pass temporaries??? + // Or if we've copied a `holder` in copyable_holder_caster... + throw std::runtime_error("Internal error: Should have non-null shared_ptr<> external_holder if use_count() == 1"); + else + return true; + } + + template + static bool accept_holder(detail::holder_erased external_holder_raw, holder_type& holder) { + // Only accept shared_ptr from `external_holder_raw`. + if (external_holder_raw.type_id() == detail::HolderTypeId::UniquePtr) { + using T = typename holder_type::element_type; + auto& external_holder = external_holder_raw.mutable_cast>(); + // Transfer to internal. + holder = std::move(external_holder); + return true; + } else { + throw std::runtime_error("Unable to transfer between holders of different types"); + } + } +}; + + +template <> +struct holder_check_impl : public holder_check_impl { + template + static bool attempt_holder_transfer(holder_type& holder, detail::holder_erased external_holder_raw) { + // Only accept shared_ptr from `external_holder_raw`. + if (external_holder_raw.type_id() == detail::HolderTypeId::SharedPtr) { + using T = typename holder_type::element_type; + auto& external_holder = external_holder_raw.mutable_cast>(); + // Transfer to external. + external_holder = std::move(holder); + return true; + } else { + return false; + } + } +}; + template class class_ : public detail::generic_type { template using is_holder = detail::is_holder_type; @@ -1021,7 +1169,9 @@ class class_ : public detail::generic_type { using type = type_; using type_alias = detail::exactly_one_t; constexpr static bool has_alias = !std::is_void::value; + constexpr static bool has_wrapper = std::is_base_of, type_alias>::value; using holder_type = detail::exactly_one_t, options...>; + constexpr static detail::HolderTypeId holder_type_id = detail::get_holder_type_id::value; static_assert(detail::all_of...>::value, "Unknown/invalid class_ template parameters provided"); @@ -1053,6 +1203,12 @@ class class_ : public detail::generic_type { record.dealloc = dealloc; record.default_holder = std::is_same>::value; + // TODO(eric.cousineau): Determine if it is possible to permit releasing without a wrapper... + auto& release_info = record.release_info; + release_info.can_derive_from_wrapper = has_wrapper; + release_info.release_to_cpp = release_to_cpp; + release_info.holder_type_id = holder_type_id; + set_operator_new(&record); /* Register base classes specified via template arguments to class_, if any */ @@ -1069,6 +1225,166 @@ class class_ : public detail::generic_type { } } + static detail::type_info* get_type_info() { + std::type_index id(typeid(type)); + return detail::get_type_info(id); + } + + typedef wrapper_interface_impl wrapper_interface; + using holder_check = holder_check_impl; + + static bool allow_destruct(detail::instance* inst, detail::holder_erased holder) { + // TODO(eric.cousineau): There should not be a case where shared_ptr<> lives in + // C++ and Python, with it being owned by C++. Check this. + return holder_check::template check_destruct(inst, holder); + } + + static void del_wrapped(handle self, object del_orig) { + // This should be called when the item is *actually* being deleted + // TODO(eric.cousineau): Do we care about use cases where the user manually calls this? + detail::instance* inst = (detail::instance*)self.ptr(); + const detail::type_info *lowest_type = detail::get_lowest_type(self); + auto& release_info = lowest_type->release_info; + // The references are as follows: + // 1. When Python calls __del__ via tp_del (default slot) + // 2. When Python gets the instance-bound __del__ method. + // TODO(eric.cousineau): Confirm this ^ + // 3. When pybind11 gets the argument + const int orig_count = self.ref_count(); + + auto v_h = inst->get_value_and_holder(lowest_type); + detail::holder_erased holder_raw(v_h.holder_ptr(), release_info.holder_type_id); + + // TODO(eric.cousineau): Ensure that this does not prevent destruction if + // the Python interpreter is finalizing... + // Is there a way to do this without a custom handler? + + // Purposely do NOT capture `object` to refcount low. + // TODO(eric.cousineau): `allow_destruct` should be registered in `type_info`. + // Right now, this doesn't really type-erase anything... + if (allow_destruct(inst, holder_raw)) { + // Call the old destructor. + if (!del_orig.is(none())) { + del_orig(self); + } + } else { + // This should have been kept alive by an increment in number of references. + unused(orig_count); + assert(self.ref_count() == orig_count + 1); + } + } + + static void release_to_cpp(detail::instance* inst, detail::holder_erased external_holder_raw, object&& obj) { + using detail::LoadType; + auto v_h = inst->get_value_and_holder(); + auto* tinfo = get_type_info(); + if (!inst->owned || !v_h.holder_constructed()) { + throw std::runtime_error("C++ object must be owned by pybind11 when attempting to release to C++"); + } + LoadType load_type = determine_load_type(obj, tinfo); + switch (load_type) { + case LoadType::PureCpp: { + // Given that `obj` is now exclusive, then once it goes out of scope, + // then the registered instance for this object should be destroyed, and this + // should become a pure C++ object, without any ties to `pybind11`. + // Also, even if this instance is of a class derived from a Base that has a + // wrapper-wrapper alias, we do not need to worry about not being in the correct + // hierarchy, since we will simply release from it. + + // TODO(eric.cousineau): Presently, there is no support for a consistent use of weak references. + // If a PureCpp object is released from Python, then all weak references are invalidated, + // even if it comes back... + // Ideally, this could check if there are weak references. But to whom should the lifetime be extended? + // Perhaps the first weak reference that is available? + break; + } + case LoadType::DerivedCppSinglePySingle: { + if (!tinfo->release_info.can_derive_from_wrapper) { + // This could be relaxed if there is an optional `release` mechanism for holders. + // However, there is still slicing. + throw std::runtime_error( + "Python-extended C++ class does not inherit from pybind11::wrapper<>, " + "and the instance will be sliced. Either avoid this situation, or " + "the type extends pybind11::wrapper<>."); + } + auto* cppobj = reinterpret_cast(v_h.value_ptr()); + wrapper_interface::use_cpp_lifetime(cppobj, std::move(obj), holder_type_id); + break; + } + default: { + throw std::runtime_error("Unsupported load type (multiple inheritance)"); + } + } + bool transfer_holder = true; + holder_type& holder = v_h.holder(); + if (!external_holder_raw.ptr()) { + if (holder_check::allow_null_external_holder(holder)) + transfer_holder = false; + else + throw std::runtime_error("Internal error: Null external holder"); + } + if (transfer_holder) { + if (external_holder_raw.type_id() == holder_type_id) { + holder_type& external_holder = external_holder_raw.mutable_cast(); + external_holder = std::move(holder); + } else { + // Only allow unique_ptr<> -> shared_ptr<> + holder_check::attempt_holder_transfer(holder, external_holder_raw); + } + } + holder.~holder_type(); + v_h.set_holder_constructed(false); + inst->owned = false; + // Register this type's reclamation procedure, since it's wrapper may have the contained object. + inst->reclaim_from_cpp = reclaim_from_cpp; + } + + static object reclaim_from_cpp(detail::instance* inst, detail::holder_erased external_holder_raw) { + using detail::LoadType; + auto v_h = inst->get_value_and_holder(); + auto* tinfo = get_type_info(); + // TODO(eric.cousineau): Should relax this to not require a holder be constructed, + // only that the holder itself be default (unique_ptr<>). + if (inst->owned || v_h.holder_constructed()) { + throw std::runtime_error("Derived Python object should live in C++"); + } + if (!external_holder_raw) { + throw std::runtime_error("Internal error - not external holder?"); + } + // Is this valid? + handle h(reinterpret_cast(inst)); + LoadType load_type = determine_load_type(h, tinfo); + { + // TODO(eric.cousineau): Consider releasing a raw pointer, to make it easier for + // interop with purely raw pointers? Nah, just rely on release. + holder_type& holder = v_h.holder(); + holder_type& external_holder = external_holder_raw.mutable_cast(); + new (&holder) holder_type(std::move(external_holder)); + v_h.set_holder_constructed(true); + + // Show that it has been reclaimed. + inst->reclaim_from_cpp = nullptr; + } + object obj; + switch (load_type) { + case LoadType::PureCpp: { + // Nothing complex needed here. + obj = reinterpret_borrow(h); + break; + } + case LoadType::DerivedCppSinglePySingle: { + auto* cppobj = reinterpret_cast(v_h.value_ptr()); + obj = wrapper_interface::release_cpp_lifetime(cppobj); + break; + } + default: { + throw std::runtime_error("Unsupported load type"); + } + } + inst->owned = true; + return obj; + } + template ::value, int> = 0> static void add_base(detail::type_record &rec) { rec.add_base(typeid(Base), [](void *src) -> void * { @@ -1297,8 +1613,15 @@ class class_ : public detail::generic_type { if (holder_ptr) { init_holder_from_existing(v_h, holder_ptr, std::is_copy_constructible()); v_h.set_holder_constructed(); - } else if (inst->owned || detail::always_construct_holder::value) { - new (&v_h.holder()) holder_type(v_h.value_ptr()); + } else { + init_holder_simple(inst, v_h, v_h.value_ptr()); + } + } + + /// Initialize a holder object simply (to be converted). + static void init_holder_simple(detail::instance *inst, detail::value_and_holder &v_h, type* value) { + if (inst->owned || detail::always_construct_holder::value) { + new (&v_h.holder()) holder_type(value); v_h.set_holder_constructed(); } } @@ -1307,13 +1630,70 @@ class class_ : public detail::generic_type { /// instance. Should be called as soon as the `type` value_ptr is set for an instance. Takes an /// optional pointer to an existing holder to use; if not specified and the instance is /// `.owned`, a new holder will be constructed to manage the value pointer. - static void init_instance(detail::instance *inst, const void *holder_ptr) { + static void init_instance(detail::instance *inst, detail::holder_erased holder_ptr) { + using namespace detail; auto v_h = inst->get_value_and_holder(detail::get_type_info(typeid(type))); + type* value_ptr = v_h.value_ptr(); if (!v_h.instance_registered()) { - register_instance(inst, v_h.value_ptr(), v_h.type); + register_instance(inst, value_ptr, v_h.type); v_h.set_instance_registered(); } - init_holder(inst, v_h, (const holder_type *) holder_ptr, v_h.value_ptr()); + if (!holder_ptr) { + init_holder(inst, v_h, nullptr, value_ptr); + } else if (holder_ptr.type_id() == holder_type_id) { + init_holder(inst, v_h, &holder_ptr.cast(), value_ptr); + } else { + // Create a new, empty holder, and then transfer. + init_holder_simple(inst, v_h, nullptr); + if (!v_h.holder_constructed()) { + throw std::runtime_error("Bad edge case"); + } + holder_type& holder = v_h.holder(); + holder_check::accept_holder(holder_ptr, holder); + } + + // TODO(eric.cousineau): Inject override of __del__ for intercepting C++ stuff + handle self((PyObject*)inst); + handle h_type = self.get_type(); + + // Use hacky Python-style inheritance check. + PyTypeObject *py_type = (PyTypeObject*)h_type.ptr(); + bool is_py_derived = py_type->tp_dealloc != detail::pybind11_object_dealloc; + + bool can_add_del = true; + + // Get non-instance-bound method (analogous `tp_del`). + // Is there a way to check if `__del__` is an instance-assigned + // method? (Rather than a class method?) + object del_orig = getattr(h_type, "__del__", none()); + +#if !defined(PYPY_VERSION) + // PyPy will not execute an arbitrarily-added `__del__` method. + // Later version of PyPy throw an error if this happens. + // Workaround: Define a no-op `__del__` if this feature is useful. + if (del_orig.is(none())) { + can_add_del = false; + } +#endif + + // Check tp_dealloc + if (is_py_derived && can_add_del) { + // TODO(eric.cousineau): Consider moving this outside of this class, + // to potentially enable multiple inheritance. + const std::string orig_field = "_pybind11_del_orig"; + if (!hasattr(h_type, orig_field.c_str())) { + // NOTE: This is NOT tied to this particular type. + auto del_new = [orig_field](handle h_self) { + // TODO(eric.cousineau): Make this global, not tied to this type. + object del_orig = getattr(h_self.get_type(), orig_field.c_str()); + del_wrapped(h_self, del_orig); + }; + // Replace with an Python-instance-unbound function. + object new_dtor_py = cpp_function(del_new, is_method(h_type)); + setattr(h_type, "__del__", new_dtor_py); + setattr(h_type, orig_field.c_str(), del_orig); + } + } } /// Deallocates an instance; via holder, if constructed; otherwise via operator delete. diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index d7fa17775c3..ebe1a777cb5 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -11,6 +11,7 @@ #include "detail/common.h" #include "buffer_info.h" +#include #include #include @@ -1297,6 +1298,114 @@ inline iterator iter(handle obj) { } /// @} python_builtins +/// Trampoline class to permit attaching a derived Python object's data +/// (namely __dict__) to an actual C++ class. +/// If the object lives purely in C++, then there should only be one reference to +/// this data. +template +class wrapper : public Base { + protected: + using Base::Base; + + public: + // TODO(eric.cousineau): Complain if this is not virtual? (and remove `virtual` specifier in dtor?) + + virtual ~wrapper() { + delete_py_if_in_cpp(); + } + + /// To be used by the holder casters, by means of `wrapper_interface<>`. + // TODO(eric.cousineau): Make this private to ensure contract? + void use_cpp_lifetime(object&& patient, detail::HolderTypeId holder_type_id) { + if (lives_in_cpp()) { + throw std::runtime_error("Instance already lives in C++"); + } + holder_type_id_ = holder_type_id; + patient_ = std::move(patient); + // @note It would be nice to put `revive_python3` here, but this is called by + // `PyObject_CallFinalizer`, which will end up reversing its effect anyways. + } + + /// To be used by `move_only_holder_caster`. + object release_cpp_lifetime() { + if (!lives_in_cpp()) { + throw std::runtime_error("Instance does not live in C++"); + } + revive_python3(); + // Remove existing reference. + object tmp = std::move(patient_); + assert(!patient_); + return tmp; + } + + protected: + /// Call this if, for whatever reason, your C++ wrapper class `Base` has a non-trivial + /// destructor that needs to keep information available to the Python-extended class. + /// In this case, you want to delete the Python object *before* you do any work in your wrapper class. + /// + /// As an example, say you have `Base`, and `PyBase` is your wrapper class which extends `wrapper`. + /// By default, if the instance is owned in C++ and deleted, then the destructor order will be: + /// ~PyBase() + /// do_stuff() + /// ~wrapper() + /// delete_py_if_in_cpp() + /// PyChild.__del__ - ERROR: Should have been called before `do_stuff() + /// ~Base() + /// If you explicitly call `delete_py_if_in_cpp()`, then you will get the desired order: + /// ~PyBase() + /// delete_py_if_in_cpp() + /// PyChild.__del__ - GOOD: Workzzz. Called before `do_stuff()`. + /// do_stuff() + /// ~wrapper() + /// delete_py_if_in_cpp() - No-op. Python object has been released. + /// ~Base() + // TODO(eric.cousineau): Verify this with an example workflow. + void delete_py_if_in_cpp() { + if (lives_in_cpp()) { + // Ensure that we still are the unique one, such that the Python classes + // destructor will be called. +#ifdef PYBIND11_WARN_DANGLING_UNIQUE_PYREF + if (holder_type_id_ == detail::HolderTypeId::UniquePtr) { + if (patient_.ref_count() != 1) { + // TODO(eric.cousineau): Add Python class name + std::string class_name = patient_.get_type().str(); + std::cerr + << "WARNING(pybind11): When destroying Python subclass (" << class_name << "), " + << "of a pybind11 class using a unique_ptr holder in C++, " + << "ref_count == " << patient_.ref_count() << " != 1, which may cause undefined behavior." << std::endl + << " Please consider reviewing your code to trim existing references, or use a move-compatible container." << std::endl; + } + } +#endif // PYBIND11_WARN_DANGLING_UNIQUE_HOLDER + // Release object. + release_cpp_lifetime(); + } + } + + // Python3 unfortunately will not implicitly call `__del__` multiple times, + // even if the object is resurrected. This is a dirty workaround. + // @see https://bugs.python.org/issue32377 + inline void revive_python3() { +#if PY_VERSION_HEX >= 0x03000000 + // Reverse single-finalization constraint in Python3. + if (_PyGC_FINALIZED(patient_.ptr())) { + _PyGC_SET_FINALIZED(patient_.ptr(), 0); + } +#endif // PY_VERSION_HEX >= 0x03000000 + } + + private: + bool lives_in_cpp() const { + // NOTE: This is *false* if, for whatever reason, the wrapper class is + // constructed in C++... Meh. Not gonna worry about that situation. + return static_cast(patient_); + } + + object patient_; + detail::HolderTypeId holder_type_id_{detail::HolderTypeId::Unknown}; +}; + + NAMESPACE_BEGIN(detail) template iterator object_api::begin() const { return iter(derived()); } template iterator object_api::end() const { return iterator::sentinel(); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5953e0baacc..81821c8cb15 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -51,6 +51,7 @@ set(PYBIND11_TEST_FILES test_numpy_vectorize.cpp test_opaque_types.cpp test_operator_overloading.cpp + test_ownership_transfer.cpp test_pickling.cpp test_pytypes.cpp test_sequences_and_iterators.cpp diff --git a/tests/test_class.py b/tests/test_class.py index d94b61bb3a9..f161ab6e21e 100644 --- a/tests/test_class.py +++ b/tests/test_class.py @@ -238,16 +238,26 @@ class PyDog(m.Dog): pass for cls in m.Dog, PyDog: - refcount_1 = getrefcount(cls) - molly = [cls("Molly") for _ in range(10)] - refcount_2 = getrefcount(cls) - - del molly - pytest.gc_collect() - refcount_3 = getrefcount(cls) - - assert refcount_1 == refcount_3 - assert refcount_2 > refcount_1 + for i in range(5): + refcount_1 = getrefcount(cls) + molly = [cls("Molly") for _ in range(10)] + refcount_2 = getrefcount(cls) + + del molly + pytest.gc_collect() + refcount_3 = getrefcount(cls) + + if cls == PyDog and i == 0: + # If this is the first time the derived class is called, then + # creating the instance created the dtor hook, introducing one more reference. + # @note This changes to `refcount_1 == refcount_3` in Python3. + assert refcount_1 + 1 == refcount_3 or refcount_1 == refcount_3 + else: + assert refcount_1 == refcount_3 + assert refcount_2 > refcount_1 + + # @note Deleting `PyDog` does not return the refcount of `m.Dog` back to zero, due to the + # destructor shim. def test_reentrant_implicit_conversion_failure(msg): diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 86b2c3b4bb7..6b7793cfa6b 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -4,10 +4,10 @@ def test_methods_and_attributes(): - instance1 = m.ExampleMandA() - instance2 = m.ExampleMandA(32) + instance1 = m.ExampleMandA() # 1 def.ctor + instance2 = m.ExampleMandA(32) # 1 ctor - instance1.add1(instance2) + instance1.add1(instance2) # 1 copy ctor + 1 move instance1.add2(instance2) instance1.add3(instance2) instance1.add4(instance2) @@ -20,7 +20,7 @@ def test_methods_and_attributes(): assert str(instance1) == "ExampleMandA[value=320]" assert str(instance2) == "ExampleMandA[value=32]" - assert str(instance1.self1()) == "ExampleMandA[value=320]" + assert str(instance1.self1()) == "ExampleMandA[value=320]" # 1 copy ctor + 1 move assert str(instance1.self2()) == "ExampleMandA[value=320]" assert str(instance1.self3()) == "ExampleMandA[value=320]" assert str(instance1.self4()) == "ExampleMandA[value=320]" @@ -58,8 +58,8 @@ def test_methods_and_attributes(): assert cstats.alive() == 0 assert cstats.values() == ["32"] assert cstats.default_constructions == 1 - assert cstats.copy_constructions == 3 - assert cstats.move_constructions >= 1 + assert cstats.copy_constructions == 2 + assert cstats.move_constructions == 2 assert cstats.copy_assignments == 0 assert cstats.move_assignments == 0 diff --git a/tests/test_multiple_inheritance.cpp b/tests/test_multiple_inheritance.cpp index 35f9d9c4e3e..0f0355e8a52 100644 --- a/tests/test_multiple_inheritance.cpp +++ b/tests/test_multiple_inheritance.cpp @@ -43,6 +43,24 @@ int WithStatic2::static_value2 = 2; int VanillaStaticMix1::static_value = 12; int VanillaStaticMix2::static_value = 12; +template > +class Container { +public: + Container(Ptr ptr) + : ptr_(std::move(ptr)) {} + T* get() const { return ptr_.get(); } + Ptr release() { return std::move(ptr_); } + + static void def(py::module &m, const std::string& name) { + py::class_(m, name.c_str()) + .def(py::init()) + .def("get", &Container::get) + .def("release", &Container::release); + } +private: + Ptr ptr_; +}; + TEST_SUBMODULE(multiple_inheritance, m) { // test_multiple_inheritance_mix1 @@ -77,6 +95,7 @@ TEST_SUBMODULE(multiple_inheritance, m) { py::class_(m, "MIType") .def(py::init()); + Container::def(m, "ContainerBase1"); // test_multiple_inheritance_python_many_bases #define PYBIND11_BASEN(N) py::class_>(m, "BaseN" #N).def(py::init()).def("f" #N, [](BaseN &b) { return b.i + N; }) @@ -124,6 +143,8 @@ TEST_SUBMODULE(multiple_inheritance, m) { std::shared_ptr>(m, "Base12a", py::multiple_inheritance()) .def(py::init()); + Container>::def(m, "ContainerBase2a"); + m.def("bar_base2a", [](Base2a *b) { return b->bar(); }); m.def("bar_base2a_sharedptr", [](std::shared_ptr b) { return b->bar(); }); diff --git a/tests/test_multiple_inheritance.py b/tests/test_multiple_inheritance.py index 475dd3b3d8a..f1bd800d047 100644 --- a/tests/test_multiple_inheritance.py +++ b/tests/test_multiple_inheritance.py @@ -347,3 +347,30 @@ def test_diamond_inheritance(): assert d is d.c0().b() assert d is d.c1().b() assert d is d.c0().c1().b().c0().b() + + +@pytest.unsupported_on_pypy +def test_mi_ownership_constraint(): + # See `test_ownership_transfer` for positive tests. + + # unique_ptr + with pytest.raises(RuntimeError) as excinfo: + c = m.ContainerBase1(m.MIType(10, 100)) + assert "multiple inheritance" in str(excinfo.value) + + # shared_ptr + # Should not throw an error. + obj = m.Base12a(10, 100) + assert obj.bar() == 100 + c = m.ContainerBase2a(obj) + + # TODO(eric.cousineau): This currently causes a segfault in both + # this branch and on `master` (a303c6f). + # Figure out if there is a way to fix this? + + # assert c.get().bar() == 100 + + # # Should throw an error. + # c = m.ContainerBase2a(m.Bae12a(10, 100)) + # assert c.get().foo() == 10 + # assert c.get().bar() == 100 diff --git a/tests/test_ownership_transfer.cpp b/tests/test_ownership_transfer.cpp new file mode 100644 index 00000000000..86c7f0ff6c1 --- /dev/null +++ b/tests/test_ownership_transfer.cpp @@ -0,0 +1,179 @@ +/* + tests/test_ownership_transfer.cpp -- test ownership transfer semantics. + + Copyright (c) 2017 Eric Cousineau + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#if defined(_MSC_VER) && _MSC_VER < 1910 +# pragma warning(disable: 4702) // unreachable code in system header +#endif + +#include +#include "pybind11_tests.h" +#include "object.h" + +enum Label : int { + BaseBadLabel, + ChildBadLabel, + BaseLabel, + ChildLabel, + + BaseBadUniqueLabel, + ChildBadUniqueLabel, + BaseUniqueLabel, + ChildUniqueLabel, +}; + +// For attaching instances of `ConstructorStats`. +template +class Stats {}; + + +template +class DefineBase { + public: + DefineBase(int value) + : value_(value) { + track_created(this, value); + } + // clang does not like having an implicit copy constructor when the + // class is virtual (and rightly so). + DefineBase(const DefineBase&) = delete; + virtual ~DefineBase() { + track_destroyed(this); + } + virtual int value() const { return value_; } + private: + int value_{}; +}; + +template +class DefineBaseContainer { + public: + using T = DefineBase