Consistently store settable property type (#18774)

Fixes #18764

There are two things important to understand this PR:
* First, mypy has a performance optimization - we store
decorator/overload type during semantic analysis in certain "trivial"
situations (to avoid deferrals). Otherwise, we infer the type of
decorated function or an overload variant during type checking.
* Second, for settable properties we store getter type in two places, as
a `Var.type` of getter (as a decorator), and also in overall
`OverloadedFuncDef.type`. The latter is ugly, but unfortunately it is
hard to get rid of, since some code in multiple plugins rely on this.

It turns out there are _three_ inconsistencies in how these two things
interact (first one causes the actual crash):
* For trivial settable properties (i.e. without extra decorators) when
we store the type in `semanal.py` we only store it the second way (i.e.
as `OverloadedFuncDef.type`).
* For non-trivial settable properties (where getter and/or setter are
themselves decorated), we only set the inferred type the first way (as
`Var.type`).
* When inferring setter type (unlike getter, that is handled correctly)
we actually ignore any extra decorators (this is probably quire niche,
but still inconsistent).

Essentially I simply remove these inconsistencies.

Author: ilevkivskyi

mypy/checker.py

@@ -654,23 +654,34 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
             assert isinstance(defn.items[0], Decorator)
             self.visit_decorator(defn.items[0])
             if defn.items[0].var.is_settable_property:
+                # TODO: here and elsewhere we assume setter immediately follows getter.
                 assert isinstance(defn.items[1], Decorator)
-                self.visit_func_def(defn.items[1].func)
-                setter_type = self.function_type(defn.items[1].func)
-                assert isinstance(setter_type, CallableType)
-                if len(setter_type.arg_types) != 2:
+                # Perform a reduced visit just to infer the actual setter type.
+                self.visit_decorator_inner(defn.items[1], skip_first_item=True)
+                setter_type = get_proper_type(defn.items[1].var.type)
+                # Check if the setter can accept two positional arguments.
+                any_type = AnyType(TypeOfAny.special_form)
+                fallback_setter_type = CallableType(
+                    arg_types=[any_type, any_type],
+                    arg_kinds=[ARG_POS, ARG_POS],
+                    arg_names=[None, None],
+                    ret_type=any_type,
+                    fallback=self.named_type("builtins.function"),
+                )
+                if setter_type and not is_subtype(setter_type, fallback_setter_type):
                     self.fail("Invalid property setter signature", defn.items[1].func)
-                    any_type = AnyType(TypeOfAny.from_error)
-                    setter_type = setter_type.copy_modified(
-                        arg_types=[any_type, any_type],
-                        arg_kinds=[ARG_POS, ARG_POS],
-                        arg_names=[None, None],
-                    )
+                if not isinstance(setter_type, CallableType) or len(setter_type.arg_types) != 2:
+                    # TODO: keep precise type for callables with tricky but valid signatures.
+                    setter_type = fallback_setter_type
                 defn.items[0].var.setter_type = setter_type
-        for fdef in defn.items:
+        for i, fdef in enumerate(defn.items):
             assert isinstance(fdef, Decorator)
             if defn.is_property:
-                self.check_func_item(fdef.func, name=fdef.func.name, allow_empty=True)
+                assert isinstance(defn.items[0], Decorator)
+                settable = defn.items[0].var.is_settable_property
+                # Do not visit the second time the items we checked above.
+                if (settable and i > 1) or (not settable and i > 0):
+                    self.check_func_item(fdef.func, name=fdef.func.name, allow_empty=True)
             else:
                 # Perform full check for real overloads to infer type of all decorated
                 # overload variants.
@@ -692,6 +703,13 @@ def _visit_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
                         item_types.append(item_type)
                 if item_types:
                     defn.type = Overloaded(item_types)
+        elif defn.type is None:
+            # We store the getter type as an overall overload type, as some
+            # code paths are getting property type this way.
+            assert isinstance(defn.items[0], Decorator)
+            var_type = get_proper_type(defn.items[0].var.type)
+            assert isinstance(var_type, CallableType)
+            defn.type = Overloaded([var_type])
         # Check override validity after we analyzed current definition.
         if defn.info:
             found_method_base_classes = self.check_method_override(defn)
@@ -5277,25 +5295,34 @@ def visit_decorator(self, e: Decorator) -> None:
                     return
         self.visit_decorator_inner(e)
 
-    def visit_decorator_inner(self, e: Decorator, allow_empty: bool = False) -> None:
+    def visit_decorator_inner(
+        self, e: Decorator, allow_empty: bool = False, skip_first_item: bool = False
+    ) -> None:
         if self.recurse_into_functions:
             with self.tscope.function_scope(e.func):
                 self.check_func_item(e.func, name=e.func.name, allow_empty=allow_empty)
 
         # Process decorators from the inside out to determine decorated signature, which
         # may be different from the declared signature.
         sig: Type = self.function_type(e.func)
-        for d in reversed(e.decorators):
+        non_trivial_decorator = False
+        # For settable properties skip the first decorator (that is @foo.setter).
+        for d in reversed(e.decorators[1:] if skip_first_item else e.decorators):
+            if refers_to_fullname(d, "abc.abstractmethod"):
+                # This is a hack to avoid spurious errors because of incomplete type
+                # of @abstractmethod in the test fixtures.
+                continue
             if refers_to_fullname(d, OVERLOAD_NAMES):
                 if not allow_empty:
                     self.fail(message_registry.MULTIPLE_OVERLOADS_REQUIRED, e)
                 continue
+            non_trivial_decorator = True
             dec = self.expr_checker.accept(d)
             temp = self.temp_node(sig, context=d)
             fullname = None
             if isinstance(d, RefExpr):
                 fullname = d.fullname or None
-            # if this is a expression like @b.a where b is an object, get the type of b
+            # if this is an expression like @b.a where b is an object, get the type of b,
             # so we can pass it the method hook in the plugins
             object_type: Type | None = None
             if fullname is None and isinstance(d, MemberExpr) and self.has_type(d.expr):
@@ -5305,7 +5332,8 @@ def visit_decorator_inner(self, e: Decorator, allow_empty: bool = False) -> None
             sig, t2 = self.expr_checker.check_call(
                 dec, [temp], [nodes.ARG_POS], e, callable_name=fullname, object_type=object_type
             )
-        self.check_untyped_after_decorator(sig, e.func)
+        if non_trivial_decorator:
+            self.check_untyped_after_decorator(sig, e.func)
         sig = set_callable_name(sig, e.func)
         e.var.type = sig
         e.var.is_ready = True
@@ -5314,8 +5342,8 @@ def visit_decorator_inner(self, e: Decorator, allow_empty: bool = False) -> 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)
-        # For overloaded functions we already checked override for overload as a whole.
-        if allow_empty:
+        # For overloaded functions/properties we already checked override for overload as a whole.
+        if allow_empty or skip_first_item:
             return
         if e.func.info and not e.func.is_dynamic() and not e.is_overload:
             found_method_base_classes = self.check_method_override(e)

mypy/semanal.py

@@ -1246,10 +1246,11 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
         with self.overload_item_set(0):
             first_item.accept(self)
 
+        bare_setter_type = None
         if isinstance(first_item, Decorator) and first_item.func.is_property:
             # This is a property.
             first_item.func.is_overload = True
-            self.analyze_property_with_multi_part_definition(defn)
+            bare_setter_type = self.analyze_property_with_multi_part_definition(defn)
             typ = function_type(first_item.func, self.named_type("builtins.function"))
             assert isinstance(typ, CallableType)
             types = [typ]
@@ -1283,6 +1284,11 @@ def analyze_overloaded_func_def(self, defn: OverloadedFuncDef) -> None:
             #   * Put decorator everywhere, use "bare" types in overloads.
             defn.type = Overloaded(types)
             defn.type.line = defn.line
+            # In addition, we can set the getter/setter type for valid properties as some
+            # code paths may either use the above type, or var.type etc. of the first item.
+            if isinstance(first_item, Decorator) and bare_setter_type:
+                first_item.var.type = types[0]
+                first_item.var.setter_type = bare_setter_type
 
         if not defn.items:
             # It was not a real overload after all, but function redefinition. We've
@@ -1502,26 +1508,37 @@ def process_static_or_class_method_in_overload(self, defn: OverloadedFuncDef) ->
             defn.is_class = class_status[0]
             defn.is_static = static_status[0]
 
-    def analyze_property_with_multi_part_definition(self, defn: OverloadedFuncDef) -> None:
+    def analyze_property_with_multi_part_definition(
+        self, defn: OverloadedFuncDef
+    ) -> CallableType | None:
         """Analyze a property defined using multiple methods (e.g., using @x.setter).
 
         Assume that the first method (@property) has already been analyzed.
+        Return bare setter type (without any other decorators applied), this may be used
+        by the caller for performance optimizations.
         """
         defn.is_property = True
         items = defn.items
         first_item = defn.items[0]
         assert isinstance(first_item, Decorator)
         deleted_items = []
+        bare_setter_type = None
         for i, item in enumerate(items[1:]):
             if isinstance(item, Decorator):
-                if len(item.decorators) >= 1:
+                item.func.accept(self)
+                if item.decorators:
                     first_node = item.decorators[0]
                     if isinstance(first_node, MemberExpr):
                         if first_node.name == "setter":
                             # The first item represents the entire property.
                             first_item.var.is_settable_property = True
                             # Get abstractness from the original definition.
                             item.func.abstract_status = first_item.func.abstract_status
+                            setter_func_type = function_type(
+                                item.func, self.named_type("builtins.function")
+                            )
+                            assert isinstance(setter_func_type, CallableType)
+                            bare_setter_type = setter_func_type
                         if first_node.name == "deleter":
                             item.func.abstract_status = first_item.func.abstract_status
                         for other_node in item.decorators[1:]:
@@ -1530,7 +1547,6 @@ def analyze_property_with_multi_part_definition(self, defn: OverloadedFuncDef) -
                         self.fail(
                             f"Only supported top decorator is @{first_item.func.name}.setter", item
                         )
-                item.func.accept(self)
             else:
                 self.fail(f'Unexpected definition for property "{first_item.func.name}"', item)
                 deleted_items.append(i + 1)
@@ -1544,6 +1560,7 @@ def analyze_property_with_multi_part_definition(self, defn: OverloadedFuncDef) -
                         item.func.deprecated = (
                             f"function {item.fullname} is deprecated: {deprecated}"
                         )
+        return bare_setter_type
 
     def add_function_to_symbol_table(self, func: FuncDef | OverloadedFuncDef) -> None:
         if self.is_class_scope():

test-data/unit/check-classes.test

@@ -8464,3 +8464,77 @@ def deco(fn: Callable[[], list[T]]) -> Callable[[], T]: ...
 @deco
 def f() -> list[str]: ...
 [builtins fixtures/property.pyi]
+
+[case testPropertySetterSuperclassDeferred2]
+import a
+[file a.py]
+import b
+class D(b.C):
+    @property
+    def foo(self) -> str: ...
+    @foo.setter  # E: Incompatible override of a setter type \
+                 # N:  (base class "C" defined the type as "str", \
+                 # N:  override has type "int")
+    def foo(self, x: int) -> None: ...
+[file b.py]
+from a import D
+class C:
+    @property
+    def foo(self) -> str: ...
+    @foo.setter
+    def foo(self, x: str) -> None: ...
+[builtins fixtures/property.pyi]
+
+[case testPropertySetterDecorated]
+from typing import Callable, TypeVar
+
+class B:
+    def __init__(self) -> None:
+        self.foo: str
+        self.bar: int
+
+class C(B):
+    @property
+    def foo(self) -> str: ...
+    @foo.setter  # E: Incompatible override of a setter type \
+                 # N:  (base class "B" defined the type as "str", \
+                 # N:  override has type "int")
+    @deco
+    def foo(self, x: int, y: int) -> None: ...
+
+    @property
+    def bar(self) -> int: ...
+    @bar.setter
+    @deco
+    def bar(self, x: int, y: int) -> None: ...
+
+    @property
+    def baz(self) -> int: ...
+    @baz.setter
+    @deco_untyped
+    def baz(self, x: int) -> None: ...
+
+c: C
+c.baz = "yes"  # OK, because of untyped decorator
+
+T = TypeVar("T")
+def deco(fn: Callable[[T, int, int], None]) -> Callable[[T, int], None]: ...
+def deco_untyped(fn): ...
+[builtins fixtures/property.pyi]
+
+[case testPropertyDeleterBodyChecked]
+class C:
+    @property
+    def foo(self) -> int: ...
+    @foo.deleter
+    def foo(self) -> None:
+        1()  # E: "int" not callable
+
+    @property
+    def bar(self) -> int: ...
+    @bar.setter
+    def bar(self, x: str) -> None: ...
+    @bar.deleter
+    def bar(self) -> None:
+        1()  # E: "int" not callable
+[builtins fixtures/property.pyi]