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

Make class definition namespace ordered by default #68442

Closed
ericsnowcurrently opened this issue May 21, 2015 · 35 comments
Closed

Make class definition namespace ordered by default #68442

ericsnowcurrently opened this issue May 21, 2015 · 35 comments
Assignees
Labels
deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@ericsnowcurrently
Copy link
Member

BPO 24254
Nosy @jcea, @ncoghlan, @vstinner, @ned-deily, @ethanfurman, @ericsnowcurrently, @berkerpeksag
Dependencies
  • bpo-16991: Add OrderedDict written in C
  • Files
  • odict-class-definition-namespace.diff
  • just-default-to-odict.diff
  • deforder.diff
  • deforder-with-tp-slot.diff
  • 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 = 'https://github.com/ericsnowcurrently'
    closed_at = <Date 2016-12-07.00:56:52.742>
    created_at = <Date 2015-05-21.05:34:48.307>
    labels = ['interpreter-core', 'deferred-blocker', 'type-bug']
    title = 'Make class definition namespace ordered by default'
    updated_at = <Date 2017-05-17.18:21:22.630>
    user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2017-05-17.18:21:22.630>
    actor = 'jcea'
    assignee = 'eric.snow'
    closed = True
    closed_date = <Date 2016-12-07.00:56:52.742>
    closer = 'ncoghlan'
    components = ['Interpreter Core']
    creation = <Date 2015-05-21.05:34:48.307>
    creator = 'eric.snow'
    dependencies = ['16991']
    files = ['39446', '42834', '43169', '43739']
    hgrepos = []
    issue_num = 24254
    keywords = ['patch']
    message_count = 35.0
    messages = ['243729', '243767', '243902', '244055', '244056', '244060', '244061', '244075', '244077', '265434', '266957', '266981', '267183', '267319', '269846', '270431', '270517', '273881', '273887', '273898', '274441', '275188', '275337', '275347', '275580', '275585', '275605', '275606', '275622', '275624', '275656', '275660', '275662', '282574', '282583']
    nosy_count = 9.0
    nosy_names = ['jcea', 'ncoghlan', 'vstinner', 'ned.deily', 'ethan.furman', 'fijall', 'python-dev', 'eric.snow', 'berker.peksag']
    pr_nums = []
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue24254'
    versions = ['Python 3.6']

    @ericsnowcurrently
    Copy link
    Member Author

    Here's a work-in-progress patch that uses OrderedDict as the default class definition namespace (instead of dict). While the actual class namespace is a dict, the definition order is preserved on a new attribute on class objects: __definition_order__. This is useful for cases such as class decorators that would make use of the definition order to post-process the class. This is something that Guido okay'ed a while back.

    It may be a stretch to get this into 3.5, but the final change should be pretty small. It also depends on the C implementation of OrderedDict (see issue bpo-16991) which has not landed and may not make it in time.

    Note that the patch is still a bit buggy, partly due to bugs in the C OrderedDict patch. Also, the attached patch is based off the C OrderedDict patch, so I expect reitveld won't be able to handle it. The code on which the patch is based may be found in the "class-namespace" branch of https://pypi.python.org/pypi/cyordereddict.

    @ericsnowcurrently ericsnowcurrently self-assigned this May 21, 2015
    @ericsnowcurrently ericsnowcurrently added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels May 21, 2015
    @ericsnowcurrently
    Copy link
    Member Author

    Here's a patch that drops adding definition_order. You can get the same effect by adding __definition_order__ = list(locals()) at the bottom of your class definition. The benefit of having __definition_order__ is that the information is automatically available for later use rather than forcing the class author to add the above line in order to benefit from external introspection/manipulation. The downside is that every syntax-defined class will have the extra attribute with the list of definition order, which might be an unnecessary waste, even if not a huge one.

    Ultimately, I'd prefer to keep __definition_order__.

    @ncoghlan
    Copy link
    Contributor

    types.prepare_class() also needs to be updated to use OrderedDict() by default.

    @ericsnowcurrently
    Copy link
    Member Author

    Per discussion on python-dev, I'm tabling the __definition_order__ part to 3.6. I'll open a thread on python-ideas on it later and open a new issue here if I get a positive response.

    So this issue is just about making OrderedDict the default namespace type for class definitions.

    @vstinner
    Copy link
    Member

    It may be a stretch to get this into 3.5, but the final change should be pretty small.

    Changing the default type of class dictionaries is a huge change. IMO it should be deferred to Python 3.6.

    @ericsnowcurrently
    Copy link
    Member Author

    This is not about changing the default type for class dictionaries. It is only for changing the default type used during class definition. Essentially, we are just changing the type of what is returned from type.__prepare__. cls.dict will remain a dict.

    @ncoghlan
    Copy link
    Contributor

    This is *not* about changing the default type of class dictionaries (which
    I agree would be far too large a change to make without a PEP), it's only
    about changing the ephemeral evaluation namespace used to execute the class
    body.

    @ericsnowcurrently
    Copy link
    Member Author

    Thanks for pointing out types.prepare_class. I've updated it.

    @ericsnowcurrently
    Copy link
    Member Author

    I've moved this to 3.6. Small as the patch might be, there just isn't enough urgency to warrant making use of an exception to get it into 3.5. If __definition_order__ were still on the table then I'd probably still push for 3.5. :)

    @ericsnowcurrently
    Copy link
    Member Author

    Here's a refresh of the patch that only sets the default definition namespace (and does not introduce __definition_order__).

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 2, 2016

    Patch review sent. The motivation for the change is relatively weak without __definition_order__, though :)

    @ericsnowcurrently
    Copy link
    Member Author

    Thanks. Yeah, I wanted to keep the patches separate for the sake of code review. I'll fold the changes into a single commit once everything's ready.

    @ericsnowcurrently
    Copy link
    Member Author

    Here's the full patch, including the addition of __definition_order__, tests, and docs.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jun 4, 2016

    The patch looks good to me as a reference implementation, but I think it's worth writing up as a short PEP so there's a clear reference from the What's New documentation, and so developers of other implementations have a chance to review and comment on the change.

    @ericsnowcurrently
    Copy link
    Member Author

    Here's an updated patch that matches the accepted PEP. Most notably I've added a tp_deforder slot. This was necessary because subclasses should not inherit their parent's __definition_order__ and the ability to delete cls.__definition_order__ makes this problematic. I didn't see a way to do that without adding a new type slot.

    @ericsnowcurrently
    Copy link
    Member Author

    FTR: PEP-520 "Preserving Class Attribute Definition Order" (https://www.python.org/dev/peps/pep-0520/)

    @ericsnowcurrently
    Copy link
    Member Author

    post-review updates

    @brettcannon
    Copy link
    Member

    Any update on this? b1 is exactly 2 weeks away at this point.

    @vstinner
    Copy link
    Member

    Brett Cannon added the comment:

    Any update on this? b1 is exactly 2 weeks away at this point.

    Why do we need changes specific to classes, if dictionaries could be ordered by default?

    Issue bpo-27350: "Compact and ordered dict" by INADA Naoki

    @ncoghlan
    Copy link
    Contributor

    Because we're not making ordered-by-default dicts a language specification level requirement, so portable code can't rely on them.

    However, the PEP at https://www.python.org/dev/peps/pep-0520/ is deliberately worded so that there's no requirement for the class body execution namespace to specifically be collections.OrderedDict - it just needs to be order preserving so that __definition_order__ can be populated.

    If someone reviews and merges the compact ordered dict patch, then this patch can likely be made a lot simpler.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 5, 2016

    New changeset 635fd3912d4d by Eric Snow in branch 'default':
    Issue bpo-24254: Preserve class attribute definition order.
    https://hg.python.org/cpython/rev/635fd3912d4d

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 8, 2016

    New changeset 9aa5424fd1df by Eric Snow in branch 'default':
    Issue bpo-24254: Drop cls.__definition_order__.
    https://hg.python.org/cpython/rev/9aa5424fd1df

    @ethanfurman
    Copy link
    Member

    Is this going to be added back? Should I add __definition_order__ to Enum?

    @brettcannon
    Copy link
    Member

    Nope, PEP-520 has been updated to drop __definition_order__ and instead state that cls.__dict__ is an ordered mapping.

    @ethanfurman
    Copy link
    Member

    Not having a __definition_order__ could be a disadvantage for classes making use of __getattr__ for virtual attributes, such as Enum and proxy classes.

    With __definition_order__, and __dir__, Enum could present a coherent view; without it it's a big jumbled mess with many attributes the user never wrote.

    @vstinner
    Copy link
    Member

    You should reopen the discussion on python-dev, since the PEP-520 has
    been accepted with:

    "Note: Since compact dict has landed in 3.6, __definition_order__ has
    been removed. cls.__dict__ now mostly accomplishes the same thing
    instead."

    The PEP status is now Final.

    @ncoghlan
    Copy link
    Contributor

    The danger of sprints, and decisions being made without adequate input from affected parties.

    @ncoghlan
    Copy link
    Contributor

    Ethan started a python-dev thread here: https://mail.python.org/pipermail/python-dev/2016-September/146358.html

    @ncoghlan
    Copy link
    Contributor

    Reopening until the __definition_order__ question has been resolved.

    @ncoghlan ncoghlan reopened this Sep 10, 2016
    @ncoghlan
    Copy link
    Contributor

    I don't think this is necessarily a blocker for beta 1 (since it can be treated as a bug fix for beta 2 if it's decided to restore the originally proposed behaviour), but it should definitely be resolved before beta 2.

    @brettcannon
    Copy link
    Member

    Do realize that the PEP author was there and made the decision along with Guido to not move forward with a new feature that has not seen the light of day in a stable release, so I don't think blaming the sprinting environment is entirely fair in this case.

    @ncoghlan
    Copy link
    Contributor

    While I hadn't read the related thread at the point where I made that comment (so thank you for at least raising the question there), I'm still on the opposite side of the planet, so any decision made in less than 24 hours from proposal to resolution is necessarily "too fast" for global collaboration (and 48 hours is better).

    I'm not particularly fond of __definition_order__ either, but one of the specific points raised in the PEP-520 discussions due to Inada-san's pending patch was whether or not making class namespaces ordered by default would eliminate the need for the attribute, and the conclusion was that it *wouldn't*. So the "We made class namespaces ordered, so now __definition_order__ is gone" sounded a lot like folks forgetting that part of the discussions, and instead getting rid of a feature that was there for multiple reasons, not just because __dict__ didn't preserve the order.

    @brettcannon
    Copy link
    Member

    All reasonable points. You just sounded upset and I wanted to point out the decision was not made lightly, without discussion with the person in charge of the proposal and the BDFL, or we were breaking backwards-compatibility due to some drunken ordered dictionary stupor we were in. :)

    Anyway, that's it and any further comments I have on this specific topic I'll put on the python-dev thread.

    @ned-deily
    Copy link
    Member

    What's the status of this issue? It's still marked as a "deferred blocker" for 3.6.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 7, 2016

    Thanks for the ping Ned.

    Closing this again, as Guido explained the rationale for the change in the python-dev thread referenced above: https://mail.python.org/pipermail/python-dev/2016-September/146371.html

    @ncoghlan ncoghlan closed this as completed Dec 7, 2016
    @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
    deferred-blocker interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants