diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 283ca36f..7d485f63 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -998,7 +998,7 @@ struct copyable_holder_caster< pybind11_fail("Passing `std::shared_ptr *` from Python to C++ is not supported " "(inherently unsafe)."); } - return std::addressof(shared_ptr_holder); + return std::addressof(shared_ptr_storage); } explicit operator std::shared_ptr &() { @@ -1011,9 +1011,9 @@ struct copyable_holder_caster< throw cast_error("Unowned pointer from a void pointer capsule cannot be converted " "to a std::shared_ptr."); } - shared_ptr_holder = sh_load_helper.load_as_shared_ptr(value); + shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); } - return shared_ptr_holder; + return shared_ptr_storage; } static handle @@ -1059,7 +1059,7 @@ struct copyable_holder_caster< } if (v_h.holder_constructed()) { value = v_h.value_ptr(); - shared_ptr_holder = v_h.template holder>(); + shared_ptr_storage = v_h.template holder>(); return; } throw cast_error("Unable to cast from non-held to held instance (T& to Holder) " @@ -1088,8 +1088,8 @@ struct copyable_holder_caster< if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { sh_load_helper.loaded_v_h = sub_caster.sh_load_helper.loaded_v_h; } else { - shared_ptr_holder - = std::shared_ptr(sub_caster.shared_ptr_holder, (type *) value); + shared_ptr_storage + = std::shared_ptr(sub_caster.shared_ptr_storage, (type *) value); } return true; } @@ -1113,8 +1113,8 @@ struct copyable_holder_caster< return false; } - std::shared_ptr shared_ptr_holder; smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl + std::shared_ptr shared_ptr_storage; bool from_direct_conversion = false; bool from_as_void_ptr_capsule = false; }; @@ -1191,7 +1191,7 @@ struct move_only_holder_caster< policy = return_value_policy::reference_internal; } if (policy != return_value_policy::reference_internal) { - throw cast_error("Invalid return_value_policy for unique_ptr&"); + throw cast_error("Invalid return_value_policy for const unique_ptr&"); } return type_caster_base::cast(src.get(), policy, parent); } @@ -1218,8 +1218,14 @@ struct move_only_holder_caster< + clean_type_id(typeinfo->cpptype->name()) + ")"); } - template - using cast_op_type = std::unique_ptr; + template + using cast_op_type + = conditional_t::type, + const std::unique_ptr &>::value + || std::is_same::type, + const std::unique_ptr &>::value, + const std::unique_ptr &, + std::unique_ptr>; explicit operator std::unique_ptr() { if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { @@ -1236,6 +1242,28 @@ struct move_only_holder_caster< pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); } + explicit operator const std::unique_ptr &() { + if (typeinfo->holder_enum_v == detail::holder_enum_t::smart_holder) { + // Get shared_ptr to ensure that the Python object is not disowned elsewhere. + shared_ptr_storage = sh_load_helper.load_as_shared_ptr(value); + // Build a temporary unique_ptr that is meant to never expire. + unique_ptr_storage = std::shared_ptr>( + new std::unique_ptr{ + sh_load_helper.template load_as_const_unique_ptr( + shared_ptr_storage.get())}, + [](std::unique_ptr *ptr) { + if (!ptr) { + pybind11_fail("FATAL: `const std::unique_ptr &` was disowned " + "(EXPECT UNDEFINED BEHAVIOR)."); + } + (void) ptr->release(); + delete ptr; + }); + return *unique_ptr_storage; + } + pybind11_fail("Expected to be UNREACHABLE: " __FILE__ ":" PYBIND11_TOSTRING(__LINE__)); + } + bool try_implicit_casts(handle src, bool convert) { for (auto &cast : typeinfo->implicit_casts) { move_only_holder_caster sub_caster(*cast.first); @@ -1270,6 +1298,8 @@ struct move_only_holder_caster< } smart_holder_type_caster_support::load_helper> sh_load_helper; // Const2Mutbl + std::shared_ptr shared_ptr_storage; // Serves as a pseudo lock. + std::shared_ptr> unique_ptr_storage; bool from_direct_conversion = false; bool from_as_void_ptr_capsule = false; }; diff --git a/include/pybind11/detail/struct_smart_holder.h b/include/pybind11/detail/struct_smart_holder.h index ccd28971..b1e24d7b 100644 --- a/include/pybind11/detail/struct_smart_holder.h +++ b/include/pybind11/detail/struct_smart_holder.h @@ -231,6 +231,24 @@ struct smart_holder { vptr_del_ptr->armed_flag = armed_flag; } + // Caller is responsible for precondition: ensure_compatible_rtti_uqp_del() must succeed. + template + std::unique_ptr extract_deleter(const char *context) const { + const auto *gd = std::get_deleter(vptr); + if (gd && gd->use_del_fun) { + const auto &custom_deleter_ptr = gd->del_fun.template target>(); + if (custom_deleter_ptr == nullptr) { + throw std::runtime_error( + std::string("smart_holder::extract_deleter() precondition failure (") + context + + ")."); + } + static_assert(std::is_copy_constructible::value, + "Required for compatibility with smart_holder functionality."); + return std::unique_ptr(new D(custom_deleter_ptr->deleter)); + } + return nullptr; + } + static smart_holder from_raw_ptr_unowned(void *raw_ptr) { smart_holder hld; hld.vptr.reset(raw_ptr, [](void *) {}); diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index af92dc11..db6ad8f6 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -797,22 +797,7 @@ struct load_helper : value_and_holder_helper { "instance cannot safely be transferred to C++."); } - // Temporary variable to store the extracted deleter in. - std::unique_ptr extracted_deleter; - - auto *gd = std::get_deleter(holder().vptr); - if (gd && gd->use_del_fun) { // Note the ensure_compatible_rtti_uqp_del() call above. - // In struct_smart_holder, a custom deleter is always stored in a guarded delete. - // The guarded delete's std::function actually points at the - // custom_deleter type, so we can verify it is of the custom deleter type and - // finally extract its deleter. - using custom_deleter_D = pybindit::memory::custom_deleter; - const auto &custom_deleter_ptr = gd->del_fun.template target(); - assert(custom_deleter_ptr != nullptr); - // Now that we have confirmed the type of the deleter matches the desired return - // value we can extract the function. - extracted_deleter = std::unique_ptr(new D(std::move(custom_deleter_ptr->deleter))); - } + std::unique_ptr extracted_deleter = holder().template extract_deleter(context); // Critical transfer-of-ownership section. This must stay together. if (self_life_support != nullptr) { @@ -832,6 +817,19 @@ struct load_helper : value_and_holder_helper { return result; } + + // This assumes load_as_shared_ptr succeeded(), and the returned shared_ptr is still alive. + // The returned unique_ptr is meant to never expire (the behavior is undefined otherwise). + template + std::unique_ptr + load_as_const_unique_ptr(T *raw_type_ptr, const char *context = "load_as_const_unique_ptr") { + if (!have_holder()) { + return unique_with_deleter(nullptr, std::unique_ptr()); + } + holder().template ensure_compatible_rtti_uqp_del(context); + return unique_with_deleter( + raw_type_ptr, std::move(holder().template extract_deleter(context))); + } }; PYBIND11_NAMESPACE_END(smart_holder_type_caster_support) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 9602387b..ac19a750 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -120,6 +120,17 @@ std::string get_mtxt(atyp const &obj) { return obj.mtxt; } std::ptrdiff_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } std::unique_ptr unique_ptr_roundtrip(std::unique_ptr obj) { return obj; } + +std::string pass_unique_ptr_cref(const std::unique_ptr &obj) { return obj->mtxt; } + +const std::unique_ptr &rtrn_unique_ptr_cref(const std::string &mtxt) { + static std::unique_ptr obj{new atyp{"static_ctor_arg"}}; + if (!mtxt.empty()) { + obj->mtxt = mtxt; + } + return obj; +} + const std::unique_ptr &unique_ptr_cref_roundtrip(const std::unique_ptr &obj) { return obj; } @@ -217,6 +228,9 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("get_ptr", get_ptr); // pass_cref m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp + + m.def("pass_unique_ptr_cref", pass_unique_ptr_cref); + m.def("rtrn_unique_ptr_cref", rtrn_unique_ptr_cref); m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); py::classh(m, "SharedPtrStash") diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 87f1f8f0..7db7d31b 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -151,19 +151,31 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): id_orig = id_rtrn -# This currently fails, because a unique_ptr is always loaded by value -# due to pybind11/detail/smart_holder_type_casters.h:689 -# I think, we need to provide more cast operators. -@pytest.mark.skip() -def test_unique_ptr_cref_roundtrip(): +def test_pass_unique_ptr_cref(): + obj = m.atyp("ctor_arg") + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.get_mtxt(obj)) + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.pass_unique_ptr_cref(obj)) + assert re.match("ctor_arg(_MvCtor)*_MvCtor", m.get_mtxt(obj)) + + +def test_rtrn_unique_ptr_cref(): + obj0 = m.rtrn_unique_ptr_cref("") + assert m.get_mtxt(obj0) == "static_ctor_arg" + obj1 = m.rtrn_unique_ptr_cref("passed_mtxt_1") + assert m.get_mtxt(obj1) == "passed_mtxt_1" + assert m.get_mtxt(obj0) == "passed_mtxt_1" + assert obj0 is obj1 + + +def test_unique_ptr_cref_roundtrip(num_round_trips=1000): + # Multiple roundtrips to stress-test implementation. orig = m.atyp("passenger") - id_orig = id(orig) mtxt_orig = m.get_mtxt(orig) - - recycled = m.unique_ptr_cref_roundtrip(orig) - assert m.get_mtxt(orig) == mtxt_orig - assert m.get_mtxt(recycled) == mtxt_orig - assert id(recycled) == id_orig + recycled = orig + for _ in range(num_round_trips): + recycled = m.unique_ptr_cref_roundtrip(recycled) + assert recycled is orig + assert m.get_mtxt(recycled) == mtxt_orig @pytest.mark.parametrize( diff --git a/tests/test_class_sh_trampoline_shared_from_this.cpp b/tests/test_class_sh_trampoline_shared_from_this.cpp index e5d688af..9c2e4ec7 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.cpp +++ b/tests/test_class_sh_trampoline_shared_from_this.cpp @@ -87,7 +87,12 @@ long pass_shared_ptr(const std::shared_ptr &obj) { return sft.use_count(); } -void pass_unique_ptr(const std::unique_ptr &) {} +std::string pass_unique_ptr_cref(const std::unique_ptr &obj) { + return obj ? obj->history : ""; +} +void pass_unique_ptr_rref(std::unique_ptr &&) { + throw std::runtime_error("Expected to not be reached."); +} Sft *make_pure_cpp_sft_raw_ptr(const std::string &history_seed) { return new Sft{history_seed}; } @@ -135,7 +140,8 @@ TEST_SUBMODULE(class_sh_trampoline_shared_from_this, m) { m.def("use_count", use_count); m.def("pass_shared_ptr", pass_shared_ptr); - m.def("pass_unique_ptr", pass_unique_ptr); + m.def("pass_unique_ptr_cref", pass_unique_ptr_cref); + m.def("pass_unique_ptr_rref", pass_unique_ptr_rref); m.def("make_pure_cpp_sft_raw_ptr", make_pure_cpp_sft_raw_ptr); m.def("make_pure_cpp_sft_unq_ptr", make_pure_cpp_sft_unq_ptr); m.def("make_pure_cpp_sft_shd_ptr", make_pure_cpp_sft_shd_ptr); diff --git a/tests/test_class_sh_trampoline_shared_from_this.py b/tests/test_class_sh_trampoline_shared_from_this.py index a074b20a..7c5ee0e8 100644 --- a/tests/test_class_sh_trampoline_shared_from_this.py +++ b/tests/test_class_sh_trampoline_shared_from_this.py @@ -137,11 +137,14 @@ def test_pass_released_shared_ptr_as_unique_ptr(): obj = PySft("PySft") stash1 = m.SftSharedPtrStash(1) stash1.Add(obj) # Releases shared_ptr to C++. + assert m.pass_unique_ptr_cref(obj) == "PySft_Stash1Add" + assert obj.history == "PySft_Stash1Add" with pytest.raises(ValueError) as exc_info: - m.pass_unique_ptr(obj) + m.pass_unique_ptr_rref(obj) assert str(exc_info.value) == ( "Python instance is currently owned by a std::shared_ptr." ) + assert obj.history == "PySft_Stash1Add" @pytest.mark.parametrize(