Repository navigation
PyType_FromSpec refuses to create classes with tp_new #103968
Description
Activity
Deprecation period started.
Reacted by Erlend E. AaslandThe 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.
Reacted by C.A.M. GerlachLate 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.
7 remaining items
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 otherMutableMappingmethods "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.ABCMetais to add aregister()method to the class, so that you can callMutableMapping.register(Foo), which will causeisinstance(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), becauseScalarMapContaineris a concrete class, not an abstract class.I suspect that anyone who uses
MutableMappingas 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 # ...
Reacted by Gregory P. Smith and David LechnerIs there anything left to do here for 3.12?
No, short of reverting the warning.
- moved this from Todo to Done in Release and Deferred blockers 🚫
on Jul 11, 2023 - added a commit that references this issue
on Jul 11, 2023 - added a commit that references this issue
on Jul 11, 2023 Just saw the warning while testing
3.12.0b4. Isstack_level=1correct for theDeprecationWarning
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.
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).
Reacted by Marc Mueller- added 3 commits that reference this issue
on Aug 10, 2023 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 owndecimalmodule does this:.cpython/Modules/_decimal/_decimal.c
Lines 6008 to 6013 in 3c99969
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
As reported in #60074, since
PyType_FromMetaclasswas added, other functions from thePyType_FromSpecfamily refuse to create classes whose metaclass has a non-defaulttp_new. IMO this is the correct default behaviour -- we don't have the arguments to calltp_newwith, 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