Skip to content
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

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

Conversation

@syastrov
Copy link
Contributor

commented Mar 24, 2019

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

Loosen base class attribute compatibility checks when attribute is re…
…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 #2619
@@ -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

This comment has been minimized.

Copy link
@syastrov

syastrov Mar 24, 2019

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

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

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Apr 2, 2019

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

@ilevkivskyi ilevkivskyi self-requested a review Apr 2, 2019

@gwrome gwrome referenced this pull request Apr 8, 2019
3 of 3 tasks complete
@ilevkivskyi
Copy link
Collaborator

left a comment

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

@@ -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 comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

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.

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")

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

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

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

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?

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

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.

return self

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

Thanks for adding all the test cases! 👍

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

Thank you for reviewing.

[out]



This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

Keep only one empty line between tests.

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

👍

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

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

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

👍

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

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

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 9, 2019

Collaborator

EDIT: (non-immediate -> immediate).

This comment has been minimized.

Copy link
@syastrov

syastrov Apr 10, 2019

Author Contributor

See my replies below.

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

@syastrov

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

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
Copy link
Collaborator

left a comment

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

@@ -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 comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

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.

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")

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 11, 2019

Collaborator

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

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
Copy link
Collaborator

left a comment

OK, LGTM now.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.