From de85cfa5ded672dd2992dd4da306298e5fd2d971 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Wed, 22 Jan 2025 00:10:56 +0100 Subject: [PATCH 01/10] Check property access from class object --- mypy/checkmember.py | 15 ++++++++++++++ mypy/semanal.py | 12 ++--------- mypy/types.py | 9 +++++++++ test-data/unit/check-classes.test | 30 ++++++++++++++++++++++++++++ test-data/unit/fixtures/property.pyi | 2 +- 5 files changed, 57 insertions(+), 11 deletions(-) diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 19ebe07b1032d..2ca9a48596627 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -38,6 +38,7 @@ is_final_node, ) from mypy.plugin import AttributeContext +from mypy.semanal import refers_to_fullname from mypy.typeops import ( bind_self, class_callable, @@ -50,6 +51,7 @@ type_object_type_from_function, ) from mypy.types import ( + PROPERTY_DECORATOR_NAMES, AnyType, CallableType, DeletedType, @@ -1110,6 +1112,19 @@ def analyze_class_attribute_access( # C[int].x -> int t = erase_typevars(expand_type_by_instance(t, isuper), {tv.id for tv in def_vars}) + if is_decorated: + property_deco_name = next( + ( + name + for d in cast(Decorator, node.node).original_decorators + for name in PROPERTY_DECORATOR_NAMES + if refers_to_fullname(d, name) + ), + None, + ) + if property_deco_name is not None: + return mx.named_type(property_deco_name) + is_classmethod = (is_decorated and cast(Decorator, node.node).func.is_class) or ( isinstance(node.node, FuncBase) and node.node.is_class ) diff --git a/mypy/semanal.py b/mypy/semanal.py index 034d8fb28b423..2c0d2107deba5 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -256,6 +256,7 @@ NEVER_NAMES, OVERLOAD_NAMES, OVERRIDE_DECORATOR_NAMES, + PROPERTY_DECORATOR_NAMES, PROTOCOL_NAMES, REVEAL_TYPE_NAMES, TPDICT_NAMES, @@ -1645,16 +1646,7 @@ def visit_decorator(self, dec: Decorator) -> None: removed.append(i) dec.func.is_explicit_override = True self.check_decorated_function_is_method("override", dec) - elif refers_to_fullname( - d, - ( - "builtins.property", - "abc.abstractproperty", - "functools.cached_property", - "enum.property", - "types.DynamicClassAttribute", - ), - ): + elif refers_to_fullname(d, PROPERTY_DECORATOR_NAMES): removed.append(i) dec.func.is_property = True dec.var.is_property = True diff --git a/mypy/types.py b/mypy/types.py index f3745695889fd..b4b8d7a1e9a7b 100644 --- a/mypy/types.py +++ b/mypy/types.py @@ -172,6 +172,15 @@ # Supported @override decorator names. OVERRIDE_DECORATOR_NAMES: Final = ("typing.override", "typing_extensions.override") +# Supported property decorators +PROPERTY_DECORATOR_NAMES: Final = ( + "builtins.property", + "abc.abstractproperty", + "functools.cached_property", + "enum.property", + "types.DynamicClassAttribute", +) + # A placeholder used for Bogus[...] parameters _dummy: Final[Any] = object() diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index d1c33c4729a92..97e27f0d5bd1f 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1665,6 +1665,36 @@ a.f = a.f # E: Property "f" defined in "A" is read-only a.f.x # E: "int" has no attribute "x" [builtins fixtures/property.pyi] +[case testPropertyAccessOnClass] +class Foo: + @property + def bar(self) -> bool: + return True + +reveal_type(Foo.bar) # N: Revealed type is "builtins.property" +reveal_type(Foo.bar(Foo())) # E: "property" not callable \ + # N: Revealed type is "Any" +[builtins fixtures/property.pyi] + +[case testPropertyAccessOnClass2] +import functools +from functools import cached_property + +class Foo: + @cached_property + def foo(self) -> bool: + return True + + @functools.cached_property + def bar(self) -> bool: + return True + +reveal_type(Foo.foo) # N: Revealed type is "functools.cached_property[Any]" +reveal_type(Foo.bar) # N: Revealed type is "functools.cached_property[Any]" +Foo.foo(Foo()) # E: "cached_property[Any]" not callable +Foo.bar(Foo()) # E: "cached_property[Any]" not callable +[builtins fixtures/property.pyi] + -- Descriptors -- ----------- diff --git a/test-data/unit/fixtures/property.pyi b/test-data/unit/fixtures/property.pyi index 667bdc02d0f58..76b4be589aca0 100644 --- a/test-data/unit/fixtures/property.pyi +++ b/test-data/unit/fixtures/property.pyi @@ -10,7 +10,7 @@ class type: class function: pass -property = object() # Dummy definition +class property: pass # Dummy definition class classmethod: pass class list: pass From 3dcf759c4ffa3909cfb504be0c1f1535c3fb74af Mon Sep 17 00:00:00 2001 From: STerliakov Date: Wed, 22 Jan 2025 00:36:18 +0100 Subject: [PATCH 02/10] Adjust the failing test --- test-data/unit/check-functions.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-data/unit/check-functions.test b/test-data/unit/check-functions.test index 18425efb9cb08..47c925dc08500 100644 --- a/test-data/unit/check-functions.test +++ b/test-data/unit/check-functions.test @@ -3276,7 +3276,7 @@ class A: @decorator def f(self) -> int: ... -reveal_type(A.f) # N: Revealed type is "__main__.something_callable" +reveal_type(A.f) # N: Revealed type is "builtins.property" reveal_type(A().f) # N: Revealed type is "builtins.str" [builtins fixtures/property.pyi] From 9c739b309c52fb14f89da3e9b7211a4c7508b5ff Mon Sep 17 00:00:00 2001 From: STerliakov Date: Tue, 28 Jan 2025 20:22:57 +0100 Subject: [PATCH 03/10] Reveal `property` at class level too --- mypy/checker.py | 29 +++++++++ mypy/checkexpr.py | 6 +- mypy/checkmember.py | 18 ++---- mypy/semanal.py | 2 + test-data/unit/check-classes.test | 72 ++++++++++++++++++++--- test-data/unit/fixtures/property-full.pyi | 43 ++++++++++++++ 6 files changed, 148 insertions(+), 22 deletions(-) create mode 100644 test-data/unit/fixtures/property-full.pyi diff --git a/mypy/checker.py b/mypy/checker.py index 5829b31447fef..eb78d263ec18a 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -170,6 +170,7 @@ ANY_STRATEGY, MYPYC_NATIVE_INT_NAMES, OVERLOAD_NAMES, + PROPERTY_DECORATOR_NAMES, AnyType, BoolTypeQuery, CallableType, @@ -2147,6 +2148,20 @@ def check_method_override_for_base_with_name( defn.func, code=codes.OVERRIDE, ) + elif ( + isinstance(defn, Decorator) + and isinstance(typ, Instance) + and typ.type.fullname == "builtins.property" + ): + self.msg.fail( + "Property setter/deletter overrides are not supported.", + defn.func, + code=codes.MISC, + ) + self.msg.note( + "Consider overriding getter explicitly with super() call.", defn.func + ) + return False if isinstance(original_type, AnyType) or isinstance(typ, AnyType): pass @@ -2215,6 +2230,20 @@ def check_method_override_for_base_with_name( ) return False + def get_property_instance(self, method: Decorator) -> Instance | None: + property_deco_name = next( + ( + name + for d in method.original_decorators + for name in PROPERTY_DECORATOR_NAMES + if refers_to_fullname(d, name) + ), + None, + ) + if property_deco_name is not None: + return self.named_type(property_deco_name) + return None + def bind_and_map_method( self, sym: SymbolTableNode, typ: FunctionLike, sub_info: TypeInfo, super_info: TypeInfo ) -> FunctionLike: diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index a10dc00bb1dea..11ac89c6957fa 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -409,7 +409,11 @@ def analyze_ref_expr(self, e: RefExpr, lvalue: bool = False) -> Type: # Reference to a module object. result = self.module_type(node) elif isinstance(node, Decorator): - result = self.analyze_var_ref(node.var, e) + property_type = self.chk.get_property_instance(node) + if property_type is not None: + result = property_type + else: + result = self.analyze_var_ref(node.var, e) elif isinstance(node, TypeAlias): # Something that refers to a type alias appears in runtime context. # Note that we suppress bogus errors for alias redefinitions, diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 2ca9a48596627..4bfb3d92ea3e2 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -38,7 +38,6 @@ is_final_node, ) from mypy.plugin import AttributeContext -from mypy.semanal import refers_to_fullname from mypy.typeops import ( bind_self, class_callable, @@ -51,7 +50,6 @@ type_object_type_from_function, ) from mypy.types import ( - PROPERTY_DECORATOR_NAMES, AnyType, CallableType, DeletedType, @@ -1112,18 +1110,10 @@ def analyze_class_attribute_access( # C[int].x -> int t = erase_typevars(expand_type_by_instance(t, isuper), {tv.id for tv in def_vars}) - if is_decorated: - property_deco_name = next( - ( - name - for d in cast(Decorator, node.node).original_decorators - for name in PROPERTY_DECORATOR_NAMES - if refers_to_fullname(d, name) - ), - None, - ) - if property_deco_name is not None: - return mx.named_type(property_deco_name) + if is_decorated and cast(Decorator, node.node).func.is_property: + property_type = mx.chk.get_property_instance(cast(Decorator, node.node)) + if property_type is not None: + return property_type is_classmethod = (is_decorated and cast(Decorator, node.node).func.is_class) or ( isinstance(node.node, FuncBase) and node.node.is_class diff --git a/mypy/semanal.py b/mypy/semanal.py index 2c0d2107deba5..a2912c8836dd1 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -7153,6 +7153,8 @@ def already_defined( self.fail( f'{noun} "{unmangle(name)}" already defined{extra_msg}', ctx, code=codes.NO_REDEF ) + if isinstance(node, Decorator) and node.func.is_property: + self.note("Property setter and deleter must be adjacent to the getter.", ctx) def name_already_defined( self, name: str, ctx: Context, original_ctx: SymbolTableNode | SymbolNode | None = None diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 97e27f0d5bd1f..643bafae783df 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1621,8 +1621,7 @@ class A: self.x = 1 self.x = '' # E: Incompatible types in assignment (expression has type "str", variable has type "int") return '' -[builtins fixtures/property.pyi] -[out] +[builtins fixtures/property-full.pyi] [case testDynamicallyTypedProperty] import typing @@ -1632,7 +1631,7 @@ class A: a = A() a.f.xx a.f = '' # E: Property "f" defined in "A" is read-only -[builtins fixtures/property.pyi] +[builtins fixtures/property-full.pyi] [case testPropertyWithSetter] import typing @@ -1649,7 +1648,7 @@ a.f.x # E: "int" has no attribute "x" a.f = '' # E: Incompatible types in assignment (expression has type "str", variable has type "int") a.f = 1 reveal_type(a.f) # N: Revealed type is "builtins.int" -[builtins fixtures/property.pyi] +[builtins fixtures/property-full.pyi] [case testPropertyWithDeleterButNoSetter] import typing @@ -1663,7 +1662,7 @@ class A: a = A() a.f = a.f # E: Property "f" defined in "A" is read-only a.f.x # E: "int" has no attribute "x" -[builtins fixtures/property.pyi] +[builtins fixtures/property-full.pyi] [case testPropertyAccessOnClass] class Foo: @@ -1671,10 +1670,13 @@ class Foo: def bar(self) -> bool: return True + reveal_type(bar) # N: Revealed type is "builtins.property" + reveal_type(Foo.bar) # N: Revealed type is "builtins.property" reveal_type(Foo.bar(Foo())) # E: "property" not callable \ # N: Revealed type is "Any" -[builtins fixtures/property.pyi] + +[builtins fixtures/property-full.pyi] [case testPropertyAccessOnClass2] import functools @@ -1689,11 +1691,67 @@ class Foo: def bar(self) -> bool: return True + reveal_type(foo) # N: Revealed type is "functools.cached_property[Any]" + reveal_type(bar) # N: Revealed type is "functools.cached_property[Any]" + reveal_type(Foo.foo) # N: Revealed type is "functools.cached_property[Any]" reveal_type(Foo.bar) # N: Revealed type is "functools.cached_property[Any]" Foo.foo(Foo()) # E: "cached_property[Any]" not callable Foo.bar(Foo()) # E: "cached_property[Any]" not callable -[builtins fixtures/property.pyi] +[builtins fixtures/property-full.pyi] + +[case testPropertySetterOverrideInCubclass] +# See https://github.com/python/mypy/issues/5936 +# Ideally we should support this, but at least be explicit that it isn't supported +class Base: + def __init__(self, value: int) -> None: + self._value = value + + @property + def value(self) -> int: + return self._value + + reveal_type(value) # N: Revealed type is "builtins.property" + reveal_type(value.setter) # N: Revealed type is "def (def (Any, Any)) -> builtins.property" + +class Sub(Base): + @Base.value.setter + def value(self, value: int) -> None: # E: Property setter/deletter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + +reveal_type(Base.value) # N: Revealed type is "builtins.property" +reveal_type(Base(0).value) # N: Revealed type is "builtins.int" +Base(0).value = 2 # E: Property "value" defined in "Base" is read-only + +reveal_type(Sub.value) # N: Revealed type is "Any" +reveal_type(Sub(0).value) # N: Revealed type is "Any" +Sub(0).value = 2 +[builtins fixtures/property-full.pyi] + +[case testPropertySetterNonAdjacent] +# See https://github.com/python/mypy/issues/1465 +# We want to support this later, but at least fail explicitly for now. +class A(object): + @property + def val(self) -> int: + return 0 + + reveal_type(val) # N: Revealed type is "builtins.property" + + def _other(self) -> None: ... + + @val.setter # E: Name "val" already defined on line 4 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self, val: int) -> None: + pass + + reveal_type(val) # N: Revealed type is "builtins.property" + +reveal_type(A.val) # N: Revealed type is "builtins.property" +reveal_type(A().val) # N: Revealed type is "builtins.int" +A().val = 1 # E: Property "val" defined in "A" is read-only +[builtins fixtures/property-full.pyi] -- Descriptors -- ----------- diff --git a/test-data/unit/fixtures/property-full.pyi b/test-data/unit/fixtures/property-full.pyi new file mode 100644 index 0000000000000..378a7b91028f5 --- /dev/null +++ b/test-data/unit/fixtures/property-full.pyi @@ -0,0 +1,43 @@ +from typing import Any, Callable, Generic, TypeVar + +_T = TypeVar('_T') + +class object: + def __init__(self) -> None: pass + +class type: + def __init__(self, x: Any) -> None: pass + +class function: pass + +class property: + fget: Callable[[Any], Any] | None + fset: Callable[[Any, Any], None] | None + fdel: Callable[[Any], None] | None + __isabstractmethod__: bool + + def __init__( + self, + fget: Callable[[Any], Any] | None = ..., + fset: Callable[[Any, Any], None] | None = ..., + fdel: Callable[[Any], None] | None = ..., + doc: str | None = ..., + ) -> None: ... + def getter(self, fget: Callable[[Any], Any], /) -> property: ... + def setter(self, fset: Callable[[Any, Any], None], /) -> property: ... + def deleter(self, fdel: Callable[[Any], None], /) -> property: ... + def __get__(self, instance: Any, owner: type | None = None, /) -> Any: ... + def __set__(self, instance: Any, value: Any, /) -> None: ... + def __delete__(self, instance: Any, /) -> None: ... +class classmethod: pass + +class list: pass +class dict: pass +class int: pass +class float: pass +class str: pass +class bytes: pass +class bool: pass +class ellipsis: pass + +class tuple(Generic[_T]): pass From 5f9e15a9f32770c4b8f16682424f634fbdb9f43c Mon Sep 17 00:00:00 2001 From: STerliakov Date: Tue, 28 Jan 2025 20:37:58 +0100 Subject: [PATCH 04/10] Only note if the replacement is also a Decorator --- mypy/semanal.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 4729e5f5a01d8..114e2df1fad73 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -7157,7 +7157,7 @@ def already_defined( self.fail( f'{noun} "{unmangle(name)}" already defined{extra_msg}', ctx, code=codes.NO_REDEF ) - if isinstance(node, Decorator) and node.func.is_property: + if isinstance(ctx, Decorator) and isinstance(node, Decorator) and node.func.is_property: self.note("Property setter and deleter must be adjacent to the getter.", ctx) def name_already_defined( From 8db72edd2477f8f68f262c9cae1ef767d132d230 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Wed, 29 Jan 2025 16:03:47 +0100 Subject: [PATCH 05/10] Handle overloaded case consistently --- mypy/checkexpr.py | 7 ++- mypy/checkmember.py | 9 ++- mypy/semanal.py | 10 +-- test-data/unit/check-classes.test | 100 +++++++++++++++++++++++++++++- 4 files changed, 116 insertions(+), 10 deletions(-) diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 2ab3af6222a0d..ed73654be51c4 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -378,12 +378,15 @@ def analyze_ref_expr(self, e: RefExpr, lvalue: bool = False) -> Type: # Reference to a global function. result = function_type(node, self.named_type("builtins.function")) elif isinstance(node, OverloadedFuncDef): + result = node.type if node.type is None: if self.chk.in_checked_function() and node.items: self.chk.handle_cannot_determine_type(node.name, e) result = AnyType(TypeOfAny.from_error) - else: - result = node.type + elif isinstance(node.items[0], Decorator): + property_type = self.chk.get_property_instance(node.items[0]) + if property_type is not None: + result = property_type elif isinstance(node, TypeInfo): # Reference to a type object. if node.typeddict_type: diff --git a/mypy/checkmember.py b/mypy/checkmember.py index 0d8d08162e396..3f7e746d60685 100644 --- a/mypy/checkmember.py +++ b/mypy/checkmember.py @@ -1122,8 +1122,13 @@ def analyze_class_attribute_access( # C[int].x -> int t = erase_typevars(expand_type_by_instance(t, isuper), {tv.id for tv in def_vars}) - if is_decorated and cast(Decorator, node.node).func.is_property: - property_type = mx.chk.get_property_instance(cast(Decorator, node.node)) + if isinstance(node.node, Decorator) and node.node.func.is_property: + property_type = mx.chk.get_property_instance(node.node) + if property_type is not None: + return property_type + if isinstance(node.node, OverloadedFuncDef) and node.node.is_property: + assert isinstance(node.node.items[0], Decorator) + property_type = mx.chk.get_property_instance(node.node.items[0]) if property_type is not None: return property_type diff --git a/mypy/semanal.py b/mypy/semanal.py index 114e2df1fad73..11134a0842fad 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -1212,9 +1212,6 @@ def visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: self.statement = defn self.add_function_to_symbol_table(defn) - if not self.recurse_into_functions: - return - # NB: Since _visit_overloaded_func_def will call accept on the # underlying FuncDefs, the function might get entered twice. # This is fine, though, because only the outermost function is @@ -7157,7 +7154,12 @@ def already_defined( self.fail( f'{noun} "{unmangle(name)}" already defined{extra_msg}', ctx, code=codes.NO_REDEF ) - if isinstance(ctx, Decorator) and isinstance(node, Decorator) and node.func.is_property: + if isinstance(ctx, Decorator) and ( + isinstance(node, Decorator) + and node.func.is_property + or isinstance(node, OverloadedFuncDef) + and node.is_property + ): self.note("Property setter and deleter must be adjacent to the getter.", ctx) def name_already_defined( diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 0d284efb48b6f..bdc502cd342f6 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1675,7 +1675,24 @@ class Foo: reveal_type(Foo.bar) # N: Revealed type is "builtins.property" reveal_type(Foo.bar(Foo())) # E: "property" not callable \ # N: Revealed type is "Any" +reveal_type(Foo.bar.fget(Foo())) # E: "None" not callable \ + # N: Revealed type is "Any" +class Bar: + @property + def bar(self) -> bool: + return True + @bar.setter + def bar(self, bar: bool) -> None: + pass + + reveal_type(bar) # N: Revealed type is "builtins.property" + +reveal_type(Bar.bar) # N: Revealed type is "builtins.property" +reveal_type(Bar.bar(Bar())) # E: "property" not callable \ + # N: Revealed type is "Any" +reveal_type(Bar.bar.fget(Bar())) # E: "None" not callable \ + # N: Revealed type is "Any" [builtins fixtures/property-full.pyi] [case testPropertyAccessOnClass2] @@ -1700,7 +1717,7 @@ Foo.foo(Foo()) # E: "cached_property[Any]" not callable Foo.bar(Foo()) # E: "cached_property[Any]" not callable [builtins fixtures/property-full.pyi] -[case testPropertySetterOverrideInCubclass] +[case testPropertySetterDefinedInSubclass] # See https://github.com/python/mypy/issues/5936 # Ideally we should support this, but at least be explicit that it isn't supported class Base: @@ -1713,6 +1730,8 @@ class Base: reveal_type(value) # N: Revealed type is "builtins.property" reveal_type(value.setter) # N: Revealed type is "def (def (Any, Any)) -> builtins.property" + reveal_type(value.fset) # N: Revealed type is "Union[def (Any, Any), None]" + reveal_type(value.fget) # N: Revealed type is "Union[def (Any) -> Any, None]" class Sub(Base): @Base.value.setter @@ -1720,6 +1739,14 @@ class Sub(Base): # N: Consider overriding getter explicitly with super() call. self._value = value +class Sub2(Base): + def value(self, value: int) -> None: # E: Signature of "value" incompatible with supertype "Base" \ + # N: Superclass: \ + # N: int \ + # N: Subclass: \ + # N: def value(self, value: int) -> None + self._value = value + reveal_type(Base.value) # N: Revealed type is "builtins.property" reveal_type(Base(0).value) # N: Revealed type is "builtins.int" Base(0).value = 2 # E: Property "value" defined in "Base" is read-only @@ -1729,10 +1756,43 @@ reveal_type(Sub(0).value) # N: Revealed type is "Any" Sub(0).value = 2 [builtins fixtures/property-full.pyi] +[case testPropertySetterOverrideInSubclass] +# See https://github.com/python/mypy/issues/5936 +# Ideally we should support this, but at least be explicit that it isn't supported +class Base: + def __init__(self, value: int) -> None: + self._value = value + + @property + def value(self) -> int: + return self._value + + @value.setter + def value(self, value: int) -> None: + self._value = value + + reveal_type(value) # N: Revealed type is "builtins.property" + reveal_type(value.setter) # N: Revealed type is "def (def (Any, Any)) -> builtins.property" + +class Sub(Base): + @Base.value.setter + def value(self, value: int) -> None: # E: Property setter/deletter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + +reveal_type(Base.value) # N: Revealed type is "builtins.property" +reveal_type(Base(0).value) # N: Revealed type is "builtins.int" +Base(0).value = 2 + +reveal_type(Sub.value) # N: Revealed type is "Any" +reveal_type(Sub(0).value) # N: Revealed type is "Any" +Sub(0).value = 2 +[builtins fixtures/property-full.pyi] + [case testPropertySetterNonAdjacent] # See https://github.com/python/mypy/issues/1465 # We want to support this later, but at least fail explicitly for now. -class A(object): +class A: @property def val(self) -> int: return 0 @@ -1748,6 +1808,42 @@ class A(object): reveal_type(val) # N: Revealed type is "builtins.property" +class B: + @property + def val(self) -> int: + return 0 + + @val.setter + def val(self, val: int) -> None: + pass + + reveal_type(val) # N: Revealed type is "builtins.property" + + def _other(self) -> None: ... + + @val.deleter # E: Name "val" already defined on line 19 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self) -> None: + pass + + reveal_type(val) # N: Revealed type is "builtins.property" + +class C: + @property + def val(self): + return 0 + + reveal_type(val) # N: Revealed type is "builtins.property" + + def _other(self) -> None: ... + + @val.setter # E: Name "val" already defined on line 38 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self, val: int) -> None: + pass + + reveal_type(val) # N: Revealed type is "builtins.property" + reveal_type(A.val) # N: Revealed type is "builtins.property" reveal_type(A().val) # N: Revealed type is "builtins.int" A().val = 1 # E: Property "val" defined in "A" is read-only From d46ba120f6b5d2b380c7d1a3a47917935f749be5 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Wed, 29 Jan 2025 17:19:11 +0100 Subject: [PATCH 06/10] Exit earlier for incremental mode, but still check properties --- mypy/semanal.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/mypy/semanal.py b/mypy/semanal.py index 11134a0842fad..4642bd74c9e73 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -1250,6 +1250,8 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: assert isinstance(typ, CallableType) types = [typ] else: + if not self.recurse_into_functions: + return # This is an a normal overload. Find the item signatures, the # implementation (if outside a stub), and any missing @overload # decorators. @@ -1267,6 +1269,9 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: elif not non_overload_indexes: self.handle_missing_overload_implementation(defn) + if not self.recurse_into_functions: + return + if types and not any( # If some overload items are decorated with other decorators, then # the overload type will be determined during type checking. From a8f44b976eb7cb108cfd818a4a336f6bd8e852da Mon Sep 17 00:00:00 2001 From: STerliakov Date: Thu, 30 Jan 2025 00:48:46 +0100 Subject: [PATCH 07/10] Revert change to recurse_into_functions, only analyze decorators on demand --- mypy/semanal.py | 34 +++++++++++++++++++++--------- test-data/unit/check-classes.test | 35 ++++++++++++++++++++----------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 4642bd74c9e73..6d5d90251da10 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -1212,6 +1212,9 @@ def visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: self.statement = defn self.add_function_to_symbol_table(defn) + if not self.recurse_into_functions: + return + # NB: Since _visit_overloaded_func_def will call accept on the # underlying FuncDefs, the function might get entered twice. # This is fine, though, because only the outermost function is @@ -1250,8 +1253,6 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: assert isinstance(typ, CallableType) types = [typ] else: - if not self.recurse_into_functions: - return # This is an a normal overload. Find the item signatures, the # implementation (if outside a stub), and any missing @overload # decorators. @@ -1269,9 +1270,6 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: elif not non_overload_indexes: self.handle_missing_overload_implementation(defn) - if not self.recurse_into_functions: - return - if types and not any( # If some overload items are decorated with other decorators, then # the overload type will be determined during type checking. @@ -7159,14 +7157,30 @@ def already_defined( self.fail( f'{noun} "{unmangle(name)}" already defined{extra_msg}', ctx, code=codes.NO_REDEF ) - if isinstance(ctx, Decorator) and ( - isinstance(node, Decorator) - and node.func.is_property - or isinstance(node, OverloadedFuncDef) - and node.is_property + + if ( + isinstance(ctx, Decorator) + and node is not None + and self.maybe_property_definition(node) ): self.note("Property setter and deleter must be adjacent to the getter.", ctx) + def maybe_property_definition(self, node: SymbolNode) -> bool: + if isinstance(node, Decorator) and node.func.is_property: + return True + elif isinstance(node, OverloadedFuncDef): + if node.is_property: + # Already analyzed + return True + elif isinstance(node.items[0], Decorator): + for dec in node.items[0].decorators: + if isinstance(dec, (NameExpr, MemberExpr)): + if not dec.fullname: + self.accept(dec) + if dec.fullname in PROPERTY_DECORATOR_NAMES: + return True + return False + def name_already_defined( self, name: str, ctx: Context, original_ctx: SymbolTableNode | SymbolNode | None = None ) -> None: diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index bdc502cd342f6..b0a351362f96d 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1803,8 +1803,7 @@ class A: @val.setter # E: Name "val" already defined on line 4 \ # N: Property setter and deleter must be adjacent to the getter. - def val(self, val: int) -> None: - pass + def val(self, val: int) -> None: ... reveal_type(val) # N: Revealed type is "builtins.property" @@ -1812,19 +1811,16 @@ class B: @property def val(self) -> int: return 0 - @val.setter - def val(self, val: int) -> None: - pass + def val(self, val: int) -> None: ... reveal_type(val) # N: Revealed type is "builtins.property" def _other(self) -> None: ... - @val.deleter # E: Name "val" already defined on line 19 \ + @val.deleter # E: Name "val" already defined on line 18 \ # N: Property setter and deleter must be adjacent to the getter. - def val(self) -> None: - pass + def val(self) -> None: ... reveal_type(val) # N: Revealed type is "builtins.property" @@ -1835,12 +1831,27 @@ class C: reveal_type(val) # N: Revealed type is "builtins.property" - def _other(self) -> None: ... + def _other(self): ... - @val.setter # E: Name "val" already defined on line 38 \ + @val.setter # E: Name "val" already defined on line 34 \ # N: Property setter and deleter must be adjacent to the getter. - def val(self, val: int) -> None: - pass + def val(self, new): ... + + reveal_type(val) # N: Revealed type is "builtins.property" + +class D: + @property + def val(self): ... + @val.setter + def val(self, val): ... + + reveal_type(val) # N: Revealed type is "builtins.property" + + def _other(self) -> None: ... + + @val.deleter # E: Name "val" already defined on line 48 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self): ... reveal_type(val) # N: Revealed type is "builtins.property" From 6ab9453120800410ee4cdfea70017eaa6f08448d Mon Sep 17 00:00:00 2001 From: STerliakov Date: Thu, 30 Jan 2025 03:01:47 +0100 Subject: [PATCH 08/10] This should work for all redefinitions --- mypy/checker.py | 53 ++++++++++---- mypy/semanal.py | 2 +- test-data/unit/check-classes.test | 111 +++++++++++++++++++++++------- 3 files changed, 128 insertions(+), 38 deletions(-) diff --git a/mypy/checker.py b/mypy/checker.py index a1667d8759a4f..0f71079e7080d 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -642,6 +642,13 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None: if not defn.items: # In this case we have already complained about none of these being # valid overloads. + # We only want to add more helpful note if possible. + if defn.info and defn.unanalyzed_items: + first_item = defn.unanalyzed_items[0] + if isinstance(first_item, Decorator): + for base in defn.info.mro[1:]: + if defn.name in base.names: + self.check_property_component_override(first_item, defn.name, base) return if len(defn.items) == 1: self.fail(message_registry.MULTIPLE_OVERLOADS_REQUIRED, defn) @@ -2035,6 +2042,7 @@ def check_method_or_accessor_override_for_base( if base: name = defn.name base_attr = base.names.get(name) + is_bad_property = False if base_attr: # First, check if we override a final (always an error, even with Any types). if is_final_node(base_attr.node) and not is_private(name): @@ -2042,8 +2050,10 @@ def check_method_or_accessor_override_for_base( # Second, final can't override anything writeable independently of types. if defn.is_final: self.check_if_final_var_override_writable(name, base_attr.node, defn) + if isinstance(defn, (OverloadedFuncDef, Decorator)): + is_bad_property = self.check_property_component_override(defn, name, base) found_base_method = True - if check_override_compatibility: + if not is_bad_property and check_override_compatibility: # Check compatibility of the override signature # (__init__, __new__, __init_subclass__ are special). if self.check_method_override_for_base_with_name(defn, name, base): @@ -2097,6 +2107,35 @@ def check_setter_type_override( if not is_subtype(original_type, typ): self.msg.incompatible_setter_override(defn.items[1], typ, original_type, base) + def check_property_component_override( + self, defn: OverloadedFuncDef | Decorator, name: str, base: TypeInfo + ) -> bool: + """Can the override refer to property setter/deleter?""" + if isinstance(defn, OverloadedFuncDef): + if isinstance(defn.items[0], Decorator): + return self.check_property_component_override(defn.items[0], name, base) + return False + + deco_type = next( + ( + deco.name + for deco in defn.decorators + if isinstance(deco, MemberExpr) and deco.name in ("setter", "deleter") + ), + None, + ) + if deco_type is None: + return False + + base_attr = base.names.get(name) + if not base_attr or base_attr.node is None or not is_property(base_attr.node): + return False + self.msg.fail( + f"Property {deco_type} overrides are not supported.", defn.func, code=codes.MISC + ) + self.msg.note("Consider overriding getter explicitly with super() call.", defn.func) + return True + def check_method_override_for_base_with_name( self, defn: FuncDef | OverloadedFuncDef | Decorator, name: str, base: TypeInfo ) -> bool: @@ -2210,16 +2249,6 @@ def check_method_override_for_base_with_name( defn.func, code=codes.OVERRIDE, ) - elif ( - isinstance(defn, Decorator) - and isinstance(typ, Instance) - and typ.type.fullname == "builtins.property" - ): - self.msg.fail( - "Property setter/deletter overrides are not supported.", defn.func, code=codes.MISC - ) - self.msg.note("Consider overriding getter explicitly with super() call.", defn.func) - return False if isinstance(original_type, AnyType) or isinstance(typ, AnyType): pass @@ -5310,7 +5339,7 @@ def visit_decorator_inner(self, e: Decorator, allow_empty: bool = False) -> None # For overloaded functions we already checked override for overload as a whole. if allow_empty: return - if e.func.info and not e.func.is_dynamic() and not e.is_overload: + if e.func.info and not e.is_overload: found_method_base_classes = self.check_method_override(e) if ( e.func.is_explicit_override diff --git a/mypy/semanal.py b/mypy/semanal.py index 6d5d90251da10..9fca20b4f2b38 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -7159,7 +7159,7 @@ def already_defined( ) if ( - isinstance(ctx, Decorator) + isinstance(ctx, (OverloadedFuncDef, Decorator)) and node is not None and self.maybe_property_definition(node) ): diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index b0a351362f96d..97fd3baf29f06 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1721,12 +1721,9 @@ Foo.bar(Foo()) # E: "cached_property[Any]" not callable # See https://github.com/python/mypy/issues/5936 # Ideally we should support this, but at least be explicit that it isn't supported class Base: - def __init__(self, value: int) -> None: - self._value = value - @property def value(self) -> int: - return self._value + return 0 reveal_type(value) # N: Revealed type is "builtins.property" reveal_type(value.setter) # N: Revealed type is "def (def (Any, Any)) -> builtins.property" @@ -1735,38 +1732,36 @@ class Base: class Sub(Base): @Base.value.setter - def value(self, value: int) -> None: # E: Property setter/deletter overrides are not supported. \ + def value(self, value: int) -> None: # E: Property setter overrides are not supported. \ # N: Consider overriding getter explicitly with super() call. self._value = value class Sub2(Base): - def value(self, value: int) -> None: # E: Signature of "value" incompatible with supertype "Base" \ - # N: Superclass: \ - # N: int \ - # N: Subclass: \ - # N: def value(self, value: int) -> None + @Base.value.setter + def value(self, value: int) -> None: # E: Property setter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. self._value = value + @Base.value.deleter # E: Name "value" already defined on line 19 + def value(self) -> None: ... reveal_type(Base.value) # N: Revealed type is "builtins.property" -reveal_type(Base(0).value) # N: Revealed type is "builtins.int" -Base(0).value = 2 # E: Property "value" defined in "Base" is read-only +reveal_type(Base().value) # N: Revealed type is "builtins.int" +Base().value = 2 # E: Property "value" defined in "Base" is read-only reveal_type(Sub.value) # N: Revealed type is "Any" -reveal_type(Sub(0).value) # N: Revealed type is "Any" -Sub(0).value = 2 +reveal_type(Sub().value) # N: Revealed type is "Any" +Sub().value = 2 [builtins fixtures/property-full.pyi] [case testPropertySetterOverrideInSubclass] # See https://github.com/python/mypy/issues/5936 # Ideally we should support this, but at least be explicit that it isn't supported class Base: - def __init__(self, value: int) -> None: - self._value = value + _value: int @property def value(self) -> int: return self._value - @value.setter def value(self, value: int) -> None: self._value = value @@ -1776,17 +1771,71 @@ class Base: class Sub(Base): @Base.value.setter - def value(self, value: int) -> None: # E: Property setter/deletter overrides are not supported. \ + def value(self, value: int) -> None: # E: Property setter overrides are not supported. \ # N: Consider overriding getter explicitly with super() call. self._value = value +class Sub2(Base): + @Base.value.setter + def value(self, value: int) -> None: # E: Property setter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + @Base.value.deleter # E: Name "value" already defined on line 22 + def value(self) -> None: ... + reveal_type(Base.value) # N: Revealed type is "builtins.property" -reveal_type(Base(0).value) # N: Revealed type is "builtins.int" -Base(0).value = 2 +reveal_type(Base().value) # N: Revealed type is "builtins.int" +Base().value = 2 reveal_type(Sub.value) # N: Revealed type is "Any" -reveal_type(Sub(0).value) # N: Revealed type is "Any" -Sub(0).value = 2 +reveal_type(Sub().value) # N: Revealed type is "Any" +Sub().value = 2 +[builtins fixtures/property-full.pyi] + +[case testUntypedPropertySetterOverrideInSubclass] +# See https://github.com/python/mypy/issues/5936 +# Ideally we should support this, but at least be explicit that it isn't supported +class Base: + _value: int + + @property + def value(self): + return self._value + @value.setter + def value(self, value): + self._value = value + + reveal_type(value) # N: Revealed type is "builtins.property" + reveal_type(value.setter) # N: Revealed type is "def (def (Any, Any)) -> builtins.property" + +class Sub(Base): + @Base.value.setter + def value(self, value): # E: Property setter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + +class Sub2(Base): + @Base.value.deleter + def value(self): # E: Property deleter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + +class Sub3(Base): + @Base.value.setter + def value(self, value): # E: Property setter overrides are not supported. \ + # N: Consider overriding getter explicitly with super() call. + self._value = value + @Base.value.deleter # E: Name "value" already defined on line 27 + def value(self): + self._value = value + +reveal_type(Base.value) # N: Revealed type is "builtins.property" +reveal_type(Base().value) # N: Revealed type is "Any" +Base().value = 2 + +reveal_type(Sub.value) # N: Revealed type is "Any" +reveal_type(Sub().value) # N: Revealed type is "Any" +Sub().value = 2 [builtins fixtures/property-full.pyi] [case testPropertySetterNonAdjacent] @@ -1855,9 +1904,21 @@ class D: reveal_type(val) # N: Revealed type is "builtins.property" -reveal_type(A.val) # N: Revealed type is "builtins.property" -reveal_type(A().val) # N: Revealed type is "builtins.int" -A().val = 1 # E: Property "val" defined in "A" is read-only +class E: + @property + def val(self): ... + + reveal_type(val) # N: Revealed type is "builtins.property" + + def _other(self) -> None: ... + + @val.setter # E: Name "val" already defined on line 63 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self, val): ... + @val.deleter # E: Name "val" already defined on line 70 + def val(self): ... + + reveal_type(val) # N: Revealed type is "builtins.property" [builtins fixtures/property-full.pyi] -- Descriptors From baf5528cf6a0b6e9348c5be3ae8eaf49abffa0de Mon Sep 17 00:00:00 2001 From: STerliakov Date: Thu, 30 Jan 2025 15:11:59 +0100 Subject: [PATCH 09/10] Only add note if we see .setter or .deleter --- mypy/semanal.py | 10 ++- test-data/unit/check-classes.test | 103 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 1 deletion(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 9fca20b4f2b38..72a17c3b87712 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -7159,12 +7159,20 @@ def already_defined( ) if ( - isinstance(ctx, (OverloadedFuncDef, Decorator)) + self.maybe_property_setter_or_deleter(ctx) and node is not None and self.maybe_property_definition(node) ): self.note("Property setter and deleter must be adjacent to the getter.", ctx) + def maybe_property_setter_or_deleter(self, node: SymbolNode) -> bool: + if isinstance(node, OverloadedFuncDef) and node.unanalyzed_items: + node = node.unanalyzed_items[0] + return isinstance(node, Decorator) and any( + isinstance(dec, MemberExpr) and dec.name in ("setter", "deleter") + for dec in node.decorators + ) + def maybe_property_definition(self, node: SymbolNode) -> bool: if isinstance(node, Decorator) and node.func.is_property: return True diff --git a/test-data/unit/check-classes.test b/test-data/unit/check-classes.test index 97fd3baf29f06..5513ec79af9e6 100644 --- a/test-data/unit/check-classes.test +++ b/test-data/unit/check-classes.test @@ -1919,6 +1919,109 @@ class E: def val(self): ... reveal_type(val) # N: Revealed type is "builtins.property" + +class F: + @property + def val(self): ... + + reveal_type(val) # N: Revealed type is "builtins.property" + def _other(self) -> None: ... + + @val.setter # E: Name "val" already defined on line 78 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self, val): ... + + reveal_type(val) # N: Revealed type is "builtins.property" + def _other2(self) -> None: ... + + @val.deleter # E: Name "val" already defined on line 78 \ + # N: Property setter and deleter must be adjacent to the getter. + def val(self): ... + + reveal_type(val) # N: Revealed type is "builtins.property" +[builtins fixtures/property-full.pyi] + +[case testPropertyRedefinition] +class A1: + @property + def fn(self) -> None: ... + + @property # E: Only supported top decorator is @fn.setter + def fn(self) -> None: ... + +class B1: + @property + def fn(self) -> None: ... + + def foo(self) -> None: ... + + @property # E: Name "fn" already defined on line 9 + def fn(self) -> None: ... + +class A2: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + @property # E: Only supported top decorator is @fn.setter + def fn(self) -> None: ... + +class B2: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + def foo(self) -> None: ... + + @property # E: Name "fn" already defined on line 27 + def fn(self) -> None: ... + +class A3: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + @property # E: Only supported top decorator is @fn.setter + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + +class B3: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + def foo(self) -> None: ... + + @property # E: Name "fn" already defined on line 49 + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + +class A4: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + @fn.setter + def fn(self, _: None) -> None: ... + +class B4: + @property + def fn(self) -> None: ... + @fn.setter + def fn(self, _: None) -> None: ... + + def foo(self) -> None: ... + + @fn.setter # E: Name "fn" already defined on line 71 \ + # N: Property setter and deleter must be adjacent to the getter. + def fn(self, _: None) -> None: ... [builtins fixtures/property-full.pyi] -- Descriptors From ef68217c10a74c5ac505f4b53dff5dc54c6525f9 Mon Sep 17 00:00:00 2001 From: STerliakov Date: Thu, 30 Jan 2025 16:31:10 +0100 Subject: [PATCH 10/10] Fix typing --- mypy/semanal.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mypy/semanal.py b/mypy/semanal.py index 72a17c3b87712..ec9aef27da4fe 100644 --- a/mypy/semanal.py +++ b/mypy/semanal.py @@ -7159,14 +7159,17 @@ def already_defined( ) if ( - self.maybe_property_setter_or_deleter(ctx) + isinstance(ctx, (OverloadedFuncDef, Decorator)) and node is not None + and self.maybe_property_setter_or_deleter(ctx) and self.maybe_property_definition(node) ): self.note("Property setter and deleter must be adjacent to the getter.", ctx) def maybe_property_setter_or_deleter(self, node: SymbolNode) -> bool: if isinstance(node, OverloadedFuncDef) and node.unanalyzed_items: + # Use unanalyzed_items: setter+deletter would have empty .items + # due to previous error node = node.unanalyzed_items[0] return isinstance(node, Decorator) and any( isinstance(dec, MemberExpr) and dec.name in ("setter", "deleter")