Issue46399
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2022-01-16 13:55 by AlexWaygood, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 30629 | closed | kumaraditya, 2022-01-17 09:14 | |
PR 30630 | closed | kumaraditya, 2022-01-17 09:51 |
Messages (21) | |||
---|---|---|---|
msg410695 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-16 13:55 | |
Issue40890 added a new `.mapping` attribute to dict_keys, dict_values and dict_items in 3.10. This addition is great for introspection. However, it has inadvertently caused some unexpected problems for type-checkers. Prior to Issue40890, the typeshed stub for the three `dict` methods was something like this: ``` from typing import MutableMapping, KeysView, ItemsView, ValuesView, TypeVar _KT = TypeVar("_KT") _VT = TypeVar("_VT") class dict(MutableMapping[_KT, _VT]): def keys(self) -> KeysView[_KT]: ... def values(self) -> ValuesView[_VT]: ... def items(self) -> ItemsView[_KT, _VT]: ... ``` In other words, typeshed did not acknowledge the existence of a "dict_keys" class at all. Instead, it viewed the precise class as an implementation detail, and merely stated in the stub that `dict.keys()` would return some class that implemented the `KeysView` interface. After Issue40890, however, it was clear that this approach would no longer suffice, as mypy (and other type-checkers) would yield false-positive errors for the following code: ``` m = dict().keys().mapping ``` Following several PRs, the typeshed stub for these `dict` methods now looks something like this: ``` # _collections_abc.pyi import sys from types import MappingProxyType from typing import Generic, KeysView, ValuesView, ItemsView, TypeVar, final _KT_co = TypeVar("_KT_co", covariant=True) _VT_co = TypeVar("_VT_co", covariant=True) @final class dict_keys(KeysView[_KT_co], Generic[_KT_co, _VT_co]): if sys.version_info >= (3, 10): mapping: MappingProxyType[_KT_co, _VT_co] @final class dict_values(ValuesView[_VT_co], Generic[_KT_co, _VT_co]): if sys.version_info >= (3, 10): mapping: MappingProxyType[_KT_co, _VT_co] @final class dict_items(ItemsView[_KT_co, _VT_co], Generic[_KT_co, _VT_co]): if sys.version_info >= (3, 10): mapping: MappingProxyType[_KT_co, _VT_co] # builtins.pyi from _collections_abc import dict_keys, dict_views, dict_items from typing import MutableMapping, TypeVar _KT = TypeVar("_KT") _VT = TypeVar("_VT") class dict(MutableMapping[_KT, _VT]): def keys(self) -> dict_keys[KT, VT]: ... def values(self) -> dict_values[_KT, _VT]: ... def items(self) -> dict_items[_KT, _VT]: ... ``` The alteration to the typeshed stub means that mypy will no longer give false-positive errors for code in which a user attempts to access the `mapping` attribute. However, it has serious downsides. Users wanting to create typed subclasses of `dict` have found that they are no longer able to annotate `.keys()`, `.values()` and `.items()` as returning `KeysView`, `ValuesView` and `ItemsView`, as mypy now flags this as an incompatible override. Instead, they now have to import `dict_keys`, `dict_values` and `dict_items` from `_collections_abc`, a private module. Moreover, they are unable to parameterise these classes at runtime. In other words, you used to be able to do this: ``` from typing import KeysView, TypeVar K = TypeVar("K") V = TypeVar("V") class DictSubclass(dict[K, V]): def keys(self) -> KeysView[K]: return super().keys() ``` But now, you have to do this: ``` from _collections_abc import dict_keys from typing import TypeVar K = TypeVar("K") V = TypeVar("V") class DictSubclass(dict[K, V]): def keys(self) -> "dict_keys[K, V]": return super().keys() ``` References: * PR where `.mapping` attribute was added to the typeshed stubs: https://github.com/python/typeshed/pull/6039 * typeshed issue where this was recently raised: https://github.com/python/typeshed/issues/6837 * typeshed PR where this was further discussed: https://github.com/python/typeshed/pull/6888 |
|||
msg410696 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-16 13:58 | |
As a fix, I propose that two changes be made to `dict_keys`, `dict_values` and `dict_items`: * They should be officially exposed in the `types` module. * `__class_getitem__` should be added to the classes. These two changes would mean that users would be able to do the following: ``` from types import dict_keys from typing import TypeVar K = TypeVar("K") V = TypeVar("V") class DictSubclass(dict[K, V]): def keys(self) -> dict_keys[K, V]: return super().keys() ``` |
|||
msg410699 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2022-01-16 16:21 | |
> I propose that two changes be made to `dict_keys`, `dict_values` > and `dict_items`: > > * They should be officially exposed in the `types` module. > * `__class_getitem__` should be added to the classes. +1 |
|||
msg410703 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-16 16:53 | |
Okay, PR welcome. (Though since this can only work for 3.11+, maybe typeshed could also be adjusted? For that you’d have to file a bug there.) |
|||
msg410752 - (view) | Author: Kumar Aditya (kumaraditya) * | Date: 2022-01-17 09:41 | |
I would like to work on this. |
|||
msg410753 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-17 10:13 | |
Thanks, Kumar — I appreciate it! > (Though since this can only work for 3.11+, maybe typeshed could also be adjusted? For that you’d have to file a bug there.) Yes — it's unfortunate, but I have some ideas for hacky workarounds we can implement in typeshed for 3.10 :) and, as you say — that's a problem to be dealt with on the typeshed issue tracker rather than here. |
|||
msg410754 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-17 10:14 | |
Do these changes warrant an entry in "What's New in 3.11"? |
|||
msg410822 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-17 20:45 | |
Probably. On Mon, Jan 17, 2022 at 02:14 Alex Waygood <report@bugs.python.org> wrote: > > Alex Waygood <Alex.Waygood@Gmail.com> added the comment: > > Do these changes warrant an entry in "What's New in 3.11"? > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue46399> > _______________________________________ > -- --Guido (mobile) |
|||
msg410921 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-01-19 02:29 | |
The types of `.keys()`, `.items()`, and `.values()` on `collections.OrderedDict` are distinct from those for dict, and they are also not exposed anywhere. Should we put them in a public, documented place too for consistency? >>> import collections >>> od = collections.OrderedDict() >>> type(od.keys()) <class 'odict_keys'> |
|||
msg410923 - (view) | Author: Inada Naoki (methane) * | Date: 2022-01-19 04:53 | |
I am not happy about exposing every internal types. I prefer duck typing. Like OrderedDict, not all dict subtypes uses `dict_keys`, `dict_views`, and `dict_items`. If typeshed annotate dict.keys() returns `dict_keys`, "incompatible override" cano not be avoided. I prefer: * Keep status-quo: keys().mapping cause false positive and user need to suppress. This is not a big problem because `.mapping` is very rarely used. * Or add `.mapping` to `KeysView`, `ValuesView`, and `ItemsView`. Force every dict subclasses to implement it. |
|||
msg410928 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-19 08:59 | |
I agree with Inada that not every internal type should be exposed, but I would make an exception for the dict views classes due to the fact that dict subclasses are much more common than subclasses of other mappings, such as OrderedDict. I don't think it's *particularly* important to expose the OrderedDict views classes in the same way. Adding the "mapping" attribute to KeysView/ValuesView/ItemsView seems like it could be quite disruptive — there may be a lot of third-party users with classes that inherit from those, who'd need to make changes to their code. From a typing perspective, it would also mean that KeysView and ValuesView would have to be parameterised with two TypeVars (key-type and value-type), whereas now they both only take one (KeysView is parameterised with the key-type, ValuesView with the value-type). |
|||
msg410929 - (view) | Author: Inada Naoki (methane) * | Date: 2022-01-19 09:48 | |
> I agree with Inada that not every internal type should be exposed, but I would make an exception for the dict views classes due to the fact that dict subclasses are much more common than subclasses of other mappings, such as OrderedDict. I don't think it's *particularly* important to expose the OrderedDict views classes in the same way. I am afraid that you misread me. I used OrderedDict as one example of dict subclass. I didn't mean dict_(keys|items|values) shouldn't exposed because of I don't want to expose odict_(keys|items|values). Anyway, OrderedDict was not good choise to explain my thought because its builtin type and defined in typeshed. Instead, I use sortedcontainers.SortedDict as example. See https://github.com/grantjenks/python-sortedcontainers/blob/dff7ef79a21b3f3ceb6a19868f302f0a680aa243/sortedcontainers/sorteddict.py#L43 It is a dict subclass. It's `keys()` method returns `SortedKeysView`. `SortedKeysView` is subclass of `collections.abc.KeysView`. But it is not subclass of `dict_keys`. If `dict.keys()` in typeshed defines it returns `dict_keys`, doesn't mypy flag it as an "incompatible override"? So I propose that typeshed defines that dict.keys() returns KeysView, not dict_keys. Although subclass of dict is very common, it is very rare that: * Override `keys()`, and * Returns `super().keys()`, instead of KeysView (or list), and * `.keys().mapping` is accessed. It is very minor inconvinience that user need to ignore false positive for this very specific cases. Or do you think this case is much more common than classes like SortedDict? Note that dict_(keys|items|values) is implementation detail and subclassing it doesn't make sense. Another option is adding more ABC or Protocol that defines `.mapping` attribute. SortedKeysView can inherit it and implement `.mapping`. |
|||
msg410930 - (view) | Author: Inada Naoki (methane) * | Date: 2022-01-19 10:10 | |
In other words, a. If `.keys()` in all dict subclasses must return subclass of `dict_keys`: `dict.keys() -> dict_keys`. b. If `.keys().mapping` must be accessible for all dict subclasses: Add `.mapping` to `KeysView`. c. If `.keys().mapping` is optional for dict subclasses: typeshed can't add `.mapping` to anywhere, AFAIK. |
|||
msg410933 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2022-01-19 10:23 | |
I share concerns of Inada-san. I also think that keeping a status quo (ignoring the mapping attribute in typing) is the lesser evil. I am not sure that exposing this attribute was a good idea. We do not expose attributes list and index for list iterators. |
|||
msg410955 - (view) | Author: Guido van Rossum (gvanrossum) * | Date: 2022-01-19 14:59 | |
Given all this discussion I defer to Serhiy and Inada-San. On Wed, Jan 19, 2022 at 02:23 Serhiy Storchaka <report@bugs.python.org> wrote: > > Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment: > > I share concerns of Inada-san. I also think that keeping a status quo > (ignoring the mapping attribute in typing) is the lesser evil. I am not > sure that exposing this attribute was a good idea. We do not expose > attributes list and index for list iterators. > > ---------- > > _______________________________________ > Python tracker <report@bugs.python.org> > <https://bugs.python.org/issue46399> > _______________________________________ > -- --Guido (mobile) |
|||
msg410960 - (view) | Author: Jelle Zijlstra (JelleZijlstra) * | Date: 2022-01-19 15:38 | |
I see the concerns about exposing too many implementation details. But I'm also not sure how much this will really help static typing use cases. Alex's examples just call super().keys(), but if you do that, there's not much point in overriding keys() in the first place. These classes don't allow subclassing or instantiation: >>> t = type({}.items()) >>> class X(t): ... pass ... Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: type 'dict_items' is not an acceptable base type >>> t({}) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: cannot create 'dict_items' instances So I can't think of much useful, well-typed code that you could write if these classes were public. For a use case like SortedDict, you'd still need to write your own class implementing KeysView, and you'd get an error from the type checker because it's incompatible with dict_keys. |
|||
msg411020 - (view) | Author: Akuli (Akuli) | Date: 2022-01-20 12:16 | |
> I also think that keeping a status quo (ignoring the mapping attribute in typing) is the lesser evil. Can you clarify this? There are several things that typing could do, and I don't know which option you are referring to. I listed most of them below, and I'd like to know which exactly of them "is the lesser evil". If we literally ignore the attribute, any usage of `.mapping` will be an error, which basically makes the whole `.mapping` feature useless for statically typed code. It also wouldn't appear in IDE autocompletions. If we add it to `KeysView` and `ValuesView`, library authors will end up using `.mapping` with arguments annotated as `Mapping` or `MutableMapping`, not realizing it is purely a dict thing, not required from an arbitrary mapping object. If we keep `.mapping` in dict but not anywhere else, as described already, it becomes difficult to override .keys() and .values() in a dict subclass. You can't just return a KeysView or a ValuesView. If that was allowed, how should people annotate code that uses `.mapping`? You can't annotate with `dict`, because that also allows subclasses of dict, which might not have a `.mapping` attribute. Yet another option would be to expose `dict_keys` and `dict_values` somewhere where they don't actually exist at runtime. This leads to code like this: from typing import Any, TYPE_CHECKING if TYPE_CHECKING: # A lie for type checkers to work. from something_that_doesnt_exist_at_runtime import dict_keys, dict_values else: # Runtime doesn't check type annotations anyway. dict_keys = Any dict_values = Any While this works, it isn't very pretty. |
|||
msg411065 - (view) | Author: Inada Naoki (methane) * | Date: 2022-01-21 01:08 | |
> If we literally ignore the attribute, any usage of `.mapping` will be an error, which basically makes the whole `.mapping` feature useless for statically typed code. It also wouldn't appear in IDE autocompletions. `.mapping` is not exist between Python 3.0~3.9. And it is not feature that is long awaited by many users. See https://bugs.python.org/issue40890#msg370841 Raymond said: Traditionally, we do expose wrapped objects: property() exposes fget, partial() exposes func, bound methods expose __func__, ChainMap() exposes maps, etc. Exposing this attribute would help with introspection, making it possible to write efficient functions that operate on dict views. Type hints is very useful for application code, especially when it is large. But introspection is very rarely used in such typed code bases. I don't think `.mapping` is useful for many users, like `.fget` of the property. So adding `# type: ignore` in such lines is the "lesser evil". > If we add it to `KeysView` and `ValuesView`, library authors will end up using `.mapping` with arguments annotated as `Mapping` or `MutableMapping`, not realizing it is purely a dict thing, not required from an arbitrary mapping object. It doesn't make sense at all, IMO. If we really need `.mapping` in typeshed, we should add it to `KeysViewWithMapping`. So mapping classes that don't inherit dict shouldn't be forced to implement `.mapping`. > If we keep `.mapping` in dict but not anywhere else, as described already, it becomes difficult to override .keys() and .values() in a dict subclass. You can't just return a KeysView or a ValuesView. If that was allowed, how should people annotate code that uses `.mapping`? You can't annotate with `dict`, because that also allows subclasses of dict, which might not have a `.mapping` attribute. `# type: ignore`. > Yet another option would be to expose `dict_keys` and `dict_values` somewhere where they don't actually exist at runtime. This leads to code like this: > > from typing import Any, TYPE_CHECKING > if TYPE_CHECKING: > # A lie for type checkers to work. > from something_that_doesnt_exist_at_runtime import dict_keys, dict_values > else: > # Runtime doesn't check type annotations anyway. > dict_keys = Any > dict_values = Any > > While this works, it isn't very pretty. What problem this problem solve? `SortedDict.keys()` can not return `dict_keys`. As far as I think, your motivation is making dict subclass happy with type checkers. But this option doesn't make dict subclass happy at all. |
|||
msg411098 - (view) | Author: Akuli (Akuli) | Date: 2022-01-21 08:55 | |
I now agree that `# type: ignore` in dict subclasses makes sense the most, and exposing the views doesn't solve practical problems. We talked more with the people who brought this up in typing. Turns out that the correct solution to their problems was to just inherit from MutableMapping instead of inheriting from dict. If you really need to inherit from dict and make .keys() do something else than it does by default, you're most likely trying to override all dict methods that do something with the keys, and you should just inherit from MutableMapping instead. |
|||
msg411115 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-21 11:59 | |
I have also been persuaded that my suggested solution is not the way to go. Thanks everybody for the useful discussion. |
|||
msg411239 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2022-01-22 09:19 | |
Thanks for the PRs, Kumar — I appreciate you putting in the time. Sorry for the wasted effort. |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:54 | admin | set | github: 90557 |
2022-01-22 09:19:51 | AlexWaygood | set | status: open -> closed resolution: not a bug messages: + msg411239 stage: patch review -> resolved |
2022-01-21 11:59:29 | AlexWaygood | set | messages: + msg411115 |
2022-01-21 08:55:19 | Akuli | set | messages: + msg411098 |
2022-01-21 01:29:27 | gvanrossum | set | nosy:
- gvanrossum |
2022-01-21 01:08:45 | methane | set | messages: + msg411065 |
2022-01-20 12:16:01 | Akuli | set | nosy:
+ Akuli messages: + msg411020 |
2022-01-19 15:38:45 | JelleZijlstra | set | messages: + msg410960 |
2022-01-19 14:59:16 | gvanrossum | set | messages: + msg410955 |
2022-01-19 10:23:29 | serhiy.storchaka | set | messages: + msg410933 |
2022-01-19 10:10:43 | methane | set | messages: + msg410930 |
2022-01-19 09:48:22 | methane | set | messages: + msg410929 |
2022-01-19 08:59:44 | AlexWaygood | set | messages: + msg410928 |
2022-01-19 04:53:13 | methane | set | nosy:
+ methane messages: + msg410923 |
2022-01-19 02:29:23 | JelleZijlstra | set | messages: + msg410921 |
2022-01-17 20:45:09 | gvanrossum | set | messages: + msg410822 |
2022-01-17 10:14:06 | AlexWaygood | set | messages: + msg410754 |
2022-01-17 10:13:18 | AlexWaygood | set | messages: + msg410753 |
2022-01-17 09:51:15 | kumaraditya | set | pull_requests: + pull_request28833 |
2022-01-17 09:41:35 | kumaraditya | set | messages: + msg410752 |
2022-01-17 09:14:25 | kumaraditya | set | keywords:
+ patch stage: patch review pull_requests: + pull_request28832 |
2022-01-17 08:56:21 | kumaraditya | set | nosy:
+ kumaraditya |
2022-01-16 16:53:17 | gvanrossum | set | messages: + msg410703 |
2022-01-16 16:21:56 | rhettinger | set | messages: + msg410699 |
2022-01-16 13:58:59 | AlexWaygood | set | messages: + msg410696 |
2022-01-16 13:55:19 | AlexWaygood | create |