From 60526d463650487ca6aa8b6e15d0522df03a4835 Mon Sep 17 00:00:00 2001
From: Jason Rhinelander <jason@imaginary.ca>
Date: Fri, 7 Jul 2017 17:26:14 -0400
Subject: [PATCH] Support `take_ownership` for custom type casters given a
 pointer

This changes the pointer `cast()` in `PYBIND11_TYPE_CASTER` to recognize
the `take_ownership` policy: if casting a pointer with take-ownership,
the `cast()` now recalls `cast()` with a dereferenced rvalue (rather
than the previous code, which was always calling it with a const lvalue
reference), and deletes the pointer after the chained `cast()` is
complete.

This makes code like:

    m.def("f", []() { return new std::vector<int>(100, 1); },
        py::return_value_policy::take_ownership);

do the expected thing by taking over ownership of the returned pointer
(which is deleted once the chained cast completes).
---
 include/pybind11/cast.h               |  6 ++++-
 tests/test_methods_and_attributes.cpp | 35 ++++++++++++++++++++++++++-
 tests/test_methods_and_attributes.py  | 27 +++++++++++++++++++++
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h
index 33dc9f38..f3a05f00 100644
--- a/include/pybind11/cast.h
+++ b/include/pybind11/cast.h
@@ -895,7 +895,11 @@ public:
         template <typename T_, enable_if_t<std::is_same<type, remove_cv_t<T_>>::value, int> = 0> \
         static handle cast(T_ *src, return_value_policy policy, handle parent) { \
             if (!src) return none().release(); \
-            return cast(*src, policy, parent); \
+            if (policy == return_value_policy::take_ownership) { \
+                auto h = cast(std::move(*src), policy, parent); delete src; return h; \
+            } else { \
+                return cast(*src, policy, parent); \
+            } \
         } \
         operator type*() { return &value; } \
         operator type&() { return value; } \
diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp
index e8e1b522..8e0fc2d8 100644
--- a/tests/test_methods_and_attributes.cpp
+++ b/tests/test_methods_and_attributes.cpp
@@ -154,7 +154,28 @@ public:
     }
 
     static handle cast(const ArgAlwaysConverts &, return_value_policy, handle) {
-        return py::none();
+        return py::none().release();
+    }
+};
+}}
+
+// test_custom_caster_destruction
+class DestructionTester {
+public:
+    DestructionTester() { print_default_created(this); }
+    ~DestructionTester() { print_destroyed(this); }
+    DestructionTester(const DestructionTester &) { print_copy_created(this); }
+    DestructionTester(DestructionTester &&) { print_move_created(this); }
+    DestructionTester &operator=(const DestructionTester &) { print_copy_assigned(this); return *this; }
+    DestructionTester &operator=(DestructionTester &&) { print_move_assigned(this); return *this; }
+};
+namespace pybind11 { namespace detail {
+template <> struct type_caster<DestructionTester> {
+    PYBIND11_TYPE_CASTER(DestructionTester, _("DestructionTester"));
+    bool load(handle, bool) { return true; }
+
+    static handle cast(const DestructionTester &, return_value_policy, handle) {
+        return py::bool_(true).release();
     }
 };
 }}
@@ -399,4 +420,16 @@ test_initializer methods_and_attributes([](py::module &m) {
 
     using Adapted = decltype(py::method_adaptor<RegisteredDerived>(&RegisteredDerived::do_nothing));
     static_assert(std::is_same<Adapted, void (RegisteredDerived::*)() const>::value, "");
+
+    // test_custom_caster_destruction
+    // Test that `take_ownership` works on types with a custom type caster when given a pointer
+
+    // default policy: don't take ownership:
+    m.def("custom_caster_no_destroy", []() { static auto *dt = new DestructionTester(); return dt; });
+
+    m.def("custom_caster_destroy", []() { return new DestructionTester(); },
+            py::return_value_policy::take_ownership); // Takes ownership: destroy when finished
+    m.def("custom_caster_destroy_const", []() -> const DestructionTester * { return new DestructionTester(); },
+            py::return_value_policy::take_ownership); // Likewise (const doesn't inhibit destruction)
+    m.def("destruction_tester_cstats", &ConstructorStats::get<DestructionTester>, py::return_value_policy::reference);
 });
diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py
index afe8d286..05573805 100644
--- a/tests/test_methods_and_attributes.py
+++ b/tests/test_methods_and_attributes.py
@@ -477,3 +477,30 @@ def test_unregistered_base_implementations():
     assert a.rw_value_prop == 49
     a.increase_value()
     assert a.ro_value_prop == 1.75
+
+
+def test_custom_caster_destruction():
+    """
+    Tests that returning a pointer to a type that gets converted with a custom type caster gets
+    destroyed when the function has py::return_value_policy::take_ownership policy applied.
+    """
+    import pybind11_tests as m
+
+    cstats = m.destruction_tester_cstats()
+    # This one *doesn't* have take_ownership: the pointer should be used but not destroyed:
+    z = m.custom_caster_no_destroy()
+    assert cstats.alive() == 1 and cstats.default_constructions == 1
+    assert z
+
+    # take_ownership applied: this constructs a new object, casts it, then destroys it:
+    z = m.custom_caster_destroy()
+    assert z
+    assert cstats.default_constructions == 2
+
+    # Same, but with a const pointer return (which should *not* inhibit destruction):
+    z = m.custom_caster_destroy_const()
+    assert z
+    assert cstats.default_constructions == 3
+
+    # Make sure we still only have the original object (from ..._no_destroy()) alive:
+    assert cstats.alive() == 1