gh-92678: Fix MRO calculation for C extension types with manual __dict__

[Fidget-Spinner]

We need to account for C extension types with don't set Py_TPFLAGS_MANAGED_DICT and manually manage their own dict instead.

This may be unsightly, but this special case is special enough to break the rules. Unless we get a proper class creation and dict management API in 3.12, we will break a lot of code.

• Issue: MRO: Behavior change from 3.10 to 3.11: multiple inheritance issue with C-API (pybind11) #92678

[pablogsal]

What issue does this corresponds to?😅 I need a bit of context here to review it 😉

[Fidget-Spinner]

This reintroduces the old code pre-3.11, but guards it with a check for not Py_TPFLAGS_MANAGED_DICT.

8319114#diff-1decebeef15f4e0b0ce106c665751ec55068d4d1d1825847925ad4f528b5b872L2254

[pablogsal]

The condition is a bit arcane. Could you capture it in a variable or use a macro or inline function so we can name what we are checking for? Currently is a bit difficult to decipher what happens when all of these are true or false

[Fidget-Spinner]

So funny thing is I also found the old code arcane and couldn't really understand it well. From my rough guesstimation

    // The __dict__ isn't automatically managed by CPython.
    if (!(type->tp_flags & Py_TPFLAGS_MANAGED_DICT) &&
        // That a dictoffset is set. Meaning it's a manual __dict__.
        // And the base class doesn't have a __dict__
        type->tp_dictoffset && base->tp_dictoffset == 0 &&
        // Huh?
        type->tp_dictoffset + sizeof(PyObject *) == t_size &&
        // Not a static type.
        type->tp_flags & Py_TPFLAGS_HEAPTYPE) {
        t_size -= sizeof(PyObject *);
    }

[Fidget-Spinner]

I think this is a really cursed example of Hyrum's law.

The extra_ivars code assumes that __weakreflist__ comes first then __dict__. In mypyc it has to follow this order to work pre-3.11. Now in 3.11 when we remove that assumption, all the C extensions using this with complicated MROs break 🤦 .

[Fidget-Spinner]

What issue does this corresponds to?😅 I need a bit of context here to review it 😉

Sorry. gh-92678

[markshannon]

Is is correct to check Py_TPFLAGS_MANAGED_DICT during class creation?
It is set during class creation: https://github.com/python/cpython/blob/main/Objects/typeobject.c#L2989

[Fidget-Spinner]

I think setting the flag happens before the MRO check. https://github.com/python/cpython/blob/main/Objects/typeobject.c#L3174-L3186
(type_new_descriptors then PyType_Ready ).
(PyType_Ready -> type_ready -> type_ready_mro)

[markshannon]

If you factor out the check into a macro, the code would be a bit clearer.

#define FIELD_AT_OFFSET(name, offset) type->tp_ ## name && base->tp_## name == 0 ...

[Fidget-Spinner]

Reopened because this is an alternative fix.

[markshannon]

This looks like it should fix the regression in #95589

Could you add some tests please?
You'll need to two classes each for tp_dictoffset and tp_weaklistoffset to the `_testcapi module then test for multiple inheritance of all pairs in test_capi.py

[markshannon]

If you factor out the check into a macro, the code would be a bit clearer.

#define FIELD_AT_OFFSET(name, offset) type->tp_ ## name && base->tp_## name == 0 ...

[markshannon]

We need to check for tp_dictoffset and tp_weaklistoffset, and shouldn't presume the order.
So we need to check for weakrefs, then dict, then weakrefs again (or vice versa).

if (FIELD_AT_OFFSET(dictoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}
if (FIELD_AT_OFFSET(weaklistoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}
if (FIELD_AT_OFFSET(dictoffset, t_size)) {
   t_size -= sizeof(PyObject *);
}

[markshannon]

@Fidget-Spinner
As the RC is on Friday, I've implemented the suggested changes as #95596
Apologies for treading on your toes.