diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 61716ad820..2c4713d5e1 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1392,11 +1392,17 @@ class class_ : public detail::generic_type { 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 (inst->owned) { + throw std::runtime_error( + "reclaim_from_cpp: Python object already owned! Did you forget to explicitly use a " + "py::return_value_policy (e.g. reference or reference internal) when passing back " + "non-owned pointers of the C++ object?"); + } + if (v_h.holder_constructed()) { + throw std::runtime_error("reclaim_from_cpp: Holder already exists - internal error?"); } if (!external_holder_raw) { - throw std::runtime_error("Internal error - not external holder?"); + throw std::runtime_error("reclaim_from_cpp: No external holder - internal error?"); } // Is this valid? handle h(reinterpret_cast(inst)); @@ -1425,7 +1431,7 @@ class class_ : public detail::generic_type { break; } default: { - throw std::runtime_error("Unsupported load type"); + throw std::runtime_error("reclaim_from_cpp: Unsupported load type"); } } inst->owned = true; @@ -1716,6 +1722,9 @@ class class_ : public detail::generic_type { // method? (Rather than a class method?) object del_orig = getattr(h_type, "__del__", none()); + // Set reclaim method. + inst->reclaim_from_cpp = reclaim_from_cpp; + #if defined(PYPY_VERSION) // PyPy will not execute an arbitrarily-added `__del__` method. // Later version of PyPy throw an error if this happens. diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index f7c46339c0..b4e2c195c7 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -462,4 +462,27 @@ TEST_SUBMODULE(smart_ptr, m) { [](std::shared_ptr obj) { return obj != nullptr && obj->value == 10; }); + + // Test passing ownership of registered, but unowned, C++ instances back to + // Python. This happens when a raw pointer is passed first, and then + // ownership is transfered. + struct UniquePtrHeldContainer { + UniquePtrHeldContainer() { + value_.reset(new UniquePtrHeld(10)); + } + UniquePtrHeld* get() const { + return value_.get(); + } + using Ptr = std::unique_ptr; + Ptr reset(Ptr to) { + Ptr from = std::move(value_); + value_ = std::move(to); + return from; + } + std::unique_ptr value_; + }; + py::class_(m, "UniquePtrHeldContainer") + .def(py::init()) + .def("get", &UniquePtrHeldContainer::get, py::return_value_policy::reference_internal) + .def("reset", &UniquePtrHeldContainer::reset); } diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index a79a84a9ac..63d9171b59 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -335,3 +335,17 @@ def test_unique_ptr_overload_fail(): out = m.unique_ptr_overload(obj, m.SecondT()) assert out["obj"] is obj assert out["overload"] == 2 + + +def test_unique_ptr_held_container_from_cpp(): + + def check_reset(obj_new): + c = m.UniquePtrHeldContainer() + obj = c.get() + assert obj is not None + obj_ref = c.reset(obj_new) + assert obj is obj_ref + assert c.get() is obj_new + + check_reset(obj_new=m.UniquePtrHeld(10)) + check_reset(obj_new=None)