-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Reject covariant overriding of attributes #3208
Comments
I agree with the idea, but I think we should have a flag to allow this ( |
I don't agree with having a flag -- I don't see why this particular case of unsafety should have a flag dedicated to it. I don't expect that this will require many |
Probably a good measure for this would be to just test a future PR against mypy itself and Dropbox internal codebase. If there will be not many errors due to prohibiting covariant overriding of mutable attributes, then probably it is OK to not have a flag. |
We have previously had to make maybe hundreds of changes to Dropbox internal repos in response to fixes to mypy, so our bar for adding a backwards-compatibility flag is pretty high. |
Ah, OK, I see. |
This is less controversial now, since in many use cases it's possible to use |
If we ever want to do this (which I suspect we may not), #14588 points out that we should prohibit:
|
#14588 is simplified as: T = TypeVar("T", covariant=True)
class Foo(Generic[T]):
foo: T Which if you ask me is very dangerous and easy to accidentally write. a: Foo[int] = Foo()
b: Foo[object] = a
b.foo = "sus" I think this issue is actually #735, perhaps that description on that issue could be expanded with an example. |
Is covariant overriding of attributes already (possibly accidentally) rejected for multiple inheritance? class A:
x: float | str
class B(A):
x: float # no error reported
class C:
x: float
class D(B, C): # error: Definition of "x" in base class "A" is incompatible with definition in base class "C" [misc]
... |
FWIW I think we can try disabling this in strict mode, and see what will be the fallout in |
Just gonna bump this for attention, as it's come up as a meaningful issue with the design of intersections that type checkers are not currently handling this. There are probably real user-land bugs not being detected from improper subclassing currently as well, but it's led to questions that to some people seem more obvious with |
This also just came up in mypyc/mypyc#1019: evidently when compiling with mypyc, we already perform this check. A good next step for anyone interested in this would be to make a PR that unconditionally enables this check, then see how bad the fallout is in mypy-primer. Then we can decide how to handle this, maybe by adding a new flag or maybe by enabling it unconditionally. (I know Jukka above opposed adding a new flag, but there's a lot more typed Python code than when he wrote that, so turning this on unconditionally may be too disruptive now.) |
Having it configurable is problematic as this is a base behavior that other things need to be able to rely on being correct. |
Can we add a new error code for this check? This way we wouldn't need an additional flag. |
I implemented a version of this. I got about 300 errors in 49 / 131 projects in mypy_primer. While my implementation was pretty quick and dirty (there are soundness holes I didn't plug and I didn't do much dedicated testing), it matched the results of microsoft/pyright#5934 (pyright only runs on 27 of projects in mypy_primer). The only difference was we report a couple extra errors on steam.py, which looked correct to me (e.g. my patch complains about https://github.com/Gobot1234/steam.py/blob/336064153a1fea5c751a5c5f60a71c1afe4b0c7f/steam/_gc/client.py#L41 but pyright's patch does not). This is more disruptive than other disruptive changes I can remember, like the ones in 0.991. I also checked a couple of these and couldn't find any "real" bugs. So I would be opposed to having it on by default. I'll try scoping it to part of |
This is not true because
It would be nice to have something to indicate that a value can't be changed, but can be overridden. Some have suggested |
This would IMO be a very reasonable follow-up to PEP-705. |
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.
Covariant overriding of attributes is allowed though it's clearly unsafe. Example:
Mypy should reject
x: int
inB
. If this is actually what the programmer intended, they can use# type: ignore
.The text was updated successfully, but these errors were encountered: