-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
During metaclass.__init__, super() of the constructed class does not work #67910
Comments
When a new class is initialized with __init__ in a metaclass, This is a known but fixable problem. The attached patch moves |
I like the change, but even though the current behaviour is arguably buggy (and certainly undesirable) the fix does introduce a new class level attribute that is visible during execution of Python level code. Perhaps it would be worth rolling this change into PEP-487 and documenting the new transient namespace entry as __classcell__? |
A note on the implementation: The compiler leaves a __cell__ entry in the class' namespace, which As the patch tampers with the compiler, when testing the patch |
Currently, a class is created as follows: the compiler turns the class statement into a call to __build_class__. This runs the class body. If __class__ or super() is used within a method of the class, an empty PyCell is created, to be filled later with the class once its done. The class body returns this cell. Then the metaclass is called to create the actual class, and finally the cell is set to whatever the metaclass returns. This has the disadvantage that in the metaclasses __new__ and __init__, __class__ and super() are not set. This is a pity, especially because the two parameter version of super() doesn't work either, as the class is not yet bound to a name. The attached patch lets the compiler add said cell as __classcell__ to the classes namespace, where it will later be taken out by type.__new__ in order to be properly filled. This resembles the approach used for __qualname__, with the difference that __qualname__ is already added at the beginning of the classes body, such that it is visible to the user. This way __class__ will be properly set immediately after it is created, thus all methods are immediately usable, already in a metaclasses __new__ or __init__. This changes the behavior if a metaclass returns another class. currently, __build_class__ will try to set the __class__ in the methods of the class body to whatever __new__ returns, which might be completely unrelated to the classes body. |
I don't think this requires adding it to the PEP, and I think doing this is fine. (But I can't review the code.) |
Martin, the patch isn't currently applying to trunk - would you have time to take a look at that? Ned, this is tangentially related to Martin's work on subclass initialization in PEP-487: one of the current problems with zero-argument super is that we don't actually populate the class cell until quite late in the type creation process, so even after the metaclass.__new__ call finishes, zero-argument super still doesn't work yet. That aspect of the change is clearly a bug fix, but fixing it will have the side-effect of making "__cell__" visible in the class body during execution as a CPython implementation detail. Would that still be OK to go into beta 2 rather than beta 1? (Assigned to Ned due to the release management question) |
I am happy to *rule* that we can treat it as a bugfix, but I disagree (Please don't respond arguing the "clearly" part, just go ahead and do it. :-) |
Now that you point it out, I agree "clearly" is overstating things when it comes to claiming bug fix status for a form of usage that has never worked in the entire life of zero-argument super :) |
This is the originial patch rebased such that it applies to the current master. As a detail in the discussion: "__classcell__" is not visible during the execution of the class body, as it is added at the end of the class body. In this regard, it is different from "__qualname__", which is set at the beginning of the class body such that it may be changed. The new __classcell__ does show up, however, in the namespace parameter to the __new__ method of the metaclass. |
Nick, if you feel like doing this, go ahead, either before or after (Off-topic: boy do I miss CI that triggers when you send a patch for review...) On Sat, Sep 10, 2016 at 11:14 AM, Martin Teichmann
|
Reassigning to myself given Guido's +1 |
New changeset feb1ae9d5381 by Nick Coghlan in branch 'default': |
And done - thanks for the patch Martin! The one additional change needed was to increment the magic number for pyc files, as this changed the code emitted for class definitions. I also picked up a latent defect in PC/launcher.c which hadn't been updated for the last couple of magic number bumps. |
Hi, this causes a regression in Django and I'm not sure if Django or cpython is at fault. For a simple model that uses super() rather than super(Model self) in save(): from django.db import models
class Model(models.Model):
def save(self, *args, **kwargs):
super().save(*args, **kwargs) >>> Model().save()
Traceback (most recent call last):
File "/home/tim/code/mysite/model/tests.py", line 8, in test
Model().save()
File "/home/tim/code/mysite/model/models.py", line 5, in save
super().save(*args, **kwargs)
RuntimeError: super(): empty __class__ cell django.db.models.Model does some things with metaclasses which is likely related to the root cause: If someone could provide guidance about what the issue might be, I'm happy to provide more details or to debug this further. Thank you! |
This step here is likely to be causing you problems: Because the original class namespace isn't being passed up to type.new, it isn't seeing the However, given that we have a least one in-the-wild example of this causing problems, I think the right thing to do on the CPython side is to restore the old behaviour where the cell reference is returned from the class creation closure, but issue a deprecation warning if it hasn't already been set by type.__new__. We're also going to need to document |
Thanks Nick. Your suggestion does fix the issue for Django: django/django#7653. |
Attached patch is some new test cases for an approach that I figured out *won't work*. The problem I hit is that "__classcell__" is only injected into the class body execution namespace when there is at least one method implementation that needs it. In any other case, including when constructing types dynamically, it's entirely legitimate for it to be missing. The attached draft test cases explored the idea of requiring that I haven't given up on providing that eager warning though - it should be possible to emit it in __build_class__ based on PyCell_GET returning NULL (as that should reliably indicate that type.__new__ never got access to the compiler provided cell object) |
This latest patch restores the behaviour where a reference to the class cell is returned from the class-defining closure. That restoration allows __build_class__ to implement a sanity check that ensures that the class referenced from the cell is the one that was just defined, and complain if they don't match. To give metaclasses like the Django one a chance to adjust, not setting it at all is just a deprecation warning for 3.6, while setting it incorrectly is a TypeError. |
Reassigning to Ned for now, pending finding another commit reviewer. |
In the meantime, I'll try to work out a suitable documentation patch. |
Attached patch covers the proposed documentation updates. |
I should have made my comments a bit clearer as to which patches they were referring to. The ones submitted for inclusion in 3.6.0rc1 are:
|
Added comments on Rietveld. |
Updated patch for Serhiy's review comments: issue23722_classcell_reference_validation_v2.diff
|
Assuming there are no further comments overnight, I'll go ahead and commit this tomorrow (after doing a local refleak hunting run). |
Added two more style comments. And please take note of my comments to issue23722_documentation_updates.diff. Nothing critical, but would be nice to add more cross-references in the documentation. |
I've hit a problem where test_builtin and test_unittest are failing for me when refleak hunting is enabled (as in actual test failures, not just leak reports), but those also appear for me without the patch applied. Current plan:
|
New changeset e33245800f1a by Nick Coghlan in branch '3.6': New changeset 9e5bc3d38de8 by Nick Coghlan in branch 'default': |
Thanks for the reviews Serhiy! The patch as merged addressed both your comments on the docs (including adding several new index entries) as well as the last couple of style comments on the code changes. I've filed separate issues for the test failures I'm seeing when refleak hunting:
|
New changeset fa4d8276d0fb by Serhiy Storchaka in branch 'default': |
Nick, should a DeprecationWarning be replaced with a RuntimeWarning or a RuntimeError? There are contradictions about this in comments and the documentation. |
PR 6931 replaces a DeprecationWarning with a RuntimeError (is it correct?). It was planned to do in 3.7, but it is too later for 3.7. |
An alternate PR 6933 replaces it with a RuntimeWarning. |
I think the reference to RuntimeWarning in the docs is a typo (if it was only going to be a warning, it could have been that from the start), and that reference to RuntimeError in the code comment is correct. So there's also a docs fix to make in 3.6 and 3.7 to provide the right info about future changes. |
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: