Fix type checking decorated method overrides (#3918)

The fix turned out to be pretty complicated, as there were a bunch of
untested method overriding scenarios which weren't quite right. I
fixed any related issues that I encountered, though I'm not certain
whether some of the issues were hidden previously by other bugs. I
also added tests for some related, previously untested scenarios.

Fixes #1441.

Author: JukkaL

mypy/checker.py

@@ -1015,13 +1015,13 @@ def expand_typevars(self, defn: FuncItem,
         else:
             return [(defn, typ)]
 
-    def check_method_override(self, defn: FuncBase) -> None:
+    def check_method_override(self, defn: Union[FuncBase, Decorator]) -> None:
         """Check if function definition is compatible with base classes."""
         # Check against definitions in base classes.
         for base in defn.info.mro[1:]:
             self.check_method_or_accessor_override_for_base(defn, base)
 
-    def check_method_or_accessor_override_for_base(self, defn: FuncBase,
+    def check_method_or_accessor_override_for_base(self, defn: Union[FuncBase, Decorator],
                                                    base: TypeInfo) -> None:
         """Check if method definition is compatible with a base class."""
         if base:
@@ -1041,13 +1041,26 @@ def check_method_or_accessor_override_for_base(self, defn: FuncBase,
                                                                   base)
 
     def check_method_override_for_base_with_name(
-            self, defn: FuncBase, name: str, base: TypeInfo) -> None:
+            self, defn: Union[FuncBase, Decorator], name: str, base: TypeInfo) -> None:
         base_attr = base.names.get(name)
         if base_attr:
             # The name of the method is defined in the base class.
 
+            # Point errors at the 'def' line (important for backward compatibility
+            # of type ignores).
+            if not isinstance(defn, Decorator):
+                context = defn
+            else:
+                context = defn.func
             # Construct the type of the overriding method.
-            typ = bind_self(self.function_type(defn), self.scope.active_self_type())
+            if isinstance(defn, FuncBase):
+                typ = self.function_type(defn)  # type: Type
+            else:
+                assert defn.var.is_ready
+                assert defn.var.type is not None
+                typ = defn.var.type
+            if isinstance(typ, FunctionLike) and not is_static(context):
+                typ = bind_self(typ, self.scope.active_self_type())
             # Map the overridden method type to subtype context so that
             # it can be checked for compatibility.
             original_type = base_attr.type
@@ -1058,23 +1071,33 @@ def check_method_override_for_base_with_name(
                     original_type = self.function_type(base_attr.node.func)
                 else:
                     assert False, str(base_attr.node)
-            if isinstance(original_type, FunctionLike):
-                original = map_type_from_supertype(
-                    bind_self(original_type, self.scope.active_self_type()),
-                    defn.info, base)
+            if isinstance(original_type, AnyType) or isinstance(typ, AnyType):
+                pass
+            elif isinstance(original_type, FunctionLike) and isinstance(typ, FunctionLike):
+                if (isinstance(base_attr.node, (FuncBase, Decorator))
+                        and not is_static(base_attr.node)):
+                    bound = bind_self(original_type, self.scope.active_self_type())
+                else:
+                    bound = original_type
+                original = map_type_from_supertype(bound, defn.info, base)
                 # Check that the types are compatible.
                 # TODO overloaded signatures
                 self.check_override(typ,
                                     cast(FunctionLike, original),
                                     defn.name(),
                                     name,
                                     base.name(),
-                                    defn)
-            elif isinstance(original_type, AnyType):
+                                    context)
+            elif is_equivalent(original_type, typ):
+                # Assume invariance for a non-callable attribute here. Note
+                # that this doesn't affect read-only properties which can have
+                # covariant overrides.
+                #
+                # TODO: Allow covariance for read-only attributes?
                 pass
             else:
                 self.msg.signature_incompatible_with_supertype(
-                    defn.name(), name, base.name(), defn)
+                    defn.name(), name, base.name(), context)
 
     def check_override(self, override: FunctionLike, original: FunctionLike,
                        name: str, name_in_super: str, supertype: str,
@@ -2364,9 +2387,11 @@ def visit_decorator(self, e: Decorator) -> None:
                     e.var.is_ready = True
                     return
 
-        e.func.accept(self)
+        self.check_func_item(e.func, name=e.func.name())
+
+        # Process decorators from the inside out to determine decorated signature, which
+        # may be different from the declared signature.
         sig = self.function_type(e.func)  # type: Type
-        # Process decorators from the inside out.
         for d in reversed(e.decorators):
             if refers_to_fullname(d, 'typing.overload'):
                 self.fail('Single overload definition, multiple required', e)
@@ -2387,6 +2412,8 @@ def visit_decorator(self, e: Decorator) -> None:
         e.var.is_ready = True
         if e.func.is_property:
             self.check_incompatible_property_override(e)
+        if e.func.info and not e.func.is_dynamic():
+            self.check_method_override(e)
 
     def check_for_untyped_decorator(self,
                                     func: FuncDef,
@@ -3316,3 +3343,13 @@ def is_untyped_decorator(typ: Optional[Type]) -> bool:
     if not typ or not isinstance(typ, CallableType):
         return True
     return typ.implicit
+
+
+def is_static(func: Union[FuncBase, Decorator]) -> bool:
+    if isinstance(func, Decorator):
+        return is_static(func.func)
+    elif isinstance(func, OverloadedFuncDef):
+        return any(is_static(item) for item in func.items)
+    elif isinstance(func, FuncItem):
+        return func.is_static
+    return False

mypy/nodes.py

@@ -565,7 +565,7 @@ class Decorator(SymbolNode, Statement):
     """
 
     func = None  # type: FuncDef                # Decorated function
-    decorators = None  # type: List[Expression] # Decorators, at least one  # XXX Not true
+    decorators = None  # type: List[Expression] # Decorators (may be empty)
     var = None  # type: Var                     # Represents the decorated function obj
     is_overload = False
 
@@ -582,6 +582,10 @@ def name(self) -> str:
     def fullname(self) -> str:
         return self.func.fullname()
 
+    @property
+    def info(self) -> 'TypeInfo':
+        return self.func.info
+
     def accept(self, visitor: StatementVisitor[T]) -> T:
         return visitor.visit_decorator(self)
 

test-data/unit/check-abstract.test

@@ -728,13 +728,10 @@ class A(metaclass=ABCMeta):
     def x(self) -> int: pass
 class B(A):
     @property
-    def x(self) -> str: pass # E
+    def x(self) -> str: pass # E: Return type of "x" incompatible with supertype "A"
 b = B()
-b.x() # E
+b.x() # E: "str" not callable
 [builtins fixtures/property.pyi]
-[out]
-main:7: error: Return type of "x" incompatible with supertype "A"
-main:9: error: "str" not callable
 
 [case testCantImplementAbstractPropertyViaInstanceVariable]
 from abc import abstractproperty, ABCMeta

test-data/unit/check-classes.test

@@ -345,6 +345,136 @@ class A:
 class B(A):
     def __init_subclass__(cls) -> None: pass
 
+[case testOverrideWithDecorator]
+from typing import Callable
+
+def int_to_none(f: Callable[..., int]) -> Callable[..., None]: ...
+def str_to_int(f: Callable[..., str]) -> Callable[..., int]: ...
+
+class A:
+    def f(self) -> None: pass
+    def g(self) -> str: pass
+    def h(self) -> None: pass
+
+class B(A):
+    @int_to_none
+    def f(self) -> int: pass
+    @str_to_int
+    def g(self) -> str: pass # E: Signature of "g" incompatible with supertype "A"
+    @int_to_none
+    @str_to_int
+    def h(self) -> str: pass
+
+[case testOverrideDecorated]
+from typing import Callable
+
+def str_to_int(f: Callable[..., str]) -> Callable[..., int]: ...
+
+class A:
+    @str_to_int
+    def f(self) -> str: pass
+    @str_to_int
+    def g(self) -> str: pass
+    @str_to_int
+    def h(self) -> str: pass
+
+class B(A):
+    def f(self) -> int: pass
+    def g(self) -> str: pass # E: Signature of "g" incompatible with supertype "A"
+    @str_to_int
+    def h(self) -> str: pass
+
+[case testOverrideWithDecoratorReturningAny]
+def dec(f): pass
+
+class A:
+    def f(self) -> str: pass
+
+class B(A):
+    @dec
+    def f(self) -> int: pass
+
+[case testOverrideWithDecoratorReturningInstance]
+def dec(f) -> str: pass
+
+class A:
+    def f(self) -> str: pass
+    @dec
+    def g(self) -> int: pass
+    @dec
+    def h(self) -> int: pass
+
+class B(A):
+    @dec
+    def f(self) -> int: pass # E: Signature of "f" incompatible with supertype "A"
+    def g(self) -> int: pass # E: Signature of "g" incompatible with supertype "A"
+    @dec
+    def h(self) -> str: pass
+
+[case testOverrideStaticMethodWithStaticMethod]
+class A:
+    @staticmethod
+    def f(x: int, y: str) -> None: pass
+    @staticmethod
+    def g(x: int, y: str) -> None: pass
+
+class B(A):
+    @staticmethod
+    def f(x: int, y: str) -> None: pass
+    @staticmethod
+    def g(x: str, y: str) -> None: pass # E: Argument 1 of "g" incompatible with supertype "A"
+[builtins fixtures/classmethod.pyi]
+
+[case testOverrideClassMethodWithClassMethod]
+class A:
+    @classmethod
+    def f(cls, x: int, y: str) -> None: pass
+    @classmethod
+    def g(cls, x: int, y: str) -> None: pass
+
+class B(A):
+    @classmethod
+    def f(cls, x: int, y: str) -> None: pass
+    @classmethod
+    def g(cls, x: str, y: str) -> None: pass # E: Argument 1 of "g" incompatible with supertype "A"
+[builtins fixtures/classmethod.pyi]
+
+[case testOverrideClassMethodWithStaticMethod]
+class A:
+    @classmethod
+    def f(cls, x: int) -> None: pass
+    @classmethod
+    def g(cls, x: int) -> int: pass
+    @classmethod
+    def h(cls) -> int: pass
+
+class B(A):
+    @staticmethod
+    def f(x: int) -> None: pass
+    @staticmethod
+    def g(x: str) -> int: pass # E: Argument 1 of "g" incompatible with supertype "A"
+    @staticmethod
+    def h() -> int: pass
+[builtins fixtures/classmethod.pyi]
+
+[case testOverrideStaticMethodWithClassMethod]
+class A:
+    @staticmethod
+    def f(x: int) -> None: pass
+    @staticmethod
+    def g(x: str) -> int: pass
+    @staticmethod
+    def h() -> int: pass
+
+class B(A):
+    @classmethod
+    def f(cls, x: int) -> None: pass
+    @classmethod
+    def g(cls, x: int) -> int: pass # E: Argument 1 of "g" incompatible with supertype "A"
+    @classmethod
+    def h(cls) -> int: pass
+[builtins fixtures/classmethod.pyi]
+
 
 -- Constructors
 -- ------------

test-data/unit/check-dynamic-typing.test

@@ -653,6 +653,42 @@ class A(B):
         x()
 [out]
 
+[case testInvalidOverrideWithImplicitSignatureAndClassMethod1]
+class B:
+    @classmethod
+    def f(cls, x, y): pass
+class A(B):
+    @classmethod
+    def f(cls, x, y, z): pass # No error since no annotations
+[builtins fixtures/classmethod.pyi]
+
+[case testInvalidOverrideWithImplicitSignatureAndClassMethod2]
+class B:
+    @classmethod
+    def f(cls, x: int, y): pass
+class A(B):
+    @classmethod
+    def f(cls, x, y, z): pass # No error since no annotations
+[builtins fixtures/classmethod.pyi]
+
+[case testInvalidOverrideWithImplicitSignatureAndStaticMethod1]
+class B:
+    @staticmethod
+    def f(x, y): pass
+class A(B):
+    @staticmethod
+    def f(x, y, z): pass # No error since no annotations
+[builtins fixtures/classmethod.pyi]
+
+[case testInvalidOverrideWithImplicitSignatureAndStaticMethod2]
+class B:
+    @staticmethod
+    def f(self, x: int, y): pass
+class A(B):
+    @staticmethod
+    def f(self, x, y, z): pass # No error since no annotations
+[builtins fixtures/classmethod.pyi]
+
 
 -- Don't complain about too few/many arguments in dynamic functions
 -- ----------------------------------------------------------------

test-data/unit/check-functions.test

@@ -1276,6 +1276,56 @@ if x:
 else:
     def f(x: int = 0) -> None: pass # E: All conditional function variants must have identical signatures
 
+[case testConditionalFunctionDefinitionUsingDecorator1]
+from typing import Callable
+
+def dec(f) -> Callable[[int], None]: pass
+
+x = int()
+if x:
+    @dec
+    def f(): pass
+else:
+    def f(x: int) -> None: pass
+
+[case testConditionalFunctionDefinitionUsingDecorator2]
+from typing import Callable
+
+def dec(f) -> Callable[[int], None]: pass
+
+x = int()
+if x:
+    @dec
+    def f(): pass
+else:
+    def f(x: str) -> None: pass # E: Incompatible redefinition (redefinition with type "Callable[[str], None]", original type "Callable[[int], None]")
+
+[case testConditionalFunctionDefinitionUsingDecorator3]
+from typing import Callable
+
+def dec(f) -> Callable[[int], None]: pass
+
+x = int()
+if x:
+    def f(x: int) -> None: pass
+else:
+    # TODO: This should be okay.
+    @dec # E: Name 'f' already defined
+    def f(): pass
+
+[case testConditionalFunctionDefinitionUsingDecorator4]
+from typing import Callable
+
+def dec(f) -> Callable[[int], None]: pass
+
+x = int()
+if x:
+    def f(x: str) -> None: pass
+else:
+    # TODO: We should report an incompatible redefinition.
+    @dec # E: Name 'f' already defined
+    def f(): pass
+
 [case testConditionalRedefinitionOfAnUnconditionalFunctionDefinition1]
 from typing import Any
 def f(x: str) -> None: pass