classification
Title: Expose dict_proxy internal type as types.MappingProxyType
Type: enhancement Stage: committed/rejected
Components: Interpreter Core Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, Jim.Jewett, eric.araujo, eric.snow, georg.brandl, giampaolo.rodola, gvanrossum, haypo, michael.foord, pitrou, poq, python-dev, r.david.murray, rhettinger, skrah
Priority: normal Keywords: patch

Created on 2012-03-22 01:50 by haypo, last changed 2012-04-19 23:42 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
dictproxy.patch haypo, 2012-03-22 01:50 review
dictproxy-2.patch haypo, 2012-03-22 12:35 review
mappingview-3.patch haypo, 2012-03-26 12:04 review
mappingview-4.patch haypo, 2012-03-29 11:52 review
mappingproxy-5.patch haypo, 2012-03-29 23:08 review
mappingproxy_abc.patch haypo, 2012-04-15 22:21 review
Messages (40)
msg156536 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 01:50
Attached patch makes the dictproxy type public. Example:

$ ./python 
Python 3.3.0a1+ (default:059489cec7b9+, Mar 22 2012, 02:45:36) 
>>> d=dictproxy({1:2})
>>> d
dict_proxy({1: 2})
>>> d[1]
2
>>> d[1]=3
TypeError: 'dictproxy' object does not support item assignment
>>> del d[1]
TypeError: 'dictproxy' object doesn't support item deletion
>>> d.copy()
{1: 2}
>>> dir(d)
[..., '__getitem__', 'copy', 'get', 'items', 'keys', 'values']

The patch doesn't have any test or documentation yet.

See also the (now rejected) PEP 416 (frozendict).
msg156538 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 01:58
dictproxy.patch: proxy_new() should check that dict is a dict (PyDict_Check).
msg156550 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 12:35
Patch version 2:

 - add tests and documentation
 - dictproxy code moved to dictproxyobject.c
 - dict_proxy replaced with dictproxy in repr(dictproxy)
 - "key in dictproxy" now handles correctly dict subclasses
 - dictproxy constructor raises a TypeError if the argument is not a dict

I added a section "Immutable Mapping" in the doc, this title is maybe wrong because a dictproxy is not truly immutable. If the dict referenced by the dictproxy is modified, the dictproxy is also modified.

dictproxy implementation supports any mapping, it is not specific to dict. It would be interesting any mapping, collections.ChainMap for example. The problem is to reject sequence in dictproxy constructor.

The "PyMapping_Check(dict) && !PyMapping_Check(dict)" fails on ChainMap. type.__new__(ChainMap) fills tp_as_sequence->sq_item slot is defined because ChainMap has a __getitem__ method.

Accepting sequences would be a bad idea because dictproxy(sequence)[missing_key] raises IndexError instead of KeyError.

Note: issubclass(collections.ChainMap, collections.abc.Sequence) is False.
msg156551 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-22 12:36
The dictproxy is useful to implement a Python sandbox: see the related issue #14385.
msg156634 - (view) Author: Jim Jewett (Jim.Jewett) Date: 2012-03-23 05:04
> The problem is to reject sequence in dictproxy constructor.

Why?  Just because you can't delegate in quite the same way?  A sequence *does* meet the (immutable) Mapping interface; it just won't happen to have any non-integer keys.

Or are you worried about IndexError vs KeyError?  Personally, I would just document it as returning LookupError, but if you're really worried, you could always catch the IndexError and raise a KeyError from it.

> The "PyMapping_Check(dict) && !PyMapping_Check(dict)" 

I'm not seeing how anything could sanely pass that ... was there a typo when pasting?
msg156780 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-25 23:07
> A sequence *does* meet the (immutable) Mapping interface

Ah? Let's try:

>>> x=[1, 2, 3]
>>> x.get(2)
AttributeError: 'list' object has no attribute 'get'
>>> x.keys()
AttributeError: 'list' object has no attribute 'keys'

list doesn't implement the collections.abc.Mapping ABC.
msg156781 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-25 23:07
> The "PyMapping_Check(dict) && !PyMapping_Check(dict)" 

Oh, I mean "PyMapping_Check(dict) && !PySequence_Check(dict)"
msg156783 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-25 23:15
I'm -1 on exposing this API.

Victor's sandboxing project doesn't warrant cluttering the public toolset.

As a consultant who gets to see many large codebases in different companies and I see zero need for this type to be exposed.  It is not hard to build proxies around any existing type.  Victor's use case is unique in that he needs the type to be implemented in C.  To that end, it would be okay to expose _dictproxy but not to make it a documented type.
msg156785 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-03-25 23:32
I believe it was actually Guido who suggested exposing dict_proxy, in response to Victor (but independent of Victor's needs, if I understood the message correctly).

It has always seemed odd to me ("always" referring my time working with Python3, of course) that a class that implements the mapping protocol has no way to return the same type of objects that real dict methods do.
msg156786 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-25 23:38
> Victor's use case is unique in that he needs the type
> to be implemented in C.

mxProxy, zope.proxy and zope.security implement object proxies in C (for security purpose). These proxies are not specific to dict, but are generic.

dictproxy is already used in Python by the type.__dict__ descriptor getter of new-style classes since Python 2.2, since this changeset:

changeset:   18933:09df3254b49d
branch:      legacy-trunk
user:        Tim Peters <tim.peters@gmail.com>
date:        Thu Aug 02 04:15:00 2001 +0000
files:       Include/Python.h Include/abstract.h Include/ceval.h Include/classobject.h Include/descrobject.h Include/dictobject.h Include/eval.h Include/func
description:
Merge of descr-branch back into trunk.

Guido van Rossum wrote in its rejection notice of the PEP 416: "On the other hand, exposing the existing read-only dict proxy as a built-in type sounds good to me. (It would need to be changed to allow calling the constructor.) GvR."

--

> To that end, it would be okay to expose _dictproxy
> but not to make it a documented type.

My sandbox only needs the issue #14385, pysandbox can implement its own dictproxy class.

If the issue #14385 is accepted, pysandbox can use any mapping, even a mapping implemented in Python! It's just simpler to implement a secure proxy in C.

So this issue is not directly related to sandboxing.
msg156788 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-25 23:47
It's clutter and we should show some restraint before adding new types.

Enthought's Python Distribution isn't shy about adding tools when they have a demonstrated need.  Likewise, Django adds any reusable tools they need.  Neither have a dict proxy.  I've never run into a use case for it and haven't seen any feature requests for it.   PyPi doesn't have package for it (at least not one with any uptake).  The languages doesn't *need* this, nor do the other implementations which are trying to keep up with CPython.

Guido may let you put in it, but that doesn't mean you should.  Again, I ask you to show some restraint.  The language has already exploded beyond what fits into most developer's heads (even core devs don't even know about many the newer features).
msg156789 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-25 23:54
> It's clutter and we should show some restraint before adding new types.

dictproxy() doesn't need to be a public builtin type. We can just implement its constructor without documenting the type. Or it may be exposed in a module like collections.
msg156792 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-03-26 00:23
Given that dictproxy is needed to implement the proper interface for classes I don't think it is an implementation detail, and so I think it ought to be constructable. We have more esoteric types constructable (e.g. function).

I also happen to think that its use to implement classes is a pretty darn good use case, and I am sure there are other use cases.

I don't really care if we make it a builtin but there should be a standard name for it somewhere in the stdlib.
msg156796 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-26 03:10
[haypo]
> dictproxy() doesn't need to be a public builtin type.

+1
 
> We can just implement its constructor without documenting the type.

+1

> Or it may be exposed in a module like collections.

Please don't use collections as a dumping ground for this sort of thing.
msg156815 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-26 12:04
New patch replacing dictproxy() builtin type with collections.mappingview() type:

 - dictproxy() type doesn't need to be a builtin type
 - it is not specific to dict so replace "dict" prefix with "mapping"
 - "view" is a better suffix than "proxy" to indicate that it is a read-only proxy
 - IMO collections is the best place for new containers

I don't understand how collections types are called: should it be MappingView or mappingview?

The code of mappingview is just moved to _collectionsmodule.c to avoid the creation of two short files. PyDictProxy_Type and PyDictProxy_New() are kept and still declared in descrobject.h for backward compatibility.

I fixed the doc to indicate that mappingview methods operate on the *underlying mapping*.

I also added test_views() test.

TODO:

 - mappingview() constructor only accepts dict: it should accept any mapping
 - mappingview.copy() creates a dict, it should create a new object of the same type than the underlying mapping
msg156860 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-26 19:56
I would very much like this to go somewhere other than the collections module.  As the maintainer of that module, I've worked hard to keep it clutter free (people frequently want to stick things in that module when they can't think of some place else to put it).

Perhaps consider the existing types module, the builtin namespace, or a new module for proxies (if dict_proxy goes in, other proxy types won't be far behind).
msg156864 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-03-26 20:10
Raymond, that sounds like a very irrational way of deciding where it should go.
msg156867 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2012-03-26 20:29
I wonder about the name: There is already a collections.abc.MappingView.
msg156868 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-26 20:38
The weakref collections objects (sets, dicts, etc) were all put in a separate module and that has worked out well.  I suggest doing the same for proxy objects.

In maintaining the itertools module, I learned long ago that adding new tools to the module made the whole module more difficult to use (increasing the number of choices increases the complexity of choosing the correct tool).  I believe that same also applies to the collections module and have been very reserved about adding new types there.  

ISTM that exposing internal types such as dict_proxy or bound/unbound methods could be collected together or that various proxy types and tools could be collected together.  I don't feel that dict_proxy belongs with namedtuple, deques, or OrderedDicts.

As the module maintainer, I request that dict_proxy be put elsewhere.  Of course, as BDFL you are free to override that request.
msg156871 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-03-26 20:57
I like the suggestion of a library module containing proxy classes.
msg156934 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-03-27 15:23
There's already a weak proxy class in the weakref module. Not sure a separate module for the other proxies is a good idea.
msg156936 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2012-03-27 15:53
If we do create a proxy module I'd like to suggest adding all or part of Phillip Eby's ProxyTypes:

http://pypi.python.org/pypi/ProxyTypes

I haven't used it directly, but did read through it a while ago and have ended up re-implementing chunks of it myself several times.
msg156971 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-03-28 05:23
weakref.mappingview sounds nice to me.
msg156973 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-03-28 06:41
But it has nothing to do with weakrefs, so...
msg156980 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-03-28 14:23
Since it has to go *somewhere*, let's put it in the types module.  It has a variety of other types that are used in the implementation of classes and functions and related things.
msg157008 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-03-28 20:00
[GvR]
> Since it has to go *somewhere*, let's put it 
> in the types module.  It has a variety of other 
> types that are used in the implementation of 
> classes and functions and related things.

+1
msg157012 - (view) Author: (poq) Date: 2012-03-29 06:25
It is exposed as types.DictProxyType in Python 2...
msg157030 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-29 11:08
"It is exposed as types.DictProxyType in Python 2..."

Yes, but the purpose of the issue is to enable its constructor. You cannot instanciate a DictProxy in Python 2:

>>> import types
>>> types.DictProxyType
<type 'dictproxy'>
>>> types.DictProxyType({})
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: cannot create 'dictproxy' instances
msg157036 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-29 11:52
Patch version 4, it is ready for a review. Summary of the patch:

 - expose the internal dict_proxy() type (used for __dict__ of user classes) as types.MappingViewType (it was exposed a types.DictProxyType in Python 2)
 - repr(types.MappingViewType) returns 'mappingview(...)' instead of 'dict_proxy(...)'
 - implement mappingview.__new__()
 - mappingview constructors (mappingview.__new__() and PyDictProxy_New) rejects sequences
 - Keep PyDictProxy_New() and PyDictProxy_Type names for backward compatibility
 - mappingview_contains() calls PySequence_Contains() for types other than dict (instead of PyDict_Contains)
 - Write a doc and a lot of tests
 - Rename the internal "dict" attribute to "mapping" (this change may be reverted if it impacts backward compatibility)

I added the new tests to test_descr. Is test_types a better place for these types (the type is exposed in the types module, but implemented in descrobject.c)? I leaved the object implementation in descrobject.c to not have to add a new short file and to keep the Mercurial history. The file does still contain this funny comment:

/* This has no reason to be in this file except that adding new files is a
   bit of a pain */

MappingViewType constructor accepts any mapping: dict, collections.ChainMap, ... Sequences are rejected: tuple, list, dict_view, etc.
msg157083 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2012-03-29 18:45
I'd like to bikeshed a little on the name. I think it should be
MappingProxy. (We don't use "view" much but the place where we do use
it, for keys/values/items views, is very different I think. Also
collections.abc already defines MappingView as the base class for
KeysView and friends.)

Also make sure there's no way for someone who has access to the proxy
to get to the underlying mapping; preventing that access is the
explicit purpose of the original dict_proxy type. You might even add a
comment to this effect to the code, so someone doesn't accidentally
add it in the future. Perhaps in the proxy_methods structure or in the
type structure just before tp_members.
msg157111 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-29 23:08
> (We don't use "view" much but the place where we do use
> it, for keys/values/items views, is very different I think.)

You are right, it's closer to a proxy because it has the same methods than a mapping (except of some methods used to modify a mapping), whereas the API of keys/values/items views is very different from the mapping API.

> Also collections.abc already defines MappingView as the base
> class for KeysView and friends.)

MappingView is a strange ABC: it should probably declare __contains__ as an abstract method. I don't see why it doesn't inherit from Set (which would avoid the need of declaring __contains__, Set is a Container) whereas KeysView, ItemsView and ValuesView do inherit from Set.

I suppose that MappingView is also present to factorize code but it should not be used directly.

Anyway, you are not the first one who remarks that we already use "view" to define something else, so I wrote a new patch to use the "mappingproxy" name (exposed as types.MappingProxyType).

> Also make sure there's no way for someone who has access
> to the proxy to get to the underlying mapping;

I don't think that we can write unit tests for such property. I suppose that the comment below is enough.

> You might even add a comment ...

Done in my last patch:

/* WARNING: mappingproxy methods must not give access to the underlying mapping */
msg157112 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-03-29 23:13
Oh, collections.abc contains also the mappingview type exposed with the name "dict_proxy":
dict_proxy = type(type.__dict__)

It was exposed as _abcoll.dict_proxy in Python 3.2.

It is not documented. Should we keep it for backward compatibility, or can it be removed? _abcoll was a private module and I didn't find dict_proxy in Python 3.2 doc.
msg157113 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-03-29 23:15
> MappingView is a strange ABC: it should probably declare __contains__ as an abstract method.
I think that was not done because it would not be very useful: MappingView seems to exist only as a base class for the other *View, which define __contains__.

> I don't see why it doesn't inherit from Set (which would avoid the need of declaring __contains__,
> Set is a Container) whereas KeysView, ItemsView and ValuesView do inherit from Set.
Not ValuesView, because you may have the same object as more than one value.
msg157517 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-04 23:05
mappingproxy-5.patch is ready for a review. msg157036 contains a summary except that types.MappingViewType is now called types.MappingProxyType.
msg157618 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-04-05 20:23
[Victor]
'''Oh, collections.abc contains also the mappingview type exposed with the name "dict_proxy": dict_proxy = type(type.__dict__)

It was exposed as _abcoll.dict_proxy in Python 3.2.
It is not documented. Should we keep it for backward compatibility, or can it be removed? _abcoll was a private module and I didn't find dict_proxy in Python 3.2 doc.
'''

You can ignore those.  As you saw, these are undocumented and not listed in __all__.  If we ever expose these, it will likely be through the types module (or less likely through the collections.abc module which is public in 3.3).  Probably, they won't get exposed at all because they are awkward to access directly and because they have implementation specific details (i.e. other implementations have their choice of how to implement various iterators and whatnot).
msg157619 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-05 20:35
> You can ignore those.

You mean that I can remove it?

It's a little bit surprising to find a concrete class in collections.abc. So I think that it's better to expose dict_proxy in types than in collections.abc.
msg157630 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2012-04-06 01:10
[Victor]
> You mean that I can remove it?

No, it needs to stay there.
Those concrete types aren't exposed to users
but they are used inside to collections.abcs
to register the types so that isinstance()
will work as expected.

Summary:  The collections.abc module is fine as-is.

Go ahead and add dictproxy to the types module.
Expose it there and document it there.
msg158374 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-15 22:17
New changeset c3a0197256ee by Victor Stinner in branch 'default':
Issue #14386: Expose the dict_proxy internal type as types.MappingProxyType
http://hg.python.org/cpython/rev/c3a0197256ee
msg158375 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2012-04-15 22:21
> Summary:  The collections.abc module is fine as-is.

Ok, but there is still an issue: issubclass(types.MappingProxyType, collections.abc.Mapping) is False. Attached registers MappingProxyType as a Mapping. Is it correct?

The patch also renames dict_proxy to mappingproxy. Is it backward incompatible? (collections.abc module was added to Python 3.3)
msg158776 - (view) Author: Roundup Robot (python-dev) Date: 2012-04-19 23:42
New changeset 34af3e74292d by Victor Stinner in branch 'default':
Close #14386: Register types.MappingProxyType as a Mapping
http://hg.python.org/cpython/rev/34af3e74292d
History
Date User Action Args
2012-06-23 21:48:09eric.snowlinkissue14226 superseder
2012-04-19 23:42:55python-devsetstatus: open -> closed
resolution: fixed
messages: + msg158776

stage: committed/rejected
2012-04-15 22:21:28hayposetfiles: + mappingproxy_abc.patch

messages: + msg158375
2012-04-15 22:17:04python-devsetnosy: + python-dev
messages: + msg158374
2012-04-06 01:10:26rhettingersetmessages: + msg157630
2012-04-05 20:35:36hayposetmessages: + msg157619
2012-04-05 20:23:46rhettingersetmessages: + msg157618
2012-04-05 12:02:41hayposettitle: Expose dict_proxy internal type as types.MappingViewType -> Expose dict_proxy internal type as types.MappingProxyType
2012-04-04 23:05:13hayposetmessages: + msg157517
2012-04-04 23:02:39hayposettitle: Expose dictproxy as a public type -> Expose dict_proxy internal type as types.MappingViewType
2012-03-30 21:27:04r.david.murraysettype: enhancement
2012-03-29 23:15:21eric.araujosetmessages: + msg157113
2012-03-29 23:13:00hayposetmessages: + msg157112
2012-03-29 23:08:35hayposetfiles: + mappingproxy-5.patch

messages: + msg157111
2012-03-29 18:45:19gvanrossumsetmessages: + msg157083
2012-03-29 11:52:30hayposetfiles: + mappingview-4.patch

messages: + msg157036
2012-03-29 11:33:21Arfreversetnosy: + Arfrever
2012-03-29 11:08:37hayposetmessages: + msg157030
2012-03-29 06:25:23poqsetnosy: + poq
messages: + msg157012
2012-03-28 20:00:00rhettingersetmessages: + msg157008
2012-03-28 14:23:48gvanrossumsetmessages: + msg156980
2012-03-28 06:41:55georg.brandlsetnosy: + georg.brandl
messages: + msg156973
2012-03-28 05:23:34eric.araujosetnosy: + eric.araujo
messages: + msg156971
2012-03-27 15:53:35michael.foordsetnosy: + michael.foord
messages: + msg156936
2012-03-27 15:23:11pitrousetnosy: + pitrou
messages: + msg156934
2012-03-27 14:06:34giampaolo.rodolasetnosy: + giampaolo.rodola
2012-03-26 20:57:38gvanrossumsetmessages: + msg156871
2012-03-26 20:38:08rhettingersetmessages: + msg156868
2012-03-26 20:29:48skrahsetnosy: + skrah
messages: + msg156867
2012-03-26 20:10:07gvanrossumsetmessages: + msg156864
2012-03-26 19:56:59rhettingersetmessages: + msg156860
2012-03-26 12:04:14hayposetfiles: + mappingview-3.patch

messages: + msg156815
2012-03-26 03:10:55rhettingersetmessages: + msg156796
2012-03-26 00:23:39gvanrossumsetmessages: + msg156792
2012-03-25 23:54:24hayposetmessages: + msg156789
2012-03-25 23:47:44rhettingersetmessages: + msg156788
2012-03-25 23:38:54hayposetmessages: + msg156786
2012-03-25 23:32:38r.david.murraysetnosy: + r.david.murray
messages: + msg156785
2012-03-25 23:15:13rhettingersetnosy: + rhettinger
messages: + msg156783
2012-03-25 23:07:56hayposetmessages: + msg156781
2012-03-25 23:07:25hayposetmessages: + msg156780
2012-03-23 05:04:40Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg156634
2012-03-22 12:36:39hayposetmessages: + msg156551
2012-03-22 12:35:58hayposetfiles: + dictproxy-2.patch

messages: + msg156550
2012-03-22 01:58:32eric.snowsetnosy: + eric.snow
2012-03-22 01:58:17hayposetmessages: + msg156538
2012-03-22 01:50:03haypocreate