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

ABCs for MappingViews should declare __slots__ so subclasses aren't forced to have __dict__/__weakref__ #65620

Closed
MojoVampire mannequin opened this issue May 3, 2014 · 7 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@MojoVampire
Copy link
Mannequin

MojoVampire mannequin commented May 3, 2014

BPO 21421
Nosy @rhettinger, @MojoVampire
Files
  • slots_for_mappingview_abcs.patch
  • collections_abc_cleanup.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 = 'https://github.com/rhettinger'
    closed_at = <Date 2014-05-04.02:09:57.033>
    created_at = <Date 2014-05-03.00:02:41.932>
    labels = ['type-feature', 'library']
    title = "ABCs for MappingViews should declare __slots__ so subclasses aren't forced to have __dict__/__weakref__"
    updated_at = <Date 2014-05-04.21:35:49.950>
    user = 'https://github.com/MojoVampire'

    bugs.python.org fields:

    activity = <Date 2014-05-04.21:35:49.950>
    actor = 'josh.r'
    assignee = 'rhettinger'
    closed = True
    closed_date = <Date 2014-05-04.02:09:57.033>
    closer = 'rhettinger'
    components = ['Library (Lib)']
    creation = <Date 2014-05-03.00:02:41.932>
    creator = 'josh.r'
    dependencies = []
    files = ['35142', '35143']
    hgrepos = []
    issue_num = 21421
    keywords = ['patch']
    message_count = 7.0
    messages = ['217809', '217810', '217811', '217815', '217852', '217853', '217893']
    nosy_count = 3.0
    nosy_names = ['rhettinger', 'python-dev', 'josh.r']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21421'
    versions = ['Python 3.5']

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented May 3, 2014

    Unlike the other collections ABCs, MappingView and its subclasses (Keys|Items|Values)View don't define __slots__, so users inheriting them to take advantage of the mix-in methods get a __dict__ and __weakref__, even if they try to avoid it by declaring their own __slots__. This is sub-optimal (I don't think any library class should leave __slots__ undefined, but it's particularly bad when they're intended to be used a base classes).

    I've attached a patch that defines __slots__ for all of them; MappingView explicitly declares its _mapping slot, and the rest declare no slots at all.

    Only negative I can think of is that if the user provides their own __init__, doesn't call the super().__init__, and uses a different name than _mapping for whatever data structure they actually use, then there will be a pointer reserved for _mapping that never gets set. That said, if they don't define _mapping, none of the mix-in methods work anyway, so they may as well not inherit directly, and instead just use *View.register to become a virtual subclass.

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented May 3, 2014

    Hmm... First patch isn't generating a review diff (I'm guessing because my VM's time sync went farkakte). Trying again.

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented May 3, 2014

    I also wrote a slightly more thorough patch that simplifies some of the method implementations (largely just replacing a bunch of for/if/return True/False else return False/True with any/all calls against a generator expression).

    The one behavior difference is that I dramatically simplified Sequence's __iter__; the original just kept indexing until it got IndexError, the replacement just runs len(self) times, on the theory that a self-mutating Sequence should write its own __iter__, and we shouldn't jump through hoops to accommodate unsupported behavior like mutating a Sequence during iteration.

    All tests pass under both patches.

    @rhettinger
    Copy link
    Contributor

    At first glance, this seems like a reasonable request.
    I leave it open for comments for a while before proceeding.

    @rhettinger rhettinger added the stdlib Python modules in the Lib dir label May 3, 2014
    @rhettinger rhettinger self-assigned this May 3, 2014
    @rhettinger rhettinger added the type-feature A feature request or enhancement label May 3, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 4, 2014

    New changeset 2c6a231e2c1e by Raymond Hettinger in branch 'default':
    Issue bpo-21421: Add __slots__ to the MappingViews ABCs.
    http://hg.python.org/cpython/rev/2c6a231e2c1e

    @rhettinger
    Copy link
    Contributor

    I've applied the __slots__ patch. Thank you for submitting it.

    Am leaving the rest of the ABCs code as-is. The current form may be a bit wordy but it is clean, fast, and easy to trace through a debugger. The expanded forms were chosen for a reason.

    @MojoVampire
    Copy link
    Mannequin Author

    MojoVampire mannequin commented May 4, 2014

    Cool. Thanks for the feedback. I split it into two patches for a reason; I wasn't wedded to the simplification.

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant