Skip to content

Permissive behavior on Mapping.get, MutableMapping.pop, and some common mapping types. #12345

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Permissive behavior on Mapping.get, MutableMapping.pop, and some common mapping types. #12345

wants to merge 5 commits into from

Conversation

mikeshardmind
Copy link
Contributor

This better matches the runtime behavior of these functions.
pop without a default has been left requiring a matching keytype

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jul 15, 2024 •
edited
Loading

This appears to remove the need for a bunch of type ignores while being more correct to runtime, there are some projects that had typings based on the less accurate stubs in the typeshed which would be affected.

I'm not sure if this should go further and have pop always accept object.

It's also going to require other changes to other stubs if this is a direction that's pursued

@mikeshardmind mikeshardmind marked this pull request as draft July 15, 2024 00:31
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@mikeshardmind
Update 3rd party stubs
eb1c4f5 
After verifying, most dicts were safe to just update, a few that do
things like case-insenstivity assuming only str keys were not, and
have been marked with type ignores
@mikeshardmind mikeshardmind marked this pull request as ready for review July 15, 2024 02:25
@github-actions GitHub Actions
Copy link
Contributor

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

beartype (https://github.com/beartype/beartype)
+ beartype/door/_cls/doorsub.py:59: error: Unused "type: ignore" comment  [unused-ignore]
+ beartype/door/_doormap.py:86: error: Unused "type: ignore" comment  [unused-ignore]

xarray (https://github.com/pydata/xarray)
+ xarray/core/utils.py: note: In member "get" of class "FrozenMappingWarningOnValuesAccess":
+ xarray/core/utils.py:472: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ xarray/core/utils.py:472: note:      Superclass:
+ xarray/core/utils.py:472: note:          @overload
+ xarray/core/utils.py:472: note:          def get(self, object, /) -> V | None
+ xarray/core/utils.py:472: note:          @overload
+ xarray/core/utils.py:472: note:          def [_T] get(self, object, /, default: V | _T) -> V | _T
+ xarray/core/utils.py:472: note:      Subclass:
+ xarray/core/utils.py:472: note:          @overload
+ xarray/core/utils.py:472: note:          def get(self, K, /) -> V | None
+ xarray/core/utils.py:472: note:          @overload
+ xarray/core/utils.py:472: note:          def [T] get(self, K, /, default: V | T) -> V | T

pydantic (https://github.com/pydantic/pydantic)
+ pydantic/_internal/_utils.py:175: error: Unused "type: ignore" comment  [unused-ignore]

discord.py (https://github.com/Rapptz/discord.py)
+ discord/state.py:364: error: Unused "type: ignore" comment  [unused-ignore]
+ discord/state.py:431: error: Unused "type: ignore" comment  [unused-ignore]
+ discord/state.py:460: error: Unused "type: ignore" comment  [unused-ignore]
+ discord/state.py:464: error: Unused "type: ignore" comment  [unused-ignore]
+ discord/state.py:483: error: Unused "type: ignore" comment  [unused-ignore]
+ discord/ui/view.py:603: error: Unused "type: ignore" comment  [unused-ignore]

pandas (https://github.com/pandas-dev/pandas)
+ pandas/core/arrays/arrow/array.py:2944: error: Unused "type: ignore" comment  [unused-ignore]

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/datastructures/mixins.pyi:84: error: Signature of "pop" incompatible with supertype "dict"  [override]
+ src/werkzeug/datastructures/mixins.pyi:84: note:      Superclass:
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, object, V, /) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def [_T] pop(self, object, _T, /) -> V | _T
+ src/werkzeug/datastructures/mixins.pyi:84: note:      Subclass:
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/datastructures/mixins.pyi:84: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ src/werkzeug/datastructures/mixins.pyi:84: note:      Superclass:
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, object, /, default: V) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def [_T] pop(self, object, /, default: _T) -> V | _T
+ src/werkzeug/datastructures/mixins.pyi:84: note:      Subclass:
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/mixins.pyi:84: note:          @overload
+ src/werkzeug/datastructures/mixins.pyi:84: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/datastructures/structures.pyi:31: error: Signature of "get" incompatible with supertype "dict"  [override]
+ src/werkzeug/datastructures/structures.pyi:31: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def get(self, object, /) -> V | None
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def get(self, object, V, /) -> V
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [_T] get(self, object, _T, /) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:31: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def get(self, key: K, default: None = ..., type: None = ...) -> V | None
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [D] get(self, key: K, default: D, type: None = ...) -> D | V
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [D, T] get(self, key: K, default: D, type: Callable[[V], T]) -> D | T
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [T] get(self, key: K, type: Callable[[V], T]) -> T | None
+ src/werkzeug/datastructures/structures.pyi:31: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ src/werkzeug/datastructures/structures.pyi:31: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def get(self, object, /) -> V | None
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [_T] get(self, object, /, default: V | _T) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:31: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def get(self, key: K, default: None = ..., type: None = ...) -> V | None
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [D] get(self, key: K, default: D, type: None = ...) -> D | V
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [D, T] get(self, key: K, default: D, type: Callable[[V], T]) -> D | T
+ src/werkzeug/datastructures/structures.pyi:31: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:31: note:          def [T] get(self, key: K, type: Callable[[V], T]) -> T | None
+ src/werkzeug/datastructures/structures.pyi:74: error: Signature of "pop" incompatible with supertype "dict"  [override]
+ src/werkzeug/datastructures/structures.pyi:74: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, object, V, /) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def [_T] pop(self, object, _T, /) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:74: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/datastructures/structures.pyi:74: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ src/werkzeug/datastructures/structures.pyi:74: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, object, /, default: V) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def [_T] pop(self, object, /, default: _T) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:74: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/structures.pyi:74: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:74: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/datastructures/structures.pyi:119: error: Signature of "pop" incompatible with supertype "dict"  [override]
+ src/werkzeug/datastructures/structures.pyi:119: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, object, V, /) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def [_T] pop(self, object, _T, /) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:119: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/datastructures/structures.pyi:119: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ src/werkzeug/datastructures/structures.pyi:119: note:      Superclass:
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, K, /) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, object, /, default: V) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def [_T] pop(self, object, /, default: _T) -> V | _T
+ src/werkzeug/datastructures/structures.pyi:119: note:      Subclass:
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def pop(self, key: K) -> V
+ src/werkzeug/datastructures/structures.pyi:119: note:          @overload
+ src/werkzeug/datastructures/structures.pyi:119: note:          def [T] pop(self, key: K, default: V | T = ...) -> V | T
+ src/werkzeug/exceptions.py:94: error: Unused "type: ignore" comment  [unused-ignore]
+ src/werkzeug/debug/__init__.py:539: error: Unused "type: ignore" comment  [unused-ignore]

scrapy (https://github.com/scrapy/scrapy)
+ scrapy/settings/__init__.py:128: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ scrapy/settings/__init__.py:128: note:      Superclass:
+ scrapy/settings/__init__.py:128: note:          @overload
+ scrapy/settings/__init__.py:128: note:          def get(self, object, /) -> Any | None
+ scrapy/settings/__init__.py:128: note:          @overload
+ scrapy/settings/__init__.py:128: note:          def [_T] get(self, object, /, default: Any | _T) -> Any | _T
+ scrapy/settings/__init__.py:128: note:      Subclass:
+ scrapy/settings/__init__.py:128: note:          def get(self, name: bool | float | int | str | None, default: Any = ...) -> Any
+ scrapy/settings/__init__.py:516: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ scrapy/settings/__init__.py:516: note:      Superclass:
+ scrapy/settings/__init__.py:516: note:          @overload
+ scrapy/settings/__init__.py:516: note:          def pop(self, bool | float | int | str | None, /) -> Any
+ scrapy/settings/__init__.py:516: note:          @overload
+ scrapy/settings/__init__.py:516: note:          def pop(self, object, /, default: Any) -> Any
+ scrapy/settings/__init__.py:516: note:          @overload
+ scrapy/settings/__init__.py:516: note:          def [_T] pop(self, object, /, default: _T) -> Any | _T
+ scrapy/settings/__init__.py:516: note:      Subclass:
+ scrapy/settings/__init__.py:516: note:          def pop(self, name: bool | float | int | str | None, default: Any = ...) -> Any
+ scrapy/utils/datatypes.py:88: error: Signature of "get" incompatible with supertype "dict"  [override]
+ scrapy/utils/datatypes.py:88: note:      Superclass:
+ scrapy/utils/datatypes.py:88: note:          @overload
+ scrapy/utils/datatypes.py:88: note:          def get(self, object, /) -> Any | None
+ scrapy/utils/datatypes.py:88: note:          @overload
+ scrapy/utils/datatypes.py:88: note:          def get(self, object, Any, /) -> Any
+ scrapy/utils/datatypes.py:88: note:          @overload
+ scrapy/utils/datatypes.py:88: note:          def [_T] get(self, object, _T, /) -> Any | _T
+ scrapy/utils/datatypes.py:88: note:      Subclass:
+ scrapy/utils/datatypes.py:88: note:          def [AnyStr in (str, bytes)] get(self, key: AnyStr, def_val: Any = ...) -> Any
+ scrapy/utils/datatypes.py:88: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ scrapy/utils/datatypes.py:88: note:      Superclass:
+ scrapy/utils/datatypes.py:88: note:          @overload
+ scrapy/utils/datatypes.py:88: note:          def get(self, object, /) -> Any | None
+ scrapy/utils/datatypes.py:88: note:          @overload
+ scrapy/utils/datatypes.py:88: note:          def [_T] get(self, object, /, default: Any | _T) -> Any | _T
+ scrapy/utils/datatypes.py:88: note:      Subclass:
+ scrapy/utils/datatypes.py:88: note:          def [AnyStr in (str, bytes)] get(self, key: AnyStr, def_val: Any = ...) -> Any
+ scrapy/utils/datatypes.py:104: error: Signature of "pop" incompatible with supertype "dict"  [override]
+ scrapy/utils/datatypes.py:104: note:      Superclass:
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def pop(self, Any, /) -> Any
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def pop(self, object, Any, /) -> Any
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def [_T] pop(self, object, _T, /) -> Any | _T
+ scrapy/utils/datatypes.py:104: note:      Subclass:
+ scrapy/utils/datatypes.py:104: note:          def [AnyStr in (str, bytes)] pop(self, key: AnyStr, *args: Any) -> Any
+ scrapy/utils/datatypes.py:104: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ scrapy/utils/datatypes.py:104: note:      Superclass:
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def pop(self, Any, /) -> Any
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def pop(self, object, /, default: Any) -> Any
+ scrapy/utils/datatypes.py:104: note:          @overload
+ scrapy/utils/datatypes.py:104: note:          def [_T] pop(self, object, /, default: _T) -> Any | _T
+ scrapy/utils/datatypes.py:104: note:      Subclass:
+ scrapy/utils/datatypes.py:104: note:          def [AnyStr in (str, bytes)] pop(self, key: AnyStr, *args: Any) -> Any
+ scrapy/http/headers.py:85: error: Signature of "get" incompatible with supertype "dict"  [override]
+ scrapy/http/headers.py:85: note:      Superclass:
+ scrapy/http/headers.py:85: note:          @overload
+ scrapy/http/headers.py:85: note:          def get(self, object, /) -> Any | None
+ scrapy/http/headers.py:85: note:          @overload
+ scrapy/http/headers.py:85: note:          def get(self, object, Any, /) -> Any
+ scrapy/http/headers.py:85: note:          @overload
+ scrapy/http/headers.py:85: note:          def [_T] get(self, object, _T, /) -> Any | _T
+ scrapy/http/headers.py:85: note:      Subclass:
+ scrapy/http/headers.py:85: note:          def [AnyStr in (str, bytes)] get(self, key: AnyStr, def_val: Any = ...) -> bytes | None
+ scrapy/http/headers.py:85: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ scrapy/http/headers.py:85: note:      Superclass:
+ scrapy/http/headers.py:85: note:          @overload
+ scrapy/http/headers.py:85: note:          def get(self, object, /) -> Any | None
+ scrapy/http/headers.py:85: note:          @overload
+ scrapy/http/headers.py:85: note:          def [_T] get(self, object, /, default: Any | _T) -> Any | _T
+ scrapy/http/headers.py:85: note:      Subclass:
+ scrapy/http/headers.py:85: note:          def [AnyStr in (str, bytes)] get(self, key: AnyStr, def_val: Any = ...) -> bytes | None

steam.py (https://github.com/Gobot1234/steam.py)
+ steam/utils.py:533: error: Signature of "pop" incompatible with supertype "ChainMap"  [override]
+ steam/utils.py:533: note:      Superclass:
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, key: _KT) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, key: object, default: _VT) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def [_T] pop(self, key: object, default: _T) -> _VT | _T
+ steam/utils.py:533: note:      Subclass:
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, key: _KT) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def [_T] pop(self, key: _KT, default: _VT | _T) -> _VT | _T
+ steam/utils.py:533: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ steam/utils.py:533: note:      Superclass:
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, _KT, /) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, object, /, default: _VT) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def [_T] pop(self, object, /, default: _T) -> _VT | _T
+ steam/utils.py:533: note:      Subclass:
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def pop(self, key: _KT) -> _VT
+ steam/utils.py:533: note:          @overload
+ steam/utils.py:533: note:          def [_T] pop(self, key: _KT, default: _VT | _T) -> _VT | _T
- steam/ext/commands/utils.py:43: note:          def get(self, str, /) -> _VT | None
+ steam/ext/commands/utils.py:43: note:          def get(self, object, /) -> _VT | None
- steam/ext/commands/utils.py:43: note:          def get(self, str, _VT, /) -> _VT
+ steam/ext/commands/utils.py:43: note:          def get(self, object, _VT, /) -> _VT
- steam/ext/commands/utils.py:43: note:          def [_T] get(self, str, _T, /) -> _VT | _T
+ steam/ext/commands/utils.py:43: note:          def [_T] get(self, object, _T, /) -> _VT | _T
- steam/ext/commands/utils.py:43: note:          def get(self, str, /) -> _VT | None
+ steam/ext/commands/utils.py:43: note:          def get(self, object, /) -> _VT | None
- steam/ext/commands/utils.py:43: note:          def [_T] get(self, str, /, default: _VT | _T) -> _VT | _T
+ steam/ext/commands/utils.py:43: note:          def [_T] get(self, object, /, default: _VT | _T) -> _VT | _T
- 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, /, default: _VT) -> _VT
+ steam/ext/commands/utils.py:52: note:          def pop(self, object, /, default: _VT) -> _VT
- steam/ext/commands/utils.py:52: note:          def [_T] pop(self, str, /, default: _T) -> _VT | _T
+ steam/ext/commands/utils.py:52: note:          def [_T] pop(self, object, /, default: _T) -> _VT | _T
+ steam/package.py:215: error: Unused "type: ignore" comment  [unused-ignore]
+ steam/package.py:216: error: Unused "type: ignore" comment  [unused-ignore]
+ steam/ext/commands/bot.py:537: error: Unused "type: ignore" comment  [unused-ignore]

mongo-python-driver (https://github.com/mongodb/mongo-python-driver)
+ bson/son.py:126: error: Signature of "pop" incompatible with supertype "dict"  [override]
+ bson/son.py:126: note:      Superclass:
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def pop(self, _Key, /) -> _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
+ bson/son.py:126: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ bson/son.py:126: note:      Superclass:
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def pop(self, _Key, /) -> _Value
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def pop(self, object, /, default: _Value) -> _Value
+ bson/son.py:126: note:          @overload
+ bson/son.py:126: note:          def [_T] pop(self, object, /, default: _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
+ pymongo/common.py:1019: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ pymongo/common.py:1019: note:      Superclass:
+ pymongo/common.py:1019: note:          @overload
+ pymongo/common.py:1019: note:          def get(self, object, /) -> Any | None
+ pymongo/common.py:1019: note:          @overload
+ pymongo/common.py:1019: note:          def [_T] get(self, object, /, default: Any | _T) -> Any | _T
+ pymongo/common.py:1019: note:      Subclass:
+ pymongo/common.py:1019: note:          def get(self, key: str, default: Any | None = ...) -> Any
+ pymongo/common.py:1022: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ pymongo/common.py:1022: note:      Superclass:
+ pymongo/common.py:1022: note:          @overload
+ pymongo/common.py:1022: note:          def pop(self, str, /) -> Any
+ pymongo/common.py:1022: note:          @overload
+ pymongo/common.py:1022: note:          def pop(self, object, /, default: Any) -> Any
+ pymongo/common.py:1022: note:          @overload
+ pymongo/common.py:1022: note:          def [_T] pop(self, object, /, default: _T) -> Any | _T
+ pymongo/common.py:1022: note:      Subclass:
+ pymongo/common.py:1022: note:          def pop(self, key: str, *args: Any, **kwargs: Any) -> Any

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

jinja (https://github.com/pallets/jinja)
+ src/jinja2/compiler.py:1423: error: Unused "type: ignore" comment  [unused-ignore]
+ src/jinja2/environment.py:514: error: Unused "type: ignore" comment  [unused-ignore]

AutoSplit (https://github.com/Toufool/AutoSplit)
+ src/capture_method/__init__.py:106:5: error: Signature of "get" incompatible with supertype "dict"  [override]
+ src/capture_method/__init__.py:106:5: note:      Superclass:
+ src/capture_method/__init__.py:106:5: note:          @overload
+ src/capture_method/__init__.py:106:5: note:          def get(self, object, /) -> type[CaptureMethodBase] | None
+ src/capture_method/__init__.py:106:5: note:          @overload
+ src/capture_method/__init__.py:106:5: note:          def get(self, object, type[CaptureMethodBase], /) -> type[CaptureMethodBase]
+ src/capture_method/__init__.py:106:5: note:          @overload
+ src/capture_method/__init__.py:106:5: note:          def [_T] get(self, object, _T, /) -> type[CaptureMethodBase] | _T
+ src/capture_method/__init__.py:106:5: note:      Subclass:
+ src/capture_method/__init__.py:106:5: note:          def get(self, key: CaptureMethodEnum, object = ..., /) -> Any
+ src/capture_method/__init__.py:106:5: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ src/capture_method/__init__.py:106:5: note:      Superclass:
+ src/capture_method/__init__.py:106:5: note:          @overload
+ src/capture_method/__init__.py:106:5: note:          def get(self, object, /) -> type[CaptureMethodBase] | None
+ src/capture_method/__init__.py:106:5: note:          @overload
+ src/capture_method/__init__.py:106:5: note:          def [_T] get(self, object, /, default: type[CaptureMethodBase] | _T) -> type[CaptureMethodBase] | _T
+ src/capture_method/__init__.py:106:5: note:      Subclass:
+ src/capture_method/__init__.py:106:5: note:          def get(self, key: CaptureMethodEnum, object = ..., /) -> Any

operator (https://github.com/canonical/operator)
- ops/model.py:953: note:          def get(self, str, /) -> Binding | None
+ ops/model.py:953: note:          def get(self, object, /) -> Binding | None
- ops/model.py:953: note:          def [_T] get(self, str, /, default: Binding | _T) -> Binding | _T
+ ops/model.py:953: note:          def [_T] get(self, object, /, default: Binding | _T) -> Binding | _T
- ops/testing.py:2572: error: No overload variant of "pop" of "dict" matches argument types "str", "None"  [call-overload]
- ops/testing.py:2572: note: Possible overload variants:
- ops/testing.py:2572: note:     def pop(self, int, /) -> dict[str, Any]
- ops/testing.py:2572: note:     def pop(self, int, dict[str, Any], /) -> dict[str, Any]
- ops/testing.py:2572: note:     def [_T] pop(self, int, _T, /) -> dict[str, Any] | _T

jax (https://github.com/google/jax)
+ jax/_src/core.py:1457: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/core.py:3118: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:643: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:1494: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:1501: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/partial_eval.py:2684: error: Unused "type: ignore" comment  [unused-ignore]
+ jax/_src/interpreters/mlir.py:1837: error: Unused "type: ignore" comment  [unused-ignore]

bidict (https://github.com/jab/bidict)
+ bidict/_bidict.py: note: In member "pop" of class "MutableBidict":
+ bidict/_bidict.py:121:6: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ bidict/_bidict.py:121:6: note:      Superclass:
+ bidict/_bidict.py:121:6: note:          @overload
+ bidict/_bidict.py:121:6: note:          def pop(self, KT, /) -> VT
+ bidict/_bidict.py:121:6: note:          @overload
+ bidict/_bidict.py:121:6: note:          def pop(self, object, /, default: VT) -> VT
+ bidict/_bidict.py:121:6: note:          @overload
+ bidict/_bidict.py:121:6: note:          def [_T] pop(self, object, /, default: _T) -> VT | _T
+ bidict/_bidict.py:121:6: note:      Subclass:
+ bidict/_bidict.py:121:6: note:          @overload
+ bidict/_bidict.py:121:6: note:          def pop(self, KT, /) -> VT
+ bidict/_bidict.py:121:6: note:          @overload
+ bidict/_bidict.py:121:6: note:          def [DT] pop(self, KT, DT = ..., /) -> VT | DT

pyodide (https://github.com/pyodide/pyodide)
+ src/py/_pyodide/_core_docs.py:1002: error: Signature of "get" incompatible with supertype "Mapping"  [override]
+ src/py/_pyodide/_core_docs.py:1002: note:      Superclass:
+ src/py/_pyodide/_core_docs.py:1002: note:          @overload
+ src/py/_pyodide/_core_docs.py:1002: note:          def get(self, object, /) -> VT_co | None
+ src/py/_pyodide/_core_docs.py:1002: note:          @overload
+ src/py/_pyodide/_core_docs.py:1002: note:          def [_T] get(self, object, /, default: VT_co | _T) -> VT_co | _T
+ src/py/_pyodide/_core_docs.py:1002: note:      Subclass:
+ src/py/_pyodide/_core_docs.py:1002: note:          @overload
+ src/py/_pyodide/_core_docs.py:1002: note:          def get(self, KT, /) -> VT_co | None
+ src/py/_pyodide/_core_docs.py:1002: note:          @overload
+ src/py/_pyodide/_core_docs.py:1002: note:          def [T] get(self, KT, VT_co | T, /) -> VT_co | T
+ src/py/_pyodide/_core_docs.py:1036: error: Signature of "pop" incompatible with supertype "MutableMapping"  [override]
+ src/py/_pyodide/_core_docs.py:1036: note:      Superclass:
+ src/py/_pyodide/_core_docs.py:1036: note:          @overload
+ src/py/_pyodide/_core_docs.py:1036: note:          def pop(self, KT, /) -> VT
+ src/py/_pyodide/_core_docs.py:1036: note:          @overload
+ src/py/_pyodide/_core_docs.py:1036: note:          def pop(self, object, /, default: VT) -> VT
+ src/py/_pyodide/_core_docs.py:1036: note:          @overload
+ src/py/_pyodide/_core_docs.py:1036: note:          def [_T] pop(self, object, /, default: _T) -> VT | _T
+ src/py/_pyodide/_core_docs.py:1036: note:      Subclass:
+ src/py/_pyodide/_core_docs.py:1036: note:          @overload
+ src/py/_pyodide/_core_docs.py:1036: note:          def pop(self, KT, /) -> VT
+ src/py/_pyodide/_core_docs.py:1036: note:          @overload
+ src/py/_pyodide/_core_docs.py:1036: note:          def [T] pop(self, KT, VT | T = ..., /) -> VT | T

prefect (https://github.com/PrefectHQ/prefect)
- src/prefect/_internal/concurrency/inspection.py:93: error: Argument 1 to "get" of "dict" has incompatible type "int | None"; expected "int"  [arg-type]
- src/prefect/utilities/dispatch.py:163: error: Argument 1 to "get" of "dict" has incompatible type "str | None"; expected "str"  [arg-type]
- src/prefect/runner/runner.py:561: error: Argument 1 to "get" of "dict" has incompatible type "UUID | None"; expected "UUID"  [arg-type]

@Daverball
Copy link
Contributor

Daverball commented Jul 15, 2024 •
edited
Loading

Using Any rather than object may be less disruptive to existing code while achieving mostly the same result, since that code may violate contravariance on object. Expecting everyone to change their Mapping-like constructs in their own code seems like a bit much, considering these functions should only ever return the default value if you pass in a key type the corresponding Mapping doesn't contain.

Also this way you will stop catching some genuine problems, where you're doing an unnecessary hash lookup, for a relatively small gain in ergonomics. Although that gap could potentially be closed by strictness settings in type checkers, similar to how you can optionally report non-overlapping equality checks in mypy.

@mikeshardmind
Copy link
Contributor Author

The choice to use object was intentional. This matches runtime behavior. The use of Any here is both unnecessary, would hide problems with subclasses that are are more restrictive, violating LSP, and would create many cases where these stubs are incompatible with strict typechecing settings. As is shown in some of the adjusted stubs, it's possible to add a type: ignore[override] in places where such an incompatible override is intentional.

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jul 15, 2024 •
edited
Loading

Also:

Also this way you will stop catching some genuine problems, where you're doing an unnecessary hash lookup, for a relatively small gain in ergonomics. Although that gap could potentially be closed by strictness settings in type checkers, similar to how you can optionally report non-overlapping equality checks in mypy.

The hash check is cheaper than the possible isinstance check to avoid it. You should see some of the places these changes fix

and faster than any of them, is

try:
    value = mapping[key]
except KeyError:
    value = default

, __getitem__, was not changed (for good reasons here involving retaining the usefulness of using type analysis to handle unreachable code), the same reason .pop without a default was not changed.

@Daverball
Copy link
Contributor

Daverball commented Jul 15, 2024 •
edited
Loading

The choice to use object was intentional. This matches runtime behavior. The use of Any here is both unnecessary, would hide problems with subclasses that are are more restrictive, violating LSP, and would create many cases where these stubs are incompatible with strict typechecing settings. As is shown in some of the adjusted stubs, it's possible to add a type: ignore[override] in places where such an incompatible override is intentional.

This assumes that allowing to pass arbitrary keys to get without encountering exceptions is part of the contract of Mapping, but that's not really the case. You are e.g. not allowed to pass in unhashable types. Hashable isn't really reliable, so the use of it is rare in typeshed, so I'm not suggesting a change to that. Any is generally what typeshed uses when we know most values are fine, but we can't ensure this at type checking time. object signals that truly any value is fine, which is not the case here. So the LSP violations you are trying to guard against here can't really reliably be deemed LSP violations.

The hash check is cheaper than the possible isinstance check to avoid it. You should see some of the places these changes fix

If we're going to get into performance characteristics, which I don't think is very productive: The vast majority of cases where this would help with is None and an identity check is at least as fast but probably slightly faster than the hash lookup.

I think improving ergonomics is a worthwhile experiment, but I personally feel it shouldn't come at this large of an upfront cost, otherwise I would've suggested adding an overload for None, rather than switching to Any.

@AlexWaygood
Copy link
Member

(Not trying to close off discussion, just linking to previous discussion for context)

This was previously discussed at #6597 and #8219

@mikeshardmind
Copy link
Contributor Author

mikeshardmind commented Jul 15, 2024 •
edited
Loading

At the end of the day, I don't think the stubs should be cutting off things that are correct at runtime and are expressible in the type system. The type system is supposed to catch type errors, the current stubs instead flag correct code incorrectly, which has in turn led to user subclasses needed to inherit this incorrectness or type ignore it when overriding it.

There are plenty of real world cases of this, evidenced by libraries that spoke out against the change to what we have now (see prior issues linked above) as well as several libraries that show up in the primer run that type ignore this rather than accommodate incorrect standard library typings.

It's also worth calling attention to the fact that user defined Mapping's are not required to only work on Hashable types, you can define a mapping today which doesn't use hash but uses some other means of binning + equality, and there are occasional reasons to do so, such as mappings that are backed by data stores being able to write only a subset of changes based on a stable binning function, the Hashable requirement is an implementation detail of some Mappings, of which dict is one.

This is probably another great example of why subclassing ABCs rather than having common things expressed as protocols is detrimental

@Daverball
Copy link
Contributor

It's also worth calling attention to the fact that user defined Mapping's are not required to only work on Hashable types, you can define a mapping today which doesn't use hash but uses some other means of binning + equality, and there are occasional reasons to do so, such as mappings that are backed by data stores being able to write only a subset of changes based on a stable binning function, the Hashable requirement is an implementation detail of some Mappings, of which dict is one.

This is exactly one of the reasons I disagree with this change. Since we're talking about a parameter type object is actually the most restrictive annotation you can come up. The statement you're citing doesn't really say anything about what the protocol itself should accept, because it's always valid for a subclass to be more permissive about its argument types, it is however not valid for it to be less permissive.

This exact problem has come up again and again in past discussions that wanted to change things in the opposite direction for methods like __eq__ and __contains__. So if you change this parameter to object, what you're actually doing is saying that every valid Mapping MUST accept object for get, but since dict already violates this, I think it's a bad expectation to set, especially since otherwise implementations are forced to directly or indirectly inherit from Mapping and ignore the LSP violation and can't rely on structural matching.

Any doesn't have that problem, because it recognizes the fact that Mapping is fairly loose about how restrictive get is allowed to be about the accepted key values. If I see Any in a Protocol in an API that is otherwise well-typed, I will go check to see if there is a comment in the stubs, since there's probably some sharp corner I need to know about. If I see object I don't think at all and just assume things will work.

I can sympathize with the motivation behind why this parameter was originally changed to _KT. Since get will likely not throw an exception at runtime if you pass it the wrong kind of key, it's very easy to have bugs in your code that are neither caught at runtime nor type checking time and will only be caught by rigorous testing. The ergonomics trade-off is admittedly pretty harsh for None and some type unions but I personally think it's worth what you get return. If we were to loosen things up again I'd want type checkers to add strictness flags to detect this kind of problem, where the passed in key type is completely disjoint from the Mapping's key type and hence likely a bug.

@mikeshardmind mikeshardmind deleted the permissive_get branch August 2, 2024 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikeshardmind @Daverball @AlexWaygood