Skip to content

Reject covariant overriding of attributes #3208

@JukkaL

Description

@JukkaL
Collaborator

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.

Activity

ilevkivskyi

ilevkivskyi commented on Apr 21, 2017

@ilevkivskyi
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

JukkaL commented on Apr 21, 2017

@JukkaL
CollaboratorAuthor

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

ilevkivskyi commented on Apr 21, 2017

@ilevkivskyi
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

JukkaL commented on Apr 21, 2017

@JukkaL
CollaboratorAuthor

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

ilevkivskyi commented on Apr 21, 2017

@ilevkivskyi
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

JukkaL commented on Jun 4, 2019

@JukkaL
CollaboratorAuthor

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.

18 remaining items

JelleZijlstra

JelleZijlstra commented on Sep 12, 2023

@JelleZijlstra
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

mikeshardmind commented on Sep 12, 2023

@mikeshardmind

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

JukkaL

JukkaL commented on Sep 13, 2023

@JukkaL
CollaboratorAuthor

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

hauntsaninja

hauntsaninja commented on Sep 14, 2023

@hauntsaninja
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

beauxq commented on Oct 11, 2023

@beauxq

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

alicederyn commented on Oct 26, 2023

@alicederyn

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      Participants

      @JelleZijlstra@alicederyn@JukkaL@beauxq@ilevkivskyi

      Issue actions

        Reject covariant overriding of attributes · Issue #3208 · python/mypy