classification
Title: ABCs for MappingViews should declare __slots__ so subclasses aren't forced to have __dict__/__weakref__
Type: enhancement Stage:
Components: Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: josh.r, python-dev, rhettinger
Priority: normal Keywords: patch

Created on 2014-05-03 00:02 by josh.r, last changed 2014-05-04 21:35 by josh.r. This issue is now closed.

Files
File name Uploaded Description Edit
slots_for_mappingview_abcs.patch josh.r, 2014-05-03 00:11 review
collections_abc_cleanup.patch josh.r, 2014-05-03 00:18 review
Messages (7)
msg217809 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-05-03 00:02
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.
msg217810 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-05-03 00:11
Hmm... First patch isn't generating a review diff (I'm guessing because my VM's time sync went farkakte). Trying again.
msg217811 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-05-03 00:18
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.
msg217815 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-05-03 04:50
At first glance, this seems like a reasonable request.
I leave it open for comments for a while before proceeding.
msg217852 - (view) Author: Roundup Robot (python-dev) Date: 2014-05-04 02:06
New changeset 2c6a231e2c1e by Raymond Hettinger in branch 'default':
Issue #21421:  Add __slots__ to the MappingViews ABCs.
http://hg.python.org/cpython/rev/2c6a231e2c1e
msg217853 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-05-04 02:09
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.
msg217893 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-05-04 21:35
Cool. Thanks for the feedback. I split it into two patches for a reason; I wasn't wedded to the simplification.
History
Date User Action Args
2014-05-04 21:35:49josh.rsetmessages: + msg217893
2014-05-04 02:09:57rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg217853
2014-05-04 02:06:40python-devsetnosy: + python-dev
messages: + msg217852
2014-05-03 04:50:08rhettingersetassignee: rhettinger
type: enhancement
components: + Library (Lib)
versions: + Python 3.5
nosy: + rhettinger

messages: + msg217815
2014-05-03 00:18:15josh.rsetfiles: + collections_abc_cleanup.patch

messages: + msg217811
2014-05-03 00:12:08josh.rsetfiles: - slots_for_mappingview_abcs.patch
2014-05-03 00:11:03josh.rsetfiles: + slots_for_mappingview_abcs.patch

messages: + msg217810
2014-05-03 00:02:41josh.rcreate