Unverified review from Codex

The direction fixes the immediate managed-dict symptom, but it introduces two blocking lifecycle bugs for compiled dict subclasses.

I reviewed PR head f86a3b246 and tested it locally.

Findings

1. Blocking: subclass-of-compiled-dict-subclass can segfault during deallocation

The generated builtin-base deallocator calls:

Py_TYPE(self)->tp_base->tp_dealloc((PyObject *)self);

Generated from emitclass.py.

That is unsafe when the object is an instance of a subclass. Example:

class A(dict):
    pass

class B(A):
    pass

def f() -> None:
    b = B()
    del b

For a B instance:

1. B_dealloc calls Py_TYPE(self)->tp_base->tp_dealloc.
2. Py_TYPE(self) is still B, so this calls A_dealloc.
3. Inside A_dealloc, Py_TYPE(self) is still B.
4. So Py_TYPE(self)->tp_base is still A.
5. A_dealloc calls itself recursively until segfault.

I reproduced this with a scratch mypyc build from the PR. A plain class A(dict); class B(A); del B() segfaulted immediately.

If this approach is kept, the deallocator must not use the dynamic type to find the base deallocator for the current generated dealloc function. It would need to call the current class’s static base deallocator, not Py_TYPE(self)->tp_base.

2. Blocking: generated tp_traverse / tp_clear stop traversing the actual dict contents

The PR installs custom GC slots for managed-dict classes here: emitclass.py.

But the generated traversal only does:

PyObject_VisitManagedDict((PyObject *)self, visit, arg);

from emitclass.py, and clear only does:

PyObject_ClearManagedDict((PyObject *)self);

from emitclass.py.

For a dict subclass, the instance owns two separate reference containers:

• the managed instance __dict__
• the underlying PyDictObject entries

The PR visits/clears the first but no longer delegates to dict’s traversal/clear for the second. So cycles through dict entries become invisible to cyclic GC.

I reproduced this:

events = []

class CyclicDict(dict):
    def __init__(self) -> None:
        self["self"] = self

    def __del__(self) -> None:
        events.append("deleted")

c = CyclicDict()
del c
gc.collect()
assert events == ["deleted"]

Pure Python collects it and calls __del__. With the PR-compiled version, events stays empty: the object leaks because GC does not see the self-reference inside the dict storage.

This matches the CPython C API contract: tp_traverse is how GC detects cycles, and tp_clear must break cycles. The managed-dict helper calls are required, but they are additive; they do not replace traversal/clearing of base-owned references. Docs: https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_traverse and https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_clear.

3. The PR’s approach is much harder to make correct than the narrower fix

The PR keeps Py_TPFLAGS_MANAGED_DICT enabled for dict subclasses via has_managed_dict, then tries to generate enough lifecycle slots to satisfy CPython.

That has to compose:

• mypyc-managed instance dict lifecycle
• builtin dict storage lifecycle
• compiled subclass chains
• finalizers
• cyclic GC
• inherited tp_traverse / tp_clear semantics

The two bugs above are both symptoms of that composition problem.

I still think the simpler fix is better: do not use managed dicts for builtin subclasses. Use managed dicts only when cl.builtin_base is None, and for PyDictObject subclasses on Python 3.12+ keep the old explicit tp_dictoffset / tp_weaklistoffset layout. That avoids needing to wrap dict’s lifecycle at all.

4. Regression tests should cover GC cycles and compiled subclass chains

The new tests cover the direct original lifetime shape well: run-dicts.test. The interpreted driver part is especially important.

I would add at least:

class A(dict):
    pass

class B(A):
    pass

def test() -> None:
    b = B()
    del b

and:

import gc

events: list[str] = []

class D(dict):
    def __init__(self) -> None:
        self["self"] = self

    def __del__(self) -> None:
        events.append("deleted")

def test() -> None:
    d = D()
    del d
    gc.collect()
    assert events == ["deleted"]

The second test specifically catches missing base dict traversal/clear.

5. Minor: traversal should probably propagate any nonzero return, not only negative

The PR emits:

if (rv < 0) return rv;

for PyObject_VisitManagedDict. Py_VISIT conventionally returns on any nonzero visitor result. I would use:

if (rv) return rv;

This is smaller than the lifecycle bugs above, but it is the usual traversal contract.

I didn't check anything it says. Apologies if spam, but posting since it did well on minimising the repro

thanks @hauntsaninja the blocking issues were legitimate and should be fixed now. i've also added tests similar to the code snippets.

I still think the simpler fix is better: do not use managed dicts for builtin subclasses. Use managed dicts only when cl.builtin_base is None, and for PyDictObject subclasses on Python 3.12+ keep the old explicit tp_dictoffset / tp_weaklistoffset layout. That avoids needing to wrap dict’s lifecycle at all.

not sure about this one since the docs suggest that the old layout will be deprecated so we might have to continue using managed dicts. but either way i think using the suggested approach would be a big change so it would be best to do it in another PR.