Skip to content
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

Improved handling of __class__ assignment #67175

Closed
njsmith opened this issue Dec 2, 2014 · 10 comments
Closed

Improved handling of __class__ assignment #67175

njsmith opened this issue Dec 2, 2014 · 10 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@njsmith
Copy link
Contributor

njsmith commented Dec 2, 2014

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
    Copy link
    Contributor Author

    njsmith commented Dec 2, 2014

    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 njsmith added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Dec 2, 2014
    @njsmith
    Copy link
    Contributor Author

    njsmith commented Dec 3, 2014

    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
    Copy link
    Contributor Author

    njsmith commented Jan 9, 2015

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

    @berkerpeksag berkerpeksag added the type-feature A feature request or enhancement label Jan 9, 2015
    @benjaminp
    Copy link
    Contributor

    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
    Copy link
    Contributor

    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
    Copy link
    Mannequin

    python-dev mannequin commented Jan 17, 2015

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

    @benjaminp
    Copy link
    Contributor

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 30, 2015

    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

    @python-dev python-dev mannequin closed this as completed Jan 30, 2015
    @ncoghlan
    Copy link
    Contributor

    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
    Copy link
    Member

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants