-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Fix handling of non-method callable attribute #3227
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 5 commits
5800819
a67b78c
a523b1f
78d0887
3c8e85f
42020fb
7aacb4f
6703816
d7a41f9
1c58d49
8c4a8ce
499d596
71faee9
2108a03
a8a0386
bd1f0f3
733fd8e
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 |
---|---|---|
|
@@ -782,8 +782,9 @@ def check_reverse_op_method(self, defn: FuncItem, typ: CallableType, | |
# TODO check self argument kind | ||
|
||
# Check for the issue described above. | ||
arg_type = typ.arg_types[1] | ||
other_method = nodes.normal_from_reverse_op[method] | ||
arg_type = typ.arg_types[1] | ||
arg_types = [arg_type] | ||
if isinstance(arg_type, Instance): | ||
if not arg_type.type.has_readable_member(other_method): | ||
return | ||
|
@@ -792,15 +793,17 @@ def check_reverse_op_method(self, defn: FuncItem, typ: CallableType, | |
elif isinstance(arg_type, UnionType): | ||
if not arg_type.has_readable_member(other_method): | ||
return | ||
arg_types = list(arg_type.iter_deep()) | ||
else: | ||
return | ||
|
||
typ2 = self.expr_checker.analyze_external_member_access( | ||
other_method, arg_type, defn) | ||
self.check_overlapping_op_methods( | ||
typ, method, defn.info, | ||
typ2, other_method, cast(Instance, arg_type), | ||
defn) | ||
# We check that each method is fine when dispatched on a proper self argument | ||
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. FYI you can make your local commit history more relevant by using 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 understand I should put more info in the commit messages, but this comment is needed regardless. Or are you referring to the code below it? 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. Neither. I meant that I would prefer to see a single commit pushed to the PR that included the previous commit plus this one (which just added a comment that you forgot on the previous commit). You can do that locally by using 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 comment is unclear. Can you improve it? |
||
for arg_type in arg_types: | ||
typ2 = self.expr_checker.analyze_external_member_access( | ||
other_method, arg_type, defn) | ||
self.check_overlapping_op_methods( | ||
typ, method, defn.info, | ||
typ2, other_method, cast(Instance, arg_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. The cast is safe (though the unsafe is not introduced by this PR). Is there an easy way to fix it? If not, can you add a TODO comment explaining that it's unsafe? 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'm not sure how to fix it. Do you think replacing it with an assertion is appropriate, or should we have "get_instance_from(Type)"? |
||
defn) | ||
|
||
def check_overlapping_op_methods(self, | ||
reverse_type: CallableType, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -286,7 +286,7 @@ def analyze_var(name: str, var: Var, itype: Instance, info: TypeInfo, node: Cont | |
# methods: the former to the instance, the latter to the | ||
# class. | ||
functype = t | ||
check_method_type(functype, itype, var.is_classmethod, node, msg) | ||
check_method_type(functype, original_type, var.is_classmethod, node, msg) | ||
signature = bind_self(functype, original_type) | ||
if var.is_property: | ||
# A property cannot have an overloaded type => the cast | ||
|
@@ -338,33 +338,23 @@ def lookup_member_var_or_accessor(info: TypeInfo, name: str, | |
return None | ||
|
||
|
||
def check_method_type(functype: FunctionLike, itype: Instance, is_classmethod: bool, | ||
def check_method_type(functype: FunctionLike, original_type: Type, is_classmethod: bool, | ||
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 docstring that at least explains the purpose of this method and the Also, can you explain why you this change helps? 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. The change is explained by point (2) above:
|
||
context: Context, msg: MessageBuilder) -> None: | ||
for item in functype.items(): | ||
if not item.arg_types or item.arg_kinds[0] not in (ARG_POS, ARG_STAR): | ||
# No positional first (self) argument (*args is okay). | ||
msg.invalid_method_type(item, context) | ||
elif not is_classmethod: | ||
# Check that self argument has type 'Any' or valid instance type. | ||
selfarg = item.arg_types[0] | ||
# If this is a method of a tuple class, correct for the fact that | ||
# we passed to typ.fallback in analyze_member_access. See #1432. | ||
if isinstance(selfarg, TupleType): | ||
selfarg = selfarg.fallback | ||
if not subtypes.is_subtype(selfarg, itype): | ||
msg.invalid_method_type(item, context) | ||
msg.fail('Attribute function of type %s does not accept self argument' | ||
% msg.format(item), context) | ||
else: | ||
# Check that cls argument has type 'Any' or valid class type. | ||
# Check that self argument has type 'Any' or valid instance/class type. | ||
selfarg = item.arg_types[0] | ||
# (This is sufficient for the current treatment of @classmethod, | ||
# but probably needs to be revisited when we implement Type[C] | ||
# or advanced variants of it like Type[<args>, C].) | ||
clsarg = item.arg_types[0] | ||
if isinstance(clsarg, CallableType) and clsarg.is_type_obj(): | ||
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. Why are these lines no longer needed? Is it because of the changes from 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. It's because classmethod is handled differently, falling from the instance to the type (instead of the other way around, bullet (4) above). I think the comment is not needed too. Actually, I think the entire function can be merged with |
||
if not subtypes.is_equivalent(clsarg.ret_type, itype): | ||
msg.invalid_class_method_type(item, context) | ||
else: | ||
if not subtypes.is_equivalent(clsarg, AnyType()): | ||
msg.invalid_class_method_type(item, context) | ||
if is_classmethod: | ||
original_type = TypeType(original_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 isn't safe, since |
||
if not subtypes.is_subtype(original_type, erase_to_bound(selfarg)): | ||
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 line needs a comment. |
||
msg.invalid_method_type(original_type, item, is_classmethod, context) | ||
|
||
|
||
def analyze_class_attribute_access(itype: Instance, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
from collections import OrderedDict | ||
from typing import ( | ||
Any, TypeVar, Dict, List, Tuple, cast, Generic, Set, Sequence, Optional, Union, Iterable, | ||
NamedTuple, Callable, | ||
NamedTuple, Callable, Iterator, | ||
) | ||
|
||
import mypy.nodes | ||
|
@@ -1062,6 +1062,13 @@ def has_readable_member(self, name: str) -> bool: | |
(isinstance(x, Instance) and x.type.has_readable_member(name)) | ||
for x in self.items) | ||
|
||
def iter_deep(self) -> Iterator[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. 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. Also currently unions are flattened on creation in |
||
for x in self.items: | ||
if isinstance(x, UnionType): | ||
yield from x.iter_deep() | ||
else: | ||
yield x | ||
|
||
def serialize(self) -> JsonDict: | ||
return {'.class': 'UnionType', | ||
'items': [t.serialize() for t in self.items], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1844,7 +1844,7 @@ class B: | |
a = A | ||
bad = lambda: 42 | ||
|
||
B().bad() # E: Invalid method type | ||
B().bad() # E: Attribute function of type Callable[[], int] does not accept self argument | ||
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'd tweak message to be |
||
reveal_type(B.a) # E: Revealed type is 'def () -> __main__.A' | ||
reveal_type(B().a) # E: Revealed type is 'def () -> __main__.A' | ||
reveal_type(B().a()) # E: Revealed type is '__main__.A' | ||
|
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.
Use
union_items()
instead ofiter_deep()
.