Skip to content
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

Add support for dict.{keys,values,items}.mapping #6039

Merged
merged 4 commits into from Sep 20, 2021

Conversation

KotlinIsland
Copy link
Contributor

I added _dict_keys, _dict_values, _dict_items classes to builtins.pyi and made dict return those instead of the base XView.

from: https://bugs.python.org/issue40890

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@KotlinIsland KotlinIsland force-pushed the support_dict_view_mapping branch 2 times, most recently from 490430a to f40d486 Compare September 17, 2021 04:41
@github-actions

This comment has been minimized.

@KotlinIsland KotlinIsland force-pushed the support_dict_view_mapping branch 2 times, most recently from 0957a1e to 20e6fc0 Compare September 17, 2021 04:48
@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

The SinbadCogs mypy-primer error looks concerning.

@github-actions

This comment has been minimized.

@KotlinIsland
Copy link
Contributor Author

Hi @JelleZijlstra, I think this is an issue with mypy, not with this change:

from typing import TypeVar, Generic, ValuesView

_KT = TypeVar("_KT")
_VT = TypeVar("_VT")
_KT_co = TypeVar("_KT_co", covariant=True)  # Key type covariant containers.
_VT_co = TypeVar("_VT_co", covariant=True)  # Value type covariant containers.

class AValues(ValuesView[_VT_co], Generic[_KT_co, _VT_co]):
    pass

class A(dict[_KT, _VT], Generic[_KT, _VT]):
    def c(self) -> AValues[_KT, _VT]:
        return AValues(self)

a = A({1: [1]})
l = [*a.c()]
print(l)  # [[1]]
reveal_type(l)  # note: Revealed type is "builtins.list[builtins.int*]"
from typing import TypeVar, Generic, ValuesView

_KT = TypeVar("_KT")
_VT = TypeVar("_VT")
_KT_co = TypeVar("_KT_co", covariant=True)  # Key type covariant containers.
_VT_co = TypeVar("_VT_co", covariant=True)  # Value type covariant containers.

class AValues(ValuesView[_VT_co], Generic[_VT_co]):
    pass

class A(dict[_KT, _VT], Generic[_KT, _VT]):
    def c(self) -> AValues[_VT]:
        return AValues(self)

a = A({1: [1]})
l = [*a.c()]
print(l)  # [[1]]
reveal_type([*a.c()])  # note: Revealed type is "builtins.list[builtins.list*[builtins.int*]]"

@DetachHead
Copy link
Contributor

python/mypy#11138

@JelleZijlstra
Copy link
Member

Mypy does seem buggy there, but I think you can work around the bug by swapping the type arguments to _dict_values (make it Generic[_VT_co, _KT_co]). That's a bit confusing for people who want to instantiate a _dict_values, but that should be rare so it's probably fine.

@KotlinIsland KotlinIsland force-pushed the support_dict_view_mapping branch 3 times, most recently from 0fb471c to 968c3c2 Compare September 19, 2021 05:29
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@KotlinIsland
Copy link
Contributor Author

KotlinIsland commented Sep 19, 2021

Hi @JelleZijlstra, just a small note: are the generics on _dict_items redundant?

class _dict_items(ItemsView[_KT_co, _VT_co], Generic[_KT_co, _VT_co]):

# could be:
class _dict_items(ItemsView[_KT_co, _VT_co]):

@github-actions

This comment has been minimized.

Comment on lines 250 to 245
class _OrderedDictKeysView(KeysView[_KT], Reversible[_KT]):
class _OrderedDictKeysView(_dict_keys[_KT, _VT], Reversible[_KT]):
def __reversed__(self) -> Iterator[_KT]: ...

class _OrderedDictItemsView(ItemsView[_KT, _VT], Reversible[Tuple[_KT, _VT]]):
class _OrderedDictItemsView(_dict_items[_KT, _VT], Reversible[Tuple[_KT, _VT]]):
def __reversed__(self) -> Iterator[Tuple[_KT, _VT]]: ...

class _OrderedDictValuesView(ValuesView[_VT], Reversible[_VT]):
class _OrderedDictValuesView(_dict_values[_VT, _KT], Reversible[_VT], Generic[_KT, _VT]):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the generics here be covariant? the base types are.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense: they are immutable, so a _dict_items[int, str] should also be a valid _dict_items[object, object].

Nit: Please do not force-push or git commit --amend, so that we can see what changed after your previous review. We will squash the commits when merging, so the whole PR will appear as a single commit in the Git history anyway.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

And yes, the generics on _dict_items are redundant. I don't have a strong opinion on whether to keep them. Maybe it's nice to have them explicit because we're doing some unusual shenanigans with its sister class's Generic parameters.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

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

aioredis (https://github.com/aio-libs/aioredis.git)
- aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "KeysView[Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[bytes, float]]")
+ aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "_dict_keys[Any, Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[bytes, float]]")
- aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "KeysView[Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[str, float]]")
+ aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "_dict_keys[Any, Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[str, float]]")
- aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "KeysView[Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[memoryview, float]]")
+ aioredis/client.py:3401: error: Incompatible types in assignment (expression has type "_dict_keys[Any, Any]", variable has type "Union[Sequence[Union[bytes, str, memoryview]], Mapping[memoryview, float]]")
- aioredis/client.py:3403: error: Incompatible types in assignment (expression has type "None", variable has type "ValuesView[Any]")
+ aioredis/client.py:3403: error: Incompatible types in assignment (expression has type "None", variable has type "_dict_values[Any, Any]")

@KotlinIsland
Copy link
Contributor Author

I think this is ready to pull.

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.

None yet

4 participants