[BUG]: PYBIND11_OVERRIDE_IMPL issue with multiple inheritance due to static_cast

Holt59 commented 2 years ago

Required prerequisites

[X] Make sure you've read the documentation. Your issue may be addressed there.
[X] Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
[X] Consider asking first in the Gitter chat room or in a Discussion.

Problem description

When using PYBIND11_OVERRIDE_PURE inside of class with multiple inheritance, e.g.

class A { virtual ~A() {} };
class B { virtual std::string fn() const = 0; virtual ~B() {} }

class PyClass : public A, public B {
public:
    std::string fn() const override {
        PYBIND11_OVERRIDE_PURE(QString, B, fn, );
    }
};

// A is not exposed in Python

py::class_<B,  PyB>(m, "B", py::multiple_inheritance())
    .def(py::init<>()) // should this be init_alias?
    .def("function", &B::fn);

This functions throws a "Tried to call pure virtual function ..." exception when constructing a new B in Python.

Removing this static_cast:

https://github.com/pybind/pybind11/blob/master/include/pybind11/pybind11.h#L2729

...solves the issue.

Is there any reason to have this static_cast here? Is there something I'm missing?

Reproducible example code

#include <iostream>

#include <pybind11/embed.h>
#include <pybind11/pybind11.h>

class A {
public:
    virtual ~A() {}
};

class B {
public:
    virtual std::string fn() const = 0;

    virtual ~B() {}
};

constexpr const char* fmt_s = "{:20}: {}\n";

// switch A and B here fixes the issue since the function is from B
class PyB : public A, public B {
public:
    std::string fn() const override {
        std::cout << "fn(), this           : " << (void*)this << "\n";
        std::cout << "fn(), static_cast<>  : " << (void*)static_cast<const B*>(this)
                  << "\n";
        std::cout << "fn(), dynamic_cast<> : " << (void*)dynamic_cast<const B*>(this)
                  << "\n";

        PYBIND11_OVERRIDE_PURE(std::string, B, fn, );
    }
};

namespace py = pybind11;

PYBIND11_EMBEDDED_MODULE(cpp_module, m) {
    py::class_<B, PyB>(m, "B", py::multiple_inheritance())
        .def(py::init<>())
        .def("fn", &B::fn);
}

int main() {
    py::scoped_interpreter guard{};
    py::exec(R"(
        from cpp_module import B

        class MyB(B):
            def fn(self): return "MyB"

        b = MyB()
    )");
}

Holt59 commented 2 years ago

The solution I found was to have an intermediate class AB that inherits both A and B, and then have PyB inherits AB instead of A and B, this seems to fix the issue.

Skylion007 commented 2 years ago

@Holt59 Could you add a PR adding this test to our test suite? That would make debugging a solution easier. The static_cast is probably to ensure consistent behavior and fix some other edge case. Does replacing it with a dynamic_cast also solve the problem?

Holt59 commented 2 years ago

@Skylion007 I will see if I can add a PR. Replacing with dynamic_cast does not solve the issue, I think the problem is that the py::object is not retrieved properly from the this when this is a B* (and not a PyB*).

Holt59 commented 2 years ago

@Skylion007 PR with test case added here: https://github.com/pybind/pybind11/pull/3915

pybind / pybind11