crates/vm/src/object/core.rs (1)

1784-1806: Consider clarifying the comment about preserved fields.

The comment on line 1794 states "typ, vtable, dict(None), weak_list, slots are preserved" which could be misleading. The weak_list was cleared in drop_slow_inner before the object was pushed to the freelist, so it's preserved only as an empty structure. The comment could be clearer about this:

-                    // typ, vtable, dict(None), weak_list, slots are preserved
+                    // typ, vtable, dict(None), weak_list (empty), slots are preserved from cached object

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 1784 - 1806, The comment saying
"typ, vtable, dict(None), weak_list, slots are preserved" is misleading because
drop_slow_inner clears weak_list before pushing to the freelist, so weak_list is
preserved only as an empty structure; update the comment near the freelist reuse
branch (around T::freelist_pop, PyInner, and Box::into_raw usage) to explicitly
state which fields are fully preserved (typ, vtable, slots), which are set to
None/cleared (dict is None), and that weak_list is preserved but intentionally
emptied by drop_slow_inner.

crates/vm/src/builtins/slice.rs (1)

36-45: ⚠️ Potential issue | 🔴 Critical

PySlice::clear() leaves stop alive on freelist-hit.

Line 40’s assumption is incorrect for the freelist path: if freelist_push succeeds, payload drop is deferred, so stop stays as a strong reference inside cached objects. That can retain arbitrary object graphs and breaks expected tp_clear behavior for freelisted objects.

Temporary safe mitigation until full `stop` extraction is implemented

 impl PyPayload for PySlice {
-    const MAX_FREELIST: usize = 1;
-    const HAS_FREELIST: bool = true;
+    const MAX_FREELIST: usize = 0;
+    const HAS_FREELIST: bool = false;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/builtins/slice.rs` around lines 36 - 45, The clear method for
PySlice leaves self.stop alive on freelist-hit; change PySlice::clear (the
function handling start/stop/step) to extract stop as well (use
self.stop.take()) and push it into out when Some, just like start and step, so
the strong reference is removed immediately even if freelist_push defers payload
drop; ensure the updated logic handles start, stop, and step symmetrically to
avoid retaining object graphs on freelist reuse.

crates/vm/src/object/core.rs (1)

1785-1807: Collapse duplicated allocation fallback in PyRef::new_ref.

The “allocate new PyInner” block is duplicated in both branches. Extracting cached-value selection first makes the control flow tighter and easier to maintain.

Refactor sketch

-        let ptr = if !has_dict && !is_heaptype {
-            if let Some(cached) = unsafe { T::freelist_pop() } {
-                let inner = cached.as_ptr() as *mut PyInner<T>;
-                unsafe {
-                    core::ptr::write(&mut (*inner).ref_count, RefCount::new());
-                    (*inner).gc_bits.store(0, Ordering::Relaxed);
-                    core::ptr::drop_in_place(&mut (*inner).payload);
-                    core::ptr::write(&mut (*inner).payload, payload);
-                    // typ, vtable, slots are preserved; dict is None, weak_list was
-                    // cleared by drop_slow_inner before freelist push
-                }
-                // Drop the caller's typ since the cached object already holds one
-                drop(typ);
-                unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
-            } else {
-                let inner = Box::into_raw(PyInner::new(payload, typ, dict));
-                unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
-            }
-        } else {
-            let inner = Box::into_raw(PyInner::new(payload, typ, dict));
-            unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
-        };
+        let cached = if !has_dict && !is_heaptype {
+            unsafe { T::freelist_pop() }
+        } else {
+            None
+        };
+
+        let ptr = if let Some(cached) = cached {
+            let inner = cached.as_ptr() as *mut PyInner<T>;
+            unsafe {
+                core::ptr::write(&mut (*inner).ref_count, RefCount::new());
+                (*inner).gc_bits.store(0, Ordering::Relaxed);
+                core::ptr::drop_in_place(&mut (*inner).payload);
+                core::ptr::write(&mut (*inner).payload, payload);
+            }
+            drop(typ);
+            unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
+        } else {
+            let inner = Box::into_raw(PyInner::new(payload, typ, dict));
+            unsafe { NonNull::new_unchecked(inner.cast::<Py<T>>()) }
+        };

As per coding guidelines: "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/vm/src/object/core.rs` around lines 1785 - 1807, The allocation branch
is duplicated; refactor PyRef::new_ref by first selecting an inner pointer
candidate (call T::freelist_pop() when !has_dict && !is_heaptype to get an
Option<NonNull<PyInner<T>>>), then in a single common block handle either
reinitializing the cached inner (reset ref_count, gc_bits, drop and write
payload, clear dict/weak handling, and drop the incoming typ) when Some(cached)
or allocating with Box::into_raw(PyInner::new(payload, typ, dict)) when None,
and finally return NonNull::new_unchecked(inner.cast::<Py<T>>()). Keep the exact
operations used now (core::ptr::write, drop_in_place, gc_bits.store, drop(typ))
and the same conditions (has_dict/is_heaptype) but remove the duplicated
Box::into_raw code paths so the common logic is executed once.