Motivation for "type does not have a non-default holder type while its base does"?

[BorisSchaeling]

What's the motivation for the if (default_holder != base_info->default_holder) check in add_base()?

I'm asking as I came across this check when I tried to pybind this C++ code:

class Base
{
protected:
  virtual ~Base() = default;
};

class Derived : public Base
{
};

As the Base class has a protected destructor, I used nodelete:

PYBIND11_MODULE(Test, m) {
  class_<Base, std::unique_ptr<Base, nodelete>> base{ m, "Base" };
  class_<Derived, Base> derived{ m, "Derived" };
}

The code compiles but when you try to import the module you get this error message because the if-check fails:

ImportError: generic_type: type "Derived" does not have a non-default holder type while its base "Base" does

Now I could just use for example std::shared_ptr as a holder type for the Derived class to satisfy the condition (as then Derived uses a non-default holder type, too). But I think I better understand what pybind11 is trying to protect me from with this if-check. Or does the if-check make no sense in my use case (as it's actually meant to protect users in another use case)?

[trelau]

I'm also curious about this. I ended up defining a Deleter template to use in the Derived classes like:

template<typename T> struct Deleter { void operator() (T *o) const { delete o; } };

class_<Derived, std::unique_ptr<Derived, Deleter<Derived>>,  Base> derived{ m, "Derived" };

Not sure if this is the best approach, but it seems to "work" on the Python side.

[jagerman]

The motivation is to catch a fairly common case where someone accidentally tries to use a std::shared_ptr<Derived> with a std::unique_ptr<Base> (or vice versa)—we can't cast a derived holder to a base holder in such a case, which breaks our ability to handle inheritance.

I think this particular case is fine, though: we ought to fix the check to recognize std::unique_ptr<T, D> as a default holder (even if the D doesn't match between base and derived holders). @trelau's workaround just basically makes both of them look like non-default holders, which ends up passing the check. (You could still do problematic things, like mixing a unique ptr and shared ptr holders: we just won't catch them in such a case.)

[BorisSchaeling]

Thanks! I played around with pybind11 to see what the actual problem is. I temporarily removed the if-check in pybind11, used a std::shared_ptr as a holder type for PolymorphicDog and called pet_store2 (from the example in the pybind11 documentation) - and got a segmentation fault.

From what I see this is happening under the hood: type_caster_base::src_and_type() can detect the dynamic type in a pointer. For a std::unique_ptr<PolymorphicPet> with a PolymorphicDog object type_caster_base::src_and_type() returns the type PolymorphicDog. type_caster_generic::cast() creates a new PolymorphicDog object and initializes the holder (which is a std::shared_ptr in my example) via class_::init_holder_from_existing(). Here the existing holder type - which is a std::unique_ptr<PolymorphicPet> - is reinterpret_cast to a std::shared_ptr<PolymorphicDog>. That's the actual problem. The copy-constructor of std::shared_ptr<PolymorphicDog> is called, and I get the segmentation fault.

Could we replace reinterpret_cast in class_::init_holder_from_existing() with a factory function - something which has to initialize a holder_type from another holder_type? There could be default implementations to initialize a std::unique_ptr from another std::unique_ptr and a std::shared_ptr from another std::shared_ptr. If someone mixes up holder types and tries to initialize a std::shared_ptr from a std::unique_ptr, it would be a compile error? The idea would be to replace the if-check in type_record::add_base() and the runtime error with a compile error and make the code extensible (you could define your own factory functions if required).

Am I'm missing anything? I'm not sure how easily the from-holder_type can be passed through all the way to the class_::init_holder_from_existing() function. But I could give it a try and see what I can come up with.

[jagerman]

I think the main issue is that unique_ptr isn't initializable from another unique_ptr. (That is, it's move constructible, but not copy constructible).

[jagerman]

But yes, generally compile time errors are preferable to run-time errors if you can figure out a way to make it work (assuming that doesn't incur noticeable overhead in the normal, working case).

[BorisSchaeling]

I haven't found a way to easily pass the from-holder type all the way through to class_::init_holder_from_existing(). If I take this as an example:

m.def("pet_store2", []() { return std::unique_ptr<PolymorphicPet>(new PolymorphicDog); });

Assuming that PolymorphicDog uses std::shared_ptr as a holder type, we would like a type-safe conversion from std::unique_ptr<PolymorphicPet> to std::shared_ptr<PolymorphicDog>. If I step through the code to see how the actual conversion is implemented, I see that detail::move_only_holder_caster knows the from-holder type (here std::unique_ptr<PolymorphicPet>) and class_::init_holder_from_existing() knows the to-holder type (here std::shared_ptr<PolymorphicDog>). But there is this call inbetween:

tinfo->init_instance(wrapper, existing_holder);

tinfo->init_instance is a function pointer whose type is void (*init_instance)(instance *, const void *). Here the from-holder type is lost, and class_::init_holder_from_existing() doesn't know what this void* pointer really is (thus doing a reinterpret_cast hoping that the void* pointer refers to the same holder type as the to-holder type).

That said I don't know how to replace the runtime error with a compile time error given the current pybind11 implementation (which I assume exists for a good reason). The idea of using a compile time error is maybe far fetched anyway given that the conversion happens at runtime (we know by looking at the code that std::unique_ptr<PolymorphicPet> should be converted to a std::shared_ptr<PolymorphicDog>; pybind11 would need to make that conclusion somehow from class definitions, base classes and holder types and set up conversion functions for every base/derived class pair).

That being said: If there was an option (like a preprocessor macro) to disable automatic upcasting, would we still need the if (default_holder != base_info->default_holder) check in add_base()? In the example above std::unique_ptr<PolymorphicPet> would remain a std::unique_ptr<PolymorphicPet> - maybe not as convenient as the automatic conversion to std::shared_ptr<PolymorphicDog>. But it could still be viable alternative for those who can live with this restriction?

[BorisSchaeling]

I came across #1138 where someone else seems to have done some work already to improve holder type conversions.

[nadir121]

Hi,
I am having the same issue, i just want to work with python already i am a noob at C++ but this issue is stopping me so how do i work around it
i have these classes:

class command_transfer
        {
        public:
            virtual std::vector<uint8_t> send_receive(
                const std::vector<uint8_t>& data,
                int timeout_ms = 5000,
                bool require_response = true) = 0;

			virtual ~command_transfer() {}//= default;
        };
class usb_device : public command_transfer
        {
        public:
		
        };

and

py::class_<platform::command_transfer> command_transfer(m, "command_transfer");
  py::class_<platform::usb_device, platform::command_transfer, std::shared_ptr<platform::usb_device>> 
  usb_device(m, "usb_device");

[jagerman]

@nadir121 - your case is exactly what this check is designed to catch: you just need to make sure that both your classes are using std::shared_ptr holders so that pybind can up/downcast as needed. It's just a matter of adding std::shared_ptr<platform::command_transfer> to your command_transfer definition template:

    py::class_<platform::command_transfer, std::shared_ptr<platform::command_transfer>>
    //                                Add: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        command_transfer(m, "command_transfer");

[nadir121]

@jagerman Thanks, i managed to get it to work but by making the usb_device a unique_ptr, it doesn't matter since my device is not a usb device so i won't use that portion of the code, that code was written by intel btw ¯_(ツ)_/¯

[jagerman]

Both being a unique_ptr works too (and in that case you can just leave off the holder--unique_ptr is the default holder type). The important thing is that the type of holder matches.

[trelau]

Is there any minimal solution (or real need) to fix the case of std::unique_ptr<T, D> not being recognized as a default holder because the D may not match? I know it would clean up my code a little, but as I outlined earlier I have a workaround if this is not really a needed or desired fix.

[jagerman]

@trelau - I think that it would suffice to change pybind11.h from:

        record.default_holder = std::is_same<holder_type, std::unique_ptr<type>>::value;

to:

        record.default_holder = detail::is_instantiation<std::unique_ptr, holder_type>::value;

if you want to give it a try and see if it works.

(Edit: is_instantiation is better here than is_template_base_of; edit 2: added missing ::value).

[trelau]

@jagerman cool and thanks, I'll give it a shot.

[trelau]

@jagerman PR submitted and new tests added #1353. You solution seems to work perfectly.

[bstaletic]

This discussion seems resolved.