-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not ignore generic type args when checking multiple inheritance compatibility #18270
base: master
Are you sure you want to change the base?
Changes from 9 commits
e067603
d9147e0
1960ad9
46ec61b
6b09c52
bdca65a
94bc355
4710651
4323ed7
f06b2b3
c81c38d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2744,21 +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. | ||
mro = typ.mro[1:] | ||
for i, base in enumerate(mro): | ||
# 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still confused by this line. Seems like we don't propogate real types to whole tree. We are using just a current class bases, so it propogates options to class+2 level maximum class A[T]: ...
class B[T](A[T]): ... # <- str propagated here only
class C(B[str]): ...
class D(C): ... I am afraid, that A doesn't know about str in D class analyzing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Case could be even harder: class A[T]: ...
class B(A[str]): ...
class C(B): ...
class D(C): ... Does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the first case
In the second case it is (also for
And the following raises a diagnostic as expected: 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 Foo(D, A[T]): ... # E: Definition of "foo" in base class "A" is incompatible with definition in base class "A" ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I am new in mypy 😄 and don't now, how |
||
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. | ||
non_overridden_attrs = base.names.keys() - typ.names.keys() | ||
# Sort for consistent messages order. | ||
non_overridden_attrs = sorted(typed_mro[i].type.names - typ.names.keys()) | ||
for name in non_overridden_attrs: | ||
if is_private(name): | ||
continue | ||
for base2 in mro[i + 1 :]: | ||
if name in seen_names: | ||
continue | ||
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). | ||
if name in base2.names and base2 not in base.mro: | ||
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, sym: SymbolTableNode) -> Type | None: | ||
if sym.type is not None: | ||
|
@@ -2783,8 +2806,23 @@ def determine_type_of_member(self, sym: SymbolTableNode) -> Type | None: | |
# 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: 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 +2847,9 @@ 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 = 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. | ||
|
@@ -2822,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, second_type, ctx, base2) | ||
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(): | ||
|
@@ -2834,8 +2871,8 @@ 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, 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): | ||
|
@@ -2844,7 +2881,7 @@ class C(B, A[int]): ... # this is unsafe because... | |
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_node = base2.type[name].node | ||
if ( | ||
isinstance(second_type, FunctionLike) | ||
and second_node is not None | ||
|
@@ -2854,22 +2891,22 @@ class C(B, A[int]): ... # this is unsafe because... | |
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) | ||
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: | ||
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 check_metaclass_compatibility(self, typ: TypeInfo) -> None: | ||
"""Ensures that metaclasses of all parent types are compatible.""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better, but I am still thinking, that this
typed_mro
should be builded at MRO calculation phase, not in checker.This logic should be used shared between all class checks, not compatibility-only, so it will be the code to copy between them, I afraid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, we can replace regular MRO to typed one at all? In could be breaking change for some checks, but the idea looks generally correct for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, does it deliver types to parent correctly in case, where we specify them not in the final inheritor?
Like in the follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, but I'd rather defer this to a follow-up PR. There are several more places (override checks) where a "typed MRO" would be handy, we should adjust them one by one - that'd make this PR a lot harder to review.
Those are of different types. Fundamentally,
TypeInfo
andInstance
are different entities that shouldn't be mixed up. It makes sense to me as a general idea, but now it would result in a very huge changeset I won't be able to handle correctly (and no one will enjoy reviewing). `mypyHope so,
map_instance_to_supertype
looks pretty robust. If I understand your snippet correctly, it's similar totestMultipleInheritanceCompatibleTypeVar
- generics are mapped there as expected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building that list is really fast, so there should be very little perf impact, if any. I'm not sure that it makes sense to cache it at all. This "typed MRO" is a property of an
Instance
, not of aTypeInfo
, so we'll build it for everyInstance
we have, but only use on a few occasions. It's actually important to build it parametrized precisely by last known arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if
property
is good (current codebase seems to follow "only trivial computations for properties" rule). A method would be more reasonable, but the problem is in dependencies between those files (note thatmypy.nodes
never importsmypy.types
- the dependency is inverted, and that was done for architectural reasons). TBH I think this should be a method ofTypeChecker
- does that sound good enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding other places - I have a strong feeling that other override-related issues would benefit from using typed MRO explicitly.
However, I looked through the issue tracker now, and seems like almost everything else is fine - the only problem I see right now is #6184 which can indeed be solved using "typed MRO" here:
mypy/mypy/checker.py
Lines 1983 to 1993 in 1497407
But you can just search for
.mro[1:]
inchecker.py
, some usages of it may be problematic.check_compatibility_all_supers
should also be using typed MRO if I understand it correctly. Anyfinal
checks, OTOH, don't need that and work with plain MRO just as good.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏿
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try to take care about #6184 if you are okay with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review! I extracted a helper method (and also stopped duplicating non-generic bases in typed MRO) and borrowed a testcase.
Sure, go ahead with that ticket, that would be great! Please also try to search the issue tracker, I might have missed other reports related to that one.