-
-
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 19 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,11 @@ 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) | ||
notes = 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 +997,28 @@ def pretty_or(args: List[str]) -> str: | |
return ", ".join(quoted[:-1]) + ", or " + quoted[-1] | ||
|
||
|
||
def invariance_notes(notes: List[str], arg_type: Type, expected_type: Type) -> List[str]: | ||
"""Explain that the type is invariant and give notes for how to solve the issue.""" | ||
if isinstance(arg_type, Instance) and isinstance(expected_type, Instance): | ||
if (arg_type.type.fullname() == 'builtins.list' and | ||
expected_type.type.fullname() == 'builtins.list'): | ||
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 | ||
arg_type.args[1].type != expected_type.args[1].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. There is something important here missing (also for x: List[int]
y: List[str]
x = y we don't need to show any notes, since using |
||
invariant_type = 'Dict' | ||
covariant_suggestion = ('Consider using "Mapping" instead, ' | ||
'which is covariant in the value') | ||
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. "... covariant in the value type", if don't agree with a previous comment #3411 (comment), then please explain, just ignoring it is not a good option, I will not forget :-) 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 just missed it, thanks for catching. 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 am also not convinced that this message is clear to a novice user (which I expect is the intended audience for this type of note), but I can't think of a better concise message. I guess this assumes that the link is the most helpful part. 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. ...at least value type suggests this is something related to types, not with the value itself (note that this error might appear in situations where list/dict literals are involved). |
||
if invariant_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. This will crash, since |
||
notes.append( | ||
'"{}" is invariant --- see '.format(invariant_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. also I proposed two dashes here instead of three. |
||
'http://mypy.readthedocs.io/en/latest/common_issues.html#variance') | ||
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. DRY, this should be VARIANCE_LINK = '"{}" is blah-blah...'
...
notes.append(VARIANCE_LINK.format('List'))
...
notes.append(VARIANCE_LINK.format('Dict')) Also please use two dashes |
||
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 |
---|---|---|
|
@@ -592,3 +592,23 @@ class C: | |
def foo(self) -> None: pass | ||
C().foo() | ||
C().foo(1) # The decorator's return type says this should be okay | ||
|
||
|
||
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. One empty line between tests is enough (also below). |
||
[case testInvariantDictArgNote] | ||
from typing import Dict, Sequence | ||
def f(a: Dict[str, Sequence[int]]) -> None: pass | ||
a = {'a': [1, 2]} | ||
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 | ||
[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 | ||
[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. |
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 proposed to place this at the call site of this function not at the definition site, to avoid unnecessary calls.
(Also this will save you 4 spaces of indent.)