From d2ac3f5c70c76c367a8af6592fc4ac2eb325b253 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Dec 2023 19:29:22 -0800 Subject: [PATCH] [smart_holder] Change `throw_if_uninitialized_or_disowned_holder()` to also show C++ type name. (#4977) * Change `throw_if_uninitialized_or_disowned_holder()` to also show C++ type name. * Fix pre-commit errors. --- .../detail/smart_holder_type_casters.h | 21 ++++++++++++------- tests/test_class_sh_basic.py | 4 +++- tests/test_class_sh_disowning_mi.py | 5 +---- tests/test_class_sh_mi_thunks.py | 6 +----- tests/test_class_sh_property.py | 18 +++++----------- tests/test_class_sh_unique_ptr_member.py | 6 +----- 6 files changed, 24 insertions(+), 36 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index bfe54c847..c5fe9cd05 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -505,7 +505,7 @@ struct smart_holder_type_caster_load { } if (void_ptr == nullptr) { if (have_holder()) { - throw_if_uninitialized_or_disowned_holder(); + throw_if_uninitialized_or_disowned_holder(typeid(T)); void_ptr = holder().template as_raw_ptr_unowned(); } else if (load_impl.loaded_v_h.vh != nullptr) { void_ptr = load_impl.loaded_v_h.value_ptr(); @@ -548,7 +548,7 @@ struct smart_holder_type_caster_load { if (!have_holder()) { return nullptr; } - throw_if_uninitialized_or_disowned_holder(); + throw_if_uninitialized_or_disowned_holder(typeid(T)); holder_type &hld = holder(); hld.ensure_is_not_disowned("loaded_as_shared_ptr"); if (hld.vptr_is_using_noop_deleter) { @@ -612,7 +612,7 @@ struct smart_holder_type_caster_load { if (!have_holder()) { return unique_with_deleter(nullptr, std::unique_ptr()); } - throw_if_uninitialized_or_disowned_holder(); + throw_if_uninitialized_or_disowned_holder(typeid(T)); throw_if_instance_is_currently_owned_by_shared_ptr(); holder().ensure_is_not_disowned(context); holder().template ensure_compatible_rtti_uqp_del(context); @@ -694,17 +694,22 @@ private: holder_type &holder() const { return load_impl.loaded_v_h.holder(); } // have_holder() must be true or this function will fail. - void throw_if_uninitialized_or_disowned_holder() const { + void throw_if_uninitialized_or_disowned_holder(const char *typeid_name) const { + static const std::string missing_value_msg = "Missing value for wrapped C++ type `"; if (!holder().is_populated) { - pybind11_fail("Missing value for wrapped C++ type:" - " Python instance is uninitialized."); + throw value_error(missing_value_msg + clean_type_id(typeid_name) + + "`: Python instance is uninitialized."); } if (!holder().has_pointee()) { - throw value_error("Missing value for wrapped C++ type:" - " Python instance was disowned."); + throw value_error(missing_value_msg + clean_type_id(typeid_name) + + "`: Python instance was disowned."); } } + void throw_if_uninitialized_or_disowned_holder(const std::type_info &type_info) const { + throw_if_uninitialized_or_disowned_holder(type_info.name()); + } + // have_holder() must be true or this function will fail. void throw_if_instance_is_currently_owned_by_shared_ptr() const { auto vptr_gd_ptr = std::get_deleter(holder().vptr); diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 7be0747c2..c7ff82c81 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -109,7 +109,9 @@ def test_pass_unique_ptr_disowns(pass_f, rtrn_f, expected): with pytest.raises(ValueError) as exc_info: pass_f(obj) assert str(exc_info.value) == ( - "Missing value for wrapped C++ type: Python instance was disowned." + "Missing value for wrapped C++ type" + + " `pybind11_tests::class_sh_basic::atyp`:" + + " Python instance was disowned." ) diff --git a/tests/test_class_sh_disowning_mi.py b/tests/test_class_sh_disowning_mi.py index 1a3741321..6fbc1f8e3 100644 --- a/tests/test_class_sh_disowning_mi.py +++ b/tests/test_class_sh_disowning_mi.py @@ -20,10 +20,7 @@ def is_disowned(callable_method): try: callable_method() except ValueError as e: - assert ( # noqa: PT017 - str(e) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) + assert "Python instance was disowned" in str(e) # noqa: PT017 return True return False diff --git a/tests/test_class_sh_mi_thunks.py b/tests/test_class_sh_mi_thunks.py index 7d6e3849a..de2a9196b 100644 --- a/tests/test_class_sh_mi_thunks.py +++ b/tests/test_class_sh_mi_thunks.py @@ -38,12 +38,8 @@ def test_get_vec_size_raw_shared(get_fn, vec_size_fn): def test_get_vec_size_unique(get_fn): obj = get_fn() assert m.vec_size_base0_unique_ptr(obj) == 5 - with pytest.raises(ValueError) as exc_info: + with pytest.raises(ValueError, match="Python instance was disowned"): m.vec_size_base0_unique_ptr(obj) - assert ( - str(exc_info.value) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) def test_get_shared_vec_size_unique(): diff --git a/tests/test_class_sh_property.py b/tests/test_class_sh_property.py index 7142b4ea8..9dc2a1977 100644 --- a/tests/test_class_sh_property.py +++ b/tests/test_class_sh_property.py @@ -9,7 +9,7 @@ from pybind11_tests import class_sh_property as m @pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred") @pytest.mark.parametrize("m_attr", ["m_valu_readonly", "m_valu_readwrite"]) -def test_valu_getter(msg, m_attr): +def test_valu_getter(m_attr): # Reduced from PyCLIF test: # https://github.com/google/clif/blob/c371a6d4b28d25d53a16e6d2a6d97305fb1be25a/clif/testing/python/nested_fields_test.py#L56 outer = m.Outer() @@ -17,15 +17,11 @@ def test_valu_getter(msg, m_attr): assert field.num == -99 with pytest.raises(ValueError) as excinfo: m.DisownOuter(outer) - assert msg(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)." + assert str(excinfo.value) == "Cannot disown use_count != 1 (loaded_as_unique_ptr)." del field m.DisownOuter(outer) - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValueError, match="Python instance was disowned") as excinfo: getattr(outer, m_attr) - assert ( - msg(excinfo.value) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) def test_valu_setter(): @@ -85,18 +81,14 @@ def test_ptr(field_type, num_default, outer_type, m_attr, r_kind): @pytest.mark.parametrize("m_attr_readwrite", ["m_uqmp_readwrite", "m_uqcp_readwrite"]) -def test_uqp(m_attr_readwrite, msg): +def test_uqp(m_attr_readwrite): outer = m.Outer() assert getattr(outer, m_attr_readwrite) is None field_orig = m.Field() field_orig.num = 39 setattr(outer, m_attr_readwrite, field_orig) - with pytest.raises(ValueError) as excinfo: + with pytest.raises(ValueError, match="Python instance was disowned"): _ = field_orig.num - assert ( - msg(excinfo.value) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) field_retr1 = getattr(outer, m_attr_readwrite) assert getattr(outer, m_attr_readwrite) is None assert field_retr1.num == 39 diff --git a/tests/test_class_sh_unique_ptr_member.py b/tests/test_class_sh_unique_ptr_member.py index e369213e0..1a50a57b7 100644 --- a/tests/test_class_sh_unique_ptr_member.py +++ b/tests/test_class_sh_unique_ptr_member.py @@ -16,12 +16,8 @@ def test_pointee_and_ptr_owner(give_up_ownership_via): obj = m.pointee() assert obj.get_int() == 213 owner = m.ptr_owner(obj) - with pytest.raises(ValueError) as exc_info: + with pytest.raises(ValueError, match="Python instance was disowned"): obj.get_int() - assert ( - str(exc_info.value) - == "Missing value for wrapped C++ type: Python instance was disowned." - ) assert owner.is_owner() reclaimed = getattr(owner, give_up_ownership_via)() assert not owner.is_owner()