From 40dd719a536589d375ce8ef6cf5f9c6588bbea29 Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Mon, 22 Aug 2022 13:40:35 +0100 Subject: [PATCH] Allow overriding attribute with a settable property (#13475) 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. --- mypy/checker.py | 54 ++++++++++++++++++++++++--- test-data/unit/check-abstract.test | 2 +- test-data/unit/check-classes.test | 23 ++++++++++++ test-data/unit/check-dataclasses.test | 2 +- test-data/unit/check-inference.test | 3 +- 5 files changed, 75 insertions(+), 9 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 9fce0195626e..45da952549ec 100644 --- 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) diff --git a/test-data/unit/check-abstract.test b/test-data/unit/check-abstract.test index e384cb89120b..e820a3a3c4fb 100644 --- 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 index 5a54f5e96e16..36f794dec780 100644 --- 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 index 6abb5597e464..d49a3a01e82d 100644 --- 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 index 04c710af10d1..fc6cb6fc456a 100644 --- 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[] = ...") 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]