Skip to content

Loosen base class attribute compatibility checks when attribute is redefined #6585

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -1602,7 +1602,10 @@ def check_multiple_inheritance(self, typ: TypeInfo) -> None:
# Verify that inherited attributes are compatible.
mro = typ.mro[1:]
for i, base in enumerate(mro):
for name in base.names:
# 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()
for name in non_overridden_attrs:
for base2 in mro[i + 1:]:
# We only need to check compatibility of attributes from classes not
# in a subclass relationship. For subclasses, normal (single inheritance)
Expand Down Expand Up @@ -1867,6 +1870,9 @@ def check_compatibility_all_supers(self, lvalue: RefExpr, lvalue_type: Optional[
# Show only one error per variable
break

direct_bases = lvalue_node.info.direct_base_classes()
last_immediate_base = direct_bases[-1] if direct_bases else None

for base in lvalue_node.info.mro[1:]:
# Only check __slots__ against the 'object'
# If a base class defines a Tuple of 3 elements, a child of
Expand All @@ -1890,7 +1896,10 @@ def check_compatibility_all_supers(self, lvalue: RefExpr, lvalue_type: Optional[
# Only show one error per variable; even if other
# base classes are also incompatible
return True
break
Copy link
Member

@ilevkivskyi ilevkivskyi Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in order to maintain consistency with the current behavior, you still need this break after last immediate base.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: (non-immediate -> immediate).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my replies below.

if base is last_immediate_base:
# At this point, the attribute was found to be compatible with all
# immediate parents.
break
return False

def check_compatibility_super(self, lvalue: RefExpr, lvalue_type: Optional[Type],
Expand Down
11 changes: 10 additions & 1 deletion test-data/unit/check-classes.test
Original file line number Diff line number Diff line change
Expand Up @@ -4009,7 +4009,7 @@ class B(A):
main:4: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")
main:5: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")

[case testClassIgnoreType]
[case testClassIgnoreType_RedefinedAttributeAndGrandparentAttributeTypesNotIgnored]
class A:
x = 0
class B(A):
Expand All @@ -4018,6 +4018,15 @@ class C(B):
x = ''
[out]

[case testClassIgnoreType_RedefinedAttributeTypeIgnoredInChildren]
class A:
x = 0
class B(A):
x = '' # type: ignore
class C(B):
x = '' # type: ignore
[out]

[case testInvalidMetaclassStructure]
class X(type): pass
class Y(type): pass
Expand Down
117 changes: 116 additions & 1 deletion test-data/unit/check-multiple-inheritance.test
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ class B(A, int): pass
from typing import Callable, TypeVar
T = TypeVar('T')
class A(B, C):
def f(self): pass
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to note that this change was necessary so that the original error would still be triggered (since redefining the method f would mean that the compatibility checks which may trigger the error would be skipped, due to my other changes)

class B:
@dec
def f(self): pass
Expand Down Expand Up @@ -461,6 +461,121 @@ class Combo(Base2, Base1): ...
[out]
main:10: error: Definition of "NestedVar" in base class "Base2" is incompatible with definition in base class "Base1"

[case testMultipleInheritance_NestedVariableOverriddenWithCompatibleType]
from typing import TypeVar, Generic
T = TypeVar('T', covariant=True)
class GenericBase(Generic[T]):
pass
class Base1:
Nested: GenericBase['Base1']
class Base2:
Nested: GenericBase['Base2']
class A(Base1, Base2):
Nested: GenericBase['A']
[out]

[case testMultipleInheritance_NestedVariableOverriddenWithIncompatibleType1]
from typing import TypeVar, Generic
T = TypeVar('T', covariant=True)
class GenericBase(Generic[T]):
pass
class Base1:
Nested: GenericBase['Base1']
class Base2:
Nested: GenericBase['Base2']
class A(Base1, Base2):
Nested: GenericBase['Base1']
[out]
main:10: error: Incompatible types in assignment (expression has type "GenericBase[Base1]", base class "Base2" defined the type as "GenericBase[Base2]")

[case testMultipleInheritance_NestedVariableOverriddenWithIncompatibleType2]
from typing import TypeVar, Generic
T = TypeVar('T', covariant=True)
class GenericBase(Generic[T]):
pass
class Base1:
Nested: GenericBase['Base1']
class Base2:
Nested: GenericBase['Base2']
class A(Base1, Base2):
Nested: GenericBase['Base2']
[out]
main:10: error: Incompatible types in assignment (expression has type "GenericBase[Base2]", base class "Base1" defined the type as "GenericBase[Base1]")

[case testMultipleInheritance_NestedVariableOverriddenWithCompatibleType]
from typing import TypeVar, Generic
T = TypeVar('T', covariant=True)
class GenericBase(Generic[T]):
pass
class Base1:
Nested: GenericBase['Base1']
class Base2:
Nested: GenericBase['Base1']
class A(Base1, Base2):
Nested: GenericBase['Base1']
[out]

[case testMultipleInheritance_MethodDefinitionsCompatibleWithOverride]
from typing import TypeVar, Union
_T = TypeVar('_T')

class Flag:
def __or__(self: _T, other: _T) -> _T: ...

# int defines __or__ as:
# def __or__(self, n: int) -> int: ...
class IntFlag(int, Flag):
def __or__(self: _T, other: Union[int, _T]) -> _T: ...
[out]

[case testMultipleInheritance_MethodDefinitionsIncompatibleOverride]
from typing import TypeVar, Union
_T = TypeVar('_T')

class Flag:
def __or__(self: _T, other: _T) -> _T: ...

class IntFlag(int, Flag):
def __or__(self: _T, other: str) -> _T: ...
[out]
main:8: error: Argument 1 of "__or__" incompatible with supertype "Flag"

[case testMultipleInheritance_MethodDefinitionsCompatibleNoOverride]
from typing import TypeVar, Union
_T = TypeVar('_T')

class Flag:
def __or__(self: _T, other: _T) -> _T: ...

class IntFlag(int, Flag):
pass
[out]

[case testMultipleInheritance_MethodsReturningSelfCompatible]
class A(object):
def x(self) -> 'A':
return self

class B(object):
def x(self) -> 'B':
return self

class C(A, B):
def x(self) -> 'C':
return self

[case testMultipleInheritance_MethodsReturningSelfIncompatible]
class A(object):
def x(self) -> 'A':
return self

class B(object):
def x(self) -> 'B':
return self

class C(A, B): # E: Definition of "x" in base class "A" is incompatible with definition in base class "B"
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding all the test cases! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing.


[case testNestedVariableRefersToSubclassOfAnotherNestedClass]
class Mixin1:
class Meta:
Expand Down