Error in metaclass search order #42380
Comments
|
In a simple class hierarchy, I have class A with A new class C, subclass of B, must have M_B or a If M_A or type are used as C metaclass, the interpreter More details in attached file. |
|
Logged In: YES I have discussed this at length with Pedro Werneck by email. |
|
Logged In: YES Yes. I think this confusion was caused because of the lack Since the "Unifying types and classes" essay seems to be the "If dict['__metaclass__'] exists, it is used." To something like: "If dict['__metaclass__'] exists and is equal to, or a |
|
Is the 3.1/2 doc still lacking in this area? |
|
I think the situation is a bit more complicated. Here is the example described by Pedro Werneck (this is py3k, but its essentially the same in 2.x): >>> class M_A(type):
... def __new__(mcls, name, bases, ns):
... print('M_A.__new__')
... return super().__new__(mcls, name, bases, ns)
...
>>> class A(metaclass=M_A): pass
...
M_A.__new__
>>>
>>>
>>> class M_B(M_A):
... def __new__(mcls, name, bases, ns):
... print('M_B.__new__')
... return super().__new__(mcls, name, bases, ns)
...
>>> class B(A, metaclass=M_B): pass
...
M_B.__new__
M_A.__new__
>>>
>>>
>>> class C(B, metaclass=M_A): pass
...
M_A.__new__
M_B.__new__
M_A.__new__
>>> As it is clear from the last three lines, the given metaclass (M_A) to C is not ignored. It is actually called (and produces the 'M_A.__new__' output line). Then M_A.__new__ calls type.__new__ (with super()). type.__new__ then searches the bases of C, find the metaclass of B, M_B, and calls its __new__. M_B.__new__ then prints the line 'M_B.__new__', then calls M_A.__new__ again (with super()). This produces the last line, 'M_A.__new__'. So the trick is in type.__new__. |
|
What also worries me is the difference between the "class" statement and the type() function. class M_A(type):
def __new__(mcls, name, bases, ns):
print('M_A.__new__', mcls, name, bases)
return super().__new__(mcls, name, bases, ns)
class M_B(M_A):
def __new__(mcls, name, bases, ns):
print('M_B.__new__', mcls, name, bases)
return super().__new__(mcls, name, bases, ns)
class A(metaclass=M_A): pass
class B(metaclass=M_B): pass
class C(A, B): pass
D = type('D', (A, B), {})The construction of C and D won't print the same messages. |
I think the reason of this is that the class statement uses the __build_class__ builtin function. This function determines the metaclass to use (by getting the metaclass of the first base class), and calls it. When one directly calls type, one doesn't call the metaclass (though type.__new__ will later call the "real" metaclass). An example: >>> class M_A(type):
... def __new__(mcls, name, bases, ns):
... print('M_A.__new__', mcls, name, bases)
... return super().__new__(mcls, name, bases, ns)
...
>>> class M_B(M_A):
... def __new__(mcls, name, bases, ns):
... print('M_B.__new__', mcls, name, bases)
... return super().__new__(mcls, name, bases, ns)
...
>>> class A(metaclass=M_A): pass
...
M_A.__new__ <class '__main__.M_A'> A ()
>>>
>>> class B(metaclass=M_B): pass
...
M_B.__new__ <class '__main__.M_B'> B ()
M_A.__new__ <class '__main__.M_B'> B ()
>>>
>>>
>>> class C(A, B): pass
...
M_A.__new__ <class '__main__.M_A'> C (<class '__main__.A'>, <class '__main__.B'>)
M_B.__new__ <class '__main__.M_B'> C (<class '__main__.A'>, <class '__main__.B'>)
M_A.__new__ <class '__main__.M_B'> C (<class '__main__.A'>, <class '__main__.B'>)
>>>Above __build_class__ calls M_A (because that is the metaclass of the first base class, A). Then M_A calls type.__new__ with super(), then type.__new__ searches the "real" metaclass, M_B, and calls its __new__. Then M_B.__new__ calls again M_A.__new__. >>> D = type('D', (A, B), {})
M_B.__new__ <class '__main__.M_B'> D (<class '__main__.A'>, <class '__main__.B'>)
M_A.__new__ <class '__main__.M_B'> D (<class '__main__.A'>, <class '__main__.B'>)
>>>Above type.__call__ directly calls type.__new__, which determines the "real" metaclass, M_B, and calls it (which then class M_A): >>> class C2(B, A): pass
...
M_B.__new__ <class '__main__.M_B'> C2 (<class '__main__.B'>, <class '__main__.A'>)
M_A.__new__ <class '__main__.M_B'> C2 (<class '__main__.B'>, <class '__main__.A'>)
>>>If we reverse the order of the base classes of C (as above for C2), __build_class__ will use M_B as the metaclass. >>> D2 = M_B('D', (A, B), {})
M_B.__new__ <class '__main__.M_B'> D (<class '__main__.A'>, <class '__main__.B'>)
M_A.__new__ <class '__main__.M_B'> D (<class '__main__.A'>, <class '__main__.B'>)
>>>And of course, if we call directly the "real" metaclass, M_B (as above), we get the same result. I used the expression "real metaclass" with the meaning "the __class__ of the class we are currently creating": >>> C.__class__
<class '__main__.M_B'>
>>> C2.__class__
<class '__main__.M_B'>
>>> D.__class__
<class '__main__.M_B'>
>>> D2.__class__
<class '__main__.M_B'>Summary: the problem seems to be, that __build_class__ doesn't call the "real" metaclass, but the metaclass of the first base. (Note: I think this is approximately consistent with the documentation: "Otherwise, if there is at least one base class, its metaclass is used." But I don't know, if this is the desired behaviour.) This behaviour of __build_class__ can result in problems. For example, if the two metaclasses define __prepare__. In some cases __build_class__ won't call the "real" metaclass' __prepare__, but the other's: >>> class M_A(type):
... def __new__(mcls, name, bases, ns):
... print('M_A.__new__', mcls, name, bases)
... return super().__new__(mcls, name, bases, ns)
... @classmethod
... def __prepare__(mcls, name, bases):
... print('M_A.__prepare__', mcls, name, bases)
... return {}
...
>>> class M_B(M_A):
... def __new__(mcls, name, bases, ns):
... print('M_B.__new__', mcls, name, bases, ns)
... return super().__new__(mcls, name, bases, ns)
... @classmethod
... def __prepare__(mcls, name, bases):
... print('M_B.__prepare__', mcls, name, bases)
... return {'M_B_was_here': True}
...The __prepare__ method of the two metaclass differs, M_B leaves a 'M_B_was_here' name in the namespace. >>> class A(metaclass=M_A): pass
...
M_A.__prepare__ <class '__main__.M_A'> A ()
M_A.__new__ <class '__main__.M_A'> A ()
>>>
>>> class B(metaclass=M_B): pass
...
M_B.__prepare__ <class '__main__.M_B'> B ()
M_B.__new__ <class '__main__.M_B'> B () {'M_B_was_here': True, '__module__': '__main__'}
M_A.__new__ <class '__main__.M_B'> B ()
>>>
>>>
>>> class C(A, B): pass
...
M_A.__prepare__ <class '__main__.M_A'> C (<class '__main__.A'>, <class '__main__.B'>)
M_A.__new__ <class '__main__.M_A'> C (<class '__main__.A'>, <class '__main__.B'>)
M_B.__new__ <class '__main__.M_B'> C (<class '__main__.A'>, <class '__main__.B'>) {'__module__': '__main__'}
M_A.__new__ <class '__main__.M_B'> C (<class '__main__.A'>, <class '__main__.B'>)
>>>
>>> 'M_B_was_here' in C.__dict__
False
>>>__build_class__ calls M_A.__prepare__, so the new class won't have a 'M_B_was_here' attribute (though its __class__ is M_B). >>> class C2(B, A): pass
...
M_B.__prepare__ <class '__main__.M_B'> C2 (<class '__main__.B'>, <class '__main__.A'>)
M_B.__new__ <class '__main__.M_B'> C2 (<class '__main__.B'>, <class '__main__.A'>) {'M_B_was_here': True, '__module__': '__main__'}
M_A.__new__ <class '__main__.M_B'> C2 (<class '__main__.B'>, <class '__main__.A'>)
>>>
>>> 'M_B_was_here' in C2.__dict__
True
>>>I we reverse the order of the bases, M_B.__prepare__ is called. >>> C.__class__
<class '__main__.M_B'>
>>> C2.__class__
<class '__main__.M_B'>But the "real" metaclass of both classes is M_B. (Sorry for the long post.) |
|
It seems, that this possible problem already came up when __build_class__ got implemented, see bpo-1681101. The second and third version of the patch at this issue (file7874 and file7875) contains a comment: "XXX Should we do the "winner" calculation here?". But the next version, which contains the C implementation of __build_class__ still uses simply the first base. The "winner calculation" probably refers to the algorithm determining the proper metaclass in lines 1950-1976 of typeobject.c (in type_new). |
|
I would make the same guess about 'winner calculation'. I am surprised that the class statement does not result in the same calculation (why else would type_new do it). Perhaps __build_class__ (which I have not read) should either call type_new or a new function with the winner calculation factored out of type_new. I suggest you repost your simplified example from the list here and use it as the basis of a test. Guido agreed that it shows a bug that might be backported to 3.2 (after discussion). If not also backported to 2.7, a separate 2.7 doc patch might be needed. |
|
The attached test case currently fails. I'll try to make a patch soon.
Yeah, that's exactly what I'm planning to do (the factoring out). |
durban
mannequin
added
interpreter-core
|
The attached patch seems to correct this issue. It contains the test attached yesterday, and it passes now. I factored out the winner calculation from type_new to a new _PyType_CalculateWinner function, and type_new calls this. I've put the declaration of this function into object.h, so __build_class__ can also call it, instead of using the metaclass of the first base. (Am I correct in thinking that the underscore prefix keeps it out of the public API?) A slight problem may be, that in some cases this function will be called twice. But it is quite simple, so I don't think it matters much: Without patch: With patch: (Note, that I generated the patch with hg extdiff, because the output of hg qdiff was much more unreadable than simple diff. I can provide an equivalent patch generated by hg if needed.) |
|
Yep, the leading underscore and the fact you added it to a block of code that is skipped when Py_LIMITED_API is defined makes it OK to include the prototype in object.h. However, I would suggest _PyType_CalculateMetaclass as the name - CalculateWinner is a bit vague without the specific context of calculating the metaclass. On a broader point, I think there is an issue that needs to be brought up on python-dev: in light of PEP-3115, "type(name, bases, ns)" is no longer an entirely safe way to create dynamic types, as it bypasses __prepare__ methods. There should be an official way to access the full class building process, including correct invocation of __prepare__ methods (likely by making __build_class__ an official part of the language spec rather than a CPython implementation detail). |
|
Removed 2.7 from the affected versions - with neither __build_class__ nor __prepare__ in 2.x, there's no alternate metaclass calculation to go wrong. This should definitely be fixed for the final 3.1 release and for 3.2 though. An updated patch against current 3.1 would be most convenient (hg should take care of the forward porting from that point). |
|
Sorry, my last review wasn't right and the patch is incorrect as it stands. After digging a little deeper, there is still a case that isn't covered correctly in __build_class__. Specifically, the case where the most derived class has a declared metatype that is the *parent* of the metaclass that *should* be used. This can be seen in type_new, where it does the "if (winner != metatype)" check. There is no equivalent check in __build_class__ - instead, the explicitly declared metaclass always wins. What needs to happen is that the call to _PyType_CalculateMetaclass in __build_class__ should be moved out so that it is unconditional. The passed in metatype will either by the explicitly declared one (if it exists) or else PyType_Type. An additional test to pick up this case is also needed. |
|
Thanks for the review! I've updated my patch:
However I noticed another problem: the declared metaclass (the object passed with the metaclass keyword in the class definition) according to PEP-3115 can be any callable object, not only a PyTypeObject. Problems:
class X(object, metaclass=func):
pass(where func is for example a function) won't work, because in _PyType_CalculateMetaclass it will detect, that func isn't a super- or subtype of object.__class__, and will raise an exception: "metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases". My first idea to solve this problem is to ignore this case in __build_class__ (check for a returned NULL, and call PyErr_Clear), and use the declared metaclass. (I don't know, if this can cause other problems, I haven't thought much about it yet.) |
|
This last point sounds like an error in PEP-3115 rather than an error |
|
That may be, but with my latest patch, this works (func is a function): class X(metaclass=func):
passBut this blows up with a TypeError: class X(object, metaclass=func):
passIs this the desired behaviour? Or should we disallow non-class metaclasses in every case? (And what about backwards-compatibility?) |
class X(metaclass=func) should definitely continue to work; that is a long-standing (if relatively unknown, and very advanced) feature. In fact, metaclasses have their origins in this, via the "Don Beaudry hook" -- see http://python-history.blogspot.com/2009/04/metaclasses-and-extension-classes-aka.html .IMO we should also keep class X(object, metaclass=func) working; it should just construct a tuple of bases (object,) and pass it to func. I realize there is now a circular dependency: you need the metaclass computation in order to find the metaclass and you need the metaclass in order to find the prepare hook, but you need to call the prepare hook before you call the metaclass and hence before the metaclass computation is carried out. I'm not sure how to reconcile that but I think we should look harder rather than give up. As to how PEP-3115 seems to imply that all classes derive from object, that only applies as long as their metaclass is (derived from) type. And that is in a sense a tautology since (dynamically) we only call something a class if its metaclass is (derived from) type. However the class statement does not necessarily create a class object! It creates whatever the metaclass creates, with suitable defaults: if there's no explicit metaclass, look at the class of the first given base; if no bases are given either, use object. But an explicit metaclass should win. Maybe the metaclass computation (which should come up with the "most derived" metaclass if there are multiple bases or bases and a metaclass) is something that we can give a name as another attribute on the "initial" metaclass, and a suitable default? Let's say __compute_metaclass__(tuple_of_bases). The default could be the initial metaclass, and type could override it to do the correct computation (which computes the most derived metaclass, and raises an error if any of the bases has a metaclass that is not in the answer's ancestor). Then this computation could run before we get __prepare__ (from the answer). When metaclass=func is given one could have __prepare__ as a function attribute and even the __compute_metacass__ could be overridden as a function attribute, so everything is still fully general. [Sorry for blathering on. Not feeling great.] |
|
I think PEP-3115 is OK - the error about all metaclasses inheriting from type was a mistake on my part (I thought the ability to create non-type metaclasses went away along with old-style classes, but I was simply wrong on that point). That got me curious as to how the explicit inheritance from object + explicit non-type metaclass case was working in 2.7, and it turns out it *does* share the same initial metaclass determination error as 3.x - it is just that build_class() is embedded in ceval.c rather than being published as a builtin. So I have two conclusions:
|
|
Thanks for diving deep! How much of this can we claim as a bug and how much as a feature? |
|
As near as I can tell, the only visible behavioural change with Daniel's patch (updated as per my last comment) will be that the two error cases noted above (i.e. when an explicit metaclass or the first parent type's metaclass is not the most derived metaclass) will now correctly invoke the real metaclass immediately, instead of first traversing up the chain to type(), which then jumps all the way back down to the most derived metaclass. The "new" special case is actually just a matter of preserving the current behaviour in the one situation where that is the right thing to do. |
|
Commenting on the latest patch here, since the Rietveld integration isn't coping with the "hg extdiff" output (I would guess that the revision numbers in the pathnames are confusing the script that attempts to determine the base revision). Aside from the note above about needing to restore the special case handling for non-type metaclasses, the only other recommendation I have is for the new metaclass invocation tests to add a second metaclass to the hierarchy and explicitly check the order of the __new__ calls, as in the original examples above that invoked the wrong M_A/M_B/M_A sequence. Those tests will then also be applicable to Python 2.7 (which doesn't have __prepare___). |
|
I'm attaching the updated patch. Changes:
The special case isn't just checking if the object is a class, but instead checking if "isinstance(obj, type) and issubclass(obj, type)", because I think only these are the objects we can do the metaclass calculation for. All other objects (including classes which are not "real" metaclasses (they do not derive from type) and functions) are used "as is", without any metaclass calculation. (It's late here, so it is quite possible that I've overlooked someting in this part.) (I'm using "hg extdiff" because the normal hg diff results in a longer and harder to read patch, but if it's needed I will send a normal diff.) |
|
I've just realized, that my patch still breaks a case, that previously worked: when the bases are not classes. This works in 3.2, but not with my patch: >>> class Foo: # not a subclass of type!
... def __new__(mcls, name='foo', bases=(), namespace={}):
... self = super().__new__(mcls)
... self.name = name
... return self
...
>>> foo1 = Foo('foo1')
>>> foo1.name
'foo1'
>>>
>>> foo2 = Foo('foo2')
>>> foo2.name
'foo2'
>>>
>>> class foo3(foo1, foo2):pass
...
>>> foo3
<__main__.Foo object at 0xb74aa96c>
>>> foo3.name
'foo3'This raises a TypeError: "metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases". In this case the *type* of all of its bases is the same (Foo), but that type is not a metaclass, but a regular class. Right now I don't know if this is a real problem, or how to solve it. |
|
Okay, probably the check in my previous patch was too strict (sorry for the noise). I'm attaching an updated patch again. Now the algorithm in __build_class__ is this:
There are also tests for these cases, and the example in msg134256 now works too. |
|
Looking at Daniel's updated patch is still on my to-do list, but I won't object if anyone else wants to take this forward (it will be at least a few weeks before I get to it). |
|
New changeset c2a89b509be4 by Nick Coghlan in branch '3.2': New changeset c72063032a7a by Nick Coghlan in branch 'default': |
|
Fix has been applied to 3.x and hence will be in 3.3 and the next 3.2 release. I have adjusted the issue metadata to reflect the fact 2,7 still exhibits the problem, but the patch requires significant work to account for the 3.x vs 2.x changes in class creation before it can be backported. |
|
After changeset c72063032a7a I get this complain: Python/bltinmodule.c: In function ‘builtin___build_class__’: |
|
New changeset b9bb9340eb0c by Florent Xicluna in branch 'default': |
|
Python 2.7 is no longer supported. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: