Allow any more general type for override of a method (#1524)

This is a prerequisite for fixing #1261 because overrides with generic
function type variables currently work by accident: the type variables
have the same ids in the original method and the overriding method, so
is_subtype on the argument type returns True.

This also allows an overriding method to generalize a specific type in
the original method to a type variable, as demonstrated in one of the
tests (testOverrideGenericMethodInNonGenericClassGeneralize).

is_subtype already determines correctly whether the override is valid,
so all the hard work here is in providing a more specific error
message when possible.

Author: rwbarton

mypy/checker.py

@@ -988,43 +988,55 @@ def check_override(self, override: FunctionLike, original: FunctionLike,
                      only used for generating error messages.
           supertype: The name of the supertype.
         """
-        if (isinstance(override, Overloaded) or
-                isinstance(original, Overloaded) or
-                len(cast(CallableType, override).arg_types) !=
-                len(cast(CallableType, original).arg_types) or
-                cast(CallableType, override).min_args !=
-                cast(CallableType, original).min_args):
-            # Use boolean variable to clarify code.
-            fail = False
-            if not is_subtype(override, original):
-                fail = True
-            elif (not isinstance(original, Overloaded) and
-                  isinstance(override, Overloaded) and
-                  name in nodes.reverse_op_methods.keys()):
-                # Operator method overrides cannot introduce overloading, as
-                # this could be unsafe with reverse operator methods.
-                fail = True
-            if fail:
+        # Use boolean variable to clarify code.
+        fail = False
+        if not is_subtype(override, original):
+            fail = True
+        elif (not isinstance(original, Overloaded) and
+              isinstance(override, Overloaded) and
+              name in nodes.reverse_op_methods.keys()):
+            # Operator method overrides cannot introduce overloading, as
+            # this could be unsafe with reverse operator methods.
+            fail = True
+
+        if fail:
+            emitted_msg = False
+            if (isinstance(override, CallableType) and
+                    isinstance(original, CallableType) and
+                    len(override.arg_types) == len(original.arg_types) and
+                    override.min_args == original.min_args):
+                # Give more detailed messages for the common case of both
+                # signatures having the same number of arguments and no
+                # overloads.
+
+                # override might have its own generic function type
+                # variables. If an argument or return type of override
+                # does not have the correct subtyping relationship
+                # with the original type even after these variables
+                # are erased, then it is definitely an incompatiblity.
+
+                override_ids = override.type_var_ids()
+
+                def erase_override(t: Type) -> Type:
+                    return erase_typevars(t, ids_to_erase=override_ids)
+
+                for i in range(len(override.arg_types)):
+                    if not is_subtype(original.arg_types[i],
+                                      erase_override(override.arg_types[i])):
+                        self.msg.argument_incompatible_with_supertype(
+                            i + 1, name, name_in_super, supertype, node)
+                        emitted_msg = True
+
+                if not is_subtype(erase_override(override.ret_type),
+                                  original.ret_type):
+                    self.msg.return_type_incompatible_with_supertype(
+                        name, name_in_super, supertype, node)
+                    emitted_msg = True
+
+            if not emitted_msg:
+                # Fall back to generic incompatibility message.
                 self.msg.signature_incompatible_with_supertype(
                     name, name_in_super, supertype, node)
-            return
-        else:
-            # Give more detailed messages for the common case of both
-            # signatures having the same number of arguments and no
-            # overloads.
-
-            coverride = cast(CallableType, override)
-            coriginal = cast(CallableType, original)
-
-            for i in range(len(coverride.arg_types)):
-                if not is_subtype(coriginal.arg_types[i],
-                                  coverride.arg_types[i]):
-                    self.msg.argument_incompatible_with_supertype(
-                        i + 1, name, name_in_super, supertype, node)
-
-            if not is_subtype(coverride.ret_type, coriginal.ret_type):
-                self.msg.return_type_incompatible_with_supertype(
-                    name, name_in_super, supertype, node)
 
     def visit_class_def(self, defn: ClassDef) -> Type:
         """Type check a class definition."""

mypy/erasetype.py

@@ -1,3 +1,5 @@
+from typing import Optional, Container
+
 from mypy.types import (
     Type, TypeVisitor, UnboundType, ErrorType, AnyType, Void, NoneTyp,
     Instance, TypeVarType, CallableType, TupleType, UnionType, Overloaded, ErasedType,
@@ -96,13 +98,20 @@ def visit_instance(self, t: Instance) -> Type:
         return Instance(t.type, [], t.line)
 
 
-def erase_typevars(t: Type) -> Type:
-    """Replace all type variables in a type with any."""
-    return t.accept(TypeVarEraser())
+def erase_typevars(t: Type, ids_to_erase: Optional[Container[int]] = None) -> Type:
+    """Replace all type variables in a type with any,
+    or just the ones in the provided collection.
+    """
+    return t.accept(TypeVarEraser(ids_to_erase))
 
 
 class TypeVarEraser(TypeTranslator):
     """Implementation of type erasure"""
 
+    def __init__(self, ids_to_erase: Optional[Container[int]]) -> None:
+        self.ids_to_erase = ids_to_erase
+
     def visit_type_var(self, t: TypeVarType) -> Type:
+        if self.ids_to_erase is not None and t.id not in self.ids_to_erase:
+            return t
         return AnyType()

mypy/test/data/check-generic-subtyping.test

@@ -235,9 +235,45 @@ class A:
 class B(A):
     def f(self, x: S, y: T) -> None: pass
 class C(A):
-    def f(self, x: T, y: T) -> None: pass # E: Argument 2 of "f" incompatible with supertype "A"
+    # Okay, because T = object allows any type for the arguments.
+    def f(self, x: T, y: T) -> None: pass
+
+[case testOverrideGenericMethodInNonGenericClassLists]
+from typing import TypeVar, List
+
+T = TypeVar('T')
+S = TypeVar('S')
+
+class A:
+    def f(self, x: List[T], y: List[S]) -> None: pass
+class B(A):
+    def f(self, x: List[S], y: List[T]) -> None: pass
+class C(A):
+    def f(self, x: List[T], y: List[T]) -> None: pass # E: Signature of "f" incompatible with supertype "A"
+[builtins fixtures/list.py]
+[out]
+main: note: In class "C":
+
+[case testOverrideGenericMethodInNonGenericClassGeneralize]
+from typing import TypeVar
+
+T = TypeVar('T')
+T1 = TypeVar('T1', bound=str)
+S = TypeVar('S')
+
+class A:
+    def f(self, x: int, y: S) -> None: pass
+class B(A):
+    def f(self, x: T, y: S) -> None: pass
+class C(A):
+    def f(self, x: T, y: str) -> None: pass
+class D(A):
+    def f(self, x: T1, y: S) -> None: pass # TODO: This error could be more specific.
 [out]
 main: note: In class "C":
+main:12: error: Argument 2 of "f" incompatible with supertype "A"
+main: note: In class "D":
+main:14: error: Signature of "f" incompatible with supertype "A"
 
 
 -- Inheritance from generic types with implicit dynamic supertype