Improved handling of __class__ assignment

[njsmith]

BPO	22986
Nosy	@ncoghlan, @benjaminp, @njsmith, @ethanfurman, @ericsnowcurrently
Files	better-class-assignment-v1.patch better-class-assignment-v2.patch

^{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:

assignee = None
closed_at = <Date 2015-01-30.18:33:54.795>
created_at = <Date 2014-12-02.23:39:21.174>
labels = ['interpreter-core', 'type-feature']
title = 'Improved handling of __class__ assignment'
updated_at = <Date 2017-09-08.21:37:20.167>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2017-09-08.21:37:20.167>
actor = 'eric.snow'
assignee = 'none'
closed = True
closed_date = <Date 2015-01-30.18:33:54.795>
closer = 'python-dev'
components = ['Interpreter Core']
creation = <Date 2014-12-02.23:39:21.174>
creator = 'njs'
dependencies = []
files = ['37346', '37347']
hgrepos = []
issue_num = 22986
keywords = ['patch']
message_count = 10.0
messages = ['232061', '232064', '233704', '234052', '234053', '234154', '234156', '235046', '270286', '301737']
nosy_count = 6.0
nosy_names = ['ncoghlan', 'benjamin.peterson', 'njs', 'ethan.furman', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22986'
versions = ['Python 3.5']

[njsmith]

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.

[njsmith]

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.

[njsmith]

I hereby invoke the one month ping rule! Patch, be pinged!

[benjaminp]

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. :))

[benjaminp]

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.

[python-dev]

New changeset d3671e6ba106 by Benjamin Peterson in branch 'default':
merge 3.4 (bpo-22986)
https://hg.python.org/cpython/rev/d3671e6ba106

[benjaminp]

(That message should have gone to bpo-23250.)

[python-dev]

New changeset c0d25de5919e by Benjamin Peterson in branch 'default':
allow changing __class__ between a heaptype and non-heaptype in some cases (closes bpo-22986)
https://hg.python.org/cpython/rev/c0d25de5919e

[ncoghlan]

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.

[ericsnowcurrently]

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