From 347a1682ebe9178463e3bb67b4db08b03c1632d7 Mon Sep 17 00:00:00 2001 From: Christoph Tyralla Date: Wed, 4 Jan 2023 10:50:32 +0100 Subject: [PATCH] Avoid false "Incompatible return value type" errors when dealing with `isinstance`, constrained type variables and multiple inheritance (fixes #13800). The idea is to make a union of the current expanded return type and the previously expanded return types if the latter are among the bases of intersections generated by `isinstance` checks. --- mypy/checker.py | 30 +++++++++- test-data/unit/check-isinstance.test | 83 ++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 7 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index c265ac4905fb..909f6e95fcbd 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -313,6 +313,9 @@ class TypeChecker(NodeVisitor[None], CheckerPluginInterface): scope: CheckerScope # Stack of function return types return_types: list[Type] + # Registry of the return types already evaluated when checking a function + # definition that includes constrained type variables + previously_expanded_return_types: list[Type] # Flags; true for dynamically typed functions dynamic_funcs: list[bool] # Stack of collections of variables with partial types @@ -383,6 +386,7 @@ def __init__( self.binder = ConditionalTypeBinder() self.globals = tree.names self.return_types = [] + self.previously_expanded_return_types = [] self.dynamic_funcs = [] self.partial_types = [] self.partial_reported = set() @@ -1302,9 +1306,10 @@ def check_func_def( ): self.note(message_registry.EMPTY_BODY_ABSTRACT, defn) - self.return_types.pop() + self.previously_expanded_return_types.append(self.return_types.pop()) self.binder = old_binder + self.previously_expanded_return_types.clear() def check_unbound_return_typevar(self, typ: CallableType) -> None: """Fails when the return typevar is not defined in arguments.""" @@ -4225,7 +4230,28 @@ def visit_if_stmt(self, s: IfStmt) -> None: # XXX Issue a warning if condition is always False? with self.binder.frame_context(can_skip=True, fall_through=2): self.push_type_map(if_map) - self.accept(b) + # Eventually, broaden the expanded return type to avoid false + # "incompatible return value type" error messages (see issue 13800). + if self.return_types and (if_map is not None): + original_return_type = self.return_types[-1] + for typ in if_map.values(): + typ = get_proper_type(typ) + if isinstance(typ, UnionType): + typs = flatten_nested_unions(typ.items) + else: + typs = [typ] + for typ in typs: + typ = get_proper_type(typ) + if isinstance(typ, Instance) and (typ.type.is_intersection): + for base in typ.type.bases: + if base in self.previously_expanded_return_types: + self.return_types[-1] = UnionType.make_union( + (self.return_types[-1], base) + ) + self.accept(b) + self.return_types[-1] = original_return_type + else: + self.accept(b) # XXX Issue a warning if condition is always True? self.push_type_map(else_map) diff --git a/test-data/unit/check-isinstance.test b/test-data/unit/check-isinstance.test index 0722ee8d91e5..d80a13b41a59 100644 --- a/test-data/unit/check-isinstance.test +++ b/test-data/unit/check-isinstance.test @@ -2486,24 +2486,97 @@ class B: attr: int class C: attr: str +class D: + attr: int + +# basic test cases: T1 = TypeVar('T1', A, B) def f1(x: T1) -> T1: if isinstance(x, A): - # The error message is confusing, but we indeed do run into problems if - # 'x' is a subclass of A and B - return A() # E: Incompatible return value type (got "A", expected "B") + # No error, because "A" takes priority over "B" when passing a subclass + # of "A" and "B" to "f1". + return A() else: return B() T2 = TypeVar('T2', B, C) def f2(x: T2) -> T2: if isinstance(x, B): - # In contrast, it's impossible for a subclass of "B" and "C" to - # exist, so this is fine + # No error, because a subclass of "B" and "C" cannot exist. return B() else: return C() + +T3 = TypeVar('T3', B, A) +def f3(x: T3) -> T3: + if isinstance(x, A): + # The error message is confusing, but we indeed do run into problems if + # 'x' is a subclass of "A" and "B" and "B" takes priority over "A". + return A() # E: Incompatible return value type (got "A", expected "B") + else: + return B() + +# more complex test cases: + +T4 = TypeVar('T4', A, B, D) +def f4(x: T4) -> T4: + if isinstance(x, A): + return A() + elif isinstance(x, B): + return B() + else: + return D() + +T5 = TypeVar('T5', A, B, D) +def f5(x: T5) -> T5: + if isinstance(x, A): + return A() + if isinstance(x, B): + return B() + return D() + +T6 = TypeVar('T6', A, B, D) +def f6(x: T6) -> T6: + if isinstance(x, A): + if isinstance(x, B): + return B() # E: Incompatible return value type (got "B", expected "A") + return A() + if isinstance(x, B): + return B() + return D() + +T7 = TypeVar('T7', A, B, D) +def f7(x: T7) -> T7: + if isinstance(x, (A, B)): + return A() # E: Incompatible return value type (got "A", expected "B") + return D() + +T8 = TypeVar('T8', A, B, D) +def f8(x: T8) -> T8: + if isinstance(x, (A, B)): + if isinstance(x, A): + return A() + return B() + return D() + +T9 = TypeVar('T9', A, D, B) +def f9(x: T9) -> T9: + if isinstance(x, A): + return A() + elif isinstance(x, B): + return B() # E: Incompatible return value type (got "B", expected "D") + else: + return D() + +T10 = TypeVar('T10', A, D, B) +def f10(x: T10) -> T10: + if isinstance(x, (A, B)): + if isinstance(x, A): + return A() + return B() # E: Incompatible return value type (got "B", expected "Union[D, A]") + return D() + [builtins fixtures/isinstance.pyi] [case testIsInstanceAdHocIntersectionUsage]