[mypyc] Generate more type methods for types with managed dicts (#21290)

Fixes #21133

Types with the `Py_TPFLAGS_MANAGED_DICT` flag must call
`PyObject_VisitManagedDict` and `PyObject_ClearManagedDict` in their
`tp_traverse` / `tp_clear` functions according to
[docs](https://docs.python.org/3/c-api/typeobj.html#c.Py_TPFLAGS_MANAGED_DICT)
but the types generated by mypyc currently don't do this. We don't
generate these functions at all so they get inherited from the base
class.

Failure to call these functions may result in a segfault in python 3.14
when accessing the managed dict after its owner has been deallocated. I
believe the crash happens because the logic for types with the
`Py_TPFLAGS_INLINE_VALUES` flag in
[`PyObject_ClearManagedDict`](https://github.com/python/cpython/blob/main/Objects/dictobject.c#L7803)
is not run. The condition to add this flag has changed in
[3.14](https://github.com/python/cpython/blob/3.14/Objects/typeobject.c#L8877)
vs
[3.13](https://github.com/python/cpython/blob/3.13/Objects/typeobject.c#L8171)
so generated types with `Py_TPFLAGS_MANAGED_DICT` get it in 3.14.

To fix, generate `tp_clear`, `tp_traverse`, and `tp_dealloc` for types
with managed dicts. Also add a special case in these functions for
classes with built-in bases to call the pointer of the base class.

Author: p-sawicki

mypyc/codegen/emit.py

@@ -1398,6 +1398,12 @@ def emit_cpyfunction_instance(
         self.emit_line(error_stmt)
         return wrapper_name
 
+    def emit_base_tp_function_call(
+        self, derived_cl: ClassIR, tp_func: str, args: str, *, prefix: str = ""
+    ) -> None:
+        type_obj = self.type_struct_name(derived_cl)
+        self.emit_line(f"{prefix}{type_obj}->tp_base->{tp_func}({args});")
+
 
 def c_array_initializer(components: list[str], *, indented: bool = False) -> str:
     """Construct an initializer for a C array variable.

mypyc/codegen/emitclass.py

@@ -262,7 +262,8 @@ def generate_class(cl: ClassIR, module: str, emitter: Emitter) -> None:
     if not cl.builtin_base:
         fields["tp_new"] = new_name
 
-    if generate_full:
+    managed_dict = has_managed_dict(cl, emitter)
+    if generate_full or managed_dict:
         fields["tp_dealloc"] = f"(destructor){name_prefix}_dealloc"
         if not cl.is_acyclic:
             fields["tp_traverse"] = f"(traverseproc){name_prefix}_traverse"
@@ -335,6 +336,14 @@ def emit_line() -> None:
     else:
         fields["tp_basicsize"] = base_size
 
+    if generate_full or managed_dict:
+        if not cl.is_acyclic:
+            generate_traverse_for_class(cl, traverse_name, emitter)
+            emit_line()
+        generate_clear_for_class(cl, clear_name, emitter)
+        emit_line()
+        generate_dealloc_for_class(cl, dealloc_name, clear_name, bool(del_method), emitter)
+        emit_line()
     if generate_full:
         assert cl.setup is not None
         emitter.emit_line(native_function_header(cl.setup, emitter) + ";")
@@ -345,13 +354,6 @@ def emit_line() -> None:
         init_fn = cl.get_method("__init__")
         generate_new_for_class(cl, new_name, vtable_name, setup_name, init_fn, emitter)
         emit_line()
-        if not cl.is_acyclic:
-            generate_traverse_for_class(cl, traverse_name, emitter)
-            emit_line()
-        generate_clear_for_class(cl, clear_name, emitter)
-        emit_line()
-        generate_dealloc_for_class(cl, dealloc_name, clear_name, bool(del_method), emitter)
-        emit_line()
 
         if cl.allow_interpreted_subclasses:
             shadow_vtable_name: str | None = generate_vtables(
@@ -380,7 +382,7 @@ def emit_line() -> None:
     emit_line()
 
     flags = ["Py_TPFLAGS_DEFAULT", "Py_TPFLAGS_HEAPTYPE", "Py_TPFLAGS_BASETYPE"]
-    if generate_full and not cl.is_acyclic:
+    if (generate_full or managed_dict) and not cl.is_acyclic:
         flags.append("Py_TPFLAGS_HAVE_GC")
     if cl.has_method("__call__"):
         fields["tp_vectorcall_offset"] = "offsetof({}, vectorcall)".format(
@@ -391,7 +393,7 @@ def emit_line() -> None:
             # This is just a placeholder to please CPython. It will be
             # overridden during setup.
             fields["tp_call"] = "PyVectorcall_Call"
-    if has_managed_dict(cl, emitter):
+    if managed_dict:
         flags.append("Py_TPFLAGS_MANAGED_DICT")
     fields["tp_flags"] = " | ".join(flags)
 
@@ -868,8 +870,14 @@ def generate_traverse_for_class(cl: ClassIR, func_name: str, emitter: Emitter) -
     for base in reversed(cl.base_mro):
         for attr, rtype in base.attributes.items():
             emitter.emit_gc_visit(f"self->{emitter.attr(attr)}", rtype)
+    base_args = "(PyObject *)self, visit, arg"
+    emitter.emit_line("int rv = 0;")
+    if cl.builtin_base:
+        emitter.emit_base_tp_function_call(cl, "tp_traverse", base_args, prefix="rv = ")
+        emitter.emit_line("if (rv != 0) return rv;")
     if has_managed_dict(cl, emitter):
-        emitter.emit_line("PyObject_VisitManagedDict((PyObject *)self, visit, arg);")
+        emitter.emit_line(f"rv = PyObject_VisitManagedDict({base_args});")
+        emitter.emit_line("if (rv != 0) return rv;")
     elif cl.has_dict:
         struct_name = cl.struct_name(emitter.names)
         # __dict__ lives right after the struct and __weakref__ lives right after that
@@ -880,7 +888,7 @@ def generate_traverse_for_class(cl: ClassIR, func_name: str, emitter: Emitter) -
             f"*((PyObject **)((char *)self + sizeof(PyObject *) + sizeof({struct_name})))",
             object_rprimitive,
         )
-    emitter.emit_line("return 0;")
+    emitter.emit_line("return rv;")
     emitter.emit_line("}")
 
 
@@ -891,8 +899,11 @@ def generate_clear_for_class(cl: ClassIR, func_name: str, emitter: Emitter) -> N
     for base in reversed(cl.base_mro):
         for attr, rtype in base.attributes.items():
             emitter.emit_gc_clear(f"self->{emitter.attr(attr)}", rtype)
+    base_args = "(PyObject *)self"
+    if cl.builtin_base:
+        emitter.emit_base_tp_function_call(cl, "tp_clear", base_args)
     if has_managed_dict(cl, emitter):
-        emitter.emit_line("PyObject_ClearManagedDict((PyObject *)self);")
+        emitter.emit_line(f"PyObject_ClearManagedDict({base_args});")
     elif cl.has_dict:
         struct_name = cl.struct_name(emitter.names)
         # __dict__ lives right after the struct and __weakref__ lives right after that
@@ -936,6 +947,18 @@ def generate_dealloc_for_class(
         emitter.emit_line("}")
     if not cl.is_acyclic:
         emitter.emit_line("PyObject_GC_UnTrack(self);")
+    if cl.builtin_base:
+        emitter.emit_line(f"{clear_func_name}(self);")
+        # For native subclasses of builtins such as dict, the base deallocator
+        # is responsible for tearing down base-owned storage and freeing memory.
+        # Re-track self if base is GC-aware to match cpython's subtype_dealloc.
+        base = f"{emitter.type_struct_name(cl)}->tp_base"
+        base_arg = "(PyObject *)self"
+        emitter.emit_line(f"if (PyType_IS_GC({base})) PyObject_GC_Track({base_arg});")
+        emitter.emit_base_tp_function_call(cl, "tp_dealloc", base_arg)
+        emitter.emit_line("done: ;")
+        emitter.emit_line("}")
+        return
     if cl.reuse_freed_instance:
         emit_reuse_dealloc(cl, emitter)
     # The trashcan is needed to handle deep recursive deallocations

mypyc/irbuild/prepare.py

@@ -424,22 +424,6 @@ def prepare_class_def(
     if attrs.get("acyclic") is True:
         ir.is_acyclic = True
 
-    free_list_len = attrs.get("free_list_len")
-    if free_list_len is not None:
-        line = attrs_lines["free_list_len"]
-        if ir.is_trait:
-            errors.error('"free_list_len" can\'t be used with traits', path, line)
-        if ir.allow_interpreted_subclasses:
-            errors.error(
-                '"free_list_len" can\'t be used in a class that allows interpreted subclasses',
-                path,
-                line,
-            )
-        if free_list_len == 1:
-            ir.reuse_freed_instance = True
-        else:
-            errors.error(f'Unsupported value for "free_list_len": {free_list_len}', path, line)
-
     # Check for subclassing from builtin types
     for cls in info.mro:
         # Special case exceptions and dicts
@@ -468,6 +452,28 @@ def prepare_class_def(
                     cdef.line,
                 )
 
+    free_list_len = attrs.get("free_list_len")
+    if free_list_len is not None:
+        line = attrs_lines["free_list_len"]
+        if ir.is_trait:
+            errors.error('"free_list_len" can\'t be used with traits', path, line)
+        if ir.allow_interpreted_subclasses:
+            errors.error(
+                '"free_list_len" can\'t be used in a class that allows interpreted subclasses',
+                path,
+                line,
+            )
+        if ir.builtin_base:
+            errors.error(
+                '"free_list_len" can\'t be used in a class that inherits from a built-in type',
+                path,
+                line,
+            )
+        if free_list_len == 1:
+            ir.reuse_freed_instance = True
+        else:
+            errors.error(f'Unsupported value for "free_list_len": {free_list_len}', path, line)
+
     # Set up the parent class
     bases = [mapper.type_to_ir[base.type] for base in info.bases if base.type in mapper.type_to_ir]
     if len(bases) > 1 and any(not c.is_trait for c in bases) and bases[0].is_trait:

mypyc/lib-rt/pythoncapi_compat.h

@@ -922,7 +922,8 @@ PyObject_VisitManagedDict(PyObject *obj, visitproc visit, void *arg)
 {
     PyObject **dict = _PyObject_GetDictPtr(obj);
     if (dict == NULL || *dict == NULL) {
-        return -1;
+        // Nothing to do.
+        return 0;
     }
     Py_VISIT(*dict);
     return 0;

mypyc/test-data/fixtures/ir.py

@@ -40,6 +40,7 @@ def __pow__(self, other: T_contra, modulo: _M) -> T_co: ...
 
 class object:
     __class__: type
+    __dict__: dict[str, Any]
     def __new__(cls) -> Self: pass
     def __init__(self) -> None: pass
     def __init_subclass__(cls, **kwargs: object) -> None: pass

mypyc/test-data/irbuild-classes.test

@@ -2138,6 +2138,10 @@ class NonNative:
 class InterpSub:
     pass
 
+@mypyc_attr(free_list_len=1)  # E: "free_list_len" can't be used in a class that inherits from a built-in type
+class InheritsBuiltIn(dict):
+    pass
+
 [case testAcyclicClassRequiresAcyclicBases]
 from typing import Generic, TypeVar
 from mypy_extensions import mypyc_attr, trait

mypyc/test-data/run-classes.test

@@ -3325,20 +3325,39 @@ def test_function():
     assert(isinstance(d.fitem, ForwardDefinedClass))
     assert(isinstance(d.fitems, ForwardDefinedClass))
 
-[case testDelForDictSubclass-xfail]
-# The crash in issue mypy#19175 is fixed.
-# But, for classes that derive from built-in Python classes, user-defined __del__ method is not
-# being invoked.
+[case testDelForDictSubclass]
+events: list[str] = []
+
+class Item:
+    def __del__(self) -> None:
+        events.append("deleting Item")
+
 class DictSubclass(dict):
-    def __del__(self):
-        print("deleting DictSubclass...")
+    def __del__(self) -> None:
+        events.append("deleting DictSubclass")
+
+def test_dict_subclass_dealloc() -> None:
+    d = DictSubclass()
+    d["item"] = Item()
+    del d
 
 [file driver.py]
-import native
-native.DictSubclass()
+import sys
 
-[out]
-deleting DictSubclass...
+from native import events, test_dict_subclass_dealloc
+
+test_dict_subclass_dealloc()
+
+expected_events: list[str] = []
+
+# TODO: Fix when compiling for older python.
+# The user-defined __del__ method is currently only invoked when __dict__ is a managed dict
+# because calling __del__ in tp_clear on older python crashes.
+if sys.version_info >= (3, 12):
+    expected_events.append("deleting DictSubclass")
+expected_events.append("deleting Item")
+
+assert events == expected_events, events
 
 [case testDel]
 class A: