-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Attribute compatibility checks should take compatibility into consideration #2454
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
Comments
As an extension to this, maybe also attributes declared in the derived class could override it:
|
In the first example Y might have a method that expects attr to be a B, and if that method is called on a Z instance it might be disappointed. So maybe Z.attr should have type B? FWIW I tried with X inheriting from Y or vice versa, and both are allowed. That seems strange? |
True, but then you lose some type safety: class A: pass
class B(A): pass
class C(A): pass
class X:
attr: A
def make_c(self): self.attr = C()
class Y:
attr: B
def ensure_b(self):
assert isinstance(self.attr, B)
class Z(X, Y):
pass
z = Z()
z.make_c()
z.ensure_b() # BOOM!!! Maybe then it should just be like I said in the second comment, that the child class can explicitly set it to a type to silence the error? That way, it's up to the user to figure out what they want to do. |
So the current error is correct.
I think you should not be allowed to override it without `# type: ignore`.
But then we still have a bug when X derives from Y or Y from X.
|
Attribute compatibility across a class hierarchy is only safe if it's invariant, since attributes are mutable. This is similar to why It's a separate issue that mypy only detects attribute incompatibility when it's caused by multiple inheritance (see #2510). Closing this as the generated errors for the original examples seem correct, though perhaps we should document this more clearly. |
IMO this should be valid;
Z.attr
should be of typeA
, since it's the "lowest common denominator" in this case at retains type safety.Similar thing goes for unions, like this:
and this:
The text was updated successfully, but these errors were encountered: