Reimplement py::init<...> to use common factory code

This reimplements the py::init<...> implementations using the various
functions added to support `py::init(...)`, and moves the implementing
structs into `detail/init.h` from `pybind11.h`.  It doesn't simply use a
factory directly, as this is a very common case and implementation
without an extra lambda call is a small but useful optimization.

This, combined with the previous lazy initialization, also avoids
needing placement new for `py::init<...>()` construction: such
construction now occurs via an ordinary `new Type(...)`.

A consequence of this is that it also fixes a potential bug when using
multiple inheritance from Python: it was very easy to write classes
that double-initialize an existing instance which had the potential to
leak for non-pod classes.  With the new implementation, an attempt to
call `__init__` on an already-initialized object is now ignored.  (This
was already done in the previous commit for factory constructors).

This change exposed a few warnings (fixed here) from deleting a pointer
to a base class with virtual functions but without a virtual destructor.
These look like legitimate warnings that we shouldn't suppress; this
adds virtual destructors to the appropriate classes.

Author: jagerman

include/pybind11/attr.h

@@ -116,8 +116,6 @@ enum op_id : int;
 enum op_type : int;
 struct undefined_t;
 template <op_id id, op_type ot, typename L = undefined_t, typename R = undefined_t> struct op_;
-template <typename... Args> struct init;
-template <typename... Args> struct init_alias;
 inline void keep_alive_impl(size_t Nurse, size_t Patient, function_call &call, handle ret);
 
 /// Internal data structure which holds metadata about a keyword argument

include/pybind11/detail/init.h

@@ -40,7 +40,7 @@ PYBIND11_NOINLINE inline value_and_holder load_v_h(handle self_, type_info *tinf
 }
 
 
-// Implementing functions for py::init(...)
+// Implementing functions for all forms of py::init<...> and py::init(...)
 template <typename Class> using Cpp = typename Class::type;
 template <typename Class> using Alias = typename Class::type_alias;
 template <typename Class> using Holder = typename Class::holder_type;
@@ -161,6 +161,62 @@ void construct(value_and_holder &v_h, Alias<Class> &&result, bool) {
     deallocate(v_h) = new Alias<Class>(std::move(result));
 }
 
+// Implementing class for py::init<...>()
+template <typename... Args> struct constructor {
+    template <typename Class, typename... Extra, enable_if_t<!Class::has_alias, int> = 0>
+    static void execute(Class &cl, const Extra&... extra) {
+        auto *cl_type = get_type_info(typeid(Cpp<Class>));
+        cl.def("__init__", [cl_type](handle self_, Args... args) {
+            auto v_h = load_v_h(self_, cl_type);
+            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
+
+            construct<Class>(v_h, new Cpp<Class>(std::forward<Args>(args)...), false);
+        }, extra...);
+    }
+
+    template <typename Class, typename... Extra,
+              enable_if_t<Class::has_alias &&
+                          std::is_constructible<Cpp<Class>, Args...>::value, int> = 0>
+    static void execute(Class &cl, const Extra&... extra) {
+        auto *cl_type = get_type_info(typeid(Cpp<Class>));
+        cl.def("__init__", [cl_type](handle self_, Args... args) {
+            auto v_h = load_v_h(self_, cl_type);
+            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
+
+            if (Py_TYPE(v_h.inst) == cl_type->type)
+                construct<Class>(v_h, new Cpp<Class>(std::forward<Args>(args)...), false);
+            else
+                construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
+        }, extra...);
+    }
+
+    template <typename Class, typename... Extra,
+              enable_if_t<Class::has_alias &&
+                          !std::is_constructible<Cpp<Class>, Args...>::value, int> = 0>
+    static void execute(Class &cl, const Extra&... extra) {
+        auto *cl_type = get_type_info(typeid(Cpp<Class>));
+        cl.def("__init__", [cl_type](handle self_, Args... args) {
+            auto v_h = load_v_h(self_, cl_type);
+            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
+            construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
+        }, extra...);
+    }
+};
+
+// Implementing class for py::init_alias<...>()
+template <typename... Args> struct alias_constructor {
+    template <typename Class, typename... Extra,
+              enable_if_t<Class::has_alias && std::is_constructible<Alias<Class>, Args...>::value, int> = 0>
+    static void execute(Class &cl, const Extra&... extra) {
+        auto *cl_type = get_type_info(typeid(Cpp<Class>));
+        cl.def("__init__", [cl_type](handle self_, Args... args) {
+            auto v_h = load_v_h(self_, cl_type);
+            if (v_h.instance_registered()) return; // Ignore duplicate __init__ calls (see above)
+            construct<Class>(v_h, new Alias<Class>(std::forward<Args>(args)...), true);
+        }, extra...);
+    }
+};
+
 // Implementation class for py::init(Func) and py::init(Func, AliasFunc)
 template <typename CFunc, typename AFuncIn, typename... Args> struct factory {
 private:

include/pybind11/pybind11.h

@@ -1046,13 +1046,13 @@ class class_ : public detail::generic_type {
     }
 
     template <typename... Args, typename... Extra>
-    class_ &def(const detail::init<Args...> &init, const Extra&... extra) {
+    class_ &def(const detail::initimpl::constructor<Args...> &init, const Extra&... extra) {
         init.execute(*this, extra...);
         return *this;
     }
 
     template <typename... Args, typename... Extra>
-    class_ &def(const detail::init_alias<Args...> &init, const Extra&... extra) {
+    class_ &def(const detail::initimpl::alias_constructor<Args...> &init, const Extra&... extra) {
         init.execute(*this, extra...);
         return *this;
     }
@@ -1351,10 +1351,10 @@ template <typename Type> class enum_ : public class_<Type> {
 };
 
 /// Binds an existing constructor taking arguments Args...
-template <typename... Args> detail::init<Args...> init() { return detail::init<Args...>(); }
+template <typename... Args> detail::initimpl::constructor<Args...> init() { return {}; }
 /// Like `init<Args...>()`, but the instance is always constructed through the alias class (even
 /// when not inheriting on the Python side).
-template <typename... Args> detail::init_alias<Args...> init_alias() { return detail::init_alias<Args...>(); }
+template <typename... Args> detail::initimpl::alias_constructor<Args...> init_alias() { return {}; }
 
 /// Binds a factory function as a constructor
 template <typename Func, typename Ret = detail::initimpl::factory_t<Func>>
@@ -1368,44 +1368,6 @@ Ret init(CFunc &&c, AFunc &&a) {
 }
 
 NAMESPACE_BEGIN(detail)
-template <typename... Args> struct init {
-    template <typename Class, typename... Extra, enable_if_t<!Class::has_alias, int> = 0>
-    static void execute(Class &cl, const Extra&... extra) {
-        using Base = typename Class::type;
-        /// Function which calls a specific C++ in-place constructor
-        cl.def("__init__", [](Base *self_, Args... args) { new (self_) Base(args...); }, extra...);
-    }
-
-    template <typename Class, typename... Extra,
-              enable_if_t<Class::has_alias &&
-                          std::is_constructible<typename Class::type, Args...>::value, int> = 0>
-    static void execute(Class &cl, const Extra&... extra) {
-        using Base = typename Class::type;
-        using Alias = typename Class::type_alias;
-        handle cl_type = cl;
-        cl.def("__init__", [cl_type](handle self_, Args... args) {
-                if (self_.get_type().is(cl_type))
-                    new (self_.cast<Base *>()) Base(args...);
-                else
-                    new (self_.cast<Alias *>()) Alias(args...);
-            }, extra...);
-    }
-
-    template <typename Class, typename... Extra,
-              enable_if_t<Class::has_alias &&
-                          !std::is_constructible<typename Class::type, Args...>::value, int> = 0>
-    static void execute(Class &cl, const Extra&... extra) {
-        init_alias<Args...>::execute(cl, extra...);
-    }
-};
-template <typename... Args> struct init_alias {
-    template <typename Class, typename... Extra,
-              enable_if_t<Class::has_alias && std::is_constructible<typename Class::type_alias, Args...>::value, int> = 0>
-    static void execute(Class &cl, const Extra&... extra) {
-        using Alias = typename Class::type_alias;
-        cl.def("__init__", [](Alias *self_, Args... args) { new (self_) Alias(args...); }, extra...);
-    }
-};
 
 
 inline void keep_alive_impl(handle nurse, handle patient) {

tests/test_class.py

@@ -141,20 +141,17 @@ class SubAliased(m.AliasedHasOpNewDelSize):
         d = m.HasOpNewDelBoth()
     assert capture == """
         A new 8
-        A placement-new 8
         B new 4
-        B placement-new 4
         D new 32
-        D placement-new 32
     """
     sz_alias = str(m.AliasedHasOpNewDelSize.size_alias)
     sz_noalias = str(m.AliasedHasOpNewDelSize.size_noalias)
     with capture:
         c = m.AliasedHasOpNewDelSize()
         c2 = SubAliased()
     assert capture == (
-        "C new " + sz_alias + "\nC placement-new " + sz_noalias + "\n" +
-        "C new " + sz_alias + "\nC placement-new " + sz_alias + "\n"
+        "C new " + sz_noalias + "\n" +
+        "C new " + sz_alias + "\n"
     )
 
     with capture:

tests/test_factory_constructors.cpp

@@ -274,8 +274,9 @@ TEST_SUBMODULE(factory_constructors, m) {
         }
         int i;
     };
-    // Workaround for a `py::init<args>` on a class without placement new support
+    // As of 2.2, `py::init<args>` no longer requires placement new
     py::class_<NoPlacementNew>(m, "NoPlacementNew")
+        .def(py::init<int>())
         .def(py::init([]() { return new NoPlacementNew(100); }))
         .def_readwrite("i", &NoPlacementNew::i)
         ;

tests/test_factory_constructors.py

@@ -284,7 +284,19 @@ def get(self):
 
 
 def test_no_placement_new(capture):
-    """Tests a workaround for `py::init<...>` with a class that doesn't support placement new."""
+    """Prior to 2.2, `py::init<...>` relied on the type supporting placement
+    new; this tests a class without placement new support."""
+    with capture:
+        a = m.NoPlacementNew(123)
+
+    found = re.search(r'^operator new called, returning (\d+)\n$', str(capture))
+    assert found
+    assert a.i == 123
+    with capture:
+        del a
+        pytest.gc_collect()
+    assert capture == "operator delete called on " + found.group(1)
+
     with capture:
         b = m.NoPlacementNew()
 

tests/test_multiple_inheritance.py

@@ -73,7 +73,8 @@ def __init__(self, i, j):
     class MI4(MI3, m.Base2):
         def __init__(self, i, j):
             MI3.__init__(self, i, j)
-            # m.Base2 is already initialized (via MI2)
+            # This should be ignored (Base2 is already initialized via MI2):
+            m.Base2.__init__(self, i + 100)
 
     class MI5(m.Base2, B1, m.Base1):
         def __init__(self, i, j):

tests/test_virtual_functions.cpp

@@ -148,6 +148,7 @@ class NCVirtTrampoline : public NCVirt {
 struct Base {
     /* for some reason MSVC2015 can't compile this if the function is pure virtual */
     virtual std::string dispatch() const { return {}; };
+    virtual ~Base() = default;
 };
 
 struct DispatchIssue : Base {
@@ -259,6 +260,7 @@ TEST_SUBMODULE(virtual_functions, m) {
         virtual std::string &str_ref() { return v; }
         virtual A A_value() { return a; }
         virtual A &A_ref() { return a; }
+        virtual ~OverrideTest() = default;
     };
 
     class PyOverrideTest : public OverrideTest {
@@ -311,6 +313,7 @@ public: \
         return say_something(1) + " " + std::to_string(unlucky_number()); \
     }
 A_METHODS
+    virtual ~A_Repeat() = default;
 };
 class B_Repeat : public A_Repeat {
 #define B_METHODS \
@@ -335,7 +338,7 @@ D_METHODS
 };
 
 // Base classes for templated inheritance trampolines.  Identical to the repeat-everything version:
-class A_Tpl { A_METHODS };
+class A_Tpl { A_METHODS; virtual ~A_Tpl() = default; };
 class B_Tpl : public A_Tpl { B_METHODS };
 class C_Tpl : public B_Tpl { C_METHODS };
 class D_Tpl : public C_Tpl { D_METHODS };