[BUG]: __getattr__ and multiple inheritance with smart holders

petersteneteg 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

Hi, I've found an other issue related to #3788 this is in regard to the smart holder branch

The example code below fails with infinite recursive getattr calls.

removing either Base2, Derived2 or the binding of __getattr__ removes the problem.

removing the following lines https://github.com/pybind/pybind11/blob/9d4b4dffce6677a7c9a9af37a45f4436d353f5e1/include/pybind11/detail/smart_holder_type_casters.h#L295-L297 also removes the issue.

Reproducible example code

// Binding code

#include <pybind11/pybind11.h>

namespace py = pybind11;

struct Base1 {int a1{};};
struct Base2 {int a2{};};

struct Base12 : Base1, Base2 {
    virtual ~Base12() = default;
    int foo() const { return 0; }
};

struct Derived1 : Base12 {
    int bar() const { return 1; }
};

struct Derived2 : Base12 {
    int bar() const { return 2; }
};

PYBIND11_MODULE(foo, m) {
    py::class_<Base1>(m, "Base1");
    py::class_<Base2>(m, "Base2");

    py::class_<Base12, Base1, Base2>(m, "Base12")
        .def(py::init<>())
        .def("foo", &Base12::foo)
       .def("__getattr__", [](Base12&, std::string key) {
            return "Base GetAttr: " + key;
        });

    py::class_<Derived1, Base12>(m, "Derived1")
        .def(py::init<>())
        .def("bar", &Derived1::bar);

    py::class_<Derived2, Base12>(m, "Derived2")
        .def(py::init<>())
        .def("bar", &Derived2::bar);
}

// Test code
import foo

d1 = foo.Derived1()
print(f"d1.foo(): {d1.foo()}")
print(f"d1.bar(): {d1.bar()}")
print(f"d1.prop1: {d1.prop1}")

d2 = foo.Derived2()
print(f"d2.foo(): {d2.foo()}")
print(f"d2.bar(): {d2.bar()}")
print(f"d2.prop1: {d2.prop2}")

rwgk commented 2 years ago

Thanks for the test case! TBH, I was afraid something like this could happen, although it's time-consuming to anticipate all edge cases. The test case is super useful. Tagging @wangxf123456, we will look into a fix.

rwgk commented 1 year ago

Hi Peter, FYI, I just merged #4612

It passes testing with your reproducers from #3788 and #3801, but please let us know if you believe #4612 causes different new issues on your end.

pybind / pybind11