Repository: python/mypy
Pull request: Fix incorrect name lookup for decorated methods
Resolves #8161 

According to comments of `lookup` in `mypy/semanal.py`, when we look up a class attribute, we require that it is defined textually before the reference statement, thus line number is used for comparison.

When function has decorators, its line number is determined by the top decorator instead of the `def`. That's why #8161's code fails because on line 8, the `A` in `Type[A]` has the line number of 8 while the @staticmethod function `A` has the line number of 7 due to the decorator. 

Thus we need to properly handle this by introducing the number of decorators when deciding textural precedence.

Problem statement:
mypy incorrectly resolves names inside decorated methods
* Are you reporting a bug, or opening a feature request?

A bug

* Please insert below the code you are checking with mypy,
  or a mock-up repro if the source is private. We would appreciate
  if you try to simplify your case to a minimal repro.
```
from typing import Type

class A:
    pass

class B:
    @staticmethod
    def A() -> Type[A]:
        return A

assert A is B.A()
```

* What is the actual behavior/output?
```
$ mypy mypy_fail.py
mypy_fail.py:8: error: Function "mypy_fail.B.A" is not valid as a type
mypy_fail.py:8: note: Perhaps you need "Callable[...]" or a callback protocol?
Found 1 error in 1 file (checked 1 source file)
```

* What is the behavior/output you expect?

I expect this to be accepted - it works fine in python

Probably of interest: regular methods work like I expect. `classmethod` and `property` complain the same way, so I assume this has to do with decorators. For functions, I can't reproduce syntax like this that's also valid python (I get `UnboundLocalError`s), so that's as far as I got with debugging.

* What are the versions of mypy and Python you are using?
  Do you see the same issue after installing mypy from Git master?

mypy `0.750` and python 3.6/3.8. I also get it on git master.

* What are the mypy flags you are using? (For example --strict-optional)

No flags required

Hints from issue discussion:
This is indeed a bug. You can use a type alias as workaround:
```python
_A = A
class B:
    @staticmethod
    def A() -> Type[_A]:
        return A
```
There's another possible workaround:
```python
from typing import Type

class C:
    pass

class B:
    @staticmethod
    def A() -> Type[C]:
        return C
```
which I confirm that at line 8, `lookup_qualified` will correctly lookup the node with fullname of `__main__.C`. However, with the failed code, the lookup result will be `__main__.B.A`, which caused the error. So I think we need to modify `lookup_qualified` to fix this. @ilevkivskyi

Commit message: fix overload methods lookup

Patch to review:
diff --git a/mypy/semanal.py b/mypy/semanal.py
index 488deb80e21b..ffd967127b14 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -3936,11 +3936,26 @@ class C:
         #       caught.
         assert self.statement  # we are at class scope
         return (node is None
-                or node.line < self.statement.line
+                or self.is_textually_before_statement(node)
                 or not self.is_defined_in_current_module(node.fullname)
                 or isinstance(node, TypeInfo)
                 or (isinstance(node, PlaceholderNode) and node.becomes_typeinfo))
 
+    def is_textually_before_statement(self, node: SymbolNode) -> bool:
+        """Check if a node is defined textually before the current statement
+
+        Note that decorated functions' line number are the same as
+        the top decorator.
+        """
+        assert self.statement
+        line_diff = self.statement.line - node.line
+        if isinstance(node, OverloadedFuncDef) and isinstance(self.statement, FuncDef):
+            return node.name != self.statement.name and line_diff > 0
+        elif isinstance(node, Decorator) and not node.is_overload:
+            return line_diff > len(node.original_decorators)
+        else:
+            return line_diff > 0
+
     def is_defined_in_current_module(self, fullname: Optional[str]) -> bool:
         if fullname is None:
             return False
diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test
index 6681d30df76a..3068b865fba0 100644
--- a/test-data/unit/check-classes.test
+++ b/test-data/unit/check-classes.test
@@ -6596,3 +6596,45 @@ reveal_type(D() + "str")  # N: Revealed type is 'Any'
 
 reveal_type(0.5 + D1())  # N: Revealed type is 'Any'
 reveal_type(D1() + 0.5)  # N: Revealed type is '__main__.D1'
+
+[case testRefMethodWithDecorator]
+from typing import Type
+
+class A:
+    pass
+
+class B:
+    @staticmethod
+    def A() -> Type[A]:
+        return A
+    @staticmethod
+    def B() -> Type[A]:  # E: Function "__main__.B.A" is not valid as a type \
+                         # N: Perhaps you need "Callable[...]" or a callback protocol?
+        return A
+
+class C:
+    @property
+    @staticmethod
+    def A() -> Type[A]:
+        return A
+
+[builtins fixtures/staticmethod.pyi]
+
+[case testRefMethodWithOverloadDecorator]
+from typing import Type, overload
+
+class A:
+    pass
+
+class B:
+    @classmethod
+    @overload
+    def A(cls, x: int) -> Type[A]: ...
+    @classmethod
+    @overload
+    def A(cls, x: str) -> Type[A]: ...
+    @classmethod
+    def A(cls, x: object) -> Type[A]:
+        return A
+
+[builtins fixtures/classmethod.pyi]
diff --git a/test-data/unit/fixtures/staticmethod.pyi b/test-data/unit/fixtures/staticmethod.pyi
index 14254e64dcb1..7d5d98634e48 100644
--- a/test-data/unit/fixtures/staticmethod.pyi
+++ b/test-data/unit/fixtures/staticmethod.pyi
@@ -9,6 +9,7 @@ class type:
 class function: pass
 
 staticmethod = object() # Dummy definition.
+property = object()  # Dummy definition
 
 class int:
     @staticmethod


Code review comments:
diff_hunk: @@ -6596,3 +6596,45 @@ reveal_type(D() + "str")  # N: Revealed type is 'Any'
 
 reveal_type(0.5 + D1())  # N: Revealed type is 'Any'
 reveal_type(D1() + 0.5)  # N: Revealed type is '__main__.D1'
+
+[case testRefMethodWithDecorator]
+from typing import Type
+
+class A:
+    pass
+
+class B:
+    @staticmethod
+    def A() -> Type[A]:
+        return A
path: test-data/unit/check-classes.test
text: @user1:
Please put `return` on the same line as the `def` (or replace with `...` altogether, this doesn't really test anything). I had it like this in my example for a reason, I wanted to be sure there is no off-by-one error.

@author:
Should I replace all the `return`s?

@user1:
Other returns are up to you, their positions are not really relevant

diff_hunk: @@ -3936,11 +3936,26 @@ class C:
         #       caught.
         assert self.statement  # we are at class scope
         return (node is None
-                or node.line < self.statement.line
+                or self.is_textually_before_statement(node)
                 or not self.is_defined_in_current_module(node.fullname)
                 or isinstance(node, TypeInfo)
                 or (isinstance(node, PlaceholderNode) and node.becomes_typeinfo))
 
+    def is_textually_before_statement(self, node: SymbolNode) -> bool:
+        """Check if a node is defined textually before the current statement
+
+        Note that decorated functions' line number are the same as
+        the top decorator.
+        """
+        assert self.statement
+        line_diff = self.statement.line - node.line
+        if isinstance(node, OverloadedFuncDef) and isinstance(self.statement, FuncDef):
+            return node.name != self.statement.name and line_diff > 0
path: mypy/semanal.py
text: @user1:
Comparing names looks like a hack as it can confuse two totally unrelated definitions. You can instead directly check if function definition is one of the overload items (the latter are stored as `.items` and `.impl`). This can be written as a separate helper method.

Also add a comment that this is a corner case where mypy technically deviates from runtime name resolution, but it is fine because we want an overloaded function to be treated as a single unit.