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

Add error code for mutable covariant override #16399

Merged
merged 4 commits into from Nov 9, 2023

Conversation

ilevkivskyi
Copy link
Member

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.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

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

This comment has been minimized.

@hauntsaninja
Copy link
Collaborator

Also #3208 (comment) is similar hole, might be worth opening separate issue for it

@ilevkivskyi
Copy link
Member Author

@hauntsaninja

We should accept:

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:

  • If there is an annotation, always create a new Var (for both x: int and self.x: int)
  • If there is no annotation, self.x = ... re-uses existing Var, but x = ... creates a new Var

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.

Also #3208 (comment) is similar hole, might be worth opening separate issue for it

Yes, this is a separate story, and there are problems not just for variables, but for methods as well.

Copy link
Contributor

github-actions bot commented Nov 4, 2023

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

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

@ilevkivskyi ilevkivskyi merged commit a1864d4 into python:master Nov 9, 2023
19 checks passed
@ilevkivskyi ilevkivskyi deleted the mutable-override branch November 9, 2023 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reject covariant overriding of attributes
2 participants