pybind11 icon indicating copy to clipboard operation
pybind11 copied to clipboard

[BUG]: __getattr__ and base class order

Open petersteneteg opened this issue 3 years ago • 2 comments

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

In the binding code below __getattr__ will only be found in the derived class if we do: py::class_<Derived, Base, OtherBase>(m, "Derived") if we instead do: py::class_<Derived, OtherBase, Base>(m, "Derived") note the order of the base classes switched, the bound __getattr__ will never be called.

From what I understand from the documentation (https://pybind11.readthedocs.io/en/latest/advanced/classes.html#multiple-inheritance) the order of the base classes should not matter?

This is tested with the latest master as of writing.

Reproducible example code

// binding code
#include <pybind11/pybind11.h>

namespace py = pybind11;

struct Base {
    virtual ~Base() = default;
    int base() const { return 0; }
};
struct OtherBase {
    int other() const { return 3; }
};
struct Derived : Base, OtherBase{
    int id() const { return 2; }
};

PYBIND11_MODULE(bar, m) {
    py::class_<Base>(m, "Base")
        .def(py::init<>())
        .def("base", &Base::base)
        .def("__getattr__", [](Base&, std::string key) {
            return "Base GetAttr: " + key;
        });

    py::class_<OtherBase>(m, "OtherBase")
        .def("other", &OtherBase::other);

    py::class_<Derived, OtherBase, Base>(m, "Derived")
        .def(py::init<>())
        .def("id", &Derived::id);
}

// test code
import bar

d = bar.Derived()
print(f"d.base(): {d.base()}")
print(f"d.other(): {d.other()}")
print(f"d.id(): {d.id()}")
print(f"d.prop: {d.prop}") # this will fail (if Base is not the first base class)

petersteneteg avatar Mar 15 '22 21:03 petersteneteg

The order absolutely does matter is it affect Python's method resolution order (MRO) (which could be the case here). However, this could be related to two more subtle issues:

First, it is searched in the current class. If not found, the search moves to parent classes. This is left-to-right, depth-first.

Now the issue, is I am not sure if we override getattr implicitly somewhere in our implementation the base class for PyBind11 @henryiii @rwgk any ideas if we do? dir(PyBind11_Generated_Object) seems to imply that we so I don't believe this is a bug since getattr is defined on the first base class.

However, I just checked in CPython with this example and this does not match CPython's behavior as that could be the cause of the confusion. In the case below one of the bases does not have getattr defined which is why it works.

class Base:
  def __getattr__(self, name):
    return "Base" + name

class OtherBase:
  pass

class A(Base,OtherBase):
  pass

class B(OtherBase, Base):
  pass

print(A().other)
print(B().other)

Still we should probably improve the documentation a bit in this regard.

Skylion007 avatar Mar 16 '22 15:03 Skylion007

Seems it is set here if I understand correctly: https://github.com/pybind/pybind11/blob/f8a532a7ded45203cd755d53f0da5f1fadbbea2d/include/pybind11/detail/class.h#L267

I'm fine with the current behavior. But it would be nice with some pointers about it in the documentation. Especially as this behaviors is different from "plain" python. It is quite hard to figure out that pybind11 is overriding the function itself.

petersteneteg avatar Mar 16 '22 21:03 petersteneteg