Skip to content

Add a 'copy' method to Mapping #7555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

DevilXD
Copy link
Contributor

@DevilXD DevilXD commented Mar 26, 2022

The copy method is missing from Mapping, which is supposedly to be an immutable representation of a dictionary. Since simple shallow copy operation doesn't modify anything, it belongs in Mapping instead of MutableMapping. This PR adds this missing method. Using the copy method avoids from copy import copy imports for creating a quick copy of the original dictionary. The return type is a MutableMapping since this method is usually used to create a local mutable copy to be passed somewhere, but if immutability needs to be preserved, one can still assign it to a Mapping variable type or use a cast.

I ran into this while trying to deal with TypedDict not being compatible with Dict[str, Any] (per python/mypy#4976) and converting a JSON-saving function to accept a mapping, which is then modified before saving.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

xarray (https://github.com/pydata/xarray)
+ xarray/core/dataset.py:1068: error: Signature of "copy" incompatible with supertype "Mapping"  [override]
+ xarray/core/dataset.py:1068: note:      Superclass:
+ xarray/core/dataset.py:1068: note:          def copy(self) -> MutableMapping[Any, Any]
+ xarray/core/dataset.py:1068: note:      Subclass:
+ xarray/core/dataset.py:1068: note:          def copy(self, deep: bool = ..., data: Optional[Mapping[Any, Any]] = ...) -> Dataset

bidict (https://github.com/jab/bidict)
+ bidict/_base.py: note: In member "copy" of class "BidictBase":
+ bidict/_base.py:471:5: error: Return type "BidictBase[KT, VT]" of "copy" incompatible with return type "MutableMapping[KT, VT]" in supertype "Mapping"  [override]

urllib3 (https://github.com/urllib3/urllib3)
+ src/urllib3/_request_methods.py:96: error: Unused "type: ignore" comment
+ src/urllib3/connectionpool.py:689: error: Unused "type: ignore" comment
+ src/urllib3/connectionpool.py:690: error: Unused "type: ignore" comment

arviz (https://github.com/arviz-devs/arviz)
+ arviz/data/inference_data.py:1708: error: Return type "InferenceData" of "copy" incompatible with return type "MutableMapping[str, Dataset]" in supertype "Mapping"
- doc/sphinxext/gallery_generator.py:370: error: Call to untyped function "create_thumbnail" in typed context
- doc/sphinxext/gallery_generator.py:375: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)
+ doc/sphinxext/gallery_generator.py:370: note: (Skipping most remaining errors due to unresolved imports or missing stubs; fix these first)

@DevilXD
Copy link
Contributor Author

DevilXD commented Mar 26, 2022

The only error that appears as the result of this change (excluding the primer) seems to be tied to the same piece of code:

typeshed/stdlib/typing.pyi

Lines 1209 to 1211 in f40747f

# Internal mypy fallback type for all typed dicts (does not exist at runtime)
class _TypedDict(Mapping[str, object], metaclass=ABCMeta):
def copy(self: TypeshedSelf) -> TypeshedSelf: ...

# Internal mypy fallback type for all typed dicts (does not exist at runtime)
class _TypedDict(Mapping[str, object], metaclass=abc.ABCMeta):
__required_keys__: frozenset[str]
__optional_keys__: frozenset[str]
__total__: bool
def copy(self: TypeshedSelf) -> TypeshedSelf: ...

This seems to be because TypedDict inherits Mapping instead of MutableMapping for some reason, despite TypedDict instances being mutable. Since I'm not familiar with why that's the case, I'd appreciate someone more knowledgeable to chime in on this topic here.

@srittau
Copy link
Collaborator

srittau commented Mar 26, 2022

typing.Mapping/collections.abc.Mapping do not have a copy() method at runtime, so we can't add it to typeshed.

@DevilXD
Copy link
Contributor Author

DevilXD commented Mar 26, 2022

That's... strange then. Isn't Mapping supposed to be an immutable representation of a dictionary? As far as I've understood it so far, Mapping should provide everything you need to work with a dictionary without modifying it (iterate over, read values, etc.), while MutableMapping adds a way to set values and modify the dictionary in some way, just like Sequence and MutableSequence does it for a list.

This sounds similar to #7153 where Set (and thus MutableSet) defines a _hash method that's also missing at runtime due to set not actually inheriting from MutableSet. But then again, I think it's better not to throw an error at a valid code instead of the other way around, hence this addition.

@JelleZijlstra
Copy link
Member

As @srittau said, there is no Mapping.copy method at runtime, so it shouldn't be in typeshed either.

You can suggest adding it to CPython first, but it may be difficult because the ABC doesn't provide a way to create an instance of an arbitrary concrete Mapping.

@DevilXD DevilXD deleted the patch-1 branch March 26, 2022 16:26
@Akuli
Copy link
Collaborator

Akuli commented Mar 26, 2022

Mapping should provide everything you need to work with a dictionary without modifying it

This is close to how Mapping is intended to work, but not quite right. Mapping represents anything that has keys and values, not necessarily something copyable. If I create a class that represents something that just can't be reasonably copied, it should still be possible to make it a mapping. You can do dict(any_mapping), but it doesn't make sense for .copy() to do that, because you would expect a copy to have the same type as the original object.

It might help to compare this to list and MutableMapping: lists have a .sort() method while MutableMappings don't. I think this makes sense because Python's sorting implementation was designed for sorting lists efficiently, and if you want to sort something else, you're supposed to convert it to a list anyway (which is what sorted() does). So it doesn't make sense to sort non-list MutableMappings in-place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants