Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
encukou opened this issue Nov 8, 2022 · 5 comments
Closed

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

encukou opened this issue Nov 8, 2022 · 5 comments
Labels
topic-C-API type-bug An unexpected behavior, bug, or error

Comments

@encukou
Copy link
Member

encukou commented Nov 8, 2022

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.

@encukou encukou added type-bug An unexpected behavior, bug, or error topic-C-API labels Nov 8, 2022
@gvanrossum
Copy link
Member

I never intended to support multiple inheritance for extension classes…

@encukou
Copy link
Member Author

encukou commented Nov 9, 2022

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
Copy link
Member

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
Copy link
Member Author

encukou commented Nov 9, 2022

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.

encukou added a commit to encukou/cpython that referenced this issue Nov 10, 2022
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.
encukou added a commit that referenced this issue Nov 28, 2022
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>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 28, 2022
…-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>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 28, 2022
…-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>
miss-islington added a commit that referenced this issue Nov 28, 2022
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>
miss-islington added a commit that referenced this issue Nov 28, 2022
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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-C-API type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants