ChainMap has wrong or unintuitive type after #6042 #8430

Activity

1. kaste commented on Jul 28, 2022

   

   kaste

   on Jul 28, 2022

   ContributorAuthor

   More actions

   @hauntsaninja wrote

   That sounds reasonable to me. I would be in favour of:

   class ChainMap(MutableMapping[_KT, _VT], Generic[_KT, _VT]):
        maps: list[Mapping[_KT, _VT]]
        def __init__(self) -> None: ...
        def __init__(self, __map: MutableMapping[_KT, _VT], *maps: Mapping[_KT, _VT]) -> None: ...
   

   Also just to call something out:

   My understanding is that the typeshed policy is to err on the side of false negatives ... Here is an example in a ChainMap subclass ...

   While I'm not sure we've stated this out loud, historically, typeshed has very consistently preferred false positives for subclassers of classes over false positives for other users of classes.
2. ssbarnea

   added a commit that references this issue on Jan 21, 2023

   fix: Fix types

   ...

   389fd4f
3. pawamoy

   added a commit that references this issue on Jan 23, 2023

   ci: Fix typing

   ...

   3cbe472
4. pawamoy

   added a commit that references this issue on Oct 9, 2023

   fix: Don't deepcopy the local config

   ...

   1300d2c
5. sanderr commented on Jan 23, 2025

   

   sanderr

   on Jan 23, 2025

   Contributor

   More actions

   I just discovered collections.ChainMap, and I have to say I was surprised that it requires all member mappings to be mutable. I like the proposed __init__ typing, and I'd like to add that perhaps it's not so bad that maps would be typed as (immutable) Mapping: if I understand correctly end users aren't expected to modify inner mappings directly anyway (chainmap.maps[1]["key"] = "value"), so from their perspective it's only right. The fact that the stdlib implementation itself "violates" it is unfortunate, but as I see it it's more an implementation detail than a true violation: it knows that the first element is mutable and acts on it. Whether it does that by storing a separate reference or by accessing it through maps is irrelevant.

   I believe that the most troublesome part with this suggestion is that the ChainMap documentation states that maps may be modified by the end user (e.g. maps[0] = {}). If we type it as suggested above, that would allow the user to set an immutable mapping at maps[0], that the stdlib implementation would then happily modify. As long as maps is intended to be mutable directly by the end user, I don't think the suggested change is feasible (except with python/typing#548 as stated in the original thread), however much I'd like to see it.

   I was intrigued by the idea of an immutable variant, so I gave it a shot. It does require (safe) type ignores if built on top of the stdlib ChainMap, but I think that's by far preferable over the alternative. I suspect it also takes a performance hit, considering that this implements the Python ABC, while ChainMap (I'm assuming, haven't checked) is written in C. That said, I'll put it here in case anyone is interested.

   I made two variations: one that is in fact still mutable but protects its member mappings from mutation. And one that is immutable itself. The benefit of the latter is that it can confidently implement the maps and parents properties, while the meanings of those get (partially) lost for the former. I did not do extensive testing on either of these.

   import collections
   from collections.abc import Iterator, Mapping, MutableMapping, Sequence
   from typing import Self
   
   
   class OverlayChainMap[K, V](MutableMapping[K, V]):
       """
       ChainMap implementation that protects its member mappings from mutation, while still allowing mutations on the chain map
       itself.
   
       See collections.ChainMap for more details. Does not implement the `maps` and `parents` properties,
       because they lose much of their meaning due to the overlay mapping sitting in between the user-supplied ones.
       """
       def __init__(self, *maps: Mapping[K, V]) -> None:
           # construct backing ChainMap with owned, mutable dict as first map
           self._chainmap: collections.ChainMap[K, V] = collections.ChainMap(
               {},
               # This is a type violation due to a limitation of the ChainMap constructor (python/typeshed#8430).
               # This workaround builds on the (documented) knowledge that ChainMap only ever modifies the first mapping,
               # and the fact that the constructed chain map is never exposed to the end user.
               # Therefore the type violation is a safe, if imperfect, workaround to the typing limitation.
               *maps,  # type: ignore
           )
   
       def new_child(self, m: Mapping[K, V] | None = None, **kwargs: V) -> Self:
           return self.__class__(
               *self._chainmap.new_child(
                   m,  # type: ignore
               ).new_child({}, **kwargs).maps
           )
   
       def __getitem__(self, key: K) -> V:
           return self._chainmap[key]
   
       def __setitem__(self, key: K, value: V) -> None:
           self._chainmap[key] = value
   
       def __delitem__(self, key: K) -> None:
           del self._chainmap[key]
   
       def __iter__(self) -> Iterator[K]:
           return iter(self._chainmap)
   
       def __len__(self) -> int:
           return len(self._chainmap)
   
   
   class ImmutableChainMap[K, V](Mapping[K, V]):
       """
       Immutable ChainMap implementation. See collections.ChainMap for more details.
       """
       def __init__(self, *maps: Mapping[K, V]) -> None:
           # construct backing ChainMap with owned, mutable dict as first map
           self._chainmap: collections.ChainMap[K, V] = collections.ChainMap(
               # This is a type violation due to a limitation of the ChainMap constructor (python/typeshed#8430).
               # This workaround builds on the fact that we own the backing chain map, we never mutate it, and we only
               # expose it or its attributes as immutable properties.
               *maps  # type: ignore
           )
   
       def new_child(self, m: Mapping[K, V] | None = None) -> Self:
           return self.__class__(
               *self._chainmap.new_child(
                   m,  # type: ignore
               ).maps
           )
   
       @property
       def maps(self) -> Sequence[Mapping[K, V]]:
           return self._chainmap.maps
   
       @property
       def parents(self) -> Self:
           return self.__class__(*self._chainmap.parents.maps)
   
       def __getitem__(self, key: K) -> V:
           return self._chainmap[key]
   
       def __iter__(self) -> Iterator[K]:
           return iter(self._chainmap)
   
       def __len__(self) -> int:
           return len(self._chainmap)
   
   
   
   d1: MutableMapping[str, int] = {"one": 1, "shared": 1}
   d1_copy: Mapping[str, int] = {**d1}
   d2: MutableMapping[str, int] = {"two": 2, "shared": 2}
   d2_copy: Mapping[str, int] = {**d2}
   
   copied: Mapping[str, int] = {**d1, **d2}
   chained: MutableMapping[str, int] = collections.ChainMap(d2, d1)
   overlayed: MutableMapping[str, int] = OverlayChainMap(d2, d1)
   immutably_chained: Mapping[str, int] = ImmutableChainMap(d2, d1)
   
   assert (
       list(chained.items())
       == list(overlayed.items())
       == list(immutably_chained.items())
       == list(copied.items())
       == [("one", 1), ("shared", 2), ("two", 2)]
   )
   
   overlayed["three"] = 3  # adds a new item
   overlayed["one"] = 42  # changes the value in overlayed, not in d1
   overlayed["two"] = 42  # changes the value in overlayed, not in d2
   assert list(overlayed.items()) == [("one", 42), ("shared", 2), ("two", 42), ("three", 3)]
   assert d1 == d1_copy
   assert d2 == d2_copy
   chained["three"] = 3  # adds a new item
   chained["one"] = 42  # changes the value in chained, not in d1
   chained["two"] = 42  # changes the value in both chained and d2
   assert list(chained.items()) == [("one", 42), ("shared", 2), ("two", 42), ("three", 3)]
   assert d1 == d1_copy
   assert d2 != d2_copy
6. viktorlashchuk

   added 2 commits that reference this issue on Feb 24, 2025

   ci: Fix typing

   ...

   73ea570

   fix: Don't deepcopy the local config

   ...

   a09aa63