PyType_FromSpec refuses to create classes with tp_new

[encukou]

As reported in #60074, since PyType_FromMetaclass was added, other functions from the PyType_FromSpec family refuse to create classes whose metaclass has a non-default tp_new. IMO this is the correct default behaviour -- we don't have the arguments to call tp_new with, and skipping it may be dangerous.
Nevertheless, it's a backwards-incompatible change. It should only be made after a deprecation period.

I'm working on a fix.

Linked PRs

• gh-103968: Deprecate creating heap types whose metaclass has custom tp_new. #103972
• gh-103968: PyType_FromMetaclass: Allow metaclasses with tp_new=NULL #105386
• [3.12] gh-103968: PyType_FromMetaclass: Allow metaclasses with tp_new=NULL (GH-105386) #105697
• gh-103968: What's New: Add porting hints for PyType_From with metaclasses #105698
• [3.12] gh-103968: What's New: Add porting hints for PyType_From with metaclasses (GH-105698) #106619

[encukou]

Deprecation period started.

[gpshead]

The 3.12 docs now say "The tp_new of the metaclass is ignored. which may result in incomplete initialization." statements, but it isn't clear if this is a part of the versionchanged in 3.12 behavior, or if this was always the case.

Basically, we need explicit instructions on what code finding itself in the situation that protobuf protocolbuffers/protobuf#12186 finds itself in is supposed to do to reconcile the situation.

3.12's "What's New" does not directly address this today. It just talks about tp_new being ignored as if that is a behavior change rather than if it was always ignored without being documented as such or what should be done to get the previous behavior.

[gpshead]

Late discussion on #60074 which created the TypeError that this issue properly made into a DeprecationWarning has suggestions for a workaround but they definitely qualify as hacks. Deeper understanding of why that tp_new was set and what it's goal was and when it was ever called or not in the past is likely required.

[gpshead]

(marking deferred-blocker as we should at least address the documentation on this during the 3.12 beta phase before release)

[encukou]

Previously, the metaclass was ignored entirely, so its tp_new wasn't considered.

The old behaviour was dangerous. Consider:

• You should be able to use PyType_FromMetaclass to extend a type you know nothing about (e.g. add a mixin).
• A metaclass can override tp_new to override what PyType_FromMetaclass normally does.

PyType_FromMetaclass cannot do the right thing if the metaclass overrides tp_new.
The old behaviour -- skipping tp_new because the metaclass wasn't considered at all -- works in simple cases, but as soon as tp_new does something important, it'll blow up. Hence the deprecation.

If you control the metaclass and know that tp_new is safe to skip, IMO the proper workaround is to stop using either tp_new or PyType_FromMetaclass. They're incompatible with each other.
In the future, we probably need a tp_cinit, so you can write an initializer/validator without overriding type creation entirely.

The hack -- unsetting metaclass tp_new while creating a type -- is explicitly writing out what's going on. It's ugly, but that's because the previous behaviour is ugly.

I'll add explicit porting instructions to What's New, including the hack.

[encukou]

I looked at protobuf.

protobuf uses PyType_FromSpecWithBases to create ScalarMapContainer and MessageMapContainer, which derive from collections.abc.MutableMapping, so their metaclass is abc.ABCMeta.

abc.ABCMeta defines __new__, meaning it overrides how ABC types should be constructed.
This __new__ is not called on protobuf's container types, which is, IMO, a bug.

I didn't find out how to get a hold of protobuf's containers, but here's a small reproducer:

C extension reproducer

mymod.c:

#include <assert.h>
#include <Python.h>

static PyType_Slot MyType_slots[] = {
    {0, NULL},
};

PyType_Spec MyType_spec = {
    "mymod.MyType", sizeof(PyObject), 0,
    Py_TPFLAGS_DEFAULT, MyType_slots};

PyObject *
get_mytype(PyObject *self, PyObject *unused)
{
  PyObject *abc = PyImport_ImportModule("collections.abc");
  assert(abc); // XXX error handling

  PyObject *mutable_mapping = PyObject_GetAttrString(abc, "MutableMapping");
  assert(mutable_mapping); // XXX error handling

  PyObject *MyType = PyType_FromSpecWithBases(&MyType_spec, mutable_mapping);
  assert(MyType); // XXX error handling

  Py_DECREF(abc);
  Py_DECREF(mutable_mapping);
  return MyType;
}

PyObject *
reproducer(PyObject *self, PyObject *unused)
{
  return PyRun_String("\
t = get_mytype()                                  \n\
for base in t.mro():                              \n\
    print(getattr(base, '_abc_impl', None), base) \n\
", Py_file_input, PyModule_GetDict(self), NULL);

}

PyMethodDef mod_methods[] = {
  {"get_mytype", get_mytype, METH_NOARGS, NULL},
  {"reproducer", reproducer, METH_NOARGS, NULL},
  {NULL},
};

PyModuleDef mod = {
  .m_base = PyModuleDef_HEAD_INIT,
  .m_name = "mymod",
  .m_methods = mod_methods,
};

PyObject *
PyInit_mymod(void) {
  return PyModuleDef_Init(&mod);
}

Hacky non-distutils compilation/run (Linux, installed Python):

V=3.11
gcc $(python$V-config --cflags --ldflags) -lpython$V$(python$V-config --abiflags) -shared mymod.c -o mymod.so
python$V -c 'import mymod; mymod.reproducer()'

Result:

<_abc._abc_data object at 0x7f8ba8aa2700> <class 'mymod.MyType'>
<_abc._abc_data object at 0x7f8ba8aa2700> <class 'collections.abc.MutableMapping'>
<_abc._abc_data object at 0x7f8ba8aa2040> <class 'collections.abc.Mapping'>
<_abc._abc_data object at 0x7f8ba8aa1640> <class 'collections.abc.Collection'>
<_abc._abc_data object at 0x7f8ba8aa12c0> <class 'collections.abc.Sized'>
<_abc._abc_data object at 0x7f8ba8aa0c40> <class 'collections.abc.Iterable'>
<_abc._abc_data object at 0x7f8ba8aa14c0> <class 'collections.abc.Container'>
None <class 'object'>

Note that mymod.MyType shares its _abc_data with its superclass. This means __new__ of abc.ABCMeta, which assigns a class-specific instance here, was skipped.

In this particular case, it appears the bug is harmless -- the type for which __new__ wasn't called isn't instantiated directly, so the setup __new__ does is not used?
But it's a fragile kind of harmlessness. The metaclass should be allowed to assume its __new__ is called.
And PyType_FromSpec can't call tp_new. It wants to call tp_alloc, set slots and call PyType_Ready itself, but tp_new normally does all of that.

I don't know what the correct general solution is, but IMO, the current behaviour is worth deprecating.

[encukou]

It would be safe, and useful in some cases, to allow tp_new=NULL (i.e. the metaclass can't be instantiated from Python). I'll try to get that in before the What's New PR.

For What's New, here's the text I have now:

---

Since tp_new overrides almost everything PyType_From* functions do,
the two are incompatible with each other.
The existing behavior -- ignoring the metaclass for several steps
of type creation -- is unsafe in general, since (meta)classes assume that
tp_new was called.
There is no simple general workaround. One of the following may work for you:

• If you control the metaclass, avoid using tp_new in it:

  • If initialization can be skipped, it can be done in
    :c:member:~PyTypeObject.tp_init instead.
  • If the metaclass doesn't need to be instantiated from Python,
    set its tp_new to NULL using
    the :const:Py_TPFLAGS_DISALLOW_INSTANTIATION flag.
    This makes it acceptable for PyType_From* functions.

• Avoid PyType_From* functions: if you don't need C-specific features
  (slots or setting the instance size), create types by :ref:calling <call>
  the metaclass.

• If you know the tp_new can be skipped safely, filter the deprecation
  warning out using :func:warnings.catch_warnings from Python.

[haberman]

protobuf uses PyType_FromSpecWithBases to create ScalarMapContainer and MessageMapContainer, which derive from collections.abc.MutableMapping, so their metaclass is abc.ABCMeta.

Protobuf is using MutableMapping as a mixin. The goal is that we can define only a core set of methods (__getitem__, __setitem__, etc.) and get correct implementations of all other MutableMapping methods "for free" (__contains__, keys, etc.).

abc.ABCMeta defines __new__, meaning it overrides how ABC types should be constructed.
This __new__ is not called on protobuf's container types, which is, IMO, a bug.

It looks like the primary purpose of abc.ABCMeta is to add a register() method to the class, so that you can call MutableMapping.register(Foo), which will cause isinstance(Foo(), MutableMapping) to return true.

We do not want that functionality in this case. We do not want people to be able to call ScalarMapContainer.register(Foo), because ScalarMapContainer is a concrete class, not an abstract class.

I suspect that anyone who uses MutableMapping as a mixin is in the same boat as us. I would consider it a bug if a class becomes an ABC merely because it mixed in an ABC.

But this seems to be what happens if you mix in an ABC in pure Python:

from collections.abc import MutableMapping

class MyMapping(MutableMapping):
    pass

class OtherMapping:
    pass

# This works -- MyMapping has become an ABC!
MyMapping.register(OtherMapping)
assert isinstance(OtherMapping(), MyMapping)

The simplest fix would be to stop inheriting from MutableMapping. Maybe we could "manually" mix in the other methods without using inheritance:

from collections.abc import MutableMapping

class ScalarMapContainer:
    # ...

MutableMapping.register(ScalarMapContainer)

# Mixin the methods manually.
ScalarMapContainer.__contains__ = MutableMapping.__contains__
ScalarMapContainer.keys = MutableMapping.keys
ScalarMapContainer.items = MutableMapping.items
ScalarMapContainer.values = MutableMapping.values
# ...

[Yhg1s]

Is there anything left to do here for 3.12?

[encukou]

No, short of reverting the warning.

[cdce8p]

Just saw the warning while testing 3.12.0b4. Is stack_level=1 correct for the DeprecationWarning
This is my test output

<frozen importlib._bootstrap>:400
<frozen importlib._bootstrap>:400
  <frozen importlib._bootstrap>:400: DeprecationWarning: Using PyType_Spec with metaclasses that have custom tp_new is deprecated and will no longer be allowed in Python 3.14.

After reading the discussion here, it could probably be Protobuf but no way to tell for the warning alone.

Happy to open a new issue if this isn't the right place.

[gpshead]

After reading the discussion here, it could probably be Protobuf but no way to tell for the warning alone.

Happy to open a new issue if this isn't the right place.

@cdce8p - If you can reproduce this on 3.12.0rc1 please open a new issue with reproducer details (and ideally a link to the protobuf code in question).

[osandov]

Leaving a note here for others that may run into this issue with collections.abc. Another workaround is to define your desired extension class as a mixin type and then create the final type by inheriting from both your mixin type and the abstract base class. CPython's own decimal module does this:

cpython/Modules/_decimal/_decimal.c

Lines 6008 to 6013 in 3c99969

	/* Create SignalDict type */
	ASSIGN_PTR(state->PyDecSignalDict_Type,
	(PyTypeObject *)PyObject_CallFunction(
	(PyObject *)&PyType_Type, "s(OO){}",
	"SignalDict", state->PyDecSignalDictMixin_Type,
	MutableMapping));

.

[haberman]

I can report that for Protobuf we ended up doing what I proposed above in #103968 (comment), which is to mix in the ABC without using inheritance. We explicitly register our class as a virtual subclass: protocolbuffers/protobuf#15999