Repository: python/mypy (Python)

Issue:
Mypy disallows overriding an attribute with a property
For example:

```python
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.


Hints:
Hm, interesting, this is already allowed for structural subtypes:
```python
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.
Yes, this is a bug. I was under the impression that this was already working.
Technically it does violate LSP; there should be a warning about the missing `@x.deleter`.
Yes, but if you add a deleter it still complains.
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.
True, but overriding an attribute with a property is also not so common, so a reminder that it's not complete might be useful.
There is a different error message when using `property()` as a function:

```python
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")
```
In ``mypy==0.720``, if I slightly modify the example in https://github.com/python/mypy/issues/4125#issuecomment-337048971 (above) based on [the documentation's guidance](https://mypy.readthedocs.io/en/latest/protocols.html#defining-subprotocols-and-subclassing-protocols) 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:

```python
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.
Setting priority to high, see duplicate issue https://github.com/python/mypy/issues/6759#issuecomment-521695300 for motivation.
I ran into a similar issue. Mypy seems to disallow overriding even a property, if not defined using a decorator:

```python
# 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.
> 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)`

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.
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.
Isn’t the idea of deleting attributes antithetical to static typing?  If
I’m using mypy to check my code, deleting attributes is not something I
want to accommodate, especially at the detriment of a useful *typing*
feature like overriding an attribute with a property.  In fact, it’s not
hard to imagine that deleting an attribute should be considered a type
error.


On Mon, Jun 28, 2021 at 7:33 PM Luciano Ramalho ***@***.***>
wrote:

> Thanks for your insight, @erictraut <https://github.com/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
> <https://github.com/JukkaL>'s original idea for Mypy. Yet another lesson
> to learn from TypeScript.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/python/mypy/issues/4125#issuecomment-870139232>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAAPOE4FJXR4ZQFCKQLMKX3TVEIENANCNFSM4D7NBVTQ>
> .
>

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.
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

```python
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
```

Patch:
diff --git a/mypy/checker.py b/mypy/checker.py
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -115,6 +115,7 @@
     ReturnStmt,
     StarExpr,
     Statement,
+    SymbolNode,
     SymbolTable,
     SymbolTableNode,
     TempNode,
@@ -1720,6 +1721,7 @@ def check_method_override_for_base_with_name(
                 context = defn.func
 
             # Construct the type of the overriding method.
+            # TODO: this logic is much less complete than similar one in checkmember.py
             if isinstance(defn, (FuncDef, OverloadedFuncDef)):
                 typ: Type = self.function_type(defn)
                 override_class_or_static = defn.is_class or defn.is_static
@@ -1769,15 +1771,37 @@ def check_method_override_for_base_with_name(
                 original_class_or_static = fdef.is_class or fdef.is_static
             else:
                 original_class_or_static = False  # a variable can't be class or static
+
+            if isinstance(original_type, FunctionLike):
+                original_type = self.bind_and_map_method(base_attr, original_type, defn.info, base)
+                if original_node and is_property(original_node):
+                    original_type = get_property_type(original_type)
+
+            if isinstance(typ, FunctionLike) and is_property(defn):
+                typ = get_property_type(typ)
+                if (
+                    isinstance(original_node, Var)
+                    and not original_node.is_final
+                    and (not original_node.is_property or original_node.is_settable_property)
+                    and isinstance(defn, Decorator)
+                ):
+                    # We only give an error where no other similar errors will be given.
+                    if not isinstance(original_type, AnyType):
+                        self.msg.fail(
+                            "Cannot override writeable attribute with read-only property",
+                            # Give an error on function line to match old behaviour.
+                            defn.func,
+                            code=codes.OVERRIDE,
+                        )
+
             if isinstance(original_type, AnyType) or isinstance(typ, AnyType):
                 pass
             elif isinstance(original_type, FunctionLike) and isinstance(typ, FunctionLike):
-                original = self.bind_and_map_method(base_attr, original_type, defn.info, base)
                 # Check that the types are compatible.
                 # TODO overloaded signatures
                 self.check_override(
                     typ,
-                    original,
+                    original_type,
                     defn.name,
                     name,
                     base.name,
@@ -1792,8 +1816,8 @@ def check_method_override_for_base_with_name(
                 #
                 pass
             elif (
-                base_attr.node
-                and not self.is_writable_attribute(base_attr.node)
+                original_node
+                and not self.is_writable_attribute(original_node)
                 and is_subtype(typ, original_type)
             ):
                 # If the attribute is read-only, allow covariance
@@ -4311,7 +4335,8 @@ def visit_decorator(self, e: Decorator) -> None:
                 if len([k for k in sig.arg_kinds if k.is_required()]) > 1:
                     self.msg.fail("Too many arguments for property", e)
             self.check_incompatible_property_override(e)
-        if e.func.info and not e.func.is_dynamic():
+        # For overloaded functions we already checked override for overload as a whole.
+        if e.func.info and not e.func.is_dynamic() and not e.is_overload:
             self.check_method_override(e)
 
         if e.func.info and e.func.name in ("__init__", "__new__"):
@@ -6066,6 +6091,8 @@ def conditional_types_with_intersection(
     def is_writable_attribute(self, node: Node) -> bool:
         """Check if an attribute is writable"""
         if isinstance(node, Var):
+            if node.is_property and not node.is_settable_property:
+                return False
             return True
         elif isinstance(node, OverloadedFuncDef) and node.is_property:
             first_item = cast(Decorator, node.items[0])
@@ -6973,6 +7000,23 @@ def is_static(func: FuncBase | Decorator) -> bool:
     assert False, f"Unexpected func type: {type(func)}"
 
 
+def is_property(defn: SymbolNode) -> bool:
+    if isinstance(defn, Decorator):
+        return defn.func.is_property
+    if isinstance(defn, OverloadedFuncDef):
+        if defn.items and isinstance(defn.items[0], Decorator):
+            return defn.items[0].func.is_property
+    return False
+
+
+def get_property_type(t: ProperType) -> ProperType:
+    if isinstance(t, CallableType):
+        return get_proper_type(t.ret_type)
+    if isinstance(t, Overloaded):
+        return get_proper_type(t.items[0].ret_type)
+    return t
+
+
 def is_subtype_no_promote(left: Type, right: Type) -> bool:
     return is_subtype(left, right, ignore_promotions=True)
 


Test patch:
diff --git a/test-data/unit/check-abstract.test b/test-data/unit/check-abstract.test
--- a/test-data/unit/check-abstract.test
+++ b/test-data/unit/check-abstract.test
@@ -789,7 +789,7 @@ class A(metaclass=ABCMeta):
     def x(self) -> int: pass
 class B(A):
     @property
-    def x(self) -> str: pass # E: Return type "str" of "x" incompatible with return type "int" in supertype "A"
+    def x(self) -> str: pass # E: Signature of "x" incompatible with supertype "A"
 b = B()
 b.x() # E: "str" not callable
 [builtins fixtures/property.pyi]
diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test
--- a/test-data/unit/check-classes.test
+++ b/test-data/unit/check-classes.test
@@ -7366,3 +7366,26 @@ class D(C[List[T]]): ...
 di: D[int]
 reveal_type(di)  # N: Revealed type is "Tuple[builtins.list[builtins.int], builtins.list[builtins.int], fallback=__main__.D[builtins.int]]"
 [builtins fixtures/tuple.pyi]
+
+[case testOverrideAttrWithSettableProperty]
+class Foo:
+    def __init__(self) -> None:
+        self.x = 42
+
+class Bar(Foo):
+    @property
+    def x(self) -> int: ...
+    @x.setter
+    def x(self, value: int) -> None: ...
+[builtins fixtures/property.pyi]
+
+[case testOverrideAttrWithSettablePropertyAnnotation]
+class Foo:
+    x: int
+
+class Bar(Foo):
+    @property
+    def x(self) -> int: ...
+    @x.setter
+    def x(self, value: int) -> None: ...
+[builtins fixtures/property.pyi]
diff --git a/test-data/unit/check-dataclasses.test b/test-data/unit/check-dataclasses.test
--- a/test-data/unit/check-dataclasses.test
+++ b/test-data/unit/check-dataclasses.test
@@ -1286,7 +1286,7 @@ class A:
 @dataclass
 class B(A):
     @property
-    def foo(self) -> int: pass  # E: Signature of "foo" incompatible with supertype "A"
+    def foo(self) -> int: pass
 
 reveal_type(B)  # N: Revealed type is "def (foo: builtins.int) -> __main__.B"
 
diff --git a/test-data/unit/check-inference.test b/test-data/unit/check-inference.test
--- a/test-data/unit/check-inference.test
+++ b/test-data/unit/check-inference.test
@@ -1475,9 +1475,8 @@ class A:
         self.x = [] # E: Need type annotation for "x" (hint: "x: List[<type>] = ...")
 
 class B(A):
-    # TODO?: This error is kind of a false positive, unfortunately
     @property
-    def x(self) -> List[int]:  # E: Signature of "x" incompatible with supertype "A"
+    def x(self) -> List[int]:  # E: Cannot override writeable attribute with read-only property
         return [123]
 [builtins fixtures/list.pyi]