wrapper: Robustify check, don't require that alias classes inherit, allow alias to be wrapped directly.

Author: EricCousineau-TRI

docs/advanced/classes.rst

@@ -65,10 +65,10 @@ helper class that is defined as follows:
 
 .. code-block:: cpp
 
-    class PyAnimal : public py::wrapper<Animal> {
+    class PyAnimal : public Animal {
     public:
         /* Inherit the constructors */
-        using py::wrapper<Animal>::wrapper;
+        using Animal::Animal;
 
         /* Trampoline (need one for each virtual function) */
         std::string go(int n_times) override {
@@ -90,15 +90,13 @@ 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
     :emphasize-lines: 2,4,5
 
     PYBIND11_MODULE(example, m) {
-        py::class_<Animal, PyAnimal /* <--- trampoline*/> animal(m, "Animal");
+        py::class_<Animal, py::wrapper<PyAnimal> /* <--- trampoline*/> animal(m, "Animal");
         animal
             .def(py::init<>())
             .def("go", &Animal::go);
@@ -119,7 +117,7 @@ Bindings should be made against the actual class, not the trampoline helper clas
 
 .. code-block:: cpp
 
-    py::class_<Animal, PyAnimal /* <--- trampoline*/> animal(m, "Animal");
+    py::class_<Animal, py::wrapper<PyAnimal> /* <--- trampoline*/> animal(m, "Animal");
         animal
             .def(py::init<>())
             .def("go", &PyAnimal::go); /* <--- THIS IS WRONG, use &Animal::go */
@@ -129,6 +127,16 @@ extend ``Animal``, but not ``Dog``: see :ref:`virtual_and_inheritance` for the
 necessary steps required to providing proper overload support for inherited
 classes.
 
+The wrapper ``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`.
+
+.. note::
+
+    If you do use ``py::wrapper<>`` and you have defined factory
+    ``__init__`` methods, you *must* ensure that the alias variants return new
+    instances of ``py::wrapper<Alias>``, rather than just ``Alias``.
+
 The Python session below shows how to override ``Animal::go`` and invoke it via
 a virtual method call.
 
@@ -234,15 +242,15 @@ override the ``name()`` method):
 
 .. code-block:: cpp
 
-    class PyAnimal : public py::wrapper<Animal> {
+    class PyAnimal : public Animal {
     public:
-        using py::wrapper<Animal>::wrapper; // Inherit constructors
+        using Animal::Animal; // 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 py::wrapper<Dog> {
+    class PyDog : public Dog {
     public:
-        using py::wrapper<Dog>::wrapper; // Inherit constructors
+        using Dog::Dog; // 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, ); }
@@ -262,24 +270,24 @@ declare or override any virtual methods itself:
 .. code-block:: cpp
 
     class Husky : public Dog {};
-    class PyHusky : public py::wrapper<Husky> {
+    class PyHusky : public Husky {
     public:
-        using py::wrapper<Husky>::wrapper; // Inherit constructors
+        using Husky::Husky; // 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, ); }
     };
 
 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 (the Curiously Recurring Template Pattern) involves using template trampoline classes, as
+methods).  The technique involves using template trampoline classes, as
 follows:
 
 .. code-block:: cpp
 
-    template <class AnimalBase = Animal> class PyAnimal : public py::wrapper<AnimalBase> {
+    template <class AnimalBase = Animal> class PyAnimal : public AnimalBase {
     public:
-        using py::wrapper<AnimalBase>::wrapper; // Inherit constructors
+        using AnimalBase::AnimalBase; // 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, ); }
     };
@@ -301,9 +309,9 @@ The classes are then registered with pybind11 using:
 
 .. code-block:: cpp
 
-    py::class_<Animal, PyAnimal<>> animal(m, "Animal");
-    py::class_<Dog, PyDog<>> dog(m, "Dog");
-    py::class_<Husky, PyDog<Husky>> husky(m, "Husky");
+    py::class_<Animal, py::wrapper<PyAnimal<>>> animal(m, "Animal");
+    py::class_<Dog, py::wrapper<PyDog<>>> dog(m, "Dog");
+    py::class_<Husky, py::wrapper<PyDog<Husky>>> husky(m, "Husky");
     // ... add animal, dog, husky definitions
 
 Note that ``Husky`` did not require a dedicated trampoline template class at
@@ -324,6 +332,12 @@ can now create a python class that inherits from ``Dog``:
     See the file :file:`tests/test_virtual_functions.cpp` for complete examples
     using both the duplication and templated trampoline approaches.
 
+.. note::
+
+    If you do not want to specify ``py::wrapper<>`` for each type, you
+    can have ``PyAnimal`` inherit from ``py::wrapper<>``. Pybind will
+    detect if ``py::wrapper<>`` is present at any point in the chain.
+
 .. _extended_aliases:
 
 Extended trampoline class functionality
@@ -1051,10 +1065,10 @@ For this example, we will build upon the above code for ``Animal`` with alias ``
         virtual std::string go(int n_times) = 0;
     };
 
-    class PyAnimal : public py::wrapper<Animal> {
+    class PyAnimal : public Animal {
     public:
         /* Inherit the constructors */
-        using py::wrapper<Animal>::wrapper;
+        using Animal::Animal;
         std::string go(int n_times) override {
             PYBIND11_OVERLOAD_PURE(std::string, Animal, go, n_times);
         }
@@ -1077,7 +1091,7 @@ And the following bindings:
 .. code-block:: cpp
 
     PYBIND11_MODULE(example, m) {
-        py::class_<Animal, PyAnimal, std::shared_ptr<Animal>> animal(m, "Animal");
+        py::class_<Animal, py::wrapper<PyAnimal>, std::shared_ptr<Animal>> animal(m, "Animal");
         animal
             .def(py::init<>())
             .def("go", &Animal::go);

include/pybind11/pybind11.h

@@ -1014,12 +1014,25 @@ auto method_adaptor(Return (Class::*pmf)(Args...) const) -> Return (Derived::*)(
     return pmf;
 }
 
+namespace detail {
+
+template <template <typename...> class Tpl>
+struct is_base_template_of_impl {
+    // Use pointers for robustness.
+    template <typename ... Base>
+    static std::true_type check(const Tpl<Base...>*);
+    static std::false_type check(void*);
+};
 
+template <template <typename...> class Tpl, typename Derived>
+using is_base_template_of =
+    decltype(is_base_template_of_impl<Tpl>::check(
+        std::declval<Derived*>()));
 
-template <typename type, bool compatible>
+template <typename type, typename alias, bool compatible>
 struct wrapper_interface_impl {
     static void use_cpp_lifetime(type* cppobj, object&& obj, detail::HolderTypeId holder_type_id) {
-        auto* tr = dynamic_cast<wrapper<type>*>(cppobj);
+        auto* tr = dynamic_cast<alias*>(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).
@@ -1034,21 +1047,18 @@ struct wrapper_interface_impl {
     }
 
     static object release_cpp_lifetime(type* cppobj) {
-        auto* tr = dynamic_cast<wrapper<type>*>(cppobj);
+        auto* tr = dynamic_cast<alias*>(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<type>* run(type*, std::false_type) {
-        return nullptr;
-    }
 };
 
-template <typename type>
-struct wrapper_interface_impl<type, false> {
+template <typename type, typename alias>
+struct wrapper_interface_impl<type, alias, false> {
     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.
@@ -1157,6 +1167,8 @@ struct holder_check_impl<detail::HolderTypeId::UniquePtr> : public holder_check_
       }
 };
 
+}  // namespace detail
+
 template <typename type_, typename... options>
 class class_ : public detail::generic_type {
     template <typename T> using is_holder = detail::is_holder_type<type_, T>;
@@ -1170,7 +1182,8 @@ class class_ : public detail::generic_type {
     using type = type_;
     using type_alias = detail::exactly_one_t<is_subtype, void, options...>;
     constexpr static bool has_alias = !std::is_void<type_alias>::value;
-    constexpr static bool has_wrapper = std::is_base_of<wrapper<type>, type_alias>::value;
+    constexpr static bool has_wrapper =
+        detail::is_base_template_of<wrapper, type_alias>::value;
     using holder_type = detail::exactly_one_t<is_holder, std::unique_ptr<type>, options...>;
     constexpr static detail::HolderTypeId holder_type_id = detail::get_holder_type_id<holder_type>::value;
 
@@ -1231,8 +1244,8 @@ class class_ : public detail::generic_type {
         return detail::get_type_info(id);
     }
 
-    typedef wrapper_interface_impl<type, has_wrapper> wrapper_interface;
-    using holder_check = holder_check_impl<holder_type_id>;
+    using wrapper_interface = detail::wrapper_interface_impl<type, type_alias, has_wrapper>;
+    using holder_check = detail::holder_check_impl<holder_type_id>;
 
     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

include/pybind11/pytypes.h

@@ -1301,23 +1301,21 @@ 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.
+/// Wrapper to permit lifetime of a Python instance which is derived from a C++
+/// pybind type to be managed by C++. Useful when adding virtual classes to
+/// containers, where Python instance being added may be collected by Python
+/// gc / refcounting.
+/// @note Do NOT use the methods in this class.
 template <typename Base>
 class wrapper : public Base {
- protected:
-    using Base::Base;
-
  public:
-  // TODO(eric.cousineau): Complain if this is not virtual? (and remove `virtual` specifier in dtor?)
+  using Base::Base;
 
   virtual ~wrapper() {
       delete_py_if_in_cpp();
   }
 
-  /// To be used by the holder casters, by means of `wrapper_interface<>`.
+  // 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()) {
@@ -1342,26 +1340,6 @@ class wrapper : public Base {
   }
 
  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<Base>`.
-    /// By default, if the instance is owned in C++ and deleted, then the destructor order will be:
-    ///    ~PyBase()
-    ///       do_stuff()
-    ///    ~wrapper<Base>()
-    ///       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<Base>()
-    ///       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()) {
@@ -1398,7 +1376,7 @@ class wrapper : public Base {
   }
 
  private:
-  bool lives_in_cpp() const {
+  inline 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<bool>(patient_);
@@ -1408,7 +1386,6 @@ class wrapper : public Base {
   detail::HolderTypeId holder_type_id_{detail::HolderTypeId::Unknown};
 };
 
-
 NAMESPACE_BEGIN(detail)
 template <typename D> iterator object_api<D>::begin() const { return iter(derived()); }
 template <typename D> iterator object_api<D>::end() const { return iterator::sentinel(); }

tests/test_ownership_transfer.cpp

@@ -75,7 +75,17 @@ class DefineBaseUniqueContainer {
 };
 
 template <int label>
-class DefinePyBase : public py::wrapper<DefineBase<label>> {
+class DefinePyBase : public DefineBase<label> {
+ public:
+  using BaseT = DefineBase<label>;
+  using BaseT::BaseT;
+  int value() const override {
+    PYBIND11_OVERLOAD(int, BaseT, value);
+  }
+};
+
+template <int label>
+class DefinePyBaseWrapped : public py::wrapper<DefineBase<label>> {
  public:
   using BaseT = py::wrapper<DefineBase<label>>;
   using BaseT::BaseT;
@@ -89,7 +99,7 @@ typedef DefineBase<BaseBadLabel> BaseBad;
 typedef DefineBaseContainer<BaseBadLabel> BaseBadContainer;
 typedef Stats<ChildBadLabel> ChildBadStats;
 
-// Base - with wrapper alias.
+// Base - wrapper alias used in pybind definition.
 typedef DefineBase<BaseLabel> Base;
 typedef DefinePyBase<BaseLabel> PyBase;
 typedef DefineBaseContainer<BaseLabel> BaseContainer;
@@ -101,9 +111,9 @@ typedef DefineBase<BaseBadUniqueLabel> BaseBadUnique;
 typedef DefineBaseUniqueContainer<BaseBadUniqueLabel> BaseBadUniqueContainer;
 typedef Stats<ChildBadUniqueLabel> ChildBadUniqueStats;
 
-// Base - with wrapper alias.
+// Base - wrapper alias used directly.
 typedef DefineBase<BaseUniqueLabel> BaseUnique;
-typedef DefinePyBase<BaseUniqueLabel> PyBaseUnique;
+typedef DefinePyBaseWrapped<BaseUniqueLabel> PyBaseUnique;
 typedef DefineBaseUniqueContainer<BaseUniqueLabel> BaseUniqueContainer;
 typedef Stats<ChildUniqueLabel> ChildUniqueStats;
 
@@ -135,6 +145,7 @@ template <typename... Args>
 using class_unique_ = py::class_<Args...>;
 
 TEST_SUBMODULE(ownership_transfer, m) {
+  // No alias - will not have lifetime extended.
   class_shared_<BaseBad>(m, "BaseBad")
       .def(py::init<int>())
       .def("value", &BaseBad::value);
@@ -144,8 +155,11 @@ TEST_SUBMODULE(ownership_transfer, m) {
       .def("release", &BaseBadContainer::release);
   class_shared_<ChildBadStats>(m, "ChildBadStats");
 
-  class_shared_<Base, PyBase>(m, "Base")
+  // Has alias - will have lifetime extended.
+  class_shared_<Base, py::wrapper<PyBase>>(m, "Base")
       .def(py::init<int>())
+      // Factory method for alias.
+      .def(py::init([]() { return new py::wrapper<PyBase>(10); }))
       .def("value", &Base::value);
   class_shared_<BaseContainer>(m, "BaseContainer")
       .def(py::init<std::shared_ptr<Base>>())
@@ -164,6 +178,8 @@ TEST_SUBMODULE(ownership_transfer, m) {
 
   class_unique_<BaseUnique, PyBaseUnique>(m, "BaseUnique")
       .def(py::init<int>())
+      // Factory method.
+      .def(py::init([]() { return new PyBaseUnique(10); }))
       .def("value", &BaseUnique::value);
   class_unique_<BaseUniqueContainer>(m, "BaseUniqueContainer")
       .def(py::init<std::unique_ptr<BaseUnique>>())