-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add informative notes to invariant function arguments #3411
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
Changes from 26 commits
4f6d386
c12fc40
2b92465
05a8ee8
1bf5a93
5a70c57
b0536d5
fa4093d
2888c5f
f8700c3
572160d
900a927
2fee5be
823e1c2
e22c4d4
add8811
c31d6e4
b938394
fbdc381
c2810e3
44a1846
bffd7a4
7d18a99
f17d6f6
35905de
a9a02d4
3599325
de2ede0
bfe20dc
c225c59
739e49f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -533,6 +533,7 @@ def incompatible_argument(self, n: int, m: int, callee: CallableType, arg_type: | |
target = 'to {} '.format(name) | ||
|
||
msg = '' | ||
notes = [] # type: List[str] | ||
if callee.name == '<list>': | ||
name = callee.name[1:-1] | ||
n -= 1 | ||
|
@@ -571,7 +572,12 @@ def incompatible_argument(self, n: int, m: int, callee: CallableType, arg_type: | |
arg_type_str = '**' + arg_type_str | ||
msg = 'Argument {} {}has incompatible type {}; expected {}'.format( | ||
n, target, arg_type_str, expected_type_str) | ||
if isinstance(arg_type, Instance) and isinstance(expected_type, Instance): | ||
notes = append_invariance_notes(notes, arg_type, expected_type) | ||
self.fail(msg, context) | ||
if notes: | ||
for note_msg in notes: | ||
self.note(note_msg, context) | ||
|
||
def invalid_index_type(self, index_type: Type, expected_type: Type, base_str: str, | ||
context: Context) -> None: | ||
|
@@ -992,6 +998,32 @@ def pretty_or(args: List[str]) -> str: | |
return ", ".join(quoted[:-1]) + ", or " + quoted[-1] | ||
|
||
|
||
def append_invariance_notes(notes: List[str], arg_type: Instance, | ||
expected_type: Instance) -> List[str]: | ||
"""Explain that the type is invariant and give notes for how to solve the issue.""" | ||
from mypy.subtypes import is_subtype | ||
invariant_type = '' | ||
covariant_suggestion = '' | ||
if (arg_type.type.fullname() == 'builtins.list' and | ||
expected_type.type.fullname() == 'builtins.list' and | ||
is_subtype(arg_type.args[0], expected_type.args[0])): | ||
invariant_type = 'List' | ||
covariant_suggestion = 'Consider using "Sequence" instead, which is covariant' | ||
elif (arg_type.type.fullname() == 'builtins.dict' and | ||
expected_type.type.fullname() == 'builtins.dict' and | ||
arg_type.args[0].type == expected_type.args[0].type and | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this construction of three lines you can just use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean |
||
is_subtype(arg_type.args[1], expected_type.args[1])): | ||
invariant_type = 'Dict' | ||
covariant_suggestion = ('Consider using "Mapping" instead, ' | ||
'which is covariant in the value type') | ||
if invariant_type and covariant_suggestion: | ||
notes.append( | ||
'"{}" is invariant -- see '.format(invariant_type) + | ||
'http://mypy.readthedocs.io/en/latest/common_issues.html#variance') | ||
notes.append(covariant_suggestion) | ||
return notes | ||
|
||
|
||
def make_inferred_type_note(context: Context, subtype: Type, | ||
supertype: Type, supertype_str: str) -> str: | ||
"""Explain that the user may have forgotten to type a variable. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -684,7 +684,7 @@ g('a')() # E: List[str] not callable | |
# to backtrack later) and defaults to T = <nothing>. The result is an | ||
# awkward error message. Either a better error message, or simply accepting the | ||
# call, would be preferable here. | ||
g(['a']) # E: Argument 1 to "g" has incompatible type List[str]; expected List[<nothing>] | ||
g(['a']) # E: Argument 1 to "g" has incompatible type List[str]; expected List[<nothing>] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the trailing space at the end of this line. |
||
|
||
h(g(['a'])) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -592,3 +592,48 @@ class C: | |
def foo(self) -> None: pass | ||
C().foo() | ||
C().foo(1) # The decorator's return type says this should be okay | ||
|
||
[case testInvariantDictArgNote] | ||
from typing import Dict, Sequence | ||
def f(x: Dict[str, Sequence[int]]) -> None: pass | ||
def g(x: Dict[str, float]) -> None: pass | ||
def h(x: Dict[str, int]) -> None: pass | ||
a = {'a': [1, 2]} | ||
b = {'b': ['c', 'd']} | ||
c = {'c': 1.0} | ||
d = {'d': 1} | ||
f(a) # E: Argument 1 to "f" has incompatible type Dict[str, List[int]]; expected Dict[str, Sequence[int]] \ | ||
# N: "Dict" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance \ | ||
# N: Consider using "Mapping" instead, which is covariant in the value type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please format this with indents like this:
here and elsewhere, it is much easier to read. |
||
f(b) # E: Argument 1 to "f" has incompatible type Dict[str, List[str]]; expected Dict[str, Sequence[int]] | ||
g(c) | ||
g(d) # E: Argument 1 to "g" has incompatible type Dict[str, int]; expected Dict[str, float] \ | ||
# N: "Dict" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance \ | ||
# N: Consider using "Mapping" instead, which is covariant in the value type | ||
h(c) # E: Argument 1 to "h" has incompatible type Dict[str, float]; expected Dict[str, int] | ||
h(d) | ||
[builtins fixtures/dict.pyi] | ||
|
||
[case testInvariantListArgNote] | ||
from typing import List, Union | ||
def f(numbers: List[Union[int, float]]) -> None: pass | ||
a = [1, 2] | ||
f(a) # E: Argument 1 to "f" has incompatible type List[int]; expected List[Union[int, float]] \ | ||
# N: "List" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance \ | ||
# N: Consider using "Sequence" instead, which is covariant | ||
x = [1] | ||
y = ['a'] | ||
x = y # E: Incompatible types in assignment (expression has type List[str], variable has type List[int]) | ||
[builtins fixtures/list.pyi] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add tests:
In general, it is a good idea to add tests for all situations that appeared in the discussion of a PR. |
||
|
||
[case testInvariantTypeConfusingNames] | ||
from typing import TypeVar | ||
class Listener: pass | ||
class DictReader: pass | ||
def f(x: Listener) -> None: pass | ||
def g(y: DictReader) -> None: pass | ||
a = [1, 2] | ||
b = {'b': 1} | ||
f(a) # E: Argument 1 to "f" has incompatible type List[int]; expected Listener | ||
g(b) # E: Argument 1 to "f" has incompatible type Dict[str, int]; expected DictReader | ||
[builtins fixtures/dict.pyi] |
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 will probably fail self-check, since
Type
(annotation forarg_type
) has no attributetype
. UseInstance
annotation andif isinstance(..., Instance )
at call site.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.
Was just finding this out for myself, thanks.