Static types only inherit sub-slots from the first base #99249

encukou · 2022-11-08T15:54:56Z

If the following is implemented using static types C API, the derived class doesn't inherit __getitem__:

class SimpleMap:
    def __getitem__(self, key): return 'value'

class SimpleObject:
    pass

class DerivedObject(SimpleObject, SimpleMap):
    pass

C implementation

#include <Python.h>

/* SimpleMap: A simple object that allows subscription */

static PyObject *
simplemap_subscript(PyObject *self, PyObject *key)
{
    return PyUnicode_FromString("value");
}

static PyMappingMethods simplemap_as_mapping = {
    .mp_subscript = simplemap_subscript,
};

static PyTypeObject simplemap_type = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "SimpleMap",
    .tp_as_mapping = &simplemap_as_mapping,
};

/* SimpleObject: A simple object that does nothing */

static PyTypeObject simpleobject_type = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "reproducer.SimpleObject",
};

/* DerivedObject: Derives from SimpleObject and SimpleMap */

static PyTypeObject derivedobject_type = {
    PyVarObject_HEAD_INIT(NULL, 0)
    .tp_name = "reproducer.DerivedObject",
    .tp_new = PyType_GenericNew,
};

static struct PyModuleDef moduledef = {
    PyModuleDef_HEAD_INIT,
    .m_name = "reproducer",
};

PyMODINIT_FUNC PyInit_reproducer(void)
{
    PyObject *m;
    m = PyModule_Create(&moduledef);
    if (!m) {
        return NULL;
    }
    if (PyType_Ready(&simplemap_type) < 0) {
        return NULL;
    }
    if (PyModule_AddType(m, &simplemap_type) < 0) {
        return NULL;
    }

    if (PyType_Ready(&simpleobject_type) < 0) {
        return NULL;
    }
    if (PyModule_AddType(m, &simpleobject_type) < 0) {
        return NULL;
    }

    derivedobject_type.tp_bases = Py_BuildValue("(OO)", &simpleobject_type, &simplemap_type);
    if (!derivedobject_type.tp_bases) {
        return NULL;
    }

    if (PyType_Ready(&derivedobject_type) < 0) {
        return NULL;
    }
    if (PyModule_AddType(m, &derivedobject_type) < 0) {
        return NULL;
    }

    return m;
}

>>> import reproducer
>>> instance = reproducer.DerivedObject()
>>> type(instance).mro()
[<class 'reproducer.DerivedObject'>, <class 'reproducer.SimpleObject'>, <class 'SimpleMap'>, <class 'object'>]
>>> instance['key']
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'reproducer.DerivedObject' object is not subscriptable

The reason is that type_ready_inherit_as_structs is only called wih one base, not all of them.

I don't think there's a good way to fix it properly. To allow mixing slots from multiple bases (e.g. if the other base had __setitem__), we'd need to allocate new sub-slot structs for the derived type -- as heap types do. Also, it sounds pretty scary to change such long-standing behavior :)
The best we could do that I can think of would be to detect the situation and emit warnings -- possibly DeprecationWarnings, which could be changed to errors in a few releases.

PR: gh-99249: Clarify "read-only" slots tp_bases & tp_mro #99342

PR: [3.11] gh-99249: Clarify "read-only" slots tp_bases & tp_mro (GH-99342) #99837

PR: [3.10] gh-99249: Clarify "read-only" slots tp_bases & tp_mro (GH-99342) #99838

The text was updated successfully, but these errors were encountered:

gvanrossum · 2022-11-08T19:01:24Z

I never intended to support multiple inheritance for extension classes…

encukou · 2022-11-09T12:14:19Z

Thanks for the comment!
(I can't tell if you just wanted to share historical info, or want to be more involved here? Either is much appreciated. You can stop reading if you want.)

Theoretically it would also be possible to deprecate multiple inheritance of static types, but, the cat is out of the bag. Apparently it's been used in NumPy for 17 years.

Now that you say this wasn't designed this way, it makes sense. The support for it does look bolted on. I assume that since putting a tuple in tp_bases started working, people started fixing bugs in multiple inheritance – and it never got any testing since it was newer a “new feature”.
A more general issue is that the documentation for tp_* slots isn't clear on which ones you're supposed to set (and which ones are OK to read). But that's largely a moot point now: if something's possible then people do it.

gvanrossum · 2022-11-09T15:11:36Z

Alas, I don't have the bandwidth to help. It looks like setting tp_bases to a tuple changes the MRO but nothing merges the "as" structs. Since numpy is apparently okay with this state of the world, maybe there's nothing that needs fixing?

encukou · 2022-11-09T16:26:52Z

OK! Thanks for the input!

(I'll write things out for anyone who stumbles upon this. The more people understand this and can point out issues, the better :)

The as structs are merged if they exist in the subclass (inherit_slots is called for all bases). If not, the pointer to the whole struct is inherited from the first base. (So the struct is shared with the base, and so we can't merge slots from other bases into it.) The end behavior is not obvious.
If users want expected behavior, they can to point tp_as_* to empty (all-zero) structs, taking care to not share them between types since PyType_Ready's inheritance code will mutate them. Heap types do this automatically.

Yes, existing users seem fine with the status quo, so it looks like a warning in the docs would be good enough.

These slots are marked "should be treated as read-only" in the table at the start of the document. That doesn't say anything about setting them in the static struct. `tp_bases` docs did say that it should be ``NULL`` (TIL!). If you ignore that, seemingly nothing bad happens. However, some slots may not be inherited, depending on which sub-slot structs are present. (FWIW, NumPy sets tp_bases and is affected by the quirk -- though to be fair, its DUAL_INHERIT code probably predates tp_bases docs, and also the result happens to be benign.) This patch makes things explicit. It also makes the summary table legend easier to scan.

These slots are marked "should be treated as read-only" in the table at the start of the document. That doesn't say anything about setting them in the static struct. `tp_bases` docs did say that it should be ``NULL`` (TIL!). If you ignore that, seemingly nothing bad happens. However, some slots may not be inherited, depending on which sub-slot structs are present. (FWIW, NumPy sets tp_bases and is affected by the quirk -- though to be fair, its DUAL_INHERIT code probably predates tp_bases docs, and also the result happens to be benign.) This patch makes things explicit. It also makes the summary table legend easier to scan. Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

…-99342) These slots are marked "should be treated as read-only" in the table at the start of the document. That doesn't say anything about setting them in the static struct. `tp_bases` docs did say that it should be ``NULL`` (TIL!). If you ignore that, seemingly nothing bad happens. However, some slots may not be inherited, depending on which sub-slot structs are present. (FWIW, NumPy sets tp_bases and is affected by the quirk -- though to be fair, its DUAL_INHERIT code probably predates tp_bases docs, and also the result happens to be benign.) This patch makes things explicit. It also makes the summary table legend easier to scan. (cherry picked from commit 219696a) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

These slots are marked "should be treated as read-only" in the table at the start of the document. That doesn't say anything about setting them in the static struct. `tp_bases` docs did say that it should be ``NULL`` (TIL!). If you ignore that, seemingly nothing bad happens. However, some slots may not be inherited, depending on which sub-slot structs are present. (FWIW, NumPy sets tp_bases and is affected by the quirk -- though to be fair, its DUAL_INHERIT code probably predates tp_bases docs, and also the result happens to be benign.) This patch makes things explicit. It also makes the summary table legend easier to scan. (cherry picked from commit 219696a) Co-authored-by: Petr Viktorin <encukou@gmail.com> Co-authored-by: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com>

hauntsaninja · 2022-11-29T06:07:56Z

Looks like all the backports have been merged, thanks for fixing!

encukou added type-bug An unexpected behavior, bug, or error topic-C-API labels Nov 8, 2022

bedevere-bot mentioned this issue Nov 10, 2022

gh-99249: Clarify "read-only" slots tp_bases & tp_mro #99342

Merged

bedevere-bot mentioned this issue Nov 28, 2022

[3.11] gh-99249: Clarify "read-only" slots tp_bases & tp_mro (GH-99342) #99837

Merged

bedevere-bot mentioned this issue Nov 28, 2022

[3.10] gh-99249: Clarify "read-only" slots tp_bases & tp_mro (GH-99342) #99838

Merged

hauntsaninja closed this as completed Nov 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static types only inherit sub-slots from the first base #99249

Static types only inherit sub-slots from the first base #99249

encukou commented Nov 8, 2022 •

edited by bedevere-bot

Loading

gvanrossum commented Nov 8, 2022

encukou commented Nov 9, 2022

gvanrossum commented Nov 9, 2022

encukou commented Nov 9, 2022 •

edited

Loading

hauntsaninja commented Nov 29, 2022

Static types only inherit sub-slots from the first base #99249

Static types only inherit sub-slots from the first base #99249

Comments

encukou commented Nov 8, 2022 • edited by bedevere-bot Loading

gvanrossum commented Nov 8, 2022

encukou commented Nov 9, 2022

gvanrossum commented Nov 9, 2022

encukou commented Nov 9, 2022 • edited Loading

hauntsaninja commented Nov 29, 2022

encukou commented Nov 8, 2022 •

edited by bedevere-bot

Loading

encukou commented Nov 9, 2022 •

edited

Loading