homepage

When a new class is initialized with __init__ in a metaclass,
the __class__ cell of the class about to be initialized is not
set yet, meaning that super() does not work.
This is a known but fixable problem. The attached patch moves
the initialization of __class__ from the end of __build_class__
into type.__new__. This avoids the proliferation of methods
which don't have the __class__ cell set.

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
is then filled by type.__new__, and removed from the namespace by
the latter. This is the same way it is done for __qualname__.
As the patch tampers with the compiler, when testing the patch
don't forget to remove old .pyc files, otherwise strange things will
happen.

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)

> That aspect of the change is clearly a bug fix
I am happy to *rule* that we can treat it as a bugfix, but I disagree
that it's *clearly* a bugfix. It's definitely debatable. This area of
the language is so obscure and so few people remember why it was done
the way that it's done that I expect that someone out there will be
unhappy about the change. But... change happens, so it's okay.
(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
beta1 (but if you want to do it before please do it quickly).
(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
<report@bugs.python.org> wrote:
>
> Martin Teichmann added the comment:
>
> 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.
>
> ----------
> Added file: http://bugs.python.org/file44533/classcell.patch
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue23722>
> _______________________________________

Reassigning to myself given Guido's +1

New changeset feb1ae9d5381 by Nick Coghlan in branch 'default':
Issue #23722: Initialize __class__ from type.__new__()
https://hg.python.org/cpython/rev/feb1ae9d5381 

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:
https://github.com/django/django/blob/6d1394182d8c4c02598e0cf47f42a5e86706411f/django/db/models/base.py
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:
https://github.com/django/django/blob/6d1394182d8c4c02598e0cf47f42a5e86706411f/django/db/models/base.py#L90
Because the original class namespace isn't being passed up to type.__new__, it isn't seeing the `__classcell__` reference it needs in order to populate the automatic reference correctly. Copying that over the same way you're already copying `__module__` should get things working again with 3.6.0b4.
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 `__classcell__`, as we didn't account for the type-subclass-passing-a-different-namespace-to-the-parent-method scenario when initially deciding we could treat it as a hidden implementation detail.

Thanks Nick. Your suggestion does fix the issue for Django: https://github.com/django/django/pull/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 `__classcell__` be set to `None` to affirmatively indicate that it wasn't needed, but that would be a *major* compatibility break for dynamic type creation.
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:
* issue23722_classcell_reference_validation.diff (the compatibility fix)
* issue23722_documentation_updates.diff (the related docs updates)

Added comments on Rietveld.

Updated patch for Serhiy's review comments: issue23722_classcell_reference_validation_v2.diff
- avoids a spurious deprecation warning for metaclasses that don't return a type() instance at all
- avoids even the appearance of a refleak in the __build_class__ fallback code
- integrates the documentation into the main patch

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:
- ensure "./python -m test -R 3:3 -x test_builtin test_unittest" is clean both with and without the patch (perhaps also removing some other tests that are unreliable even without the patch)
- file a separate issue for the refleak hunting problem with the error tracebacks
- push the fix for the __classcell__ problems to 3.6

New changeset e33245800f1a by Nick Coghlan in branch '3.6':
Issue #23722: improve __classcell__ compatibility
https://hg.python.org/cpython/rev/e33245800f1a
New changeset 9e5bc3d38de8 by Nick Coghlan in branch 'default':
Merge #23722 from 3.6
https://hg.python.org/cpython/rev/9e5bc3d38de8 

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:
* test_builtin: http://bugs.python.org/issue28872
* test_unittest: http://bugs.python.org/issue28873 

New changeset fa4d8276d0fb by Serhiy Storchaka in branch 'default':
Fixed merge error in Misc/NEWS for issue #23722.
https://hg.python.org/cpython/rev/fa4d8276d0fb 

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.

New changeset 8ae8e6af37f29163ee263e293570cb892dc5b5d5 by Serhiy Storchaka in branch 'master':
bpo-23722: Fix docs for future __classcell__ changes. (GH-6999)
https://github.com/python/cpython/commit/8ae8e6af37f29163ee263e293570cb892dc5b5d5

New changeset 10a122c0d55b01b053126ef3fd4d9e05ab8f2372 by Miss Islington (bot) in branch '3.6':
bpo-23722: Fix docs for future __classcell__ changes. (GH-6999)
https://github.com/python/cpython/commit/10a122c0d55b01b053126ef3fd4d9e05ab8f2372

New changeset f5e7b1999f46e592d42dfab51563ea5411946fb7 by Serhiy Storchaka in branch 'master':
bpo-23722: Raise a RuntimeError for absent __classcell__. (GH-6931)
https://github.com/python/cpython/commit/f5e7b1999f46e592d42dfab51563ea5411946fb7

New changeset f0af69faee902d4b80c07c100dbd528fd8df6832 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.7':
bpo-23722: Fix docs for future __classcell__ changes. (GH-6999) (GH-7000)
https://github.com/python/cpython/commit/f0af69faee902d4b80c07c100dbd528fd8df6832