Generalize holder compatibility check for derived classes

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.

Author: rhaschke

include/pybind11/attr.h

@@ -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;
@@ -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;
 
@@ -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);

include/pybind11/cast.h

@@ -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>();
     }
 

include/pybind11/detail/internals.h

@@ -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;
 };

include/pybind11/pybind11.h

@@ -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();
@@ -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);
 

tests/test_class.py

@@ -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),
     )
 

tests/test_smart_ptr.py

@@ -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():