Skip to content

class with __setattr__ reports properties are read-only #9160

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

Closed
rhshadrach opened this issue Jul 16, 2020 · 6 comments · Fixed by #9196
Closed

class with __setattr__ reports properties are read-only #9160

rhshadrach opened this issue Jul 16, 2020 · 6 comments · Fixed by #9196
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal

Comments

@rhshadrach
Copy link

rhshadrach commented Jul 16, 2020

When a class implements __setattr__, mypy still reports properties as read-only.

Minimum example:

class Foo:
    def __init__(self) -> None:
        self._d = {'a': 1}

    def __setattr__(self, name: str, value: Any) -> None:
        if hasattr(self, '_d'):
            self._d[name] = value
        else:
            super(self.__class__, self).__setattr__(name, value)

    @property
    def a(self) -> int:
        return self._d['a']

f = Foo()
f.a = 2

The error points to the last line in the above code: error: Property "a" defined in "Foo" is read-only.

Expectation: Since Foo implements __setattr__, mypy should allow the attribute to be set, even though it overlaps with a class property.

There are no issues identified if the property a is removed, and I am using version 0.781.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 22, 2020

Yeah, this looks like false positive.

@JukkaL JukkaL added bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal labels Jul 22, 2020
@JukkaL
Copy link
Collaborator

JukkaL commented Jul 22, 2020

Anybody want to contribute a fix -- it should be easy?

Add a check about __setattr__ in mypy.checkmember.analyze_var (before the call to mx.msg.read_only_property).

@kamilturek
Copy link
Contributor

@JukkaL I can take care of this.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Sep 23, 2020

I reverted the fix #9196 in #9474. Copying my comment from the revert:

...

I realised that object in typeshed has __setattr__ defined. It's a pretty common idiom for marking read-only attributes using properties, and since every class has a __setattr__ in its MRO, this breaks that.

E.g, this would no longer produce an error:

from datetime import date
dt = date(2000, 1, 1)
dt.year = 2003

Additionally, it introduces a broken test on master, since we (unintentionally) don't run some Python2 tests in CI (filed as #9473 )

...

I'm not sure what the path forward is. Maybe it's okay to merge a version that doesn't check the MRO (as in the original PR), or maybe we exclude object's __setattr__ from the check. cc @kamilturek @JukkaL

@rhshadrach
Copy link
Author

rhshadrach commented Oct 7, 2020

Thanks for the details @hauntsaninja. Even if __setattr__ is defined on a class, it may still not allow setting properties, for which I can see users wanting mypy to still identify this as an error. I don't quite see a way for mypy to tell the difference here (but I know really nothing about how mypy internals work).

Here is my workaround; it allows for:

  • Settable properties to be handled using a single function (my use-case).
  • mypy to differentiate between a settable and non-settable property.

mypy will identify the last line as an error, but not the penultimate line.

def settable_property(s):
    def mysetattr(self, value):
        name = s.__name__
        if hasattr(self, f'_{name}'):
            setattr(self, f'_{name}', value)
        else:
            super(self.__class__, self).__setattr__(name, value)

    return property(s, mysetattr)


class Foo:
    def __init__(self) -> None:
        self._a = 1

    @settable_property
    def a(self) -> int:
        return self._a

    @property
    def b(self) -> int:
        return self._a


f = Foo()
f.a = 2
f.b = 2

@hauntsaninja
Copy link
Collaborator

I haven't seen much request for this feature in the intervening years, and I think fixing the false positive here would hurt many more common cases.

@hauntsaninja hauntsaninja closed this as not planned Won't fix, can't repro, duplicate, stale Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-1-normal
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants