Fix multiple inheritance with static properties

[dean0x7d]

This fixes #597 and also (unintentionally) resolves #594.

@pschella: This fixes the original test code that you posted in #597 and I think the fix is quite thorough. But it might be good if you could test these changes in case there are any missing corner cases.

Changes

The main issue for multiple inheritance is that Python not only requires that all base types are layout compatible, but also that they have a common metaclass. This resulted in problems when mixing classes with different py::metaclass and py::dynamic_attr specifications. This PR attempts to bring harmony to the different type hierarchies. This is a bit involved but most of the changes are organizational, not fundamental (the types are just stacked a bit differently).

Benefits

• Multiple inheritance now works with combinations of py::metaclass, py::dynamic_attr and 'vanilla' classes, independent of base list order. This works for multiple inheritance in C++: py::class_<T, CppType1, CppType2>(m, "T").

  The new uniform layout also makes it possible to inherit from multiple C++ types in Python: defining class PyType(CppType1, CppType2) works just like py::class_<T, CppType1, CppType2>. (Not so much this part. See comments below.)

• The new metaclass implementation is cheap (essentially free) making it possible to enable py::metaclass for all classes by default. py::metaclass is thus deprecated.

• The new metaclass/static property implementation also brings a minor semantics improvement: it's now possible to access static properties via instances (x = instance.static_prop) where this was previously type-only (x = Type.static_prop).

Cost

• There's some extra global data (see internals) and a an extra pointer in the per-type data (type_info). But the per-type unique metaclass is now replaced with a single shared metaclass which reduces overall memory requirements.

• Not counting the new tests, the binary size of the test module stays about the same (+140 bytes = +0.01% on clang/macOS).

Implementation

I hope the following quick ASCII diagrams will help illustrate the implementation change. A general relationship between instance, type and metatype looks something like this:

               TypeBase     MetatypeBase
                  |              |
instance  -->    Type   -->   Metatype
                  |
               Derived

Here Type is some user-define class and instance = Type(). Metatype is the type's type and the inheritance is shown top-down, separately for the type and the metatype.

Typically, this is pretty simple in Python:

                object     
                  |
instance  -->    Type  -->  type
                  |
               Derived

Here object and type are Python builtins and the above just illustrates the following in Python3:

class Type:
    pass

class Derived(Type):
    pass

instance = Type()

Now, the existing pybind11 implementation for classes with static properties (py::metaclass) looks a bit like this:

                object             type
                  |                 |
instance  -->    Type   -->   UniqueMetatype
                    \                      \
                     \-> property           \-> property (static)

Regular properties (instance.prop) are stored in Type while static properties (Type.prop) are stored in a metaclass which is unique for each Type. This doubles the amount of data allocated per type which makes it expensive and thus guarded by py::metaclass.

This PR changes things around to look like this:

                object             type
                  |                 |
instance  -->    Type   -->   SharedMetatype
                    \
                     \-> property
                     |-> static_property

This approach is inspired by boost.python but with a slimmer implementation (most notably it doesn't replicate any CPython internal structures). The basic idea is that we extend Python's builtin property type and adapt it for static data. The following Python code is the exact implementation, but we just create it using the C API:

class pybind11_static_property(property):
    """Exactly like property, but we always pass the type (`cls`)
    instead of the instance (`obj`)"""

    def __get__(self, obj, cls):
        return property.__get__(self, cls, cls)

    def __set__(self, obj, value):
        cls = obj if isinstance(obj, type) else type(obj)
        property.__set__(self, cls, value)

The static_property still needs a bit of support from a metaclass (see the code for details), but this is only a uniform behavior change and the metaclass doesn't need to store any data. Therefore, the metaclass doesn't need to be unique per type. We create a single one and share it among all the types which need it. And since that's all there is, we can also just let all types point to it and have "free" static properties.

This resolves one part of the multiple inheritance MRO issues: having a common metaclass for all types makes them compatible while the unique metatypes were blockers. However, there is an additional hurdle. In order to fully satisfy Python's type layout requirements, all types should have a common 'solid' base. A solid base is one which has the same instance size as the derived type (not counting the space required for the optional dict_ptr and weakrefs_ptr). Thus, object does not qualify as a solid base for pybind11 types and this can lead to issues with multiple inheritance.

             object
            /      \
Pybind11Type1      Pybind11Type2
            \      /
             MIType

In the example above, instance_size(Pybind11Type1) > instance_size(object) and instance_size(Pybind11Type1) == instance_size(Pybind11Type2) so object is not considered a solid base of MIType. This can result in PyType_Ready() complaining about layout and refusing to finalize the type for some base combinations.

The solution is to create a new base type:

         pybind11_object
            /      \
Pybind11Type1      Pybind11Type2
            \      /
             MIType

Here instance_size(pybind11_object) == instance_size(Pybind11Type1, Pybind11Type2) and thus PyType_Ready() is perfectly happy to support it.

Notes

Since this changes a good bit of the py::class_ implementation, I've also used this opportunity to move most of the details into a separate file (detail/class_.h) since the main pybind11.h is loaded enough as it is.

[jagerman]

I haven't looked at the code yet, but from the description: Wow, very nice!

[wjakob]

I agree -- this looks just fantastic! Suspecting a problem of this sort, I've also been working on a potential workaround by extending the py::metaclass() tag to accept an actual class that's used as the metaclass. Much to my surprise, I wasn't able to get PyType_Ready() to accept types involving multiple inheritance even if all classes refer to the same metaclass -- this seems to be exactly the problem you point out above and nicely solve with pybind11_object.

Just one quick clarification: since you modified the way the property itself works, there is no need to touch the (global) metaclass to add a new static property to some class, right? This would have been the case for the previous scheme if all classes shared the same metaclass, which could potentially lead to nasty long overload chains if many static properties have the same name.

I'm wondering if there is some use in not quite deprecating py::metaclass() yet: manually specifying the metaclass to be used (which pybind11 can't yet do but which was going to be in my PR) could be useful in some contexts.

I'll need a bit more time to go through the nitty gritty details, but overall the approach sounds very sensible, and de-cluttering pybind11.h is also a good idea.

[dean0x7d]

Just one quick clarification: since you modified the way the property itself works, there is no need to touch the (global) metaclass to add a new static property to some class, right?

That's right. The metatype is no longer mutated when adding new static properties. It's only required to override the default type.__setattr__(). That's a one-time upfront change -- here is pretty much the entire implementation of the shared pybind11_type metatype:

class pybind11_type(type):
    def __setattr__(cls, name, value):
        prop = cls.__dict__[name]
        if isinstance(prop, pybind11_static_property):
            pybind11_static_property.__set__(prop, cls, value)
        else:
            type.__setattr__(cls, name, value)

And this is only needed since Type.prop = a becomes Type.__setattr__(a) instead of the needed Type.prop.__set__(a). The other direction (a = Type.prop) is fine and results in a = Type.prop.__get__(). The instance properties are also fine (a = instance.prop and instance.prop = a regularly resolve to __get__() and __set__()). It's just this one case when assigning to the Type itself.

I'm wondering if there is some use in not quite deprecating py::metaclass() yet: manually specifying the metaclass to be used (which pybind11 can't yet do but which was going to be in my PR) could be useful in some contexts.

I think that shouldn't be a problem. The default metaclass() constructor can be deprecated while adding new (non-deprecated) metaclass(something, ...) constructor(s). (I'm assuming specifying a custom metaclass would require at least one parameter to be passed to the constructor.)

[pschella]

Wow, that looks excellent! I also haven't had time to go over the code, but it looks like a great solution. I'll see if I have time to look at it in more detail, but in the mean time will test it out on our (quite large) codebase to see if there are any issues.

[wjakob]

@pschella: that sounds great -- please keep us posted

[wjakob]

Overall this looks fantastic! I left a few minor comments scattered throughout.

One more that came to me just now is that the differences in this patch also potentially create new opportunities for reference leaks (for instance, are the special static properties collected when a type goes out of scope? This is relevant in cases where types are created dynamically.)

[wjakob]

Many of these header files are actually implementation details, so the separation wrt. class_.h seems artificial. I'd either leave it at the top level (and rename to something without a trailing underscore like class_support.h) or refactor the header files more significantly (though that doesn't necessarily need to be in this commit).

[dean0x7d]

Yeah, this PR probably shouldn't introduce a one-off change like that. Renamed detail/class_.h -> class_support.h. I squashed back the change right away to keep the commits clean.

[wjakob]

Propose changing to

    handle value;
    PYBIND11_DEPRECATED("py::metaclass() is no longer required. It's turned on by default now.")
    metaclass() { }
    metaclass(handle value) : value(value) { }

[wjakob]

I propose adding

    /// Custom metaclass (optional)
    handle metaclass;

below the doc member

[wjakob]

Proposal:

template <>
 struct process_attribute<metaclass> : process_attribute_default<metaclass> {
    static void init(const metaclass &m, type_record *r) { r->metaclass = m.value; }
 };

[wjakob]

Potentially this function (and many others below) could be lambda functions? Or is the point to force "C" linkage, which may be tricky in a Lambda function? (though AFAIK there is no difference on any platform that we support)

[dean0x7d]

Yeah, the intent was to stand by the letter of the standard with regard to linkage. The current compilers don't make a distinction between C and C++ linkage, but I'm a bit apprehensive about how stable that is.

I made a separate commit here to see what the lambda approach would look like: dean0x7d@0c44e74 It's a pretty nice readability improvement for short functions like tp_descr_get/tp_descr_set, but I'm not sure that the longer ones benefit quite as much.

Overall, my vote would still be to err on the side of portability since it doesn't look like a huge readability improvement to me.

[wjakob]

Ok -- I think it is fairly readable even with the separation. In case you're still willing to make a few changes, then one thing that would be nice to have is a 1-line comment for each hook that says roughly why its' needed. E.g.

/// Traverse the internal dictionary of an instance. Used to implement garbage collection on types with the dynamic_attr tag.
extern "C" inline int object_traverse(PyObject *self, visitproc visit, void *arg) {

(This one is especially confusing since the surrounding context is missing in the non-lambda implementation.)

[dean0x7d]

Sure, I'll add extra comments for the functions.

[wjakob]

Same here

[wjakob]

A bit of documentation would be nice here.

[wjakob]

Switch over to rec.metaclass if specified here.

[dean0x7d]

Regarding the metaclass proposal: Users can now override the default metaclass using py::metaclass(handle).

Renamed metatype -> metaclass everywhere for consistency. I used the metatype name originally since that's what the CPython implementation uses most often, but since py::metaclass is sticking around, it makes sense to keep it consistently metaclass inside pybind11. I also renamed internals.metatype to internals.default_metaclass since this can now be overridden by user-defined metaclasses.

One more that came to me just now is that the differences in this patch also potentially create new opportunities for reference leaks (for instance, are the special static properties collected when a type goes out of scope? This is relevant in cases where types are created dynamically.)

AFAIK there shouldn't be any reference leaks with regard to static_property since we don't do any manual incref/decref -- Python manages everything. static_property_type is only ever used in def_property_static_impl() (as a borrowed reference) where we only invoke its __call__ operator. After that, everything is handed off to Python which is going to do the correct reference counting, just like it does for the default property.

[jagerman]

I like the detail/ separation, and think we should pursue it, but a separate PR to do it everywhere at once makes much more sense.

[wjakob]

ok, that sounds good. Regarding metaclass vs metatype: the former is used much more frequently at least according to a google search, so I suggest that we stick with that terminology :)

[wjakob]

@jagerman, did you still plan to do a review of this PR? Let me know in case you already did, and I'll go ahead and merge it.

[dean0x7d]

OK, I added some docs for the various PyTypeObject functions. I factored out enable_dynamic_attributes and enable_buffer_protocol to localize the functions and their usage.

[jagerman]

did you still plan to do a review of this PR?

Yeah, I'm taking a detailed look at it now.

[jagerman]

I was particular excited by this part:

The new uniform layout also makes it possible to inherit from multiple C++ types in Python: defining class PyType(CppType1, CppType2) works just like py::class_<T, CppType1, CppType2>.

but it doesn't seem to be working properly:

inh_test.cpp:

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

class B {
public:
    B(int v) : v{v} { std::cout << "B ctor @ " << this << "\n"; }
    int v;
    virtual ~B() = default;
};

class C {
public:
    C(double v) : v{v} { std::cout << "C ctor @ " << this << "\n"; }
    double v;
    virtual ~C() = default;
};

class BC : public B, public C {
public:
    BC(int v1, double v2) : B(v1), C(v2) { std::cout << "BC ctor @ " << this << "\n"; }
};

namespace py = pybind11;

PYBIND11_PLUGIN(inh_test) {
    py::module m("inh_test"); 

    py::class_<B>(m, "B")
        .def(py::init<int>())
        ;
    py::class_<C>(m, "C")
        .def(py::init<double>())
        ;

    py::class_<BC, B, C>(m, "BC")
        .def(py::init<int, double>())
        ;

    m.def("get_bv", [](B &b) { return b.v; });
    m.def("get_cv", [](C &c) { return c.v; });

    return m.ptr();
}

inh_test.py:

from inh_test import B, C, BC, get_bv, get_cv

class X4(B, C):
    def __init__(self):
        B.__init__(self, 42)
        C.__init__(self, 42.5)

class X5(C, B):
    def __init__(self):
        C.__init__(self, 43.5)
        B.__init__(self, 43)

x4 = X4()
x5 = X5()
bc = BC(123, 4.56)
print("Getting X4 b-val:", get_bv(x4))
print("Getting X4 c-val:", get_cv(x4))
print("Getting BC b-val:", get_bv(bc))
print("Getting BC c-val:", get_cv(bc))
print("Getting X5 b-val:", get_bv(x5))
print("Getting X5 c-val:", get_cv(x5))

Outputs:

B ctor @ 0x5605e5662110
C ctor @ 0x5605e5662110
C ctor @ 0x5605e5662150
B ctor @ 0x5605e5662150
B ctor @ 0x5605e566a5e0
C ctor @ 0x5605e566a5f0
BC ctor @ 0x5605e566a5e0
Getting X4 b-val: 0
Getting X4 c-val: 42.5
Getting BC b-val: 123
Getting BC c-val: 4.56
Getting X5 b-val: 43
Getting X5 c-val: 43.500000000000306

Note how the two C++ instances are being wrongly initialized at the same location in the MI-from-Python version (which is also apparent from the wrong (due to being overwritten) X4 b-val and X5 c-val results).

[wjakob]

I think there are two issues here: the first is to get the class layout to pass through PyType_Ready, which this commit accomplishes. The second is that this type of inheritance would require a rather different type of dispatch mechanism -- we'll have to keep track of two different instances -- one for CppType1, and one for CppType2. (A joint CppType1,CPPType2 type with virtual table and all will generally not exist, so we can't presume it to be there/instantiate it.) At runtime, it would be necessary to determine which of the instances should be used for dispatch and then forward to the appropriate type caster. It seems really complicated -- unless somebody has thoughts on how to do this without severely complicating the rest of pybind11, I'd say that it's probably not worth it.

[wjakob]

(Boost.Python seems to offer this feature, but their dispatch mechanism is really complicated -- we don't want to go there)

[pschella]

I'm also seeing some strange behaviour. It seems that base class constructors now seem to take part in the overload resolution of the derived class. This was not the case before and is unexpected both in C++ and Python.

[dean0x7d]

Yeah, I was a bit eager with that one. I realized class C(A, B) (just the definition) was working without errors while I was writing the PR and I figured I'd add some tests and remove MI docs caveat #2 as a followup commit. But the tests revealed the larger issues as stated by @wjakob (also, PyPy seems to handle MI type creation slightly differently than CPython).

I don't think it can be solved without complicating the whole type handling mechanism in pybind11. I'm not sure it's worth pursuing (at least not within this PR). What would be good is to make the type definition an error in Python in order to avoid confusion. I'm trying to put something together now.

@pschella Are you also referring to inheritance in Python in your last comment? (The C++ side should be OK.)

[wjakob]

I think @pschella refers to something a bit more worrisome: that in a class definition such as

py::class_<Base>(..)
   .def(py::init<int>());

py::class_<Derived, Base>(..)
   .def(py::init<std::string>());

The Base(int) constructor may be called given the expression Derived(1) (rather than issuing an error message). Or did I get that wrong?

[jagerman]

@pschella - I can't seem to reproduce that (I interpreted it the same way as @wjakob). Can you give a bit more detail (or, if you have it, a "working" example)?

[pschella]

Yes (I think). What I'm seeing if Derived has no constructors exposed (i.e. wrapped) and is called with an unsupported argument it actually prints the list of overloads from Base. And these also seem to be used (even if not reported) when Derived does have constructors exposed.

[pschella]

(Pdb) lsst.afw.image._mask.MaskU(12)
*** TypeError: __init__(): incompatible constructor arguments. The following argument types are supported:
    1. lsst.afw.image._image.ImageBaseU(dimensions: lsst.afw.geom._extent.Extent2I=Extent2I(0, 0))
    2. lsst.afw.image._image.ImageBaseU(src: lsst.afw.image._image.ImageBaseU, deep: bool=False)
    3. lsst.afw.image._image.ImageBaseU(src: lsst.afw.image._image.ImageBaseU, bbox: lsst.afw.geom._box.Box2I, origin: lsst.afw.image._image.ImageOrigin=ImageOrigin.PARENT, deep: bool=False)

Where ImageBaseU is (the only) base of MaskU and MaskU has no wrapped constructors.

[dean0x7d]

I believe that should fix the constructor inheritance. (Also, I now realized that a pybind11_init function already exists in common.h... I'll have to rename the one in class_support.h.)

[jagerman]

explicit?

[dean0x7d]

Indeed, good catch.

[dean0x7d]

OK, that last commit should prevent multiple inheritance from C++ bases in Python + a couple of smaller things. Let me know if there is anything else. If not, I'll clean up the commit history a little.

[pschella]

Ok, so that change seems to have fixed one issue. I am however seeing another, likely unrelated, problem that is preventing me from running this test on all our code. It looks like constructors (and probably functions) taking std::string arguments are also called when a numpy.ndarray object is given.

[pschella]

That seems to be an unrelated issue. I'll report it separately.

[wjakob]

Shall we wait for @pschella's issue to be cleared up or merge this now? Either is fine to me.

[dean0x7d]

Rebased and squashed a bit to clean up the history. I guess it would be good to merge now to help out #693.

[jagerman]

Merging now sounds good to me. I went through much of this pretty thoroughly in #693 and didn't notice any issues.

[wjakob]

OK -- merged, thank you!

[pschella]

Thanks! Together with #693 I can now confirm that this works for our codebase.
Is there any chance a point release can be issued for this? Perhaps 2.1.0 given the size of the change?
We are about to use pybind11 in production and would like to be based on a fixed released version.

[wjakob]

Just to clarify: Are you sure that you need #693 for your project? Or is #679 enough? (The static properties issue should be fixed by the latter)

Pushing out a 2.1.0 point release of the master branch fairly soon sounds absolutely doable, but #693 will still require a redesign and time for stabilization?

[pschella]

Ah, no sorry, mixed up the numbers. I need fixes for #688, #685 and #597. I have tested on 2a75784, but latest master should be fine.
If an RC could be tagged, or a candidate commit for release can be announced beforehand I'll test it to make sure.

[pschella]

Now also tested successfully on 5143989