From e57a3db72e020c450b72ba5ed68d42260e79a019 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 8 Nov 2020 21:36:38 +0100 Subject: [PATCH] fixup! fixup! Replace global map holders_seen with local holder_type in registered type_info Runtime costs of checking holder compatibility for function return types during can be limited to definition time (instead of calling time), if we require types to be registered before defining a function. This allows to lookup the type_info record and validate the holder type as expected. As we cannot call functions with an unregistered return type anyway, I think this is a good compromise. --- include/pybind11/cast.h | 14 +++++++------- include/pybind11/pybind11.h | 7 ++++++- tests/test_smart_ptr.cpp | 10 +++++++++- tests/test_smart_ptr.py | 13 ++++++++++--- 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 13b99e267b..121dcfd478 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1492,13 +1492,13 @@ struct holder_helper { }; template -void check_for_holder_mismatch_impl() { +bool check_for_holder_mismatch_impl(bool throw_if_missing = true) { using iholder = intrinsic_t; using base_type = decltype(*holder_helper::get(std::declval())); auto &holder_typeinfo = typeid(iholder); - auto base_info = detail::get_type_info(typeid(base_type)); + auto base_info = detail::get_type_info(typeid(base_type), throw_if_missing); if (!base_info) - return; // Don't complain if we see this type the first time + return false; // Return false if the type is not yet registered auto debug = type_id(); if (!same_type(*base_info->holder_type, holder_typeinfo)) { @@ -1512,6 +1512,7 @@ void check_for_holder_mismatch_impl() { " was declared using holder type " + holder_name); #endif } + return true; } /// Type caster for holder types like std::shared_ptr, etc. @@ -1538,7 +1539,6 @@ struct copyable_holder_caster : public type_caster_base { explicit operator holder_type&() { return holder; } static handle cast(const holder_type &src, return_value_policy, handle) { - check_for_holder_mismatch_impl(); const auto *ptr = holder_helper::get(src); return type_caster_base::cast_holder(ptr, &src); } @@ -1635,10 +1635,10 @@ template using is_holder = any_of< is_template_base_of>>; template -void check_for_holder_mismatch(enable_if_t::value, int> = 0) {} +bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { return true; } template -void check_for_holder_mismatch(enable_if_t::value, int> = 0) { - check_for_holder_mismatch_impl(); +bool check_for_holder_mismatch(enable_if_t::value, int> = 0) { + return check_for_holder_mismatch_impl(false); } template struct handle_type_name { static constexpr auto name = _(); }; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index d511cd1ee2..45b59ad5b5 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -158,7 +158,12 @@ class cpp_function : public function { "The number of argument annotations does not match the number of function arguments"); // Fail if the type was previously registered with a different holder type - detail::check_for_holder_mismatch(); + if (!detail::check_for_holder_mismatch()) { + // If Return type was not yet registered, check_for_holder_mismatch() returns false w/o raising + std::string tname(typeid(Return).name()); + detail::clean_type_id(tname); + pybind11_fail("Cannot register function with not yet registered return type \"" + tname + "\""); + } /* Dispatch code which converts function arguments and performs the actual function call */ rec->impl = [](function_call &call) -> handle { diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index 6cc9aedcad..eacf572a32 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -377,7 +377,15 @@ TEST_SUBMODULE(smart_ptr, m) { // Fails: the class was already registered with a shared_ptr holder m.def("bad1", []() { return std::unique_ptr(new HeldByShared()); }); }); - m.def("return_shared", []() { return std::make_shared(); }); + m.def("register_mismatch_class", [](py::module m) { + // Fails: the class was already registered with a shared_ptr holder + py::class_>(m, "bad"); + }); + + m.def("register_return_shared", [](py::module m) { + // Fails if HeldByUnique is not yet registered or, if registered, due to mismatching holder + m.def("return_shared", []() { return std::make_shared(); }); + }); m.def("register_HeldByUnique", [](py::module m) { py::class_(m, "HeldByUnique"); }); diff --git a/tests/test_smart_ptr.py b/tests/test_smart_ptr.py index 5142de8a85..edf1e59623 100644 --- a/tests/test_smart_ptr.py +++ b/tests/test_smart_ptr.py @@ -312,12 +312,19 @@ def test_holder_mismatch(): with pytest.raises(RuntimeError) as excinfo: m.register_mismatch_return(m) assert "Mismatched holders detected" in str(excinfo) + with pytest.raises(RuntimeError) as excinfo: + m.register_mismatch_class(m) + assert "is already registered" in str(excinfo) + + with pytest.raises(RuntimeError) as excinfo: + m.register_return_shared(m) + expected_error = "Cannot register function with not yet registered return type" + assert expected_error in str(excinfo) - with pytest.raises(TypeError) as excinfo: - m.return_shared() # calling a function returning an unregistered type m.register_HeldByUnique(m) # register the type + with pytest.raises(RuntimeError) as excinfo: - m.return_shared() # calling a function returning a mismatching holder type + m.register_return_shared(m) assert "Mismatched holders detected" in str(excinfo) with pytest.raises(RuntimeError) as excinfo: