Skip to content

Commit

Permalink
[smart_holder] Change throw_if_uninitialized_or_disowned_holder() t…
Browse files Browse the repository at this point in the history
…o also show C++ type name. (#4977)

* Change `throw_if_uninitialized_or_disowned_holder()` to also show C++ type name.

* Fix pre-commit errors.
  • Loading branch information
Ralf W. Grosse-Kunstleve committed Dec 16, 2023
1 parent feed8b1 commit d2ac3f5
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 36 deletions.
21 changes: 13 additions & 8 deletions include/pybind11/detail/smart_holder_type_casters.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>();
} else if (load_impl.loaded_v_h.vh != nullptr) {
void_ptr = load_impl.loaded_v_h.value_ptr();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -612,7 +612,7 @@ struct smart_holder_type_caster_load {
if (!have_holder()) {
return unique_with_deleter<T, D>(nullptr, std::unique_ptr<D>());
}
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<T, D>(context);
Expand Down Expand Up @@ -694,17 +694,22 @@ struct smart_holder_type_caster_load {
holder_type &holder() const { return load_impl.loaded_v_h.holder<holder_type>(); }

// 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<pybindit::memory::guarded_delete>(holder().vptr);
Expand Down
4 changes: 3 additions & 1 deletion tests/test_class_sh_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
)


Expand Down
5 changes: 1 addition & 4 deletions tests/test_class_sh_disowning_mi.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 1 addition & 5 deletions tests/test_class_sh_mi_thunks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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():
Expand Down
18 changes: 5 additions & 13 deletions tests/test_class_sh_property.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,19 @@

@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()
field = getattr(outer, 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():
Expand Down Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions tests/test_class_sh_unique_ptr_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit d2ac3f5

Please sign in to comment.