From e067603b0ae4ea1c9cbbdfd65bfd61fec7880099 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 04:05:52 +0100 Subject: [PATCH 01/10] Use bases with substituted generic args when checking multiple inheritance compatibility --- mypy/checker.py | 154 ++++++++++++------ mypy/checkexpr.py | 7 +- mypy/nodes.py | 2 +- test-data/unit/check-generic-subtyping.test | 35 +++- .../unit/check-multiple-inheritance.test | 9 +- 5 files changed, 151 insertions(+), 56 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 2edcaa6bc5c5..162218f0c1a9 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2104,7 +2104,9 @@ def check_method_override_for_base_with_name( original_class_or_static = False # a variable can't be class or static if isinstance(original_type, FunctionLike): - original_type = self.bind_and_map_method(base_attr, original_type, defn.info, base) + original_type = self.bind_and_map_method( + base_attr.node, original_type, defn.info, base + ) if original_node and is_property(original_node): original_type = get_property_type(original_type) @@ -2200,7 +2202,7 @@ def check_method_override_for_base_with_name( return False def bind_and_map_method( - self, sym: SymbolTableNode, typ: FunctionLike, sub_info: TypeInfo, super_info: TypeInfo + self, node: Node | None, typ: FunctionLike, sub_info: TypeInfo, super_info: TypeInfo ) -> FunctionLike: """Bind self-type and map type variables for a method. @@ -2210,13 +2212,11 @@ def bind_and_map_method( sub_info: class where the method is used super_info: class where the method was defined """ - if isinstance(sym.node, (FuncDef, OverloadedFuncDef, Decorator)) and not is_static( - sym.node - ): - if isinstance(sym.node, Decorator): - is_class_method = sym.node.func.is_class + if isinstance(node, (FuncDef, OverloadedFuncDef, Decorator)) and not is_static(node): + if isinstance(node, Decorator): + is_class_method = node.func.is_class else: - is_class_method = sym.node.is_class + is_class_method = node.is_class mapped_typ = cast(FunctionLike, map_type_from_supertype(typ, sub_info, super_info)) active_self_type = self.scope.active_self_type() @@ -2745,46 +2745,45 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: # No multiple inheritance. return # Verify that inherited attributes are compatible. - mro = typ.mro[1:] - for i, base in enumerate(mro): + bases = typ.bases + all_names = [{n for p in b.type.mro for n in p.names} for b in bases] + for i, base in enumerate(bases): # Attributes defined in both the type and base are skipped. # Normal checks for attribute compatibility should catch any problems elsewhere. - non_overridden_attrs = base.names.keys() - typ.names.keys() + non_overridden_attrs = all_names[i] - typ.names.keys() for name in non_overridden_attrs: if is_private(name): continue - for base2 in mro[i + 1 :]: + for j, base2 in enumerate(bases[i + 1 :], i + 1): # We only need to check compatibility of attributes from classes not # in a subclass relationship. For subclasses, normal (single inheritance) # checks suffice (these are implemented elsewhere). - if name in base2.names and base2 not in base.mro: + if name in all_names[j] and base.type != base2.type: self.check_compatibility(name, base, base2, typ) - def determine_type_of_member(self, sym: SymbolTableNode) -> Type | None: - if sym.type is not None: - return sym.type - if isinstance(sym.node, FuncBase): - return self.function_type(sym.node) - if isinstance(sym.node, TypeInfo): - if sym.node.typeddict_type: + def determine_type_of_member(self, node: SymbolNode) -> Type | None: + if isinstance(node, FuncBase): + return self.function_type(node) + if isinstance(node, TypeInfo): + if node.typeddict_type: # We special-case TypedDict, because they don't define any constructor. - return self.expr_checker.typeddict_callable(sym.node) + return self.expr_checker.typeddict_callable(node) else: - return type_object_type(sym.node, self.named_type) - if isinstance(sym.node, TypeVarExpr): + return type_object_type(node, self.named_type) + if isinstance(node, TypeVarExpr): # Use of TypeVars is rejected in an expression/runtime context, so # we don't need to check supertype compatibility for them. return AnyType(TypeOfAny.special_form) - if isinstance(sym.node, TypeAlias): + if isinstance(node, TypeAlias): with self.msg.filter_errors(): # Suppress any errors, they will be given when analyzing the corresponding node. # Here we may have incorrect options and location context. - return self.expr_checker.alias_type_in_runtime_context(sym.node, ctx=sym.node) + return self.expr_checker.alias_type_in_runtime_context(node, ctx=node) # TODO: handle more node kinds here. return None def check_compatibility( - self, name: str, base1: TypeInfo, base2: TypeInfo, ctx: TypeInfo + self, name: str, base1: Instance, base2: Instance, ctx: TypeInfo ) -> None: """Check if attribute name in base1 is compatible with base2 in multiple inheritance. @@ -2809,10 +2808,41 @@ class C(B, A[int]): ... # this is unsafe because... if name in ("__init__", "__new__", "__init_subclass__"): # __init__ and friends can be incompatible -- it's a special case. return - first = base1.names[name] - second = base2.names[name] - first_type = get_proper_type(self.determine_type_of_member(first)) - second_type = get_proper_type(self.determine_type_of_member(second)) + first_type = first_node = None + second_type = second_node = None + orig_var = ctx.get(name) + if orig_var is not None and orig_var.node is not None: + if (b1type := base1.type.get_containing_type_info(name)) is not None: + base1 = map_instance_to_supertype(base1, b1type) + first_type, first_node = self.attribute_type_from_base( + orig_var.node, base1.type, base1 + ) + + if (b2type := base2.type.get_containing_type_info(name)) is not None: + base2 = map_instance_to_supertype(base2, b2type) + second_type, second_node = self.attribute_type_from_base( + orig_var.node, base2.type, base2 + ) + + # Fix the order. We iterate over the explicit bases, which means we may + # end up with the following structure: + # class A: + # def fn(self, x: int) -> None: ... + # class B(A): ... + # class C(A): + # def fn(self, x: int|str) -> None: ... + # class D(B, C): ... + # Here D.fn will actually be dispatched to C.fn which is assignable to A.fn, + # but without this fixup we'd check A.fn against C.fn instead. + # See testMultipleInheritanceTransitive in check-multiple-inheritance.test + if ( + b1type is not None + and b2type is not None + and ctx.mro.index(b1type) > ctx.mro.index(b2type) + ): + b1type, b2type = b2type, b1type + first_type, second_type = second_type, first_type + first_node, second_node = second_node, first_node # TODO: use more principled logic to decide is_subtype() vs is_equivalent(). # We should rely on mutability of superclass node, not on types being Callable. @@ -2822,7 +2852,7 @@ class C(B, A[int]): ... # this is unsafe because... if isinstance(first_type, Instance): call = find_member("__call__", first_type, first_type, is_operator=True) if call and isinstance(second_type, FunctionLike): - second_sig = self.bind_and_map_method(second, second_type, ctx, base2) + second_sig = self.bind_and_map_method(second_node, second_type, ctx, base2.type) ok = is_subtype(call, second_sig, ignore_pos_arg_names=True) elif isinstance(first_type, FunctionLike) and isinstance(second_type, FunctionLike): if first_type.is_type_obj() and second_type.is_type_obj(): @@ -2834,42 +2864,70 @@ class C(B, A[int]): ... # this is unsafe because... ) else: # First bind/map method types when necessary. - first_sig = self.bind_and_map_method(first, first_type, ctx, base1) - second_sig = self.bind_and_map_method(second, second_type, ctx, base2) + first_sig = self.bind_and_map_method(first_node, first_type, ctx, base1.type) + second_sig = self.bind_and_map_method(second_node, second_type, ctx, base2.type) ok = is_subtype(first_sig, second_sig, ignore_pos_arg_names=True) elif first_type and second_type: - if isinstance(first.node, Var): - first_type = expand_self_type(first.node, first_type, fill_typevars(ctx)) - if isinstance(second.node, Var): - second_type = expand_self_type(second.node, second_type, fill_typevars(ctx)) + if isinstance(first_node, Var): + first_type = expand_self_type(first_node, first_type, fill_typevars(ctx)) + if isinstance(second_node, Var): + second_type = expand_self_type(second_node, second_type, fill_typevars(ctx)) ok = is_equivalent(first_type, second_type) if not ok: - second_node = base2[name].node + second_var = base2.type.get(name) if ( isinstance(second_type, FunctionLike) - and second_node is not None - and is_property(second_node) + and second_var is not None + and second_var.node is not None + and is_property(second_var.node) ): second_type = get_property_type(second_type) ok = is_subtype(first_type, second_type) else: if first_type is None: - self.msg.cannot_determine_type_in_base(name, base1.name, ctx) + self.msg.cannot_determine_type_in_base(name, base1.type.name, ctx) if second_type is None: - self.msg.cannot_determine_type_in_base(name, base2.name, ctx) + self.msg.cannot_determine_type_in_base(name, base2.type.name, ctx) ok = True # Final attributes can never be overridden, but can override # non-final read-only attributes. - if is_final_node(second.node) and not is_private(name): - self.msg.cant_override_final(name, base2.name, ctx) - if is_final_node(first.node): - self.check_if_final_var_override_writable(name, second.node, ctx) + if is_final_node(second_node) and not is_private(name): + self.msg.cant_override_final(name, base2.type.name, ctx) + if is_final_node(first_node): + self.check_if_final_var_override_writable(name, second_node, ctx) # Some attributes like __slots__ and __deletable__ are special, and the type can # vary across class hierarchy. - if isinstance(second.node, Var) and second.node.allow_incompatible_override: + if isinstance(second_node, Var) and second_node.allow_incompatible_override: ok = True if not ok: - self.msg.base_class_definitions_incompatible(name, base1, base2, ctx) + self.msg.base_class_definitions_incompatible(name, base1.type, base2.type, ctx) + + def attribute_type_from_base( + self, expr_node: SymbolNode, base: TypeInfo, self_type: Instance + ) -> tuple[ProperType | None, Node | None]: + """For a NameExpr that is part of a class, walk all base classes and try + to find the first class that defines a Type for the same name.""" + expr_name = expr_node.name + base_var = base.names.get(expr_name) + + if base_var: + base_node = base_var.node + base_type = base_var.type + + if base_type: + if not has_no_typevars(base_type): + itype = map_instance_to_supertype(self_type, base) + base_type = expand_type_by_instance(base_type, itype) + + return get_proper_type(base_type), base_node + + if ( + base_node is not None + and (base_type := self.determine_type_of_member(base_node)) is not None + ): + return get_proper_type(base_type), base_node + + return None, None def check_metaclass_compatibility(self, typ: TypeInfo) -> None: """Ensures that metaclasses of all parent types are compatible.""" diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index adb65a126f38..84db7f03d1dc 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -462,7 +462,12 @@ def module_type(self, node: MypyFile) -> Instance: continue if isinstance(n.node, Var) and n.node.is_final: immutable.add(name) - typ = self.chk.determine_type_of_member(n) + + typ = None + if n.type is not None: + typ = n.type + elif n.node is not None: + typ = self.chk.determine_type_of_member(n.node) if typ: module_attrs[name] = typ else: diff --git a/mypy/nodes.py b/mypy/nodes.py index 9e26103e2f58..fe7a5a5b61e9 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -4202,7 +4202,7 @@ def is_class_var(expr: NameExpr) -> bool: return False -def is_final_node(node: SymbolNode | None) -> bool: +def is_final_node(node: Node | None) -> bool: """Check whether `node` corresponds to a final attribute.""" return isinstance(node, (Var, FuncDef, OverloadedFuncDef, Decorator)) and node.is_final diff --git a/test-data/unit/check-generic-subtyping.test b/test-data/unit/check-generic-subtyping.test index 90180e0f83f6..78f5e1e28069 100644 --- a/test-data/unit/check-generic-subtyping.test +++ b/test-data/unit/check-generic-subtyping.test @@ -946,7 +946,7 @@ x1: X1[str, int] reveal_type(list(x1)) # N: Revealed type is "builtins.list[builtins.int]" reveal_type([*x1]) # N: Revealed type is "builtins.list[builtins.int]" -class X2(Generic[T, U], Iterator[U], Mapping[T, U]): +class X2(Generic[T, U], Iterator[U], Mapping[T, U]): # E: Definition of "__iter__" in base class "Iterable" is incompatible with definition in base class "Iterable" pass x2: X2[str, int] @@ -1017,10 +1017,7 @@ x1: X1[str, int] reveal_type(iter(x1)) # N: Revealed type is "typing.Iterator[builtins.int]" reveal_type({**x1}) # N: Revealed type is "builtins.dict[builtins.int, builtins.str]" -# Some people would expect this to raise an error, but this currently does not: -# `Mapping` has `Iterable[U]` base class, `X2` has direct `Iterable[T]` base class. -# It would be impossible to define correct `__iter__` method for incompatible `T` and `U`. -class X2(Generic[T, U], Mapping[U, T], Iterable[T]): +class X2(Generic[T, U], Mapping[U, T], Iterable[T]): # E: Definition of "__iter__" in base class "Iterable" is incompatible with definition in base class "Iterable" pass x2: X2[str, int] @@ -1065,3 +1062,31 @@ class F(E[T_co], Generic[T_co]): ... # E: Variance of TypeVar "T_co" incompatib class G(Generic[T]): ... class H(G[T_contra], Generic[T_contra]): ... # E: Variance of TypeVar "T_contra" incompatible with variance in parent type + +[case testMultipleInheritanceCompatibleTypeVar] +from typing import Generic, TypeVar + +T = TypeVar("T") +U = TypeVar("U") + +class A(Generic[T]): + x: T + def fn(self, t: T) -> None: ... + +class A2(A[T]): + y: str + z: str + +class B(Generic[T]): + x: T + def fn(self, t: T) -> None: ... + +class C1(A2[str], B[str]): pass +class C2(A2[str], B[int]): pass # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" \ + # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" +class C3(A2[T], B[T]): pass +class C4(A2[U], B[U]): pass +class C5(A2[U], B[T]): pass # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" \ + # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" + +[builtins fixtures/tuple.pyi] diff --git a/test-data/unit/check-multiple-inheritance.test b/test-data/unit/check-multiple-inheritance.test index d03f2e35e1c4..bb233d7d50e9 100644 --- a/test-data/unit/check-multiple-inheritance.test +++ b/test-data/unit/check-multiple-inheritance.test @@ -669,7 +669,6 @@ class D2(B[Union[int, str]], C2): ... class D3(C2, B[str]): ... class D4(B[str], C2): ... # E: Definition of "foo" in base class "A" is incompatible with definition in base class "C2" - [case testMultipleInheritanceOverridingOfFunctionsWithCallableInstances] from typing import Any, Callable @@ -706,3 +705,11 @@ class C34(B3, B4): ... class C41(B4, B1): ... class C42(B4, B2): ... class C43(B4, B3): ... + +[case testMultipleInheritanceTransitive] +class A: + def fn(self, x: int) -> None: ... +class B(A): ... +class C(A): + def fn(self, x: "int | str") -> None: ... +class D(B, C): ... From d9147e06de8bf4b5f7de1eb337f114c61319ce13 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 21:10:52 +0100 Subject: [PATCH 02/10] Ignore transitive errors in multiple inheritance --- mypy/checker.py | 10 ++++++++++ test-data/unit/check-generic-subtyping.test | 11 +++++++++++ 2 files changed, 21 insertions(+) diff --git a/mypy/checker.py b/mypy/checker.py index 162218f0c1a9..65505e702e3f 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2841,9 +2841,19 @@ class C(B, A[int]): ... # this is unsafe because... and ctx.mro.index(b1type) > ctx.mro.index(b2type) ): b1type, b2type = b2type, b1type + base1, base2 = base2, base1 first_type, second_type = second_type, first_type first_node, second_node = second_node, first_node + if ( + base1 is not None + and base2 is not None + and is_subtype(base1, base2, ignore_promotions=True) + ): + # If base1 already inherits from base2 with correct type args, + # we have reported errors if any. Avoid reporting them again. + return + # TODO: use more principled logic to decide is_subtype() vs is_equivalent(). # We should rely on mutability of superclass node, not on types being Callable. diff --git a/test-data/unit/check-generic-subtyping.test b/test-data/unit/check-generic-subtyping.test index 78f5e1e28069..b4bc9fd426e9 100644 --- a/test-data/unit/check-generic-subtyping.test +++ b/test-data/unit/check-generic-subtyping.test @@ -1090,3 +1090,14 @@ class C5(A2[U], B[T]): pass # E: Definition of "x" in base class "A" is incompa # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" [builtins fixtures/tuple.pyi] + +[case testMultipleInheritanceCompatErrorPropagation] +class A: + foo: bytes +class B(A): + foo: str # type: ignore[assignment] + +class Ok(B, A): pass + +class C(A): pass +class Ok2(B, C): pass From 1960ad9214053a260564c95f3f77b7a3808ac293 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 21:20:22 +0100 Subject: [PATCH 03/10] Sort for fixed messages order --- mypy/checker.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mypy/checker.py b/mypy/checker.py index 65505e702e3f..04234dc41c4a 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2750,7 +2750,8 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: for i, base in enumerate(bases): # Attributes defined in both the type and base are skipped. # Normal checks for attribute compatibility should catch any problems elsewhere. - non_overridden_attrs = all_names[i] - typ.names.keys() + # Sort for consistent messages order. + non_overridden_attrs = sorted(all_names[i] - typ.names.keys()) for name in non_overridden_attrs: if is_private(name): continue From 46ec61b0f983a103d14d019fda1aaeaddb7d32be Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 21:30:24 +0100 Subject: [PATCH 04/10] Place inheritance check earlier --- mypy/checker.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 04234dc41c4a..732a568f0dca 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2809,9 +2809,16 @@ class C(B, A[int]): ... # this is unsafe because... if name in ("__init__", "__new__", "__init_subclass__"): # __init__ and friends can be incompatible -- it's a special case. return + + if is_subtype(base1, base2, ignore_promotions=True): + # If base1 already inherits from base2 with correct type args, + # we have reported errors if any. Avoid reporting them again. + return + first_type = first_node = None second_type = second_node = None orig_var = ctx.get(name) + if orig_var is not None and orig_var.node is not None: if (b1type := base1.type.get_containing_type_info(name)) is not None: base1 = map_instance_to_supertype(base1, b1type) @@ -2846,15 +2853,6 @@ class C(B, A[int]): ... # this is unsafe because... first_type, second_type = second_type, first_type first_node, second_node = second_node, first_node - if ( - base1 is not None - and base2 is not None - and is_subtype(base1, base2, ignore_promotions=True) - ): - # If base1 already inherits from base2 with correct type args, - # we have reported errors if any. Avoid reporting them again. - return - # TODO: use more principled logic to decide is_subtype() vs is_equivalent(). # We should rely on mutability of superclass node, not on types being Callable. From 6b09c526667e4f3cae0cdaa9a1207fbbedee3c0c Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 21:39:58 +0100 Subject: [PATCH 05/10] Fix the messages order in a test --- test-data/unit/check-generic-subtyping.test | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test-data/unit/check-generic-subtyping.test b/test-data/unit/check-generic-subtyping.test index b4bc9fd426e9..6b1183c55d13 100644 --- a/test-data/unit/check-generic-subtyping.test +++ b/test-data/unit/check-generic-subtyping.test @@ -1082,12 +1082,12 @@ class B(Generic[T]): def fn(self, t: T) -> None: ... class C1(A2[str], B[str]): pass -class C2(A2[str], B[int]): pass # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" \ - # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" +class C2(A2[str], B[int]): pass # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" \ + # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" class C3(A2[T], B[T]): pass class C4(A2[U], B[U]): pass -class C5(A2[U], B[T]): pass # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" \ - # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" +class C5(A2[U], B[T]): pass # E: Definition of "fn" in base class "A" is incompatible with definition in base class "B" \ + # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" [builtins fixtures/tuple.pyi] From bdca65ad11bfd022608ebdc102720bbdea061a25 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 22:44:35 +0100 Subject: [PATCH 06/10] Try the "typed MRO" approach instead --- mypy/checker.py | 76 ++++++++----------- test-data/unit/check-generic-subtyping.test | 11 --- .../unit/check-multiple-inheritance.test | 36 +++++++++ 3 files changed, 69 insertions(+), 54 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 732a568f0dca..756886a985c7 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2744,23 +2744,44 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: if len(typ.bases) <= 1: # No multiple inheritance. return + # Verify that inherited attributes are compatible. - bases = typ.bases - all_names = [{n for p in b.type.mro for n in p.names} for b in bases] - for i, base in enumerate(bases): + # Construct a "typed" MRO that follows regular MRO order, but includes instances + # parametrized with their generic args. + # This detects e.g. `class A(Mapping[int, str], Iterable[str])` correctly. + # For each MRO entry, include it parametrized according to each base inheriting + # from it. + typed_mro = [ + map_instance_to_supertype(base, parent) + for parent in typ.mro[1:] + for base in typ.bases + if parent in base.type.mro + ] + # If the first MRO entry is compatible with everything following, we don't need + # (and shouldn't) compare further pairs + # (see testMultipleInheritanceExplcitDiamondResolution) + seen_names = set() + for i, base in enumerate(typed_mro): # Attributes defined in both the type and base are skipped. # Normal checks for attribute compatibility should catch any problems elsewhere. # Sort for consistent messages order. - non_overridden_attrs = sorted(all_names[i] - typ.names.keys()) + non_overridden_attrs = sorted(typed_mro[i].type.names - typ.names.keys()) for name in non_overridden_attrs: if is_private(name): continue - for j, base2 in enumerate(bases[i + 1 :], i + 1): + if name in seen_names: + continue + for j, base2 in enumerate(typed_mro[i + 1 :], i + 1): # We only need to check compatibility of attributes from classes not # in a subclass relationship. For subclasses, normal (single inheritance) # checks suffice (these are implemented elsewhere). - if name in all_names[j] and base.type != base2.type: + if name in base2.type.names and not is_subtype( + base, base2, ignore_promotions=True + ): + # If base1 already inherits from base2 with correct type args, + # we have reported errors if any. Avoid reporting them again. self.check_compatibility(name, base, base2, typ) + seen_names.add(name) def determine_type_of_member(self, node: SymbolNode) -> Type | None: if isinstance(node, FuncBase): @@ -2810,48 +2831,17 @@ class C(B, A[int]): ... # this is unsafe because... # __init__ and friends can be incompatible -- it's a special case. return - if is_subtype(base1, base2, ignore_promotions=True): - # If base1 already inherits from base2 with correct type args, - # we have reported errors if any. Avoid reporting them again. - return - first_type = first_node = None second_type = second_node = None orig_var = ctx.get(name) if orig_var is not None and orig_var.node is not None: - if (b1type := base1.type.get_containing_type_info(name)) is not None: - base1 = map_instance_to_supertype(base1, b1type) - first_type, first_node = self.attribute_type_from_base( - orig_var.node, base1.type, base1 - ) - - if (b2type := base2.type.get_containing_type_info(name)) is not None: - base2 = map_instance_to_supertype(base2, b2type) - second_type, second_node = self.attribute_type_from_base( - orig_var.node, base2.type, base2 - ) - - # Fix the order. We iterate over the explicit bases, which means we may - # end up with the following structure: - # class A: - # def fn(self, x: int) -> None: ... - # class B(A): ... - # class C(A): - # def fn(self, x: int|str) -> None: ... - # class D(B, C): ... - # Here D.fn will actually be dispatched to C.fn which is assignable to A.fn, - # but without this fixup we'd check A.fn against C.fn instead. - # See testMultipleInheritanceTransitive in check-multiple-inheritance.test - if ( - b1type is not None - and b2type is not None - and ctx.mro.index(b1type) > ctx.mro.index(b2type) - ): - b1type, b2type = b2type, b1type - base1, base2 = base2, base1 - first_type, second_type = second_type, first_type - first_node, second_node = second_node, first_node + first_type, first_node = self.attribute_type_from_base( + orig_var.node, base1.type, base1 + ) + second_type, second_node = self.attribute_type_from_base( + orig_var.node, base2.type, base2 + ) # TODO: use more principled logic to decide is_subtype() vs is_equivalent(). # We should rely on mutability of superclass node, not on types being Callable. diff --git a/test-data/unit/check-generic-subtyping.test b/test-data/unit/check-generic-subtyping.test index 6b1183c55d13..c98ae238fc12 100644 --- a/test-data/unit/check-generic-subtyping.test +++ b/test-data/unit/check-generic-subtyping.test @@ -1090,14 +1090,3 @@ class C5(A2[U], B[T]): pass # E: Definition of "fn" in base class "A" is incomp # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" [builtins fixtures/tuple.pyi] - -[case testMultipleInheritanceCompatErrorPropagation] -class A: - foo: bytes -class B(A): - foo: str # type: ignore[assignment] - -class Ok(B, A): pass - -class C(A): pass -class Ok2(B, C): pass diff --git a/test-data/unit/check-multiple-inheritance.test b/test-data/unit/check-multiple-inheritance.test index bb233d7d50e9..00804c2b7f2c 100644 --- a/test-data/unit/check-multiple-inheritance.test +++ b/test-data/unit/check-multiple-inheritance.test @@ -713,3 +713,39 @@ class B(A): ... class C(A): def fn(self, x: "int | str") -> None: ... class D(B, C): ... + +[case testMultipleInheritanceCompatErrorPropagation] +class A: + foo: bytes +class B(A): + foo: str # type: ignore[assignment] + +class Ok(B, A): pass + +class C(A): pass +class Ok2(B, C): pass + +[case testMultipleInheritanceExplcitDiamondResolution] +class A: + class M: + pass + +class B0(A): + class M(A.M): + pass + +class B1(A): + class M(A.M): + pass + +class C(B0,B1): + class M(B0.M, B1.M): + pass + +class D0(B0): + pass +class D1(B1): + pass + +class D(D0,D1,C): + pass From 94bc3554709cfd82b93e05e55a8b8119d7946ca8 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Mon, 9 Dec 2024 22:55:15 +0100 Subject: [PATCH 07/10] Remove unrelated changes --- mypy/checker.py | 122 +++++++++++++++++++--------------------------- mypy/checkexpr.py | 6 +-- mypy/nodes.py | 2 +- 3 files changed, 53 insertions(+), 77 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 756886a985c7..416492626027 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2104,9 +2104,7 @@ def check_method_override_for_base_with_name( original_class_or_static = False # a variable can't be class or static if isinstance(original_type, FunctionLike): - original_type = self.bind_and_map_method( - base_attr.node, original_type, defn.info, base - ) + original_type = self.bind_and_map_method(base_attr, original_type, defn.info, base) if original_node and is_property(original_node): original_type = get_property_type(original_type) @@ -2202,7 +2200,7 @@ def check_method_override_for_base_with_name( return False def bind_and_map_method( - self, node: Node | None, typ: FunctionLike, sub_info: TypeInfo, super_info: TypeInfo + self, sym: SymbolTableNode, typ: FunctionLike, sub_info: TypeInfo, super_info: TypeInfo ) -> FunctionLike: """Bind self-type and map type variables for a method. @@ -2212,11 +2210,13 @@ def bind_and_map_method( sub_info: class where the method is used super_info: class where the method was defined """ - if isinstance(node, (FuncDef, OverloadedFuncDef, Decorator)) and not is_static(node): - if isinstance(node, Decorator): - is_class_method = node.func.is_class + if isinstance(sym.node, (FuncDef, OverloadedFuncDef, Decorator)) and not is_static( + sym.node + ): + if isinstance(sym.node, Decorator): + is_class_method = sym.node.func.is_class else: - is_class_method = node.is_class + is_class_method = sym.node.is_class mapped_typ = cast(FunctionLike, map_type_from_supertype(typ, sub_info, super_info)) active_self_type = self.scope.active_self_type() @@ -2783,27 +2783,44 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: self.check_compatibility(name, base, base2, typ) seen_names.add(name) - def determine_type_of_member(self, node: SymbolNode) -> Type | None: - if isinstance(node, FuncBase): - return self.function_type(node) - if isinstance(node, TypeInfo): - if node.typeddict_type: + def determine_type_of_member(self, sym: SymbolTableNode) -> Type | None: + if sym.type is not None: + return sym.type + if isinstance(sym.node, FuncBase): + return self.function_type(sym.node) + if isinstance(sym.node, TypeInfo): + if sym.node.typeddict_type: # We special-case TypedDict, because they don't define any constructor. - return self.expr_checker.typeddict_callable(node) + return self.expr_checker.typeddict_callable(sym.node) else: - return type_object_type(node, self.named_type) - if isinstance(node, TypeVarExpr): + return type_object_type(sym.node, self.named_type) + if isinstance(sym.node, TypeVarExpr): # Use of TypeVars is rejected in an expression/runtime context, so # we don't need to check supertype compatibility for them. return AnyType(TypeOfAny.special_form) - if isinstance(node, TypeAlias): + if isinstance(sym.node, TypeAlias): with self.msg.filter_errors(): # Suppress any errors, they will be given when analyzing the corresponding node. # Here we may have incorrect options and location context. - return self.expr_checker.alias_type_in_runtime_context(node, ctx=node) + return self.expr_checker.alias_type_in_runtime_context(sym.node, ctx=sym.node) # TODO: handle more node kinds here. return None + def attribute_type_from_base( + self, name: str, base: Instance + ) -> tuple[ProperType | None, SymbolTableNode]: + """For a NameExpr that is part of a class, walk all base classes and try + to find the first class that defines a Type for the same name.""" + base_var = base.type[name] + base_type = self.determine_type_of_member(base_var) + if base_type is None: + return None, base_var + + if not has_no_typevars(base_type): + base_type = expand_type_by_instance(base_type, base) + + return get_proper_type(base_type), base_var + def check_compatibility( self, name: str, base1: Instance, base2: Instance, ctx: TypeInfo ) -> None: @@ -2831,17 +2848,8 @@ class C(B, A[int]): ... # this is unsafe because... # __init__ and friends can be incompatible -- it's a special case. return - first_type = first_node = None - second_type = second_node = None - orig_var = ctx.get(name) - - if orig_var is not None and orig_var.node is not None: - first_type, first_node = self.attribute_type_from_base( - orig_var.node, base1.type, base1 - ) - second_type, second_node = self.attribute_type_from_base( - orig_var.node, base2.type, base2 - ) + first_type, first = self.attribute_type_from_base(name, base1) + second_type, second = self.attribute_type_from_base(name, base2) # TODO: use more principled logic to decide is_subtype() vs is_equivalent(). # We should rely on mutability of superclass node, not on types being Callable. @@ -2851,7 +2859,7 @@ class C(B, A[int]): ... # this is unsafe because... if isinstance(first_type, Instance): call = find_member("__call__", first_type, first_type, is_operator=True) if call and isinstance(second_type, FunctionLike): - second_sig = self.bind_and_map_method(second_node, second_type, ctx, base2.type) + second_sig = self.bind_and_map_method(second, second_type, ctx, base2.type) ok = is_subtype(call, second_sig, ignore_pos_arg_names=True) elif isinstance(first_type, FunctionLike) and isinstance(second_type, FunctionLike): if first_type.is_type_obj() and second_type.is_type_obj(): @@ -2863,22 +2871,21 @@ class C(B, A[int]): ... # this is unsafe because... ) else: # First bind/map method types when necessary. - first_sig = self.bind_and_map_method(first_node, first_type, ctx, base1.type) - second_sig = self.bind_and_map_method(second_node, second_type, ctx, base2.type) + first_sig = self.bind_and_map_method(first, first_type, ctx, base1.type) + second_sig = self.bind_and_map_method(second, second_type, ctx, base2.type) ok = is_subtype(first_sig, second_sig, ignore_pos_arg_names=True) elif first_type and second_type: - if isinstance(first_node, Var): - first_type = expand_self_type(first_node, first_type, fill_typevars(ctx)) - if isinstance(second_node, Var): - second_type = expand_self_type(second_node, second_type, fill_typevars(ctx)) + if isinstance(first.node, Var): + first_type = expand_self_type(first.node, first_type, fill_typevars(ctx)) + if isinstance(second.node, Var): + second_type = expand_self_type(second.node, second_type, fill_typevars(ctx)) ok = is_equivalent(first_type, second_type) if not ok: - second_var = base2.type.get(name) + second_node = base2.type[name].node if ( isinstance(second_type, FunctionLike) - and second_var is not None - and second_var.node is not None - and is_property(second_var.node) + and second_node is not None + and is_property(second_node) ): second_type = get_property_type(second_type) ok = is_subtype(first_type, second_type) @@ -2890,44 +2897,17 @@ class C(B, A[int]): ... # this is unsafe because... ok = True # Final attributes can never be overridden, but can override # non-final read-only attributes. - if is_final_node(second_node) and not is_private(name): + if is_final_node(second.node) and not is_private(name): self.msg.cant_override_final(name, base2.type.name, ctx) - if is_final_node(first_node): - self.check_if_final_var_override_writable(name, second_node, ctx) + if is_final_node(first.node): + self.check_if_final_var_override_writable(name, second.node, ctx) # Some attributes like __slots__ and __deletable__ are special, and the type can # vary across class hierarchy. - if isinstance(second_node, Var) and second_node.allow_incompatible_override: + if isinstance(second.node, Var) and second.node.allow_incompatible_override: ok = True if not ok: self.msg.base_class_definitions_incompatible(name, base1.type, base2.type, ctx) - def attribute_type_from_base( - self, expr_node: SymbolNode, base: TypeInfo, self_type: Instance - ) -> tuple[ProperType | None, Node | None]: - """For a NameExpr that is part of a class, walk all base classes and try - to find the first class that defines a Type for the same name.""" - expr_name = expr_node.name - base_var = base.names.get(expr_name) - - if base_var: - base_node = base_var.node - base_type = base_var.type - - if base_type: - if not has_no_typevars(base_type): - itype = map_instance_to_supertype(self_type, base) - base_type = expand_type_by_instance(base_type, itype) - - return get_proper_type(base_type), base_node - - if ( - base_node is not None - and (base_type := self.determine_type_of_member(base_node)) is not None - ): - return get_proper_type(base_type), base_node - - return None, None - def check_metaclass_compatibility(self, typ: TypeInfo) -> None: """Ensures that metaclasses of all parent types are compatible.""" if ( diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 84db7f03d1dc..f3089da0af50 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -463,11 +463,7 @@ def module_type(self, node: MypyFile) -> Instance: if isinstance(n.node, Var) and n.node.is_final: immutable.add(name) - typ = None - if n.type is not None: - typ = n.type - elif n.node is not None: - typ = self.chk.determine_type_of_member(n.node) + typ = self.chk.determine_type_of_member(n) if typ: module_attrs[name] = typ else: diff --git a/mypy/nodes.py b/mypy/nodes.py index fe7a5a5b61e9..9e26103e2f58 100644 --- a/mypy/nodes.py +++ b/mypy/nodes.py @@ -4202,7 +4202,7 @@ def is_class_var(expr: NameExpr) -> bool: return False -def is_final_node(node: Node | None) -> bool: +def is_final_node(node: SymbolNode | None) -> bool: """Check whether `node` corresponds to a final attribute.""" return isinstance(node, (Var, FuncDef, OverloadedFuncDef, Decorator)) and node.is_final From 4323ed70a250eecbf1207287a0a68265b319f738 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Tue, 10 Dec 2024 01:23:49 +0100 Subject: [PATCH 08/10] Final cleanup --- mypy/checker.py | 2 +- mypy/checkexpr.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 416492626027..4eae3860dbee 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2771,7 +2771,7 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: continue if name in seen_names: continue - for j, base2 in enumerate(typed_mro[i + 1 :], i + 1): + for base2 in typed_mro[i + 1 :]: # We only need to check compatibility of attributes from classes not # in a subclass relationship. For subclasses, normal (single inheritance) # checks suffice (these are implemented elsewhere). diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index f3089da0af50..adb65a126f38 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -462,7 +462,6 @@ def module_type(self, node: MypyFile) -> Instance: continue if isinstance(n.node, Var) and n.node.is_final: immutable.add(name) - typ = self.chk.determine_type_of_member(n) if typ: module_attrs[name] = typ From f06b2b3a8750c5376a93409a76cf36a33d173fbf Mon Sep 17 00:00:00 2001 From: STerliakov Date: Tue, 10 Dec 2024 21:27:02 +0100 Subject: [PATCH 09/10] Extract a helper to create typed MRO. Avoid introducing duplicates of non-generic classes. --- mypy/checker.py | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 4eae3860dbee..95ab05d2a96a 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -2739,6 +2739,24 @@ def check_protocol_variance(self, defn: ClassDef) -> None: if expected != tvar.variance: self.msg.bad_proto_variance(tvar.variance, tvar.name, expected, defn) + def get_parameterized_base_classes(self, typ: TypeInfo) -> list[Instance]: + """Build an MRO-like structure with generic type args substituted. + + Excludes the class itself. + + When several bases have a common ancestor, includes an :class:`Instance` + for each param. + """ + bases = [] + for parent in typ.mro[1:]: + if parent.is_generic(): + for base in typ.bases: + if parent in base.type.mro: + bases.append(map_instance_to_supertype(base, parent)) + else: + bases.append(Instance(parent, [])) + return bases + def check_multiple_inheritance(self, typ: TypeInfo) -> None: """Check for multiple inheritance related errors.""" if len(typ.bases) <= 1: @@ -2746,17 +2764,7 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None: return # Verify that inherited attributes are compatible. - # Construct a "typed" MRO that follows regular MRO order, but includes instances - # parametrized with their generic args. - # This detects e.g. `class A(Mapping[int, str], Iterable[str])` correctly. - # For each MRO entry, include it parametrized according to each base inheriting - # from it. - typed_mro = [ - map_instance_to_supertype(base, parent) - for parent in typ.mro[1:] - for base in typ.bases - if parent in base.type.mro - ] + typed_mro = self.get_parameterized_base_classes(typ) # If the first MRO entry is compatible with everything following, we don't need # (and shouldn't) compare further pairs # (see testMultipleInheritanceExplcitDiamondResolution) From c81c38d1a88a81236c09e673c018daa79b0c6c83 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Tue, 10 Dec 2024 21:28:19 +0100 Subject: [PATCH 10/10] Include a testcase from review --- test-data/unit/check-generic-subtyping.test | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test-data/unit/check-generic-subtyping.test b/test-data/unit/check-generic-subtyping.test index c98ae238fc12..efe427e5580b 100644 --- a/test-data/unit/check-generic-subtyping.test +++ b/test-data/unit/check-generic-subtyping.test @@ -1090,3 +1090,18 @@ class C5(A2[U], B[T]): pass # E: Definition of "fn" in base class "A" is incomp # E: Definition of "x" in base class "A" is incompatible with definition in base class "B" [builtins fixtures/tuple.pyi] + +[case testMultipleInheritanceNestedTypeVarPropagation] +from typing import Generic, TypeVar + +T = TypeVar("T") + +class A(Generic[T]): + foo: T +class B(A[str]): ... +class C(B): ... +class D(C): ... + +class Bad(D, A[T]): ... # E: Definition of "foo" in base class "A" is incompatible with definition in base class "A" +class Good(D, A[str]): ... # OK +[builtins fixtures/tuple.pyi]