Ensure that we can use reference_internal on existing instances.

Author: EricCousineau-TRI

include/pybind11/cast.h

@@ -476,18 +476,50 @@ inline PyThreadState *get_thread_state_unchecked() {
 }
 
 // Forward declarations
+inline bool has_patient(PyObject *nurse, PyObject *patient);
 inline void keep_alive_impl(handle nurse, handle patient);
 inline PyObject *make_new_instance(PyTypeObject *type);
 
-inline bool reclaim_existing_if_needed(
-        instance *inst, const detail::type_info *tinfo, const void *existing_holder) {
-    // Only reclaim if (a) we have an existing holder and (b) if it's a move-only holder.
-    // TODO: Remove `default_holder`, store more descriptive holder information.
-    if (existing_holder && tinfo->default_holder) {
-        // Requesting reclaim from C++.
-        value_and_holder v_h = inst->get_value_and_holder(tinfo);
-        // TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`.
-        tinfo->ownership_info.reclaim(v_h, const_cast<void*>(existing_holder));
+inline void reclaim_instance(
+        instance *inst, const detail::type_info *tinfo, const void *existing_holder = nullptr) {
+    value_and_holder v_h = inst->get_value_and_holder(tinfo);
+    // TODO(eric.cousineau): Add `holder_type_erased` to avoid need for `const_cast`.
+    tinfo->ownership_info.reclaim(v_h, const_cast<void*>(existing_holder));
+}
+
+inline bool reclaim_instance_if_needed(
+        instance *inst, const detail::type_info *tinfo, const void *existing_holder,
+        return_value_policy policy, handle parent) {
+    // TODO(eric.cousineau): Remove `default_holder`, store more descriptive holder information.
+    // Only handle reclaim if it's a move-only holder, and not currently owned.
+    // Let copyable holders do their own thing.
+    if (!tinfo->default_holder || inst->owned)
+        // TODO(eric.cousineau): For shared ptrs, there was a point where the holder was constructed,
+        // but the instance was not owned. What does that mean?
+        return false;
+    bool do_reclaim = false;
+    if (existing_holder) {
+        // This means that we're coming from a holder caster.
+        do_reclaim = true;
+    } else {
+        // Check existing policies.
+        switch (policy) {
+            case return_value_policy::reference_internal: {
+                handle h = handle((PyObject *) inst);
+                if (!has_patient(h.ptr(), parent.ptr()))
+                    keep_alive_impl(h, parent);
+                break;
+            }
+            case return_value_policy::take_ownership: {
+                do_reclaim = true;
+                break;
+            }
+            default:
+                break;
+        }
+    }
+    if (do_reclaim) {
+        reclaim_instance(inst, tinfo, existing_holder);
         return true;
     }
     return false;
@@ -523,7 +555,7 @@ class type_caster_generic {
                 if (instance_type && same_type(*instance_type->cpptype, *tinfo->cpptype)) {
                     // Casting for an already registered type. Return existing reference.
                     instance *inst = it_i->second;
-                    reclaim_existing_if_needed(inst, tinfo, existing_holder);
+                    reclaim_instance_if_needed(inst, tinfo, existing_holder, policy, parent);
                     return handle((PyObject *) inst).inc_ref();
                 }
             }

include/pybind11/detail/class.h

@@ -9,6 +9,8 @@
 
 #pragma once
 
+#include <algorithm>
+
 #include "../attr.h"
 
 NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
@@ -294,6 +296,15 @@ inline void add_patient(PyObject *nurse, PyObject *patient) {
     internals.patients[nurse].push_back(patient);
 }
 
+inline bool has_patient(PyObject *nurse, PyObject *patient) {
+    auto &internals = get_internals();
+    auto instance = reinterpret_cast<detail::instance *>(nurse);
+    if (!instance->has_patients)
+        return false;
+    auto& cur = internals.patients[nurse];
+    return (std::find(cur.begin(), cur.end(), patient) != cur.end());
+}
+
 inline void clear_patients(PyObject *self) {
     auto instance = reinterpret_cast<detail::instance *>(self);
     if (!instance->has_patients)

include/pybind11/pybind11.h

@@ -1087,9 +1087,15 @@ class class_ : public detail::generic_type {
     static void holder_reclaim(detail::value_and_holder& v_h, void* external_holder_raw) {
         // Reclaim from `external_holder` into `v_h.holder<...>()`.
         assert(!v_h.inst->owned && !v_h.holder_constructed() && "Internal error: Object must not be owned");
-        assert(external_holder_raw && "Internal error: External holder must not be null");
-        holder_type& external_holder = *reinterpret_cast<holder_type*>(external_holder_raw);
-        new (&v_h.holder<holder_type>()) holder_type(std::move(external_holder));
+        holder_type& holder = v_h.holder<holder_type>();
+        if (external_holder_raw) {
+            // Take from external holder.
+            holder_type& external_holder = *reinterpret_cast<holder_type*>(external_holder_raw);
+            new (&holder) holder_type(std::move(external_holder));
+        } else {
+            // Construct new holder, using existing value.
+            new (&holder) holder_type(v_h.value_ptr<type>());
+        }
         v_h.set_holder_constructed();
         v_h.inst->owned = true;
         // If this instance is now owend by pybind, release any existing

tests/test_smart_ptr.cpp

@@ -89,8 +89,7 @@ class Container {
         } else {
             cls.def(py::init<Ptr>());
         }
-        // TODO: Figure out why reference_internal does not work???
-        cls.def("get", &Container::get, py::keep_alive<0, 1>()); //py::return_value_policy::reference_internal);
+        cls.def("get", &Container::get, py::return_value_policy::reference_internal);
         cls.def("release", &Container::release);
         cls.def("reset", &Container::reset);
     }