mypy
mypy copied to clipboard
python
Reject covariant overriding of attributes
Covariant overriding of attributes is allowed though it's clearly unsafe. Example:
class A:
x: float
class B(A):
x: int # no error, though unsafe
def f(a: A) -> None:
a.x = 1.1
b = B()
f(b)
b.x # float at runtime, though declared as int
Mypy should reject x: int in B. If this is actually what the programmer intended, they can use # type: ignore.
I agree with the idea, but I think we should have a flag to allow this (--alow-mutable-overriding?), otherwise it could be a bit annoying to put # type: ignores. It will be easy to implement such a flag.
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 # type: ignores for most users, and a better fix is to not redefine attributes covariantly (or to use a read-only property).
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.
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 Final attributes, for which we can allow safe covariant overriding. This still wouldn't help when the attribute needs to be modified, but I believe that these use cases are somewhat rare, and it's still possible to use a # type: ignore comment as a workaround.
If we ever want to do this (which I suspect we may not), https://github.com/python/mypy/issues/14588 points out that we should prohibit:
T = TypeVar("T", covariant=True)
class Foo(Generic[T]):
def __init__(self, value: T) -> None:
self.foo = value
#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 mypy_primer. If we are going to actually prohibit it sooner or later, it is better to do it sooner.
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 Never in play, but that Never was not the true cause of.
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 https://github.com/microsoft/pyright/pull/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 --strict and see what that looks like. And of course, no harm as having it be an opt-in error code.
This is less controversial now, since in many use cases it's possible to use
Finalattributes, for which we can allow safe covariant overriding.
This is not true because Final also prevents overriding.
https://github.com/python/typing/discussions/1486
https://mypy.readthedocs.io/en/stable/final_attrs.html
You can use the typing.Final qualifier to indicate that a name or attribute should not be reassigned, redefined, or overridden.
It would be nice to have something to indicate that a value can't be changed, but can be overridden. Some have suggested typing.ReadOnly
It would be nice to have something to indicate that a value can't be changed, but can be overridden. Some have suggested typing.ReadOnly
This would IMO be a very reasonable follow-up to PEP-705.
