Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[smart_holder] Change throw_if_uninitialized_or_disowned_holder() to also show C++ type name. #4977

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' suppose you can use f-strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea, but no. We don't have f-strings in C++ (I never thought about that before, not sure how feasible that is), and compared to relying on std::string operator+, the overhead of using the Python C API is not something I'd want to have in an error handling code path like here.

+ "`: 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):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this function name is apparently misspelled

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intentional, see comment at top of this file, which points here:

// The compact 4-character naming matches that in test_class_sh_basic.cpp
// Variable names are intentionally terse, to not distract from the more important C++ type names:
// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const,
// sh = shared_ptr, uq = unique_ptr.

(The original idea was actually to make this code in test_class_sh_basic.cpp more readable.)

# 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