[mypyc] Propagate property setter return value (#21814)

The property setter wrapper put in `tp_getset` returns 0 regardless of
the value returned by the function generated for the property setter.
This can cause strange errors reported by cpython when the property
setter raises an exception since the wrapper still returns a non-error
value.

Fix by checking the return value of the property setter in the wrapper
and returning -1 on error.

Author: p-sawicki

mypyc/codegen/emitclass.py

@@ -1358,15 +1358,18 @@ def generate_property_setter(
         )
     )
     emitter.emit_line("{")
+    ret_type = func_ir.ret_type
+    emitter.emit_line(f"{emitter.ctype(ret_type)} retval = {emitter.c_undefined_value(ret_type)};")
     if arg_type.is_unboxed:
         emitter.emit_unbox("value", "tmp", arg_type, error=ReturnHandler("-1"), declare_dest=True)
         emitter.emit_line(
-            f"{NATIVE_PREFIX}{func_ir.cname(emitter.names)}((PyObject *) self, tmp);"
+            f"retval = {NATIVE_PREFIX}{func_ir.cname(emitter.names)}((PyObject *) self, tmp);"
         )
     else:
         emitter.emit_line(
-            f"{NATIVE_PREFIX}{func_ir.cname(emitter.names)}((PyObject *) self, value);"
+            f"retval = {NATIVE_PREFIX}{func_ir.cname(emitter.names)}((PyObject *) self, value);"
         )
+    emitter.emit_error_check("retval", ret_type, "return -1;")
     emitter.emit_line("return 0;")
     emitter.emit_line("}")
 

mypyc/codegen/emitfunc.py

@@ -548,7 +548,7 @@ def visit_set_attr(self, op: SetAttr) -> None:
         rtype = op.class_type
         cl = rtype.class_ir
         attr_rtype, decl_cl = cl.attr_details(op.attr)
-        if op.is_propset:
+        if op.propset is not None:
             # Again, use vtable access for properties...
             assert not op.is_init and op.error_kind == ERR_FALSE, "%s %d %d %s" % (
                 op.attr,
@@ -557,20 +557,27 @@ def visit_set_attr(self, op: SetAttr) -> None:
                 rtype,
             )
             version = "_TRAIT" if cl.is_trait else ""
+            ret_type = op.propset.sig.ret_type
+            c_ret_type = self.emitter.ctype(ret_type)
+            tmp = self.temp_name()
             self.emit_line(
-                "%s = CPY_SET_ATTR%s(%s, %s, %d, %s, %s, %s); /* %s */"
+                "%s %s = CPY_SET_ATTR%s(%s, %s, %d, %s, %s, %s, %s); /* %s */"
                 % (
-                    dest,
+                    c_ret_type,
+                    tmp,
                     version,
                     obj,
                     self.emitter.type_struct_name(rtype.class_ir),
                     rtype.setter_index(op.attr),
                     src,
                     rtype.struct_name(self.names),
                     self.ctype(rtype.attr_type(op.attr)),
+                    c_ret_type,
                     op.attr,
                 )
             )
+            self.emit_line(f"{dest} = 1;")
+            self.emitter.emit_error_check(tmp, ret_type, f"{dest} = 0;")
         elif IS_FREE_THREADED and is_simple_refcounted_pointer(attr_rtype):
             # In free-threaded builds, publishing a single reference-counted
             # 'PyObject *' field must be atomic so a concurrent reader (see

mypyc/ir/ops.py

@@ -945,14 +945,18 @@ def __init__(self, obj: Value, attr: str, src: Value, line: int) -> None:
         self.is_init = False
 
         cl = self.class_type.class_ir
-        is_propset = False
+        self.propset: FuncDecl | None = None
         for ir in cl.mro:
             propset = ir.method_decls.get(PROPSET_PREFIX + attr)
             if propset is not None:
-                is_propset = not propset.implicit
+                if not propset.implicit:
+                    self.propset = propset
                 break
-        # If True, this op represents calling a property setter.
-        self.is_propset = is_propset
+
+    @property
+    def is_propset(self) -> bool:
+        """If True, this op represents calling a property setter."""
+        return self.propset is not None
 
     def mark_as_initializer(self) -> None:
         self.is_init = True

mypyc/lib-rt/CPy.h

@@ -107,13 +107,13 @@ static inline size_t CPy_FindAttrOffset(PyTypeObject *trait, CPyVTableItem *vtab
 #define CPY_GET_ATTR_TRAIT(obj, trait, vtable_index, object_type, attr_type)   \
     ((attr_type (*)(object_type *))(CPy_FindTraitVtable(trait, ((object_type *)obj)->vtable))[vtable_index])((object_type *)obj)
 
-// Set attribute value using vtable
-#define CPY_SET_ATTR(obj, type, vtable_index, value, object_type, attr_type) \
-    ((bool (*)(object_type *, attr_type))((object_type *)obj)->vtable[vtable_index])( \
+// Set attribute value using vtable.
+#define CPY_SET_ATTR(obj, type, vtable_index, value, object_type, attr_type, ret_type) \
+    ((ret_type (*)(object_type *, attr_type))((object_type *)obj)->vtable[vtable_index])( \
         (object_type *)obj, value)
 
-#define CPY_SET_ATTR_TRAIT(obj, trait, vtable_index, value, object_type, attr_type) \
-    ((bool (*)(object_type *, attr_type))(CPy_FindTraitVtable(trait, ((object_type *)obj)->vtable))[vtable_index])( \
+#define CPY_SET_ATTR_TRAIT(obj, trait, vtable_index, value, object_type, attr_type, ret_type) \
+    ((ret_type (*)(object_type *, attr_type))(CPy_FindTraitVtable(trait, ((object_type *)obj)->vtable))[vtable_index])( \
         (object_type *)obj, value)
 
 #define CPY_GET_METHOD(obj, type, vtable_index, object_type, method_type) \

mypyc/test-data/run-classes.test

@@ -6280,3 +6280,103 @@ def comp(ns: list[int]) -> list[int]:
 def test_borrowed_final_attribute_in_comprehension() -> None:
     for _ in range(1000):
         assert comp([1, 2, 3, 4, 5]) == [112, 113, 114, 115, 116]
+
+[case testPropertyException]
+from mypy_extensions import i16
+
+from testutil import assertRaises
+
+class T:
+    def __init__(self, val: int) -> None:
+        self._val = val
+        self._val_overlaps: i16 = -1
+        self._tuple_val = (0, 0)
+        self._locked = 0
+
+    @property
+    def val(self) -> int:
+        return self._val
+
+    @val.setter
+    def val(self, x: int) -> None:
+        if x < 0:
+            raise ValueError("No")
+        self._val = x
+
+    @property
+    def val_overlaps(self) -> i16:
+        return self._val_overlaps
+
+    @val_overlaps.setter
+    def val_overlaps(self, x: i16) -> i16:
+        if x > 0:
+            raise ValueError("Invalid")
+        self._val_overlaps = x
+        return self._val_overlaps
+
+    @property
+    def tuple_val(self) -> tuple[int, int]:
+        return self._tuple_val
+
+    @tuple_val.setter
+    def tuple_val(self, x: tuple[int, int]) -> tuple[int, int]:
+        if x[0] < 0:
+            raise ValueError("Invalid tuple")
+        self._tuple_val = x
+        return self._tuple_val
+
+    @property
+    def locked(self) -> int:
+        raise ValueError("Locked")
+
+def test_property_setter_exception() -> None:
+    t = T(1)
+    assert t.val == 1
+    assert t.val_overlaps == -1
+
+    t.val = 2
+    assert t.val == 2
+
+    t.val_overlaps = -113
+    assert t.val_overlaps == -113
+
+    t.val_overlaps = -1
+    assert t.val_overlaps == -1
+
+    t.tuple_val = (1, 2)
+    assert t.tuple_val == (1, 2)
+
+    setattr(t, "val_overlaps", -113)
+    assert t.val_overlaps == -113
+
+    setattr(t, "tuple_val", (3, 4))
+    assert t.tuple_val == (3, 4)
+
+    with assertRaises(ValueError):
+        t.val = -1
+
+    with assertRaises(ValueError):
+        t.val_overlaps = 1
+
+    with assertRaises(ValueError):
+        t.tuple_val = (-1, 0)
+
+    # Generic setattr goes through the Python descriptor setter instead of
+    # calling the native property setter directly through the vtable.
+    with assertRaises(ValueError):
+        setattr(t, "val", -1)
+
+    with assertRaises(ValueError):
+        setattr(t, "val_overlaps", 1)
+
+    with assertRaises(ValueError):
+        setattr(t, "tuple_val", (-1, 0))
+
+def test_property_getter_exception() -> None:
+    t = T(1)
+
+    with assertRaises(ValueError):
+        print(t.locked)
+
+    with assertRaises(ValueError):
+        getattr(t, "locked")