From 4dad8e2fb2370d2cb8f57d2c0d02343c06596012 Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Sun, 21 Aug 2022 21:42:00 +0100 Subject: [PATCH 1/6] Allow overriding attribute with a settable property --- mypy/checker.py | 14 ++++++++++++-- test-data/unit/check-classes.test | 23 +++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 9fce0195626e..c5e2ec52224d 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1720,8 +1720,17 @@ 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) + if isinstance(defn, OverloadedFuncDef) and defn.is_property: + # Type for a settable property is stored in a non-trivial way. + first = defn.items[0] + assert isinstance(first, Decorator) + first_type = first.var.type + assert isinstance(first_type, ProperType) and isinstance(first_type, CallableType) + typ: Type = first_type.ret_type + else: + typ = self.function_type(defn) override_class_or_static = defn.is_class or defn.is_static override_class = defn.is_class else: @@ -4311,7 +4320,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__"): diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 8adf2e7ed5f1..71d967923b7f 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] From ee8c2f4091d94b42eb2dd2f42d7f605880d42686 Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Sun, 21 Aug 2022 22:58:06 +0100 Subject: [PATCH 2/6] Fix existing tests --- mypy/checker.py | 31 +++++++++++++++++++++------ test-data/unit/check-abstract.test | 2 +- test-data/unit/check-dataclasses.test | 2 +- test-data/unit/check-inference.test | 3 +-- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index c5e2ec52224d..bfafe05e9990 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1723,12 +1723,10 @@ def check_method_override_for_base_with_name( # TODO: this logic is much less complete than similar one in checkmember.py if isinstance(defn, (FuncDef, OverloadedFuncDef)): if isinstance(defn, OverloadedFuncDef) and defn.is_property: - # Type for a settable property is stored in a non-trivial way. - first = defn.items[0] - assert isinstance(first, Decorator) - first_type = first.var.type - assert isinstance(first_type, ProperType) and isinstance(first_type, CallableType) - typ: Type = first_type.ret_type + property_type = get_settable_property_type(defn) + # TODO: are this and below asserts correct, what if decorator was deferred? + assert property_type is not None + typ = property_type else: typ = self.function_type(defn) override_class_or_static = defn.is_class or defn.is_static @@ -1737,6 +1735,9 @@ def check_method_override_for_base_with_name( assert defn.var.is_ready assert defn.var.type is not None typ = defn.var.type + if defn.func.is_property: + assert isinstance(typ, ProperType) and isinstance(typ, CallableType) + typ = typ.ret_type override_class_or_static = defn.func.is_class or defn.func.is_static override_class = defn.func.is_class typ = get_proper_type(typ) @@ -1744,8 +1745,15 @@ def check_method_override_for_base_with_name( typ = bind_self(typ, self.scope.active_self_type(), is_classmethod=override_class) # Map the overridden method type to subtype context so that # it can be checked for compatibility. - original_type = get_proper_type(base_attr.type) + if isinstance(base_attr.node, OverloadedFuncDef) and base_attr.node.is_property: + original_type = get_settable_property_type(base_attr.node) + else: + original_type = base_attr.type + original_type = get_proper_type(original_type) original_node = base_attr.node + if isinstance(original_node, Decorator) and original_node.func.is_property: + if isinstance(original_type, CallableType): + original_type = get_proper_type(original_type.ret_type) # `original_type` can be partial if (e.g.) it is originally an # instance variable from an `__init__` block that becomes deferred. if original_type is None or isinstance(original_type, PartialType): @@ -6983,6 +6991,15 @@ def is_static(func: FuncBase | Decorator) -> bool: assert False, f"Unexpected func type: {type(func)}" +def get_settable_property_type(defn: OverloadedFuncDef) -> Type | None: + assert isinstance(defn.items[0], Decorator) + first_type = defn.items[0].var.type + if first_type is None: + return None + assert isinstance(first_type, ProperType) and isinstance(first_type, CallableType) + return first_type.ret_type + + 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-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..0a5f6e6bd365 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]: return [123] [builtins fixtures/list.pyi] From 97830e6380a5f150aa333f710efc64de73d65c0f Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Mon, 22 Aug 2022 00:11:13 +0100 Subject: [PATCH 3/6] Use a more robust way --- mypy/checker.py | 69 +++++++++++++++++------------ test-data/unit/check-inference.test | 2 +- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index bfafe05e9990..9a0ef03fa27f 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -115,6 +115,7 @@ ReturnStmt, StarExpr, Statement, + SymbolNode, SymbolTable, SymbolTableNode, TempNode, @@ -1722,22 +1723,13 @@ def check_method_override_for_base_with_name( # 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)): - if isinstance(defn, OverloadedFuncDef) and defn.is_property: - property_type = get_settable_property_type(defn) - # TODO: are this and below asserts correct, what if decorator was deferred? - assert property_type is not None - typ = property_type - else: - typ = self.function_type(defn) + typ: Type = self.function_type(defn) override_class_or_static = defn.is_class or defn.is_static override_class = defn.is_class else: assert defn.var.is_ready assert defn.var.type is not None typ = defn.var.type - if defn.func.is_property: - assert isinstance(typ, ProperType) and isinstance(typ, CallableType) - typ = typ.ret_type override_class_or_static = defn.func.is_class or defn.func.is_static override_class = defn.func.is_class typ = get_proper_type(typ) @@ -1745,15 +1737,8 @@ def check_method_override_for_base_with_name( typ = bind_self(typ, self.scope.active_self_type(), is_classmethod=override_class) # Map the overridden method type to subtype context so that # it can be checked for compatibility. - if isinstance(base_attr.node, OverloadedFuncDef) and base_attr.node.is_property: - original_type = get_settable_property_type(base_attr.node) - else: - original_type = base_attr.type - original_type = get_proper_type(original_type) + original_type = get_proper_type(base_attr.type) original_node = base_attr.node - if isinstance(original_node, Decorator) and original_node.func.is_property: - if isinstance(original_type, CallableType): - original_type = get_proper_type(original_type.ret_type) # `original_type` can be partial if (e.g.) it is originally an # instance variable from an `__init__` block that becomes deferred. if original_type is None or isinstance(original_type, PartialType): @@ -1786,15 +1771,33 @@ 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 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. + self.msg.fail( + "Cannot override writeable attribute with read-only property", defn + ) + 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, @@ -1810,7 +1813,7 @@ def check_method_override_for_base_with_name( pass elif ( base_attr.node - and not self.is_writable_attribute(base_attr.node) + and not self.is_writable_attribute(original_node) and is_subtype(typ, original_type) ): # If the attribute is read-only, allow covariance @@ -6084,6 +6087,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]) @@ -6991,13 +6996,21 @@ def is_static(func: FuncBase | Decorator) -> bool: assert False, f"Unexpected func type: {type(func)}" -def get_settable_property_type(defn: OverloadedFuncDef) -> Type | None: - assert isinstance(defn.items[0], Decorator) - first_type = defn.items[0].var.type - if first_type is None: - return None - assert isinstance(first_type, ProperType) and isinstance(first_type, CallableType) - return first_type.ret_type +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: diff --git a/test-data/unit/check-inference.test b/test-data/unit/check-inference.test index 0a5f6e6bd365..59e22d756df7 100644 --- a/test-data/unit/check-inference.test +++ b/test-data/unit/check-inference.test @@ -1475,7 +1475,7 @@ class A: self.x = [] # E: Need type annotation for "x" (hint: "x: List[] = ...") class B(A): - @property + @property # E: Cannot override writeable attribute with read-only property def x(self) -> List[int]: return [123] [builtins fixtures/list.pyi] From 1a08008953cd01b5774e7aa52b918c17c0cc1d3f Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Mon, 22 Aug 2022 00:30:48 +0100 Subject: [PATCH 4/6] Fix self-check --- mypy/checker.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index 9a0ef03fa27f..a6109e576fc0 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1774,7 +1774,7 @@ def check_method_override_for_base_with_name( if isinstance(original_type, FunctionLike): original_type = self.bind_and_map_method(base_attr, original_type, defn.info, base) - if is_property(original_node): + if original_node and is_property(original_node): original_type = get_property_type(original_type) if isinstance(typ, FunctionLike) and is_property(defn): @@ -1812,7 +1812,7 @@ def check_method_override_for_base_with_name( # pass elif ( - base_attr.node + original_node and not self.is_writable_attribute(original_node) and is_subtype(typ, original_type) ): From eb4dc4630a88e6542a275e317fd4fa4c1c31964a Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Mon, 22 Aug 2022 01:57:03 +0100 Subject: [PATCH 5/6] Adjust new error --- mypy/checker.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index a6109e576fc0..45da952549ec 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -1786,9 +1786,13 @@ def check_method_override_for_base_with_name( and isinstance(defn, Decorator) ): # We only give an error where no other similar errors will be given. - self.msg.fail( - "Cannot override writeable attribute with read-only property", defn - ) + 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 From f29edcad50637d8ea834fc5554bc8bd8443e1bc2 Mon Sep 17 00:00:00 2001 From: Ivan Levkivskyi Date: Mon, 22 Aug 2022 02:14:06 +0100 Subject: [PATCH 6/6] Update line number in a test --- test-data/unit/check-inference.test | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-data/unit/check-inference.test b/test-data/unit/check-inference.test index 59e22d756df7..fc6cb6fc456a 100644 --- a/test-data/unit/check-inference.test +++ b/test-data/unit/check-inference.test @@ -1475,8 +1475,8 @@ class A: self.x = [] # E: Need type annotation for "x" (hint: "x: List[] = ...") class B(A): - @property # E: Cannot override writeable attribute with read-only property - def x(self) -> List[int]: + @property + def x(self) -> List[int]: # E: Cannot override writeable attribute with read-only property return [123] [builtins fixtures/list.pyi]