Repository: python/mypy
Pull request: Fix checking of self when accessing a non-method callable attribute (take 2)
Fix #3223. Minimal examples:
```python
from typing import Callable
class X:
    f: Callable[[object], None]
X().f  # E: Invalid method type
```
```python
class X:
    @property
    def g(self: object): pass
X().g  # E: Invalid method type
```
The problem is that for non-methods the check performed on access to `self` is whether `object <: X` instead of the other way around.

1. Reverse subtyping check. This change alone fixes the issue described above.
2. For classmethod, fallback on the argument instead of on the parameter
3. Mimic dispatch better: meet `original_type` with static class. This handles the case were instead of `X()` above there was something of type `Union[X, Y]`.
4. Better error message

Take 2 of #3227, but without the reverse-operator part

Problem statement:
self type on property creates a spurious error
Running mypy master on this file:
```python
from typing import TypeVar, List
_T = TypeVar('_T')

class X:
    @property
    def __members__(self: _T) -> List[_T]: ...

reveal_type(X().__members__)
```
gives
```
../bin/selftypeproperty.py:8: error: Revealed type is 'builtins.list[selftypeproperty.X*]'
../bin/selftypeproperty.py:8: error: Invalid method type
```
The revealed type is correct but the "Invalid method type" doesn't make sense.

Hints from issue discussion:
Is it the same if you rename `__members__` to `foo`?
Yes, doesn't make a difference.
I should look into it.

Note that adding a bound, i.e. `_T = TypeVar('_T', bound='X')` seems to fix this. 
Thanks! FYI, the code where I found this had a `Type` over a bound typevar (python/typeshed#1195). It has `_T = TypeVar('_T', bound=Enum)` and the property is `def __members__(self: Type[_T]) -> Mapping[str, _T]: ...`.

Commit message: Fix handling of non-method callable attribute (#3223)

1. Reverse subtyping check for self argument
2. For classmethod, fallback on the argument instead of on the parameter
3. Mimic dispatch better: meet original_type with static class
4. Better error message

Patch to review:
diff --git a/mypy/checkmember.py b/mypy/checkmember.py
index ea8aff82d209..8dcf272f375b 100644
--- a/mypy/checkmember.py
+++ b/mypy/checkmember.py
@@ -20,6 +20,8 @@
 from mypy.plugin import Plugin, AttributeContext
 from mypy import messages
 from mypy import subtypes
+from mypy import meet
+
 MYPY = False
 if MYPY:  # import for forward declaration only
     import mypy.checker
@@ -314,7 +316,9 @@ 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)
+                # Use meet to simulate dispatch - e.g. reduce Union[A, B] to A on dispatch to A
+                dispatched_type = meet.meet_types(original_type, itype)
+                check_self_arg(functype, dispatched_type, var.is_classmethod, node, name, msg)
                 signature = bind_self(functype, original_type, var.is_classmethod)
                 if var.is_property:
                     # A property cannot have an overloaded type => the cast
@@ -370,33 +374,22 @@ def lookup_member_var_or_accessor(info: TypeInfo, name: str,
         return None
 
 
-def check_method_type(functype: FunctionLike, itype: Instance, is_classmethod: bool,
-                      context: Context, msg: MessageBuilder) -> None:
+def check_self_arg(functype: FunctionLike, original_type: Type, is_classmethod: bool,
+                   context: Context, name: str, msg: MessageBuilder) -> None:
+    """Check that the the most precise type of the self argument is compatible
+    with the declared type of each of the overloads.
+    """
     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 with type %s does not accept self argument'
+                     % msg.format(item), context)
         else:
-            # Check that cls argument has type 'Any' or valid class type.
-            # (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():
-                if not subtypes.is_equivalent(clsarg.ret_type, itype):
-                    msg.invalid_class_method_type(item, context)
-            else:
-                if not subtypes.is_equivalent(clsarg, AnyType(TypeOfAny.special_form)):
-                    msg.invalid_class_method_type(item, context)
+            selfarg = item.arg_types[0]
+            if is_classmethod:
+                original_type = TypeType.make_normalized(original_type)
+            if not subtypes.is_subtype(original_type, erase_to_bound(selfarg)):
+                msg.invalid_method_type(name, original_type, item, is_classmethod, context)
 
 
 def analyze_class_attribute_access(itype: Instance,
diff --git a/mypy/messages.py b/mypy/messages.py
index 2f3473b3e59d..1f25f1990d2a 100644
--- a/mypy/messages.py
+++ b/mypy/messages.py
@@ -856,11 +856,11 @@ def cannot_determine_type(self, name: str, context: Context) -> None:
     def cannot_determine_type_in_base(self, name: str, base: str, context: Context) -> None:
         self.fail("Cannot determine type of '%s' in base class '%s'" % (name, base), context)
 
-    def invalid_method_type(self, sig: CallableType, context: Context) -> None:
-        self.fail('Invalid method type', context)
-
-    def invalid_class_method_type(self, sig: CallableType, context: Context) -> None:
-        self.fail('Invalid class method type', context)
+    def invalid_method_type(self, name: str, arg: Type, sig: CallableType, is_classmethod: bool,
+                            context: Context) -> None:
+        kind = 'class attribute function' if is_classmethod else 'attribute function'
+        self.fail('Invalid self argument %s to %s "%s" with type %s'
+                  % (self.format(arg), kind, name, self.format(sig)), context)
 
     def incompatible_conditional_function_def(self, defn: FuncDef) -> None:
         self.fail('All conditional function variants must have identical '
diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test
index 2ff0b8d84d78..6f299d5c5647 100644
--- a/test-data/unit/check-classes.test
+++ b/test-data/unit/check-classes.test
@@ -2105,7 +2105,7 @@ class B:
     a = A
     bad = lambda: 42
 
-B().bad() # E: Invalid method type
+B().bad() # E: Attribute function with type "Callable[[], int]" does not accept self argument
 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'
diff --git a/test-data/unit/check-functions.test b/test-data/unit/check-functions.test
index e39606251f9d..f6f3d7a971b6 100644
--- a/test-data/unit/check-functions.test
+++ b/test-data/unit/check-functions.test
@@ -507,8 +507,8 @@ class A:
     f = x # type: Callable[[], None]
     g = x # type: Callable[[B], None]
 a = None # type: A
-a.f() # E: Invalid method type
-a.g() # E: Invalid method type
+a.f() # E: Attribute function with type "Callable[[], None]" does not accept self argument
+a.g() # E: Invalid self argument "A" to attribute function "g" with type "Callable[[B], None]"
 
 [case testMethodWithDynamicallyTypedMethodAsDataAttribute]
 from typing import Any, Callable
@@ -568,7 +568,7 @@ class A(Generic[t]):
 ab = None # type: A[B]
 ac = None # type: A[C]
 ab.f()
-ac.f()   # E: Invalid method type
+ac.f()   # E: Invalid self argument "A[C]" to attribute function "f" with type "Callable[[A[B]], None]"
 
 [case testPartiallyTypedSelfInMethodDataAttribute]
 from typing import Any, TypeVar, Generic, Callable
diff --git a/test-data/unit/check-selftype.test b/test-data/unit/check-selftype.test
index d0c4a56f2038..d32b74261371 100644
--- a/test-data/unit/check-selftype.test
+++ b/test-data/unit/check-selftype.test
@@ -354,16 +354,21 @@ class E:
 [case testSelfTypeProperty]
 from typing import TypeVar
 
-T = TypeVar('T', bound='A')
+Q = TypeVar('Q')
+T = TypeVar('T', bound='X')
 
-class A:
+class X:
     @property
-    def member(self: T) -> T:
-        pass
+    def __members__(self: Q) -> Q: return self
+
+class A(X):
+    @property
+    def member(self: T) -> T: return self
 
 class B(A):
     pass
 
+reveal_type(X().__members__)  # E: Revealed type is '__main__.X*'
 reveal_type(A().member)  # E: Revealed type is '__main__.A*'
 reveal_type(B().member)  # E: Revealed type is '__main__.B*'
 
@@ -376,3 +381,21 @@ class A:
     # def g(self: None) -> None: ... see in check-python2.test
 [out]
 main:3: error: Self argument missing for a non-static method (or an invalid type for self)
+
+[case testUnionPropertyField]
+from typing import Union
+
+class A:
+    x: int
+
+class B:
+    @property
+    def x(self) -> int: return 1
+
+class C:
+    @property
+    def x(self) -> int: return 1
+
+ab: Union[A, B, C]
+reveal_type(ab.x)  # E: Revealed type is 'builtins.int'
+[builtins fixtures/property.pyi]


Code review comments:
diff_hunk: @@ -370,33 +374,22 @@ def lookup_member_var_or_accessor(info: TypeInfo, name: str,
         return None
 
 
-def check_method_type(functype: FunctionLike, itype: Instance, is_classmethod: bool,
-                      context: Context, msg: MessageBuilder) -> None:
+def check_self_arg(functype: FunctionLike, original_type: Type, is_classmethod: bool,
+                   context: Context, name: str, msg: MessageBuilder) -> None:
+    """Check that the the most precise type of the self argument is compatible
+    with the declared type of each of the overloads.
+    """
     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 with type %s does not accept self argument'
path: mypy/checkmember.py
text: @user1:
Add a method to `Messages` instead of building the message here.

diff_hunk: @@ -314,7 +316,9 @@ 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)
+                # Use meet to simulate dispatch - e.g. reduce Union[A, B] to A on dispatch to A
path: mypy/checkmember.py
text: @user1:
The comment is unclear. Can you add a comment with an example where this makes a difference?

@author:
I added a comment and a test. This kind of dispatch with not-exactly-the-right-type occur in reverse operators.

I believe the same thing should also be done with plain functions, but it's not checked at all now. i.e. there should be a call to `check_self_arg` before `bind_self` [here](https://github.com/python/mypy/blob/master/mypy/checkmember.py#L93). For example:
```python
class A:
    def f(self: B) -> None: pass  # type: ignore - because reasons. probably useful for metaclasses

class B(A): pass
A().f()  # should be an error
```
but changing _plain functions_ breaks some typeshed code (specifically, AbstractSet operators and the `version_info` tests break).

diff_hunk: @@ -370,33 +374,22 @@ def lookup_member_var_or_accessor(info: TypeInfo, name: str,
         return None
 
 
-def check_method_type(functype: FunctionLike, itype: Instance, is_classmethod: bool,
-                      context: Context, msg: MessageBuilder) -> None:
+def check_self_arg(functype: FunctionLike, original_type: Type, is_classmethod: bool,
+                   context: Context, name: str, msg: MessageBuilder) -> None:
+    """Check that the the most precise type of the self argument is compatible
path: mypy/checkmember.py
text: @user1:
Style nit: in a multi-line docstring there should be an empty line after the first line.

@author:
I'll fix it on the next commit or before merge.

@user1:
The docstring could also be clarified a bit. The meaning of "the most precise type of the self argument" wasn't clear initially. Maybe something about generic self, and using the actual type of the self (based on the receiver type) rather than the declared type in the function signature? Would also be good to describe `original_type` here -- it would even make sense to rename it.  What about `actual_self_type`? It's always the type of self, and it's the actual instead of the formal/declared type. If you rename it, it would also make sense to rename it elsewhere.

@author:
There are 4 potentially distinct types in play here:
1. The declared type of the first parameter `self` (potentially generic)
2. The inferred/declared type of `x` in `x.f`, ignoring the function signature
3. The same type as (2) but after narrowing it using the current class
4. The type of the current class

(4) is almost irrelevant at this point, but it is used to construct (3), restricts the valid types for (2), and adds to the potential confusion.
So both (2) and (3) are "actual self type". Maybe I should elaborate on the docstring/comment, but it will become lengthy.