Skip to content

Commit

Permalink
Generalize holder compatibility check for derived classes
Browse files Browse the repository at this point in the history
A derived class needs to use the same holder type as its base class(es).
So far, the check was constrained to the default holder vs. custom holder types.
Thus, we replace the simple check (based on the default_holder flag) with
a more elaborate one, comparing holder type names.
  • Loading branch information
rhaschke committed Nov 6, 2020
1 parent 502bf24 commit 7852e7d
Show file tree
Hide file tree
Showing 6 changed files with 29 additions and 25 deletions.
35 changes: 24 additions & 11 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ struct function_record {
struct type_record {
PYBIND11_NOINLINE type_record()
: multiple_inheritance(false), dynamic_attr(false), buffer_protocol(false),
default_holder(true), module_local(false), is_final(false) { }
module_local(false), is_final(false) { }

/// Handle to the parent scope
handle scope;
Expand Down Expand Up @@ -271,9 +271,6 @@ struct type_record {
/// Does the class implement the buffer protocol?
bool buffer_protocol : 1;

/// Is the default (unique_ptr) holder type used?
bool default_holder : 1;

/// Is the class definition local to the module shared object?
bool module_local : 1;

Expand All @@ -289,13 +286,29 @@ struct type_record {
"\" referenced unknown base type \"" + tname + "\"");
}

if (default_holder != base_info->default_holder) {
std::string tname(base.name());
detail::clean_type_id(tname);
pybind11_fail("generic_type: type \"" + std::string(name) + "\" " +
(default_holder ? "does not have" : "has") +
" a non-default holder type while its base \"" + tname + "\" " +
(base_info->default_holder ? "does not" : "does"));
// Check for holder compatibility
// We cannot simply check for same_type(*holder_type, *base_info->holder_type)
// as the typeids naturally differ as the base type differs from this type
auto clean_holder_name = [](const std::type_info* holder_type, const std::type_info* base_type) -> std::string {
std::string base_name(base_type->name());
detail::clean_type_id(base_name);
std::string holder_name(holder_type->name());
detail::clean_type_id(holder_name);
// replace all occurences of base_name within holder_name with T
size_t start_pos = 0;
while((start_pos = holder_name.find(base_name, start_pos)) != std::string::npos) {
holder_name.replace(start_pos, base_name.length(), "T");
start_pos += 1;
}
return holder_name;
};
std::string holder_name = clean_holder_name(holder_type, this->type);
std::string base_holder_name = clean_holder_name(base_info->holder_type, base_info->cpptype);
if (holder_name != base_holder_name) {
std::string base_name(base.name());
detail::clean_type_id(base_name);
pybind11_fail("generic_type: type \"" + std::string(name) +
"\" uses different holder than its base \"" + base_name + "\" (" + base_holder_name + " vs " + holder_name + ")");
}

bases.append((PyObject *) base_info->type);
Expand Down
2 changes: 0 additions & 2 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1546,8 +1546,6 @@ struct copyable_holder_caster : public type_caster_base<type> {
protected:
friend class type_caster_generic;
void check_holder_compat() {
if (typeinfo->default_holder)
throw cast_error("Unable to load a custom holder type from a default-holder instance");
check_for_holder_mismatch_impl<holder_type>();
}

Expand Down
2 changes: 0 additions & 2 deletions include/pybind11/detail/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,6 @@ struct type_info {
bool simple_type : 1;
/* True if there is no multiple inheritance in this type's inheritance tree */
bool simple_ancestors : 1;
/* for base vs derived holder_type checks */
bool default_holder : 1;
/* true if this is a type registered with py::module_local */
bool module_local : 1;
};
Expand Down
2 changes: 0 additions & 2 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -1054,7 +1054,6 @@ class generic_type : public object {
tinfo->dealloc = rec.dealloc;
tinfo->simple_type = true;
tinfo->simple_ancestors = true;
tinfo->default_holder = rec.default_holder;
tinfo->module_local = rec.module_local;

auto &internals = get_internals();
Expand Down Expand Up @@ -1235,7 +1234,6 @@ class class_ : public detail::generic_type {
record.holder_size = sizeof(holder_type);
record.init_instance = init_instance;
record.dealloc = dealloc;
record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;

set_operator_new<type>(&record);

Expand Down
8 changes: 4 additions & 4 deletions tests/test_class.py
Original file line number Diff line number Diff line change
Expand Up @@ -219,16 +219,16 @@ def test_mismatched_holder():
with pytest.raises(RuntimeError) as excinfo:
m.mismatched_holder_1()
assert re.match(
'generic_type: type ".*MismatchDerived1" does not have a non-default '
'holder type while its base ".*MismatchBase1" does',
'generic_type: type ".*MismatchDerived1" uses different holder '
'than its base ".*MismatchBase1"',
str(excinfo.value),
)

with pytest.raises(RuntimeError) as excinfo:
m.mismatched_holder_2()
assert re.match(
'generic_type: type ".*MismatchDerived2" has a non-default holder type '
'while its base ".*MismatchBase2" does not',
'generic_type: type ".*MismatchDerived2" uses different holder '
'than its base ".*MismatchBase2"',
str(excinfo.value),
)

Expand Down
5 changes: 1 addition & 4 deletions tests/test_smart_ptr.py
Original file line number Diff line number Diff line change
Expand Up @@ -294,10 +294,7 @@ def test_smart_ptr_from_default():
instance = m.HeldByDefaultHolder()
with pytest.raises(RuntimeError) as excinfo:
m.HeldByDefaultHolder.load_shared_ptr(instance)
assert (
"Unable to load a custom holder type from a "
"default-holder instance" in str(excinfo.value)
)
assert "Mismatched holders detected" in str(excinfo.value)


def test_shared_ptr_gc():
Expand Down

0 comments on commit 7852e7d

Please sign in to comment.