Add undocumented _mapping to collections.abc MappingView, KeysView, ValuesView, ItemsView

[laundmo]

Added self._mapping attribute which was missing from collections.abc.MappingView and subclasses.

fixes: #7576

[laundmo]

I'm unsure what's causing the allowlist failures, since I'm unsure what the allowlist does. Could anyone explain or help out?

[AlexWaygood]

I'm unsure what's causing the allowlist failures, since I'm unsure what the allowlist does. Could anyone explain or help out?

The stubtest failures are actually a good thing, in this case!

Stubtest is a script we run in CI that tests whether the stubs are consistent with the runtime — e.g. if a function at runtime has three arguments, but it only has two arguments in the stub, stubtest will complain about it. But it's not always possible for us to match the runtime implementation 100%, so we have an "allowlist" of objects that have slight discrepancies with the runtime implementation, and stubtest ignores for us.

In this situation, it seems like several allowlist entries are no longer needed for some reason. The stubtest failures are telling us that we should now delete those entries from the allowlists, since they're no longer used. I'm not quite sure why this PR has that effect, tbh, but fewer allowlist entries is always a good thing!

[laundmo]

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

core (https://github.com/home-assistant/core)
+ homeassistant/components/yeelight/scanner.py:133: error: Incompatible return value type (got "dict_values[str, Any]", expected "ValuesView[Any]")  [return-value]
+ homeassistant/components/fritz/switch.py:201: error: Argument 3 to "device_filter_out_from_trackers" has incompatible type "dict_values[Any, Any]"; expected "ValuesView[Any]"  [arg-type]
+ homeassistant/components/fritz/device_tracker.py:62: error: Argument 3 to "device_filter_out_from_trackers" has incompatible type "dict_values[Any, Any]"; expected "ValuesView[Any]"  [arg-type]

Interesting, looking at these issues, they seem to typehint a function that takes dict.values() as collections.abc.ValuesView - which will now fail because dict_values can't contain _mapping.

I wonder whether this is common practice, and whether it should be. To me it seems wrong to use collections.abc here... but there might also be a way to prevent these errors that i dont know about.

[AlexWaygood]

The allowlists are found in this folder here: https://github.com/python/typeshed/tree/master/tests/stubtest_allowlists

If you delete the relevant unused entries, the CI will go green :)

[AlexWaygood]

I wonder whether this is common practice, and whether it should be. To me it seems wrong to use collections.abc here...

To the contrary: the documentation states that typing.{Mapping,Keys,Values,Items}View are all deprecated in 3.9+. Using the equivalent classes from collections.abc is the recommended practice for this kind of situation.

[laundmo]

I wonder whether this is common practice, and whether it should be. To me it seems wrong to use collections.abc here...

To the contrary: as the documentation states, typing.{Mapping,Keys,Values,Items}View are all deprecated in 3.9+. Using the equivalent classes from collections.abc is the recommended practice for this kind of situation.

Would you have any idea how I could work around the issue that the collections.abc are supposed to be used for type hinting dict.values(), but also have the undocumented _mapping? It seems to me that adding _mapping will always cause errors in that spot, but not adding it will cause errors when subclassing the views.

[AlexWaygood]

It seems to me that adding _mapping will always cause errors in that spot

Yes, this is partly why I'm still not 100% sure that merging this PR is a good idea :)

For the subclassing use case, have you considered just doing this? It would mean we wouldn't have to add the undocumented attribute to the stub, and silences MyPy's complaints:

import collections.abc
from typing import Any

class MyView(collections.abc.KeysView):
   _mapping: dict[int, Any]

   def __iter__(self):
        for key in self._mapping:
            yield key + 1

[laundmo]

For the subclassing use case, have you considered just doing this?
_mapping: dict[str, Any]

Yes, that's actually what I'm currently doing, and what SQLAlchemy does too (they also subclass the views)
If it proves to be this much of a hassle, it might be better not to merge. Though, I wonder what might happen if a similar issue arises with more critical pieces of code.

[AlexWaygood]

Another way of getting rid of the mypy_primer diff might be to just add the _mapping attribute to typing.Mapping, instead of attempting to decouple the typing/collections.abc classes. That's basically what we did with collections.abc.Set._hash() in #7153

[github-actions]

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

core (https://github.com/home-assistant/core)
+ homeassistant/components/yeelight/scanner.py:133: error: Incompatible return value type (got "dict_values[str, Any]", expected "ValuesView[Any]")  [return-value]
+ homeassistant/components/fritz/switch.py:201: error: Argument 3 to "device_filter_out_from_trackers" has incompatible type "dict_values[Any, Any]"; expected "ValuesView[Any]"  [arg-type]
+ homeassistant/components/fritz/device_tracker.py:62: error: Argument 3 to "device_filter_out_from_trackers" has incompatible type "dict_values[Any, Any]"; expected "ValuesView[Any]"  [arg-type]

[laundmo]

Another way of getting rid of the mypy_primer diff might be to just add the _mapping attribute to typing.Mapping, instead of attempting to decouple the typing/collections.abc classes. That's basically what we did with collections.abc.Set._hash() in #7153

Currently, I inherit dict_{items,key,values} from typing.{Items,Keys,Values}View, which is why they are erroring out when compared to collections.abc.{Items,Keys,Values}View.

I could instead inherit them from the new collections.abc Views which contain the _mapping, achieving the same effect as adding the _mapping to typing.MappingView (which is what i assume you meant) with the added benefit of working TypeVars.

This would make dict.values() etc show up as having the _mapping attribute (false positive). I wonder whether there's a way to make _mapping fail for dict_values while still not raising errors when passed to something type hinted as ValuesView.

[AlexWaygood]

I don't think it's a good idea to start adding attributes to collections.abc.MappingView, and deliberately not include them on typing.MappingView. For typing purposes, you're meant to be able to use the two interchangeably, and the mypy_primer output confirms that this would have unexpected and unfortunate consequences.

Please update your PR to simply add the _mapping attribute to typing.MappingView, instead of adding a whole new class to _collections_abc.pyi.

[AlexWaygood]

Closing due to lack of response from OP.

@laundmo, if you're still interested in making this change, feel free to file a new PR that adds the attribute to typing.MappingView (etc.), instead of trying to separate collections.abc.MappingView into a separate class. Thanks! 🙂