Actionable comments posted: 3

In `@crates/codegen/src/compile.rs`:
- Around line 2159-2166: The current interactive printing (guarded by
self.interactive && !self.ctx.in_func() && !self.ctx.in_class) also prints
expression statements nested inside compound blocks; restrict printing to true
top-level only by adding a check that there is no block nesting (e.g., require
ctx.block_depth == 0 or !self.ctx.in_block, or add/use a helper like
ctx.is_module_toplevel()) before emitting Instruction::CallIntrinsic1 { Print },
or alternatively move the echo logic out of the per-statement path into
compile_program_single so only top-level expressions are printed; update the
condition that currently references self.interactive, self.ctx.in_func(), and
self.ctx.in_class accordingly.

In `@crates/vm/src/frame.rs`:
- Around line 261-281: The locals_to_fast method clears locals_dirty before
syncing, so if mapping().subscript returns an Err and the function returns early
the dirty flag is lost; move the locals_dirty.store(false,
atomic::Ordering::Release) to after the for loop completes successfully (just
before returning Ok(())) so the flag is only cleared when all code.varnames have
been processed and fastlocals updated, ensuring a retry on error; update
references in locals_to_fast accordingly (keep use of fastlocals.lock() and the
existing error handling for key_error).

crates/vm/src/vm/vm_new.rs (1)

555-560: narrow_caret logic is unreachable when python_end_location() returns None.

If the parser ever produces an InvalidStarredExpressionUsage error without an end location, narrow_caret = true would have no effect since this code is gated on if let Some(...). In practice this is likely fine since ruff should always provide end locations for this error type, but worth noting.

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

46-49: Minor: locals_dirty is set even when self.locals(vm) fails.

If self.locals(vm) returns an Err, the caller never receives the locals dict, so marking locals_dirty = true is unnecessary. It's harmless (just triggers a no-op sync later), but for correctness:

Suggested fix

     fn f_locals(&self, vm: &VirtualMachine) -> PyResult {
-        let result = self.locals(vm).map(Into::into);
-        self.locals_dirty
-            .store(true, core::sync::atomic::Ordering::Release);
-        result
+        let result = self.locals(vm)?;
+        self.locals_dirty
+            .store(true, core::sync::atomic::Ordering::Release);
+        Ok(result.into())
     }

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

155-169: Duplicated destructor logic between PyGenerator and PyCoroutine.

The del implementation here is identical to PyGenerator::del in generator.rs (lines 151-168). Consider extracting the common finalization logic into a method on Coro itself (e.g., Coro::finalize(&self, jen: &PyObject, vm: &VirtualMachine) -> PyResult<()>) and having both destructors delegate to it.

Sketch

Add to Coro:

pub fn finalize(&self, jen: &PyObject, vm: &VirtualMachine) -> PyResult<()> {
    if self.closed() || self.running() {
        return Ok(());
    }
    if self.frame.lasti() == 0 {
        self.closed.store(true);
        return Ok(());
    }
    if let Err(e) = self.close(jen, vm) {
        vm.run_unraisable(e, None, jen.to_owned());
    }
    Ok(())
}

Then both destructors become:

fn del(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<()> {
    zelf.inner.finalize(zelf.as_object(), vm)
}

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."

crates/vm/src/protocol/callable.rs (1)

55-56: is_python_callable check may miss other Python-level callables.

The check only covers PyFunction and PyBoundMethod. Other Python-level callables like instances with __call__ or class objects won't match and will get c_call/c_return events instead of call/return. This diverges from CPython, where any Python-level __call__ triggers call/return events.

That said, this is likely acceptable for an initial implementation since the main goal is to avoid double-tracing for PyFunction and PyBoundMethod which already emit events via with_frame().

crates/vm/src/ospath.rs (1)

330-346: Consider extracting shared logic with with_filename.

with_filename_from_errno duplicates with_filename (lines 318–329) almost entirely — the only difference is the #[cfg(windows)] without_winerror() call. You could extract a common helper and pass a flag or closure for the Windows-specific transform, though given how small these methods are, this is not urgent.

♻️ One way to reduce duplication

+    fn build_with_filename_inner<'a>(
+        error: &std::io::Error,
+        filename: impl Into<OsPathOrFd<'a>>,
+        strip_winerror: bool,
+        vm: &VirtualMachine,
+    ) -> crate::builtins::PyBaseExceptionRef {
+        use crate::exceptions::ToOSErrorBuilder;
+        let builder = error.to_os_error_builder(vm);
+        #[cfg(windows)]
+        let builder = if strip_winerror { builder.without_winerror() } else { builder };
+        let builder = builder.filename(filename.into().filename(vm));
+        builder.build(vm).upcast()
+    }
+
     #[must_use]
     pub(crate) fn with_filename<'a>(
         error: &std::io::Error,
         filename: impl Into<OsPathOrFd<'a>>,
         vm: &VirtualMachine,
     ) -> crate::builtins::PyBaseExceptionRef {
-        use crate::exceptions::ToOSErrorBuilder;
-        let builder = error.to_os_error_builder(vm);
-        let builder = builder.filename(filename.into().filename(vm));
-        builder.build(vm).upcast()
+        Self::build_with_filename_inner(error, filename, false, vm)
     }
 
     #[must_use]
     pub(crate) fn with_filename_from_errno<'a>(
         error: &std::io::Error,
         filename: impl Into<OsPathOrFd<'a>>,
         vm: &VirtualMachine,
     ) -> crate::builtins::PyBaseExceptionRef {
-        use crate::exceptions::ToOSErrorBuilder;
-        let builder = error.to_os_error_builder(vm);
-        #[cfg(windows)]
-        let builder = builder.without_winerror();
-        let builder = builder.filename(filename.into().filename(vm));
-        builder.build(vm).upcast()
+        Self::build_with_filename_inner(error, filename, true, vm)
     }

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."

crates/vm/src/stdlib/itertools.rs (1)

959-983: Reentrancy guard and cache-aware get_item look correct.

The design mirrors CPython's teedataobject pattern well: serve from cache on fast path, guard iterable.next() with an atomic running flag, and append to cache on miss.

One minor observation: on line 982, values[index] relies on the invariant that index is always either < values.len() (served from the fast path on lines 965–967) or == values.len() (pushed on line 980). If this invariant were ever violated (e.g., values.len() < index), it would panic at runtime. Since the tee iterators enforce sequential access, this is safe in practice—same assumption CPython makes—but a debug assertion could catch future regressions:

💡 Optional: add a debug assertion

             if values.len() == index {
                 values.push(obj);
             }
+            debug_assert!(
+                index < values.len(),
+                "tee iterator cache invariant violated: index {index} >= len {}",
+                values.len()
+            );
             Ok(PyIterReturn::Return(values[index].clone()))

crates/vm/src/frame.rs (1)

466-479: Locking self.object.trace on every instruction while tracing is active — acceptable but worth noting.

The self.object.trace.lock() inside the hot loop (line 473) is only reached when vm.use_tracing is set, so normal execution is unaffected. However, when a trace function is active, this acquires the mutex on every instruction — even when the line hasn't changed. For a tighter fast path you could cache whether the per-frame trace is set outside the loop, or at least check the line-change condition before acquiring the lock.

That said, this mirrors CPython's approach (check tracing flags, then consult per-frame state), and tracing is inherently slow, so this is fine for now.

crates/vm/src/protocol/callable.rs (1)

52-69: Unconditional to_owned() clone for non-Python callables even when tracing is off.

Line 60 creates an Arc clone of the callable before checking use_tracing inside trace_event. When tracing is disabled (the overwhelmingly common case), this is a wasted atomic increment/decrement on every non-Python call (builtins, C extensions, etc.).

A quick guard avoids the overhead:

Proposed optimization

         } else {
-            let callable = self.obj.to_owned();
-            vm.trace_event(TraceEvent::CCall, Some(callable.clone()))?;
-            let result = (self.call)(self.obj, args, vm);
-            if result.is_ok() {
-                vm.trace_event(TraceEvent::CReturn, Some(callable))?;
-            } else {
-                let _ = vm.trace_event(TraceEvent::CException, Some(callable));
+            if vm.use_tracing.get() {
+                let callable = self.obj.to_owned();
+                vm.trace_event(TraceEvent::CCall, Some(callable.clone()))?;
+                let result = (self.call)(self.obj, args, vm);
+                if result.is_ok() {
+                    vm.trace_event(TraceEvent::CReturn, Some(callable))?;
+                } else {
+                    let _ = vm.trace_event(TraceEvent::CException, Some(callable));
+                }
+                result
+            } else {
+                (self.call)(self.obj, args, vm)
             }
-            result
         }

crates/vm/src/stdlib/itertools.rs (1)

959-983: Reentrancy guard logic is sound but not panic-safe.

The two-phase approach (cache-first, then fetch-with-guard) is correct. The running flag properly prevents reentrant calls to the underlying iterator, and the values.len() == index check on line 979 correctly handles the case where another thread already populated the cache entry.

However, if self.iterable.next(vm) on line 973 panics (unwind), running remains true permanently, bricking the tee. Consider using a scope guard (e.g., a drop-based RAII wrapper) to ensure running is reset even on panic:

Suggested panic-safe pattern

             // Prevent concurrent/reentrant calls to iterable.next()
             if self.running.swap(true, Ordering::Acquire) {
                 return Err(vm.new_runtime_error("cannot re-enter the tee iterator"));
             }
-            let result = self.iterable.next(vm);
-            self.running.store(false, Ordering::Release);
+            let result = {
+                struct ResetOnDrop<'a>(&'a AtomicBool);
+                impl Drop for ResetOnDrop<'_> {
+                    fn drop(&mut self) {
+                        self.0.store(false, Ordering::Release);
+                    }
+                }
+                let _guard = ResetOnDrop(&self.running);
+                self.iterable.next(vm)
+            };

This is a minor robustness improvement—VM calls generally shouldn't panic, but a drop guard is a cheap safety net.

crates/vm/src/vm/vm_new.rs (1)

554-560: Redundant call to error.python_location() — consider reusing the earlier result.

python_location() is already called on line 541 but the result is immediately converted to PyInt, making it unavailable here. A small refactor could store the (u32, u32) tuple before converting, avoiding the duplicate call.

♻️ Suggested refactor

-        let (lineno, offset) = error.python_location();
-        let lineno = self.ctx.new_int(lineno);
-        let offset = self.ctx.new_int(offset);
+        let (lineno_raw, offset_raw) = error.python_location();
+        let lineno = self.ctx.new_int(lineno_raw);
+        let offset = self.ctx.new_int(offset_raw);

Then at line 555:

         if let Some((end_lineno, end_offset)) = error.python_end_location() {
             let (end_lineno, end_offset) = if narrow_caret {
-                let (l, o) = error.python_location();
-                (l, o + 1)
+                (lineno_raw, offset_raw + 1)
             } else {
                 (end_lineno, end_offset)
             };

Actionable comments posted: 2

In `@crates/vm/src/frame.rs`:
- Around line 261-281: locals_to_fast currently leaves fastlocals[i] untouched
when locals.mapping().subscript(varname, vm) returns a KeyError, but it should
clear the fast slot like CPython's PyFrame_LocalsToFast; update the match in
locals_to_fast so that Err(e) if e.fast_isinstance(vm.ctx.exceptions.key_error)
sets fastlocals[i] = None (unbound) instead of doing nothing, ensuring deleted
keys in f_locals unbind the corresponding fastlocal; keep the existing Err(e) =>
return Err(e) path and retain locals_dirty store/release logic.

In `@crates/vm/src/protocol/callable.rs`:
- Around line 52-69: The code currently always calls self.obj.to_owned() for
non-Python callables, bumping refcounts even when tracing is disabled; change
the logic to first check the VM tracing flag (e.g., vm.use_tracing) and only
call self.obj.to_owned() when tracing is enabled, then call
vm.trace_event(TraceEvent::CCall, Some(callable.clone())) before invoking
(self.call) and vm.trace_event(TraceEvent::CReturn) /
vm.trace_event(TraceEvent::CException) afterwards using that owned `callable`;
otherwise, when tracing is disabled, directly invoke (self.call)(self.obj, args,
vm) without calling self.obj.to_owned() or any clone to avoid the hot-path
refcount bump.