It would be good if PyType_Ready() will check that base class of static type is static.

By static, you mean not a heap type?

Yup.

> By static, you mean not a heap type?

Yes. Sorry for bad wording.

Here is a patch.

Could anyone please make a review?

Ping.

New changeset c087ac6fc171 by Serhiy Storchaka in branch '2.7':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/c087ac6fc171

New changeset 747855f29b9d by Serhiy Storchaka in branch '3.4':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/747855f29b9d

New changeset eb26255e11f1 by Serhiy Storchaka in branch 'default':
Issue #22079: PyType_Ready() now checks that statically allocated type has
https://hg.python.org/cpython/rev/eb26255e11f1

reopening, this breaks some stuff in several places ...

https://bugs.launchpad.net/ubuntu/+source/terminator/+bug/1426294

This restriction was added because bad thing could happen when statically allocated class inherits dynamically allocated class. I don't remember the number of the issue where such example was exposed, may be Alex remember it.

Likely pygobject creates classes in not safe manner. It would be more correct to create pygobject classes with PyType_FromSpec(), but current code worked for it for years. May be it is not so easy to change pygobject. So may be we should revert changes in 2.7 or convert the error to the deprecation warning.

Is there similar issue with 3.4? There may be difference due to the difference of the lifetime of types in 2.x and 3.x.

New changeset aa79a04e9bf5 by Serhiy Storchaka in branch '2.7':
Issue #22079: Py3k warning now is issued in  PyType_Ready() instead of
https://hg.python.org/cpython/rev/aa79a04e9bf5

New changeset 57a457ea84e4 by Serhiy Storchaka in branch '3.4':
Issue #22079: Deprecation warning now is issued in  PyType_Ready() instead of
https://hg.python.org/cpython/rev/57a457ea84e4

I think we can start from index 1 instead of 0.

Can you please explain?

In the mro list I think the first is always the type itself which has already been checked to be a statically allocated type if we can touch the loop. Starting from 0 checks it twice and it's not a base either.

BTW, I don't receive an email from the tracker. :(

__bases__ != mro()

Of course, I know it. But doesn't the local variable `bases` refer to mro[1]?

[1] https://hg.python.org/cpython/file/tip/Objects/typeobject.c#l4900

Good point! tp_bases should be iterated instead of tp_mro in this check.

I thought that too at first. But if we use __bases__, can we still guarantee that 'all bases of statically allocated type should be statically allocated'? How about its grand bases? Is it possible that a statically allocated type inherit a dynamic type but does not go through PyType_Ready and then be inherited?

This is interesting question. It looks that base classes (as well as mro classes) may not go through PyType_Ready. There is no even a check that mro() returns only ancestor classes and contains the initial class itself.

Oh yes. The mro list can be customized so we can not even guarantee the type appears at index 0 and object at index n-1. It seems we should still keep what it is now, iterating the whole mro list.

Sorry that my previous comments seem to be noise. But maybe we can add one comment to clarify that iterating the whole list is needed.

> It would be good if PyType_Ready() will check that base class of static type is static.

What's the rationale for this change? It's not explained in this bug report nor in the code.

The workaround that Cython had to added for this (temporarily enable Py_TPFLAGS_HEAPTYPE when calling PyType_Ready()) is fragile. It would be nice to rethink the approach here, or disable the check altogether.

(or perhaps, expose another C API function - such as PyType_ReadyEx() - that allows disabling the check)

Since it's not clear from this ticket what the original problem was, is there a chance it could have been related to issue 35810?

Coming back to this after a while. I would like to get rid of the work-around (read: huge hack) that we have in Cython for this check and thus would ask for the check to be removed in Py3.10.

According to the discussion, no-one seemed to remember why it was added in the first place, just that "bad things happened", but not what or how.

With the fix for issue 35810 in place since Py3.8, my guess is that the situation that this check is preventing has actually become safe now. Unless someone can motivate that the risks were other than a premature deallocation of the base type.

I recently reworked type_new() and PyType_New() in bpo-43770. I wrote documentation for the type_ready_inherit_as_structs() helper function of PyType_Ready():

// For static types, inherit tp_as_xxx structures from the base class
// if it's NULL.
//
// For heap types, tp_as_xxx structures are not NULL: they are set to the
// PyHeapTypeObject.as_xxx fields by type_new_alloc().
static void
type_ready_inherit_as_structs(PyTypeObject *type, PyTypeObject *base)

I hesitated to add assertions to ensure that fields are set if the type is a heap type.

One issue with static type is that it doesn't have the following fields of PyHeapTypeObject:

    PyAsyncMethods as_async;
    PyNumberMethods as_number;
    PyMappingMethods as_mapping;
    PySequenceMethods as_sequence; /* as_sequence comes after as_mapping,
                                      so that the mapping wins when both
                                      the mapping and the sequence define
                                      a given operator (e.g. __getitem__).
                                      see add_operators() in typeobject.c . */
    PyBufferProcs as_buffer;

Is there a reason to not add these fields to PyTypeObject? PyTypeObject is not part of the stable ABI.

Type inheritance without these fields make PyType_Ready() weird and more complex.

> Is there a reason to not add these fields to PyTypeObject?


Can't say I haven't thought of that, but AFAIK it would mean breaking the C API substantially. Even if not it'd be a PEP-sized change, IMO.