classification
Title: During metaclass.__init__, super() of the constructed class does not work
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Martin.Teichmann, Tim.Graham, ebarry, eric.snow, larry, ncoghlan, ned.deily, python-dev, serhiy.storchaka
Priority: Keywords: patch

Created on 2015-03-20 13:44 by Martin.Teichmann, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
classcell.patch Martin.Teichmann, 2016-07-17 16:25 The patch for the changes described here review
classcell.patch Martin.Teichmann, 2016-09-10 18:14 the rebased patch review
issue23722_enhanced_classcell_tests.diff ncoghlan, 2016-12-03 06:45 Rejected idea recorded for design history purposes review
issue23722_classcell_reference_validation.diff ncoghlan, 2016-12-04 06:47 Revised implementation with stricter self-validation review
issue23722_documentation_updates.diff ncoghlan, 2016-12-04 07:28 Documentation updates for __classcell__ & PEP 487 hooks review
issue23722_classcell_reference_validation_v2.diff ncoghlan, 2016-12-04 12:43 Address Serhiy's review comments, includes docs in same patch review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (30)
msg238672 - (view) Author: Martin Teichmann (Martin.Teichmann) * Date: 2015-03-20 13:44
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.
msg238795 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-03-21 12:07
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__?
msg238992 - (view) Author: Martin Teichmann (Martin.Teichmann) * Date: 2015-03-23 09:11
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.
msg270642 - (view) Author: Martin Teichmann (Martin.Teichmann) * Date: 2016-07-17 14:27
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.
msg270675 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-07-18 02:16
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.)
msg275620 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-10 10:53
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)
msg275652 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-10 16:42
> 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. :-)
msg275655 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-10 16:46
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 :)
msg275665 - (view) Author: Martin Teichmann (Martin.Teichmann) * Date: 2016-09-10 18:14
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.
msg275697 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-10 20:59
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>
> _______________________________________
msg275731 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-11 04:20
Reassigning to myself given Guido's +1
msg275732 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-11 04:46
New changeset feb1ae9d5381 by Nick Coghlan in branch 'default':
Issue #23722: Initialize __class__ from type.__new__()
https://hg.python.org/cpython/rev/feb1ae9d5381
msg275733 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-09-11 04:49
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.
msg282240 - (view) Author: Tim Graham (Tim.Graham) * Date: 2016-12-02 14:34
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!
msg282246 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-02 15:46
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.
msg282248 - (view) Author: Tim Graham (Tim.Graham) * Date: 2016-12-02 16:12
Thanks Nick. Your suggestion does fix the issue for Django: https://github.com/django/django/pull/7653.
msg282270 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-03 06:45
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)
msg282317 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 06:47
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.
msg282318 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 06:49
Reassigning to Ned for now, pending finding another commit reviewer.
msg282320 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 06:52
In the meantime, I'll try to work out a suitable documentation patch.
msg282323 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 07:28
Attached patch covers the proposed documentation updates.
msg282324 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 08:49
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)
msg282328 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-04 10:26
Added comments on Rietveld.
msg282335 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 12:43
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
msg282336 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-04 13:00
Assuming there are no further comments overnight, I'll go ahead and commit this tomorrow (after doing a local refleak hunting run).
msg282337 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-12-04 13:14
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.
msg282383 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-05 03:50
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
msg282387 - (view) Author: Roundup Robot (python-dev) Date: 2016-12-05 06:59
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
msg282391 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-12-05 07:12
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
msg282474 - (view) Author: Roundup Robot (python-dev) Date: 2016-12-05 22:24
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
History
Date User Action Args
2017-03-31 16:36:27dstufftsetpull_requests: + pull_request1013
2016-12-05 22:24:33python-devsetmessages: + msg282474
2016-12-05 19:43:48ned.deilysetpriority: release blocker ->
assignee: ned.deily ->
2016-12-05 07:12:09ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg282391

stage: commit review -> resolved
2016-12-05 06:59:36python-devsetmessages: + msg282387
2016-12-05 03:50:10ncoghlansetmessages: + msg282383
2016-12-04 17:07:50gvanrossumsetnosy: - gvanrossum
2016-12-04 13:14:14serhiy.storchakasetmessages: + msg282337
2016-12-04 13:00:21ncoghlansetmessages: + msg282336
2016-12-04 12:43:29ncoghlansetfiles: + issue23722_classcell_reference_validation_v2.diff

messages: + msg282335
2016-12-04 12:04:05serhiy.storchakalinkissue24329 dependencies
2016-12-04 10:26:01serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg282328
2016-12-04 08:49:27ncoghlansetmessages: + msg282324
2016-12-04 07:28:21ncoghlansetfiles: + issue23722_documentation_updates.diff

messages: + msg282323
2016-12-04 07:19:00ebarrysetnosy: + ebarry
2016-12-04 06:52:20ncoghlansetmessages: + msg282320
2016-12-04 06:49:25ncoghlansetassignee: ncoghlan -> ned.deily
messages: + msg282318
2016-12-04 06:47:54ncoghlansetfiles: + issue23722_classcell_reference_validation.diff

stage: needs patch -> commit review
messages: + msg282317
versions: + Python 3.6, Python 3.7, - Python 3.5
2016-12-03 06:45:19ncoghlansetfiles: + issue23722_enhanced_classcell_tests.diff

messages: + msg282270
2016-12-02 16:12:29Tim.Grahamsetmessages: + msg282248
2016-12-02 15:46:44ncoghlansetstatus: closed -> open
priority: normal -> release blocker


nosy: + larry
messages: + msg282246
resolution: fixed -> (no value)
stage: resolved -> needs patch
2016-12-02 14:34:52Tim.Grahamsetnosy: + Tim.Graham
messages: + msg282240
2016-09-11 04:49:08ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg275733

stage: commit review -> resolved
2016-09-11 04:46:09python-devsetnosy: + python-dev
messages: + msg275732
2016-09-11 04:20:58ncoghlansetassignee: ned.deily -> ncoghlan
type: behavior
messages: + msg275731
stage: commit review
2016-09-10 20:59:12gvanrossumsetmessages: + msg275697
2016-09-10 18:14:19Martin.Teichmannsetfiles: + classcell.patch

messages: + msg275665
2016-09-10 16:46:50ncoghlansetmessages: + msg275655
2016-09-10 16:42:22gvanrossumsetmessages: + msg275652
2016-09-10 10:53:41ncoghlansetassignee: ned.deily

messages: + msg275620
nosy: + ned.deily
2016-07-18 02:16:32gvanrossumsetnosy: + gvanrossum
messages: + msg270675
2016-07-17 16:25:01Martin.Teichmannsetfiles: + classcell.patch
2016-07-17 16:23:52Martin.Teichmannsetfiles: - pep487.patch
2016-07-17 14:27:35Martin.Teichmannsetfiles: - patch
2016-07-17 14:27:19Martin.Teichmannsetfiles: + pep487.patch
keywords: + patch
messages: + msg270642
2016-07-14 16:47:24eric.snowsetnosy: + eric.snow
2015-03-23 09:11:00Martin.Teichmannsetmessages: + msg238992
2015-03-21 12:07:20ncoghlansetnosy: + ncoghlan
messages: + msg238795
2015-03-20 13:44:21Martin.Teichmanncreate