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

Reject covariant overriding of attributes #3208

Closed
JukkaL opened this issue Apr 21, 2017 · 17 comments · Fixed by #16399
Closed

Reject covariant overriding of attributes #3208

JukkaL opened this issue Apr 21, 2017 · 17 comments · Fixed by #16399
Labels
priority-1-normal topic-inheritance Inheritance and incompatible overrides

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 21, 2017

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.

@ilevkivskyi
Copy link
Member

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.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Apr 21, 2017

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).

@ilevkivskyi
Copy link
Member

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.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Apr 21, 2017

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.

@ilevkivskyi
Copy link
Member

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.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Jun 4, 2019

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.

@hauntsaninja
Copy link
Collaborator

If we ever want to do this (which I suspect we may not), #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

@KotlinIsland
Copy link
Contributor

#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.

@tyralla
Copy link
Collaborator

tyralla commented Feb 14, 2023

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]
    ...

@ilevkivskyi
Copy link
Member

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.

@mikeshardmind
Copy link

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.

@JelleZijlstra
Copy link
Member

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.)

@mikeshardmind
Copy link

Having it configurable is problematic as this is a base behavior that other things need to be able to rely on being correct.

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 13, 2023

Can we add a new error code for this check? This way we wouldn't need an additional flag.

@hauntsaninja
Copy link
Collaborator

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 --strict and see what that looks like. And of course, no harm as having it be an opt-in error code.

@beauxq
Copy link

beauxq commented Oct 11, 2023

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 is not true because Final also prevents overriding.
python/typing#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

@alicederyn
Copy link

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.

ilevkivskyi added a commit that referenced this issue Nov 9, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-1-normal topic-inheritance Inheritance and incompatible overrides
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants