Repository navigation
Add error code for mutable covariant override - #16399
ilevkivskyi merged 4 commits into
Conversation
Sorry, something went wrong.
hauntsaninja
left a comment
There was a problem hiding this comment.
We should accept:
class X:
x: float
class Y(X):
x = 5
When I made this same patch I did this by only doing the check if lvalue_type
Sorry, something went wrong.
This comment has been minimized.
|
Also #3208 (comment) is similar hole, might be worth opening separate issue for it |
Sorry, something went wrong.
Why should we allow this? x = 5 at class scope creates a new Var symbol (you can check that y: Y; reveal_type(y.x) will show an int), so it causes exactly the same unsafety (and also changing inference logic depending on an error code is a bad idea). The rules for whether we create a new Var or assign to the superclass' Var are these:
You may be tempted to change these rules, but believe me they are very old, and may break some unexpected things (especially in the daemon). Or did you mean we should allow self.x = 5? This should be already allowed (for exactly the rules I listed above, since there is just one Var, there is nothing to compare with in superclass). Btw, this is a very good thing to test.
Yes, this is a separate story, and there are problems not just for variables, but for methods as well. |
Sorry, something went wrong.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, I didn't realise x = 5 and self.x = 5 were different. Thanks for adding the test!
Then yes, this looks good to me! Note that if we ever want to turn this code on by default, we should consider the change... IIRC fixed several mypy_primer hits when I tried out my diff
Sorry, something went wrong.
Fixes #3208
Interestingly, we already prohibit this when the override is a mutable property (goes through FuncDef-related code), and in multiple inheritance. The logic there is not very principled, but I just left a TODO instead of extending the scope of this PR.