Repository navigation
Expose dict_proxy internal type as types.MappingProxyType #58594
Description
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 2012-04-19.23:42:55.098>
created_at = <Date 2012-03-22.01:50:03.533>
labels = ['interpreter-core', 'type-feature']
title = 'Expose dict_proxy internal type as types.MappingProxyType'
updated_at = <Date 2012-04-19.23:42:55.097>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2012-04-19.23:42:55.097>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2012-04-19.23:42:55.098>
closer = 'python-dev'
components = ['Interpreter Core']
creation = <Date 2012-03-22.01:50:03.533>
creator = 'vstinner'
dependencies = []
files = ['24990', '24991', '25028', '25063', '25070', '25229']
hgrepos = []
issue_num = 14386
keywords = ['patch']
message_count = 40.0
messages = ['156536', '156538', '156550', '156551', '156634', '156780', '156781', '156783', '156785', '156786', '156788', '156789', '156792', '156796', '156815', '156860', '156864', '156867', '156868', '156871', '156934', '156936', '156971', '156973', '156980', '157008', '157012', '157030', '157036', '157083', '157111', '157112', '157113', '157517', '157618', '157619', '157630', '158374', '158375', '158776']
nosy_count = 15.0
nosy_names = ['gvanrossum', 'georg.brandl', 'rhettinger', 'pitrou', 'vstinner', 'giampaolo.rodola', 'eric.araujo', 'Arfrever', 'r.david.murray', 'michael.foord', 'skrah', 'python-dev', 'eric.snow', 'poq', 'Jim.Jewett']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue14386'
versions = ['Python 3.3']
Activity
vstinner commentedon Mar 22, 2012
Attached patch makes the dictproxy type public. Example:
The patch doesn't have any test or documentation yet.
See also the (now rejected) PEP-416 (frozendict).
vstinner commentedon Mar 22, 2012
dictproxy.patch: proxy_new() should check that dict is a dict (PyDict_Check).
vstinner commentedon Mar 22, 2012
Patch version 2:
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.
vstinner commentedon Mar 22, 2012
The dictproxy is useful to implement a Python sandbox: see the related issue bpo-14385.
jimjjewett commentedon Mar 23, 2012
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.
I'm not seeing how anything could sanely pass that ... was there a typo when pasting?
vstinner commentedon Mar 25, 2012
Ah? Let's try:
list doesn't implement the collections.abc.Mapping ABC.
vstinner commentedon Mar 25, 2012
Oh, I mean "PyMapping_Check(dict) && !PySequence_Check(dict)"
rhettinger commentedon Mar 25, 2012
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.
bitdancer commentedon Mar 25, 2012
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.
vstinner commentedon Mar 25, 2012
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 <[email protected]>
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."
--
My sandbox only needs the issue bpo-14385, pysandbox can implement its own dictproxy class.
If the issue bpo-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.
rhettinger commentedon Mar 25, 2012
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).
vstinner commentedon Mar 25, 2012
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.
gvanrossum commentedon Mar 26, 2012
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.
rhettinger commentedon Mar 26, 2012
[haypo]
+1
+1
Please don't use collections as a dumping ground for this sort of thing.
16 remaining items
vstinner commentedon Mar 29, 2012
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.
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).
I don't think that we can write unit tests for such property. I suppose that the comment below is enough.
Done in my last patch:
/* WARNING: mappingproxy methods must not give access to the underlying mapping */
vstinner commentedon Mar 29, 2012
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.
merwok commentedon Mar 29, 2012
[-]Expose dictproxy as a public type[/-][+]Expose dict_proxy internal type as types.MappingViewType[/+]vstinner commentedon Apr 4, 2012
mappingproxy-5.patch is ready for a review. msg157036 contains a summary except that types.MappingViewType is now called types.MappingProxyType.
[-]Expose dict_proxy internal type as types.MappingViewType[/-][+]Expose dict_proxy internal type as types.MappingProxyType[/+]rhettinger commentedon Apr 5, 2012
[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).
vstinner commentedon Apr 5, 2012
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.
rhettinger commentedon Apr 6, 2012
[Victor]
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.
python-dev commentedon Apr 15, 2012
New changeset c3a0197256ee by Victor Stinner in branch 'default':
Issue bpo-14386: Expose the dict_proxy internal type as types.MappingProxyType
http://hg.python.org/cpython/rev/c3a0197256ee
vstinner commentedon Apr 15, 2012
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)
python-dev commentedon Apr 19, 2012
New changeset 34af3e74292d by Victor Stinner in branch 'default':
Close bpo-14386: Register types.MappingProxyType as a Mapping
http://hg.python.org/cpython/rev/34af3e74292d