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

syastrov · 2019-03-24T18:57:31Z

This eliminates the error Definition of "Nested" in base class "Base1" is incompatible with definition in base class "Base2" in the following code, where Nested is redefined in A:

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']

In the case of multiple inheritance, don't give errors about definitions of an
attribute in base classes being incompatible when the attribute is redefined.
The redefinition must itself be compatible with all (non-type-ignored)
definitions of the attribute in all base classes.
This is achieved by making the following change to checking of incompatible
types in assignments.
Don't stop checking after the first base where the attribute is defined
when checking for incompatible types in assignments.
There is still a maximum of one "Incompatible type in assignment" error
per assignment.

Resolves #2619

…defined - In the case of multiple inheritance, don't give errors about definitions of an attribute in base classes being incompatible when the attribute is redefined. The redefinition must itself be compatible with all (non-type-ignored) definitions of the attribute in all base classes. This is achieved by making the following change to checking of incompatible types in assignments. - Don't stop checking after the first base where the attribute is defined when checking for incompatible types in assignments. There is still a maximum of one "Incompatible type in assignment" error per assignment. Resolves python#2619

syastrov · 2019-03-24T19:24:50Z

test-data/unit/check-multiple-inheritance.test

@@ -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


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)

syastrov · 2019-03-28T17:37:11Z

Does anyone have any comments about the change in logic or code nitpicks? :)

ilevkivskyi · 2019-04-02T15:55:35Z

I just returned from vacation. I will likely take a look at this later this week.

ilevkivskyi

Thanks for the PR! Generally looks good. I have few suggestions/comments.

ilevkivskyi · 2019-04-09T19:42:09Z

test-data/unit/check-classes.test

@@ -3725,6 +3725,7 @@ class C(B):
    a = "a"
 [out]
 main:4: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")
+main:6: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")


This looks like a false positive. It can cause errors in code that already passes mypy.

I assume this is related to the break statement you removed.

Yes, it's related to the break statement being removed.

I wouldn't say that it is a false positive. C.a is incompatible with A.a, just as B.a is incompatible with A.a.

Mypy simply would not show you the error before. If you fixed the compatibility problem in B.a, then you would get an error about C.a. Isn't it unexpected to start getting new errors when mypy could have reported it the first time?

A user might want to ignore the existing error for some reasons (legacy interfaces, code owned by other team, etc.) Breaking their build will be not helpful, it will not discover any new type errors and will be just annoying.

ilevkivskyi · 2019-04-09T19:43:55Z

test-data/unit/check-classes.test

 class A:
    x = 0
 class B(A):
    x = ''  # type: ignore
 class C(B):
    x = ''
 [out]
+main:6: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")


Same as above. A user might want to ignore the error once in the base class, instead of ignoring it in every subclass.

This is slightly different than the above situation, as here, there was no error before.

I think it depends on how you want to interpret the # type: ignore.
In the current behavior, because B.x has an ignore, you are no longer warned when you redefine x in C.x that that redefinition is actually incompatible with ancestor's A.x.

A library author may have defined B.x with the ignore comment, and let us say that you are writing class C yourself. I would find it unexpected that because the library author chose to add an ignore comment on B.x that that would suppress a certain type of error when defining my own class.
I may not know in detail how B is defined/type-annotated, so I may reasonably expect that the type checker would perform its normal checks for my definition of class C (in this case, that it is compatible with A).

Are there other places in mypy where # type: ignore comments "spread" in that sense?

Oops:
~~(in this case, that it is compatible with A)~~
should be:
(in this case, that C.x is actually compatible with A.x)

Are there other places in mypy where # type: ignore comments "spread" in that sense?

Practically everywhere, an invalid type, a failed attribute access, an operator, or sometimes even failed function call, produce an Any type thus preventing subsequent errors.

ilevkivskyi · 2019-04-09T19:47:22Z

test-data/unit/check-multiple-inheritance.test

+        return self
+
+class C(A, B): # E: Definition of "x" in base class "A" is incompatible with definition in base class "B"
+    pass


Thanks for adding all the test cases! 👍

Thank you for reviewing.

ilevkivskyi · 2019-04-09T19:47:44Z

test-data/unit/check-multiple-inheritance.test

+[out]
+
+
+


Keep only one empty line between tests.

ilevkivskyi · 2019-04-09T19:48:26Z

test-data/unit/check-multiple-inheritance.test

@@ -461,6 +461,128 @@ 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]
+


No need for empty line after test case header (here and everywhere below).

ilevkivskyi · 2019-04-09T19:51:23Z

mypy/checker.py

@@ -1890,7 +1893,6 @@ 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


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

EDIT: (non-immediate -> immediate).

See my replies below.

syastrov · 2019-04-10T20:38:57Z

Thank you for the review.
I am not 100% convinced that the we should change the current behavior regarding incompatible types in assignment checks, although I wanted to challenge it and get your feedback, since it is a similar issue. Perhaps that part belongs in a separate issue though, and we can retain the original behavior here. What do you think?

ilevkivskyi

I didn't change my mind. Please don't add new false positives.

ilevkivskyi · 2019-04-11T16:56:31Z

test-data/unit/check-classes.test

@@ -3725,6 +3725,7 @@ class C(B):
    a = "a"
 [out]
 main:4: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")
+main:6: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")


A user might want to ignore the existing error for some reasons (legacy interfaces, code owned by other team, etc.) Breaking their build will be not helpful, it will not discover any new type errors and will be just annoying.

ilevkivskyi · 2019-04-11T16:59:37Z

test-data/unit/check-classes.test

 class A:
    x = 0
 class B(A):
    x = ''  # type: ignore
 class C(B):
    x = ''
 [out]
+main:6: error: Incompatible types in assignment (expression has type "str", base class "A" defined the type as "int")


Are there other places in mypy where # type: ignore comments "spread" in that sense?

Practically everywhere, an invalid type, a failed attribute access, an operator, or sometimes even failed function call, produce an Any type thus preventing subsequent errors.

syastrov · 2019-04-11T20:31:45Z

Having understood the code a bit more and reading your comments, I agree with you that this behavior should be kept, so I have restored it. I followed your suggestion, so it should still handle the case of multiple inheritance.

Thanks for taking the time to respond!

ilevkivskyi

OK, LGTM now.

syastrov commented Mar 24, 2019

View reviewed changes

syastrov mentioned this pull request Mar 24, 2019

52/model subtypes dont typecheck typeddjango/django-stubs#55

Merged

ilevkivskyi self-requested a review April 2, 2019 15:55

gwrome mentioned this pull request Apr 8, 2019

Fix type annotations in pandas.core.dtypes pandas-dev/pandas#26029

Merged

3 tasks

ilevkivskyi reviewed Apr 9, 2019

View reviewed changes

Remove extra newlines.

8adafdb

syastrov force-pushed the definition-in-base-class-incompatible-when-attr-redefined branch from 2c68eed to 8adafdb Compare April 10, 2019 20:37

ilevkivskyi reviewed Apr 11, 2019

View reviewed changes

Restore the current behavior regarding incompatible type in assignment.

3a38d4b

Style fixes

e807d81

ilevkivskyi approved these changes Apr 12, 2019

View reviewed changes

ilevkivskyi merged commit d920bd7 into python:master Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

syastrov commented Mar 24, 2019 •

edited

Loading

syastrov Mar 24, 2019

syastrov commented Mar 28, 2019

ilevkivskyi commented Apr 2, 2019

ilevkivskyi left a comment

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

syastrov Apr 10, 2019

ilevkivskyi Apr 11, 2019

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

syastrov Apr 10, 2019

ilevkivskyi Apr 11, 2019

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

ilevkivskyi Apr 9, 2019 •

edited

Loading

ilevkivskyi Apr 9, 2019

syastrov Apr 10, 2019

syastrov commented Apr 10, 2019

ilevkivskyi left a comment

ilevkivskyi Apr 11, 2019

ilevkivskyi Apr 11, 2019

syastrov commented Apr 11, 2019

ilevkivskyi left a comment

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

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

Conversation

syastrov commented Mar 24, 2019 • edited Loading

Choose a reason for hiding this comment

syastrov commented Mar 28, 2019

ilevkivskyi commented Apr 2, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syastrov commented Apr 10, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syastrov commented Apr 11, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

syastrov commented Mar 24, 2019 •

edited

Loading

ilevkivskyi Apr 9, 2019 •

edited

Loading