Add type inference for `dict.keys` membership #13372

matthewhughes934 · 2022-08-09T19:24:48Z

Add type inference for dict.keys membership

Also for containership checks on typing.KeysView. This is to bring the
following cases into alignment:
```
  from typing import KeysView

  key: str | None
  d: dict[str, int]
  kv: KeysView[str]

  if key in d:
      # type of 'key' is inferred to be 'str'
      ...

  if key in d.keys():
      # type of 'key' should be inferred to be 'str'
      ...

  if key in kv:
      # type of 'key' should be inferred to be 'str'
      ...
```
Before this change only the first if would narrow the type of key.

I've just added a test under test-data/unit/pythoneval.test as
test-data/unit/fixtures/dict.pyi doesn't include dict.keys, and
adding it requires importing dict_keys from _collections_abc in
those stubs, which then requires adding _collections_abc.pyi stubs,
which would have to be complete since e.g.
testGenericAliasCollectionsABCReveal expects most of the types in
those stubs to be defined (this is the same approach as
7678f28).

GH: issue No type narrowing for in on dict.keys() #13360

Test Plan

Test added under test-data/unit/check-isinstance.test

matthewhughes934 · 2022-08-09T19:25:50Z

test-data/unit/fixtures/dict.pyi

@@ -30,6 +31,7 @@ class dict(Mapping[KT, VT]):
    @overload
    def get(self, k: KT, default: Union[KT, T]) -> Union[VT, T]: pass
    def __len__(self) -> int: ...
+    def keys(self) -> dict_keys[KT, VT]: ...


This was just the easiest way to get things working, happy to hear alternatives if there's some other approach I'm not aware of

matthewhughes934 · 2022-08-09T19:26:12Z

test-data/unit/check-isinstance.test

@@ -2065,6 +2065,18 @@ def f() -> None:
 [builtins fixtures/dict.pyi]
 [out]

+[case testNarrowTypeAfterInDictKeys]


I just added this here because there was a similar test just above for TypedDicts

sobolevn · 2022-08-09T20:46:32Z

test-data/unit/check-isinstance.test

+def f() -> None:
+   d: Dict[str, str] = {}
+   key: Optional[str]
+   if key not in d.keys():


Let's change this test a bit: we need to make sure that if key in d.keys() narrows the type correctly.

Placing reveal_type inside if is the easiest way to do that.

Testing not in is appreciated as well!

Let's change this test a bit: we need to make sure that if key in d.keys() narrows the type correctly.

Placing reveal_type inside if is the easiest way to do that.
Testing not in is appreciated as well!

Updated b5050c0

sobolevn · 2022-08-09T20:48:02Z

mypy/checker.py

@@ -6285,6 +6285,7 @@ def builtin_item_type(tp: Type) -> Optional[Type]:
            "builtins.dict",
            "builtins.set",
            "builtins.frozenset",
+            "_collections_abc.dict_keys",


What about typing.KeysView? https://github.com/python/cpython/blob/70fc9641b56144854777aef29c145cd10789e3df/Lib/typing.py#L2701

And collections.abc.KeysView?
What about ValuesView and ItemsView?

And dict_values as well as dict_items 🙂

What about typing.KeysView?

Sounds good. I wasn't sure exactly what would be best to be included in this changeset. Let me drag all of those in too

What about typing.KeysView? https://github.com/python/cpython/blob/70fc9641b56144854777aef29c145cd10789e3df/Lib/typing.py#L2701

This is included with 75ca813

matthewhughes934 · 2022-08-10T20:38:05Z

test-data/unit/fixtures/dict.pyi

@@ -1,5 +1,6 @@
 # Builtins stub used in dictionary-related test cases.

+from _collections_abc import dict_keys


This line looks to be the cause of I think all of the test failures, e.g.

$ pytest -q mypy/test/testcheck.py::TypeCheckSuite::check-incremental.test::testIncrementalWithDifferentKindsOfNestedTypesWithinMethod F [100%] ========================================================================== FAILURES =========================================================================== _________________________________________________ testIncrementalWithDifferentKindsOfNestedTypesWithinMethod __________________________________________________ data: /home/mjh/src/mypy/test-data/unit/check-incremental.test:5662: /home/mjh/src/mypy/mypy/test/testcheck.py:80: in run_case self.run_case_once(testcase, ops, step) /home/mjh/src/mypy/mypy/test/testcheck.py:174: in run_case_once assert_string_arrays_equal(output, a, msg.format(testcase.file, testcase.line)) E AssertionError: Unexpected type checker output in incremental, run 2 (/home/mjh/src/mypy/test-data/unit/check-incremental.test, line 5662) -------------------------------------------------------------------- Captured stderr call --------------------------------------------------------------------- Expected: tmp/a.py:2: error: "object" has no attribute "xyz" (diff) Actual: tmp/a.py:2: error: Module has no attribute "xyz" (diff) Alignment of first line difference: E: tmp/a.py:2: error: "object" has no attribute "xyz" A: tmp/a.py:2: error: Module has no attribute "xyz" ^ =================================================================== short test summary info =================================================================== FAILED mypy/test/testcheck.py::TypeCheckSuite::check-incremental.test::testIncrementalWithDifferentKindsOfNestedTypesWithinMethod 1 failed in 0.33s

I'm going to have a look around, but would be happy to hear any suggestions 🙃

Presumably the stubs used for these tests don't include stubs for _collections_abc. As I recall there are stubs for various important standard library modules somewhere in the fixtures directory.

This may be difficult to get right as we have had some instability around the type of dict.keys() before; what you're using here (_collections_abc.dict_keys) is arguably a private implementation detail.

This may be difficult to get right as we have had some instability around the type of dict.keys() before; what you're using here (_collections_abc.dict_keys) is arguably a private implementation detail.

👍 I'm also not 100% on the approach, so happy to hear any alternatives you might have. So far I've just been following typeshed https://github.com/python/typeshed/blob/a92da58328aa259184ff45bb7408e82426fb563e/stdlib/builtins.pyi#L4, https://github.com/python/typeshed/blob/a92da58328aa259184ff45bb7408e82426fb563e/stdlib/builtins.pyi#L1039

matthewhughes934 · 2022-08-13T16:54:28Z

With 5fd5a93 I've moved the tests under test-data/unit/pythoneval.test just to verify they would work under a full set of stubs.

I've (very roughly) documented some of the issues I ran into while trying to add stubs for these into the test libraries, but I think I'd need to add some complete (at least sufficient to satisfy the tests in mypy/test/testcheck.py::TypeCheckSuite::check-generic-alias.test::testGenericAliasCollectionsABCReveal) stubs for _collections_abc in order to have everything under the test stubs.

I assume having test stubs for dict.keys would be the preference? Happy to give it a shot but I might need to be pointed in the right direction 🙂

EDIT: aaand I forgot to commit some changes, see 68b2881

matthewhughes934 · 2022-08-16T17:53:01Z

01efd35 contains the work for _collections_abc.dict_keys and typing.KeysView.

For dict.items I think I'll need to look a bit deeper, since collections.ItemsView inherits AbstractSet[tuple[_KT_co, _VT_co]] but I don't think mypy knows how to handle narrowing when tuple unpacking? Running mypy over the following on `master:

from typing import Dict, Optional, Set, Tuple

d: Dict[str, int]
s: Set[Tuple[str, int]]

v: Optional[int]
k: Optional[str]
p: Optional[Tuple[str, int]]

if (k, v) in d.items():
    reveal_type(k)
    reveal_type(v)

if (k, v) in s:
    reveal_type(k)
    reveal_type(v)

if p in s:
    reveal_type(p)

gives:

script.py:11: note: Revealed type is "Union[builtins.str, None]"
script.py:12: note: Revealed type is "Union[builtins.int, None]"
script.py:15: note: Revealed type is "Union[builtins.str, None]"
script.py:16: note: Revealed type is "Union[builtins.int, None]"
script.py:19: note: Revealed type is "Tuple[builtins.str, builtins.int]"

Where I'd expect none of those to be optional.

Also for containership checks on `typing.KeysView.` This is to bring the following cases into alignment: from typing import KeysView key: str | None d: dict[str, int] kv: KeysView[str] if key in d: # type of 'key' is inferred to be 'str' ... if key in d.keys(): # type of 'key' should be inferred to be 'str' ... if key in kv: # type of 'key' should be inferred to be 'str' ... Before this change only the first `if` would narrow the type of `key`. I've just added a test under `test-data/unit/pythoneval.test` as `test-data/unit/fixtures/dict.pyi` doesn't include `dict.keys`, and adding it requires importing `dict_keys` from `_collections_abc` in those stubs, which then requires adding `_collections_abc.pyi` stubs, which would have to be complete since e.g. `testGenericAliasCollectionsABCReveal` expects most of the types in those stubs to be defined (this is the same approach as 7678f28). GH: issue python#13360

matthewhughes934 · 2022-08-23T16:06:59Z

@sobolevn any objections to keeping this changeset just for dict.keys? I'm thinking it might be worth having a separate discussion for .items and .values (see my comment above)

JelleZijlstra · 2022-08-23T16:29:20Z

I'd be OK with supporting only .keys(); narrowing on .items() and .values() doesn't make nearly as much sense to me. I don't see any mention of those in the issue either.

sobolevn · 2022-08-23T16:58:49Z

+1 to @JelleZijlstra 🙂

github-actions · 2022-08-23T17:10:25Z

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

spark (https://github.com/apache/spark)
+ python/pyspark/sql/types.py:401: error: Redundant cast to "int"  [redundant-cast]
+ python/pyspark/sql/types.py:402: error: Redundant cast to "int"  [redundant-cast]

hauntsaninja

Thank you!

matthewhughes934 commented Aug 9, 2022

View reviewed changes

sobolevn reviewed Aug 9, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

matthewhughes934 commented Aug 10, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

matthewhughes934 force-pushed the add-type-inference-from-dict-keys branch from 5fd5a93 to 68b2881 Compare August 13, 2022 21:22

This comment has been minimized.

Sign in to view

matthewhughes934 force-pushed the add-type-inference-from-dict-keys branch from 68b2881 to 01efd35 Compare August 16, 2022 17:44

This comment has been minimized.

Sign in to view

matthewhughes934 force-pushed the add-type-inference-from-dict-keys branch from 01efd35 to 75ca813 Compare August 23, 2022 16:05

matthewhughes934 requested review from JelleZijlstra and sobolevn and removed request for JelleZijlstra and sobolevn August 24, 2022 19:46

sobolevn approved these changes Aug 24, 2022

View reviewed changes

JelleZijlstra approved these changes Sep 5, 2022

View reviewed changes

hauntsaninja approved these changes Oct 27, 2022

View reviewed changes

hauntsaninja merged commit 507fc5c into python:master Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type inference for `dict.keys` membership #13372

Add type inference for `dict.keys` membership #13372

matthewhughes934 commented Aug 9, 2022 •

edited

Loading

matthewhughes934 Aug 9, 2022

matthewhughes934 Aug 9, 2022

sobolevn Aug 9, 2022

sobolevn Aug 9, 2022

matthewhughes934 Aug 9, 2022

sobolevn Aug 9, 2022

sobolevn Aug 9, 2022

matthewhughes934 Aug 9, 2022

matthewhughes934 Aug 24, 2022

This comment has been minimized.

This comment has been minimized.

matthewhughes934 Aug 10, 2022

JelleZijlstra Aug 10, 2022 •

edited

Loading

matthewhughes934 Aug 11, 2022 •

edited

Loading

matthewhughes934 commented Aug 13, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

matthewhughes934 commented Aug 16, 2022 •

edited

Loading

This comment has been minimized.

matthewhughes934 commented Aug 23, 2022

JelleZijlstra commented Aug 23, 2022

sobolevn commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

hauntsaninja left a comment

		@@ -1,5 +1,6 @@
		# Builtins stub used in dictionary-related test cases.

		from _collections_abc import dict_keys

Add type inference for dict.keys membership #13372

Add type inference for dict.keys membership #13372

Conversation

matthewhughes934 commented Aug 9, 2022 • edited Loading

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

JelleZijlstra Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

matthewhughes934 Aug 11, 2022 • edited Loading

Choose a reason for hiding this comment

matthewhughes934 commented Aug 13, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

matthewhughes934 commented Aug 16, 2022 • edited Loading

This comment has been minimized.

matthewhughes934 commented Aug 23, 2022

JelleZijlstra commented Aug 23, 2022

sobolevn commented Aug 23, 2022

github-actions bot commented Aug 23, 2022

hauntsaninja left a comment

Choose a reason for hiding this comment

Add type inference for `dict.keys` membership #13372

Add type inference for `dict.keys` membership #13372

matthewhughes934 commented Aug 9, 2022 •

edited

Loading

JelleZijlstra Aug 10, 2022 •

edited

Loading

matthewhughes934 Aug 11, 2022 •

edited

Loading

matthewhughes934 commented Aug 13, 2022 •

edited

Loading

matthewhughes934 commented Aug 16, 2022 •

edited

Loading