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

Mypy disallows overriding an attribute with a property #4125

Closed
rowillia opened this issue Oct 16, 2017 · 16 comments · Fixed by #13475
Closed

Mypy disallows overriding an attribute with a property #4125

rowillia opened this issue Oct 16, 2017 · 16 comments · Fixed by #13475
Labels
bug mypy got something wrong false-positive mypy gave an error on correct code priority-0-high refactoring Changing mypy's internals topic-descriptors Properties, class vs. instance attributes

Comments

@rowillia
Copy link
Contributor

For example:

class Foo:
    def __init__(self) -> None:
        self.x = 42

class Bar(Foo):
    def __init__(self) -> None:
        self.y = 9000

    @property
    def x(self) -> int:
        print('got x!!!')
        return self.y

    @x.setter
    def x(self, value: int) -> None:
        print('set x!!!')
        self.y = value

This should be allowed as it doesn't violate LSP as long as x implements both a getter and a setter of the correct type.

@ilevkivskyi
Copy link
Member

Hm, interesting, this is already allowed for structural subtypes:

class Foo(Protocol):
    x: int

class Bar:
    def __init__(self) -> None:
        self.y = 9000

    @property
    def x(self) -> int:
        print('got x!!!')
        return self.y

    @x.setter
    def x(self, value: int) -> None:
        print('set x!!!')
        self.y = value

x: Foo = Bar() # OK

I think this is a bug, it should be allowed for nominal classes as well.

@ilevkivskyi ilevkivskyi added bug mypy got something wrong priority-1-normal labels Oct 16, 2017
@JukkaL
Copy link
Collaborator

JukkaL commented Oct 17, 2017

Yes, this is a bug. I was under the impression that this was already working.

@elazarg
Copy link
Contributor

elazarg commented Oct 17, 2017

Technically it does violate LSP; there should be a warning about the missing @x.deleter.

@emmatyping
Copy link
Collaborator

Yes, but if you add a deleter it still complains.

@JelleZijlstra
Copy link
Member

I don't think mypy should actually care about whether a deleter is present though, since deleting attributes is rare and if you do it your code is unlikely to be type-safe anyway.

@elazarg
Copy link
Contributor

elazarg commented Oct 18, 2017

True, but overriding an attribute with a property is also not so common, so a reminder that it's not complete might be useful.

@srittau
Copy link
Contributor

srittau commented Nov 19, 2018

There is a different error message when using property() as a function:

class Base:
    prop: bool

class Sub(Base):
    def _get_prop(self) -> bool:
        return False
    def _set_prop(self, value: bool) -> None:
        pass
    prop = property(_get_prop, _set_prop)
foo.py:9: error: Incompatible types in assignment (expression has type "property", base class "Base" defined the type as "bool")

@erickpeirson
Copy link

erickpeirson commented Aug 16, 2019

In mypy==0.720, if I slightly modify the example in #4125 (comment) (above) based on the documentation's guidance that:

Explicitly including a protocol as a base class is also a way of documenting that your class implements a particular protocol, and it forces mypy to verify that your class implementation is actually compatible with the protocol.

...I get the following behavior:

class Foo(Protocol):
    x: int

class Bar(Foo):
    def __init__(self) -> None:
        self.y = 9000

    @property            # Signature of "x" incompatible with supertype "Foo"
    def x(self) -> int:  # Signature of "x" incompatible with supertype "Foo"
        print('got x!!!')
        return self.y

    @x.setter
    def x(self, value: int) -> None:
        print('set x!!!')
        self.y = value

x: Foo = Bar()

I would expect this to be OK for the same reasons that the original protocol example (no inheritance) is OK.

@ilevkivskyi
Copy link
Member

Setting priority to high, see duplicate issue #6759 (comment) for motivation.

@ziima
Copy link

ziima commented Aug 7, 2020

I ran into a similar issue. Mypy seems to disallow overriding even a property, if not defined using a decorator:

# Annotations in here doesn't seem to matter.
def _not_implemented(*args, **kwargs):
    raise NotImplementedError


class AbstractClass:
    foo = property(_not_implemented, _not_implemented)


class MyClass(AbstractClass):
    # Here the mypy reports:
    # error: Signature of "foo" incompatible with supertype "AbstractClass"  [override]
    @property
    def foo(self) -> str:
        return 'Foo'

    @foo.setter
    def foo(self, value: str) -> None:
        pass

If AbstractClass.foo is defined with property as a decorator, it works fine.

@ramalho
Copy link

ramalho commented Jun 28, 2021

True, but overriding an attribute with a property is also not so common, so a reminder that it's not complete might be useful.

I disagree. Overriding an attribute with a property is a recommended best practice. You start your class with public attributes, and if a subclass needs some getter or setter logic, then you make it into a property. I agree that, pragmatically the lack of a deleter is not relevant in practice, and should be ignored.

The following advice appears in Alex Martelli's classic and highly influential Python in a Nutshell since the first edition:

The crucial importance of properties is that their existence makes it perfectly safe (and indeed advisable) for you to expose public data attributes as part of your class’s public interface. Should it ever become necessary, in future versions of your class or other classes that need to be polymorphic to it, to have some code executed when the attribute is referenced, rebound, or unbound, you know you will be able to change the plain attribute into a property and get the desired effect without any impact on any other code that uses your class (AKA “client code”). This lets you avoid goofy idioms, such as accessor and mutator methods, required by OO languages that lack properties or equivalent machinery.

some_instance.widget_count += 1

rather than being forced into contorted nests of accessors and mutators such as:

some_instance.set_widget_count(some_instance.get_widget_count() + 1)

@erictraut
Copy link

For what it's worth, pyright does not allow attributes to be overridden with properties or vice versa. Our reasoning is that the semantics for an attribute are not the same as a property. In the sample at the top of this issue, for example, the attribute would work with del but the property would not. From a type safety perspective, it is not safe to substitute one for the other.

@ramalho
Copy link

ramalho commented Jun 29, 2021

Thanks for your insight, @erictraut. That's yet another situation where "type safety" trumps established practices in Python. It makes me think everyone would be happier if there was a derivative language with static types—which was @JukkaL's original idea for Mypy. Yet another lesson to learn from TypeScript.

@chadrik
Copy link
Contributor

chadrik commented Jun 29, 2021 via email

@ramalho
Copy link

ramalho commented Jun 29, 2021

Same with adding attributes at runtime, which is a feature, not a bug of Python and dynamic languages in general. But with time, the widespread, uncritical use of type hints will make those language features become taboo, and then you have—in effect—a different language defined by different usage reinforced by tooling. Which is why a derivative with a different name communicates better. TypeScript, not JavaScript with types bolted on.

@GBeauregard
Copy link

GBeauregard commented Oct 18, 2021

There's a workaround that works for both mypy and pyright if you're willing to use descriptors. Method follows typing advice from https://twitter.com/AdamChainz/status/1450044899093618688

from __future__ import annotations

from typing import cast, overload


class YIntProp(int):
    @overload
    def __get__(self, obj: None, objtype: None) -> YIntProp:
        ...

    @overload
    def __get__(self, obj: object, objtype: type[object]) -> int:
        ...

    def __get__(
        self, obj: object | None, objtype: type[object] | None = None
    ) -> YIntProp | int:
        if obj is None:
            return self
        return cast(int, obj.__dict__["y"])

    def __set__(self, obj: object, value: int) -> None:
        obj.__dict__["y"] = value


class Foo:
    def __init__(self) -> None:
        self.x = 42


class Bar(Foo):
    x = YIntProp()

    def __init__(self) -> None:
        self.y = 9000


a = Bar()
print(a.x)
reveal_type(a.x)  # revealed as int

juergbi added a commit to apache/buildstream that referenced this issue Oct 27, 2021
mypy 0.730 is not compatible with Python 3.9, resulting in the following
bogus error message:

    python/mypy#4125
@AlexWaygood AlexWaygood added the topic-descriptors Properties, class vs. instance attributes label Mar 27, 2022
ilevkivskyi added a commit that referenced this issue Aug 22, 2022
Fixes #4125

Previously the code compared the original signatures for properties. Now we compare just the return types, similar to how we do it in `checkmember.py`.

Note that we still only allow invariant overrides, which is stricter that for regular variables that where we allow (unsafe) covariance.
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-0-high refactoring Changing mypy's internals topic-descriptors Properties, class vs. instance attributes
Projects
None yet
Development

Successfully merging a pull request may close this issue.