TYP: core.computations.scope
State: closed

mypy does _not_ play nicely with DeepChainMap.  @WillAyd @simonjayhawkins any ideas about the 4 places where i had to put `# type: ignore`?

Made `_update` private just to make it easier to reason about this thing

Labels:
name: Internals
description: Related to non-user accessible pandas implementation
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
> mypy does _not_ play nicely with DeepChainMap

The design of ChainMap does not appear to be intended to be a generic container. i.e. the interface does not suggest that if a list of Dicts is given, that maps will contain a list of Dicts.

Although our implementation of DeepChainMap works, I guess that it could break in the future.

Trying to modify DeepChainMap to convince mypy that is a list of Mutuable mappings gives rise to errors such as `error: Signature of "maps" incompatible with supertype "ChainMap"` and `error: Return type "List[MutableMapping[Any, Any]]" of "maps" incompatible with return type "List[Mapping[Any, Any]]" in supertype "ChainMap"`

So maybe ChainMap shouldn't be subclassed for this intent.

here's a DeepChainMap using composition instead, passing mypy without ignores and passing tests. (probably not what we want though)

```diff
diff --git a/pandas/compat/chainmap.py b/pandas/compat/chainmap.py
index 493ba68bf..77ec507b9 100644
--- a/pandas/compat/chainmap.py
+++ b/pandas/compat/chainmap.py
@@ -1,22 +1,46 @@
 from collections import ChainMap
+from typing import List, MutableMapping
 
 
-class DeepChainMap(ChainMap):
+class DeepChainMap:
+    def __init__(self, *maps):
+        self.chainmap = ChainMap(*maps)
+
     def __setitem__(self, key, value):
-        for mapping in self.maps:
+        for mapping in self.chainmap.maps:
             if key in mapping:
                 mapping[key] = value
                 return
-        self.maps[0][key] = value
+        self.chainmap.maps[0][key] = value
 
     def __delitem__(self, key):
-        for mapping in self.maps:
+        for mapping in self.chainmap.maps:
             if key in mapping:
                 del mapping[key]
                 return
         raise KeyError(key)
 
+    @property
+    def maps(self) -> List[MutableMapping]:
+        return self.chainmap.maps
+
     def new_child(self, *args, **kwargs) -> "DeepChainMap":
-        # ChainMap.new_child returns self.__class__(...) but mypy
-        #  doesn't know that, so we annotate it explicitly here.
-        return super().new_child(*args, **kwargs)  # type: ignore
+        return DeepChainMap(self.chainmap.new_child(*args, **kwargs))
+
+    def __iter__(self):
+        return iter(self.chainmap)
+
+    def update(self, *args, **kwargs):
+        return self.chainmap.update(*args, **kwargs)
+
+    def keys(self, *args, **kwargs):
+        return self.chainmap.keys(*args, **kwargs)
+
+    def __getitem__(self, key):
+        return self.chainmap[key]
+
+    def __len__(self):
+        return len(self.chainmap)
+
+    def copy(self):
+        return DeepChainMap(*self.chainmap.copy().maps)
diff --git a/pandas/core/computation/scope.py b/pandas/core/computation/scope.py
index 9b9dde83f..f9bdf96ab 100644
--- a/pandas/core/computation/scope.py
+++ b/pandas/core/computation/scope.py
@@ -222,14 +222,10 @@ class Scope:
         new_value : object
             Value to be replaced along with the possible renaming
         """
-        maps: List[MutableMapping]
-        mapping: MutableMapping
-
-        # TODO: convince mypy that these maps are in fact mutable
         if self.has_resolvers:
-            maps = self.resolvers.maps + self.scope.maps  # type: ignore
+            maps = self.resolvers.maps + self.scope.maps
         else:
-            maps = self.scope.maps  # type: ignore
+            maps = self.scope.maps
 
         maps.append(self.temps)
 
@@ -323,6 +319,5 @@ class Scope:
         vars : DeepChainMap
             All variables in this scope.
         """
-        # TODO: convince mypy that all of the maps are mutable
-        maps = [self.temps] + self.resolvers.maps + self.scope.maps  # type: ignore
+        maps = [self.temps] + self.resolvers.maps + self.scope.maps
         return DeepChainMap(*maps)
```


@simonjayhawkins thanks for taking a look at this.  You've outlined several options for how to deal with these issues; do you have a preferred one?  Leaving this un-typed is also an option.
I think mypy is correctly identifying an potential issue with subclassing ChainMap that we should be aware of. But not sure whether making significant changes is warranted at this time.

(also we don't check for MutableMappings when instantiating a DeepChainMap or in `__setitem__`)

The DeepChainMap code above was while establishing the api of ChainMap required in our usage. Again whether we do need duck type compatibility is another issue.

Maybe we don't even need to use a ChainMap at all here and just store the maps in a List within our DeepChainMap implementation.


> You've outlined several options for how to deal with these issues; do you have a preferred one?

my preference would be to move any `# type: ignores`, casts, asserts etc. out of the main code and into DeepChainMap. You've successfully done this with new_child. I've tried unsuccessfully to do the same with maps, hence ending up with a DeepChainMap  not inheriting from ChainMap.

so if you can get it to work by doing something with maps inside the DeepChainMap  implementation that'll be great.

otherwise we just need to have good comments for future readers and future self. (which you have done)

@WillAyd any preference?
Closing following #29667.  ill follow up if i find a nice solution for the DeepChainMap issues