Allow arbitrary types in dict.pop (3 overloads)

[randolf-scholz]

Fixes #15271

new overloads:

class dict[K, V]:
    @overload
    def pop(self, key: object, /) -> V: ...
    @overload
    def pop(self, key: object, default: V, /) -> V: ...
    @overload
    def pop[T](self, key: object, default: T, /) -> V | T: ...

[randolf-scholz]

Although the primer looks nicer as in #15296, I think this is mostly due to false negatives (see #15296 (comment) and #15296 (comment))

[randolf-scholz]

Reopened due to suggestion of splitting #15296 into 2 steps: (1) convert key to object, (2) investigate the removal of middle overload.

[github-actions]

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

mongo-python-driver (https://github.com/mongodb/mongo-python-driver)
+ bson/son.py:126: error: Signature of "pop" incompatible with supertype "builtins.dict"  [override]
+ bson/son.py:126: note:      Superclass:
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def pop(self, object, /) -> _Value
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def pop(self, object, _Value, /) -> _Value
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def [_T] pop(self, object, _T, /) -> _Value | _T
+ bson/son.py:126: note:      Subclass:
+ bson/son.py:126: note:          def [_T] pop(self, key: _Key, *args: _Value | _T) -> _Value | _T

pylox (https://github.com/sco1/pylox)
+ pylox/containers/array.py:83: note:     def pop(self, object, /) -> Any
- pylox/containers/array.py:83: note:     def pop(self, Any, /) -> Any
+ pylox/containers/array.py:83: note:     def pop(self, object, Any, /) -> Any
- pylox/containers/array.py:83: note:     def pop(self, Any, Any, /) -> Any
- pylox/containers/array.py:83: note:     def [_T] pop(self, Any, _T, /) -> Any | _T
+ pylox/containers/array.py:83: note:     def [_T] pop(self, object, _T, /) -> Any | _T

rotki (https://github.com/rotki/rotki)
+ rotkehlchen/chain/aggregator.py:727: error: Unused "type: ignore" comment  [unused-ignore]

django-stubs (https://github.com/typeddjango/django-stubs)
+ django-stubs/http/request.pyi:202: error: Signature of "pop" incompatible with supertype "builtins.dict"  [override]
+ django-stubs/http/request.pyi:202: note:      Superclass:
+ django-stubs/http/request.pyi:202: note:          @overload
+ django-stubs/http/request.pyi:202: note:          def pop(self, object, /) -> str
+ django-stubs/http/request.pyi:202: note:          @overload
+ django-stubs/http/request.pyi:202: note:          def pop(self, object, str, /) -> str
+ django-stubs/http/request.pyi:202: note:          @overload
+ django-stubs/http/request.pyi:202: note:          def [_T] pop(self, object, _T, /) -> str | _T
+ django-stubs/http/request.pyi:202: note:      Subclass:
+ django-stubs/http/request.pyi:202: note:          @overload
+ django-stubs/http/request.pyi:202: note:          def pop(self, str | bytes, /) -> Never
+ django-stubs/http/request.pyi:202: note:          @overload
+ django-stubs/http/request.pyi:202: note:          def [_Z] pop(self, str | bytes, str | _Z = ..., /) -> Never

pylint (https://github.com/pycqa/pylint)
+ pylint/config/arguments_provider.py:54: error: Incompatible types in "yield" (actual type "tuple[None, list[tuple[str, dict[str, str | bool | int | Pattern[str] | Iterable[str | int | Pattern[str]] | type[_CallbackAction] | Callable[[Any], Any] | Callable[[Any, Any, Any, Any], Any] | None], Any]]]", expected type "tuple[str, list[tuple[str, dict[str, str | bool | int | Pattern[str] | Iterable[str | int | Pattern[str]] | type[_CallbackAction] | Callable[[Any], Any] | Callable[[Any, Any, Any, Any], Any] | None], Any]]] | tuple[None, dict[str, list[tuple[str, dict[str, str | bool | int | Pattern[str] | Iterable[str | int | Pattern[str]] | type[_CallbackAction] | Callable[[Any], Any] | Callable[[Any, Any, Any, Any], Any] | None], Any]]]]")  [misc]
+ pylint/config/arguments_provider.py:54: note: Error code "misc" not covered by "type: ignore" comment
+ pylint/config/arguments_provider.py:54: error: Unused "type: ignore" comment  [unused-ignore]

scrapy (https://github.com/scrapy/scrapy)
+ scrapy/utils/datatypes.py:98: error: Signature of "pop" incompatible with supertype "builtins.dict"  [override]
+ scrapy/utils/datatypes.py:98: note:      Superclass:
+ scrapy/utils/datatypes.py:98: note:          @overload
+ scrapy/utils/datatypes.py:98: note:          def pop(self, object, /) -> Any
+ scrapy/utils/datatypes.py:98: note:          @overload
+ scrapy/utils/datatypes.py:98: note:          def pop(self, object, Any, /) -> Any
+ scrapy/utils/datatypes.py:98: note:          @overload
+ scrapy/utils/datatypes.py:98: note:          def [_T] pop(self, object, _T, /) -> Any | _T
+ scrapy/utils/datatypes.py:98: note:      Subclass:
+ scrapy/utils/datatypes.py:98: note:          def [AnyStr: (str, bytes)] pop(self, key: AnyStr, *args: Any) -> Any

discord.py (https://github.com/Rapptz/discord.py)
+ discord/ui/view.py:932: error: Unused "type: ignore" comment  [unused-ignore]

steam.py (https://github.com/Gobot1234/steam.py)
- steam/ext/commands/utils.py:52: note:          def pop(self, str, /) -> _VT
+ steam/ext/commands/utils.py:52: note:          def pop(self, object, /) -> _VT
- steam/ext/commands/utils.py:52: note:          def pop(self, str, _VT, /) -> _VT
+ steam/ext/commands/utils.py:52: note:          def pop(self, object, _VT, /) -> _VT
- steam/ext/commands/utils.py:52: note:          def [_T] pop(self, str, _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:52: note:          def [_T] pop(self, object, _T, /) -> _VT | _T
- steam/ext/commands/utils.py:52: note:          def pop(self, str, /) -> _VT
+ steam/ext/commands/utils.py:52: note:          def pop(self, Any, /) -> _VT
- steam/ext/commands/utils.py:52: note:          def pop(self, str, _VT, /) -> _VT
+ steam/ext/commands/utils.py:52: note:          def pop(self, Any, _VT, /) -> _VT
- steam/ext/commands/utils.py:52: note:          def [_T] pop(self, str, _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:52: note:          def [_T] pop(self, Any, _T, /) -> _VT | _T

operator (https://github.com/canonical/operator)
- ops/_private/harness.py:2645: error: No overload variant of "pop" of "dict" matches argument types "str", "None"  [call-overload]
- ops/_private/harness.py:2645: note: Possible overload variants:
- ops/_private/harness.py:2645: note:     def pop(self, int, /) -> dict[str, Any]
- ops/_private/harness.py:2645: note:     def pop(self, int, dict[str, Any], /) -> dict[str, Any]
- ops/_private/harness.py:2645: note:     def [_T] pop(self, int, _T, /) -> dict[str, Any] | _T

[srittau]

Why are you sometimes using object and sometimes Any?

The primer output is not very convincing. Lots of churn, and only one weird case – which could arguably point to a real problem or at least weird typing – and one case that could be solved by using _KT | None. The tests point out a real problem, though. On the other hand, we don't know how many problems are prevented by the current type annotations.

I remain unconvinced that this change is actually beneficial to our users, although I won't argue if other maintainers feel differently.

[randolf-scholz]

I am using Any for MutableMapping since it's an abstract base class, and subclasses may restrict the signature of pop somehow. For dict I used object since it's a concrete class, and we know for sure pop accepts any Hashable type at runtime.

Let me do a test of only removing the second overload without changing the annotation. I have a suspicion that these two things may be entangled, so disentangling them into 2 separate PRs may not be appropriate after all.

[AlexWaygood]

I don't have a strong opinion on Any vs object, but I agree that we should try to be consistent between the two and state clearly in comments why we're using Any when we're using it.

I remain unconvinced that this change is actually beneficial to our users, although I won't argue if other maintainers feel differently.

This change would be very beneficial to ty users. ty attempts to do type narrowing in a stricter, more principled way than other type checkers, but this causes false positives when encountering methods that are annotated like this in typeshed, so I would be in favour of landing something along these lines. For example, mypy's behaviour here is as follows:

def f(x: object):
    if isinstance(x, dict):
        reveal_type(x)  # revealed: dict[Any, Any]
        x.pop("foo")  # no error

whereas ty's behaviour is:

def f(x: object):
    if isinstance(x, dict):
        reveal_type(x)  # revealed: Top[dict[Unknown, Unknown]]
        x.pop("foo")  # Argument to bound method `pop` is incorrect: Expected `Never`, found `Literal["foo"]`

Top[dict[Unknown, Unknown]] is the "top materialization" of dict: it's a strict type that represents the "infinite union" of all possible specialized dict types. Similar to the way that tuple[int, ...] is the "infinite union" of tuple[int] | tuple[int, int] | tuple[int, int, int] | ... etc., so Top[dict[Unknown, Unknown]] is the infinite union of dict[str, str] | dict[str, bytes] | dict[bytes, str] | dict[bytes, bytes] | dict[int, int] | ... etc. for all possible types. It's okay to call a method on an instance of Top[dict[Unknown, Unknown]] if that method can be validly called for any specialization of dict, but it's not okay to call any method on an instance of Top[dict[Unknown, Unknown]] if that method is only valid for some specializations of dict. As such, the current annotations cause false-positive errors here for ty users, but the proposed change here would fix that.

Our reasoning for using Top[dict[Unknown, Unknown]] for the narrowed type inside the if branch is that we don't think Any should be introduced into the type of an object as a result of type narrowing -- it should be something that the user explicitly opts into via type annotations or casts.

[srittau]

Cf. python/typing#2154 for a potential future "best of both worlds" solution. If this would be beneficial for ty, this has my blessing (for now). Agree on comments for the Anys. (As is our policy.)

[srittau]

Actually, I think we should use object consistently. If we go this path with object, we want sub-classes to also allow object, or otherwise clearly state their violation of the LSP by using # type: ignore.

[randolf-scholz]

That was my rationale for using object in dict. For MutableMapping I am still not sure, because it is an ABC, and the types were not historically specified. Here's the mypy primer when we only change MutableMapping.pop to object: #15304 (comment)

This is the default implementation: https://github.com/python/cpython/blob/3c9c3d33cbdef257526871cbc12e93635026f5d6/Lib/_collections_abc.py#L929-L941

    def pop(self, key, default=__marker):
        '''D.pop(k[,d]) -> v, remove specified key and return the corresponding value.
          If key is not found, d is returned if given, otherwise KeyError is raised.
        '''
        try:
            value = self[key]
        except KeyError:
            if default is self.__marker:
                raise
            return default
        else:
            del self[key]
            return value

It will catch KeyError originating from __getitem__, but not TypeError. __getitem__ is an abstract method, so if we use the object annotation, we implicitly demand from the implementer that their __getitem__ also can consume object and will raise KeyError rather than TypeError on a key with bad type. This is how it works for dict, up to hashability.

But I am easily convinced that there are MutableMapping implementations in the wild that do not behave like that and will raise TypeError rather than KeyError. If we use Any then implementers can override the signature appropriately if their mappings __getitem__ does not accept arbitrary types.