-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 type inference for dict.keys
membership
#13372
Add type inference for dict.keys
membership
#13372
Conversation
test-data/unit/fixtures/dict.pyi
Outdated
@@ -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]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
test-data/unit/check-isinstance.test
Outdated
@@ -2065,6 +2065,18 @@ def f() -> None: | |||
[builtins fixtures/dict.pyi] | |||
[out] | |||
|
|||
[case testNarrowTypeAfterInDictKeys] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added this here because there was a similar test just above for TypedDict
s
test-data/unit/check-isinstance.test
Outdated
def f() -> None: | ||
d: Dict[str, str] = {} | ||
key: Optional[str] | ||
if key not in d.keys(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing not in
is appreciated as well!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
insideif
is the easiest way to do that.
Testing not in is appreciated as well!
Updated b5050c0
@@ -6285,6 +6285,7 @@ def builtin_item_type(tp: Type) -> Optional[Type]: | |||
"builtins.dict", | |||
"builtins.set", | |||
"builtins.frozenset", | |||
"_collections_abc.dict_keys", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about typing.KeysView
? https://github.com/python/cpython/blob/70fc9641b56144854777aef29c145cd10789e3df/Lib/typing.py#L2701
And collections.abc.KeysView
?
What about ValuesView
and ItemsView
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And dict_values
as well as dict_items
🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
typing.KeysView
? https://github.com/python/cpython/blob/70fc9641b56144854777aef29c145cd10789e3df/Lib/typing.py#L2701
This is included with 75ca813
This comment has been minimized.
This comment has been minimized.
1 similar comment
This comment has been minimized.
This comment has been minimized.
test-data/unit/fixtures/dict.pyi
Outdated
@@ -1,5 +1,6 @@ | |||
# Builtins stub used in dictionary-related test cases. | |||
|
|||
from _collections_abc import dict_keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
With 5fd5a93 I've moved the tests under 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 I assume having test stubs for EDIT: aaand I forgot to commit some changes, see 68b2881 |
This comment has been minimized.
This comment has been minimized.
5fd5a93
to
68b2881
Compare
This comment has been minimized.
This comment has been minimized.
68b2881
to
01efd35
Compare
01efd35 contains the work for For 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:
Where I'd expect none of those to be optional. |
This comment has been minimized.
This comment has been minimized.
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
01efd35
to
75ca813
Compare
@sobolevn any objections to keeping this changeset just for |
I'd be OK with supporting only |
+1 to @JelleZijlstra 🙂 |
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]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Add type inference for
dict.keys
membershipAlso for containership checks on
typing.KeysView.
This is to bring thefollowing cases into alignment:
Before this change only the first
if
would narrow the type ofkey
.I've just added a test under
test-data/unit/pythoneval.test
astest-data/unit/fixtures/dict.pyi
doesn't includedict.keys
, andadding it requires importing
dict_keys
from_collections_abc
inthose stubs, which then requires adding
_collections_abc.pyi
stubs,which would have to be complete since e.g.
testGenericAliasCollectionsABCReveal
expects most of the types inthose stubs to be defined (this is the same approach as
7678f28).
GH: issue No type narrowing for
in
on dict.keys() #13360Test Plan
Test added under
test-data/unit/check-isinstance.test