classification
Title: Improved handling of __class__ assignment
Type: enhancement Stage: resolved
Components: Interpreter Core Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, eric.snow, ethan.furman, ncoghlan, njs, python-dev
Priority: normal Keywords: patch

Created on 2014-12-02 23:39 by njs, last changed 2017-09-08 21:37 by eric.snow. This issue is now closed.

Files
File name Uploaded Description Edit
better-__class__-assignment-v1.patch njs, 2014-12-02 23:39 review
better-__class__-assignment-v2.patch njs, 2014-12-03 00:04 review
Messages (10)
msg232061 - (view) Author: Nathaniel Smith (njs) * Date: 2014-12-02 23:39
Following on from the discussion starting here:

http://thread.gmane.org/gmane.comp.python.devel/150438/focus=150604

Here's a patch to improve __class__ assignment.

1) We remove the HEAPTYPE check from object_set_class, and move it to same_slots_added. This fixes an outright bug: previously it was possible for non-HEAPTYPE types to get passed into same_slots_added (either b/c the loop in compatible_for_assignment ended up with non-HEAPTYPE types, or via __bases__ assignment), and it would then wander off following pointers through random memory.

2) Now that object_set_class can potentially accept non-HEAPTYPE types, adjust the reference counting appropriately.

3) To clarify the logic of compatible_for_assignment, rename equiv_structs to compatible_with_tp_base, tweak accordingly, and add a nice comment explaining the logic. (compatible_for_assignment is equiv_structs's only caller so this is totally safe.)

4) Now that equiv_structs/compatible_with_tp_base has a clearer purpose, also move the tp_dealloc check from compatible_for_assignment into compatible_with_tp_base. In the process, add special-case handling for subclass_dealloc, b/c subclass_dealloc delegates to the parent class tp_dealloc anyway.

These changes together make it possible to assign to module instances's __class__ slot, which is useful for reasons described in the above thread. So I added a test for this to check the new code.
msg232064 - (view) Author: Nathaniel Smith (njs) * Date: 2014-12-03 00:04
Here's a slightly improved patch spurred by a parenthetical comment of Antoine's on the mailing list :-).

The only change is that it adds a check in subclass_dealloc to correct the reference counting in the weird case that someone converts a HEAPTYPE object into a non-HEAPTYPE object while it is being deallocated.

I should probably also mention that I ran the full test suite against the patched version and everything passed; and with -R 3:2 the only difference is a new reference leak in test_zipfile. I'm guessing this might be spurious, given that AFAIK this test shouldn't be touching __class__ assignment at all? But IDK.
msg233704 - (view) Author: Nathaniel Smith (njs) * Date: 2015-01-09 01:00
I hereby invoke the one month ping rule! Patch, be pinged!
msg234052 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-01-15 05:27
Why do you have all these special cases of ref-counting based on Py_TPFLAGS_HEAPTYPE? "static" types are generally ref-counted, too. (They just hopefully don't reach 0 often. :))
msg234053 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-01-15 05:31
Oh, I forgot references to non-heaptypes in the header don't count... objects now being able to transmute between heap and heap-type really makes for a nice can of worms.
msg234154 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-17 01:46
New changeset d3671e6ba106 by Benjamin Peterson in branch 'default':
merge 3.4 (#22986)
https://hg.python.org/cpython/rev/d3671e6ba106
msg234156 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-01-17 01:48
(That message should have gone to #23250.)
msg235046 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-30 18:33
New changeset c0d25de5919e by Benjamin Peterson in branch 'default':
allow changing __class__ between a heaptype and non-heaptype in some cases (closes #22986)
https://hg.python.org/cpython/rev/c0d25de5919e
msg270286 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2016-07-13 01:33
Just noting I filed http://bugs.python.org/issue27505 regarding the lack of documentation for the new-in-Python-3.5 ability to set module __class__ attributes.
msg301737 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-08 21:37
The gmane link isn't working for me.  Here's the mailing list archive thread:

https://mail.python.org/pipermail/python-dev/2014-November/thread.html#137262
https://mail.python.org/pipermail/python-dev/2014-December/thread.html#137370
History
Date User Action Args
2017-09-08 21:37:20eric.snowsetmessages: + msg301737
2016-07-13 01:33:02ncoghlansetmessages: + msg270286
2016-07-13 01:32:13ncoghlanlinkissue27505 dependencies
2015-01-30 18:33:54python-devsetstatus: open -> closed
resolution: fixed
messages: + msg235046

stage: patch review -> resolved
2015-01-17 01:48:01benjamin.petersonsetmessages: + msg234156
2015-01-17 01:46:55python-devsetnosy: + python-dev
messages: + msg234154
2015-01-15 05:31:37benjamin.petersonsetmessages: + msg234053
2015-01-15 05:27:38benjamin.petersonsetmessages: + msg234052
2015-01-09 16:09:03berker.peksagsetnosy: + benjamin.peterson
stage: patch review
type: enhancement

versions: + Python 3.5
2015-01-09 01:01:00njssetmessages: + msg233704
2014-12-03 00:43:15eric.snowsetnosy: + eric.snow
2014-12-03 00:17:15ncoghlansetnosy: + ncoghlan
2014-12-03 00:04:19njssetfiles: + better-__class__-assignment-v2.patch

messages: + msg232064
2014-12-02 23:47:31ethan.furmansetnosy: + ethan.furman
2014-12-02 23:39:21njscreate