New .mapping attribute is broken for some existing uses of dict views

[jab]

BPO	45670
Nosy	@rhettinger, @jab

^{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 2021-10-31.19:46:24.280>
created_at = <Date 2021-10-29.16:36:50.417>
labels = ['interpreter-core', 'type-bug', 'invalid', '3.10']
title = 'New .mapping attribute is broken for some existing uses of dict views'
updated_at = <Date 2022-02-10.20:48:44.969>
user = 'https://github.com/jab'

bugs.python.org fields:

activity = <Date 2022-02-10.20:48:44.969>
actor = 'jab'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2021-10-31.19:46:24.280>
closer = 'rhettinger'
components = ['Interpreter Core']
creation = <Date 2021-10-29.16:36:50.417>
creator = 'jab'
dependencies = []
files = []
hgrepos = []
issue_num = 45670
keywords = []
message_count = 6.0
messages = ['405317', '405417', '405421', '409431', '409443', '413023']
nosy_count = 2.0
nosy_names = ['rhettinger', 'jab']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue45670'
versions = ['Python 3.10']

[jab]

As of bpo-40890 (released in Python 3.10), dict views now provide a public .mapping attribute, intended to allow users to recover a mappingproxy pointing to the original mapping.

However, this new attribute can actually point to the wrong mapping for some existing uses of dict views. And since the .mapping attribute is read-only, these existing uses have no way to set it to the correct value.

My bidict library (see https://github.com/jab/bidict) provides an example of this.

A bidict implements a bidirectional mapping by building on top of two dicts (i.e. regular old one-directional mappings) internally -- one for the forward direction, and one for the inverse. When you call e.g. keys() or values() on a bidict, you get back a dict_keys view from one of the backing dicts, because this is a much more optimized implementation of these views than collections.abc.KeysView would be:

>>> import bidict
>>> b = bidict.bidict(one=1, two=2)
>>> b
bidict({'one': 1, 'two': 2})
>>> b.keys()
dict_keys(['one', 'two'])
>>> b.values()
dict_keys([1, 2])

However, now that these built-in dict_keys objects provide a .mapping attribute in Python 3.10, it points to one of the internal, backing dicts in this case, which is an implementation detail, rather than to the bidict instance:

>>> b.keys().mapping  # wrong
mappingproxy({'one': 1, 'two': 2})
>>> b.values().mapping  # wrong
mappingproxy({1: 'one', 2: 'two'})

Instead of the above, you should get:

>>> b.keys().mapping  # corrected:
mappingproxy(bidict({'one': 1, 'two': 2}))
>>> b.values().mapping  # corrected:
mappingproxy(bidict({'one': 1, 'two': 2}))

Since the .mapping attribute is read-only, there's no way for bidict to both keep exposing the optimized dict_keys implementations, which up till now have been perfectly correct, while now exposing a correct .mapping attribute for users of Python 3.10+.

(Other bidict types demonstrate this problem more by exposing even more obviously-unintended implementation details via this new .mapping attribute:

>>> f = bidict.FrozenOrderedBidict(b)
>>> f
FrozenOrderedBidict([('one', 1), ('two', 2)])
>>> f.keys().mapping  # ouch
mappingproxy({'one': _Node(prv=..., self=..., nxt=...), 'two': _Node(prv=..., self=..., nxt=...)})

Those internal _Node objects were never meant to be exposed to consumers, they're an implementation detail.)

It looks like cases like this were not considered when discussing bpo-40890, and if they had been, I wonder if the implementation would have been accepted as-is.

Here are some potential directions for how to improve things for the future:

1. Expose a way for dict view users like bidict to continue to use optimized dict view implementations while opting out of the new .mapping attribute

2. Make the .mapping attribute no longer read-only, so libraries like bidict can set it correctly before exposing it to users

3. Merely update the documentation in https://docs.python.org/3/library/stdtypes.html#:~:text=dictview.mapping,in%20version%203.10. to mention that, because the .mapping attribute is read-only, it may not point to the original, intended mapping, but rather some internal mapping that the user was not intended to be exposed to.

Looking forward to hearing your thoughts, and thanks for your consideration.

[rhettinger]

Sorry, but this isn't bug in Python. The documented and supported API is the MappingView ABC where the _mapping attribute is private.

As an optimization, the bidict project has elected to forgo the supported API and instead use a concrete implementation designed specifically for actual dictionaries. This is an unsupported use and the constructor is not a public API.

We don't even do this internally in the standard library. OrderedDict, for example, has its own odict_keys() type for the C version and _OrderedDictKeysView() for the pure python version.

It would be possible for us to add a set_mapping() method or make the attribute writeable but that would send us down the path for having to support non-dicts throughout and would tie our hands with respect to implementation techniques that rely on the mapping being an actual dictionary. That isn't worth it for a code optimization and it loses the benefits arising from separate concrete and abstract classes.

Likewise, I don't think a documentation update makes sense because we don't document a constructor, so there is no documented way for a user to get into this position.

For the bidict project, several options come to mind:

1. To continue down the path of using dict_keys, dict_values, and dict_items, consider building a subclass that hides the mapping attribute or that raises a NotImplementedError. That is what collections.Counter() does with the inherited fromkeys() classmethod.

2. Build a C or Cython extension to implement an optimized concrete implementation designed specifically for bidict. That is what collections.OrderedDict() does for keys, values, and items.

3. Forgo the optimization and use the supported MappingView ABC. That is what collections.ChainMap() does.

4. Just document that *mapping* behaves differently in bidict.

Thank you for the clear bug report with examples. It made it easier to drill into what was happening.

[rhettinger]

Hmm, I'm looking through the bidict code a bit more. Rather than saying the dict views are being used in an unsupported way, it is more accurate to say that your intended meaning for the *mapping* attribute differs from the published meaning.

Note, the implementation is already skating on thin ice. The values() call unexpectedly returns an instance of dict_keys(). At first, I was surprised that this got past the type checker -- you can do set operations with KeysView but not with a ValuesView.

    >>> b.values()
    dict_keys([1, 2])

One suggestion is to document *mapping* do exactly what it currently does. The mappingproxy means that you aren't promising a specific upstream implementation.

Also consider that bidict could guarantee the meaning of *mapping* and its underlying store. This would allow users to make fast conversions to other mapping types:

    # This is weird, but useful. 
    # It might be nice to guarantee it.
    >>> OrderedDict(b.values().mapping)
    >>> OrderedDict([(1, 'a'), (2, 'b')])