crates/derive-impl/src/pymodule.rs (1)

233-243: Generated submodule init code panics on failure with no diagnostic context.

The four .unwrap() calls in the generated block will panic at runtime without any indication of which submodule failed or why. While the rest of the file also uses .unwrap() for __module_set_attr, those are single-attribute operations; here you have an entire module lifecycle (create_module → __init_methods → module_exec → set_attr) that could fail at multiple stages.

Consider using .expect(...) with the submodule name to improve debuggability:

Suggested improvement

 submodule_inits.push(quote! {
     #(`#cfgs`)*
     {
         let child_def = `#mod_ident`::module_def(ctx);
-        let child = child_def.create_module(vm).unwrap();
-        child.__init_methods(vm).unwrap();
-        `#mod_ident`::module_exec(vm, &child).unwrap();
+        let child = child_def.create_module(vm)
+            .expect(concat!("failed to create submodule ", `#py_name`));
+        child.__init_methods(vm)
+            .expect(concat!("failed to init methods for submodule ", `#py_name`));
+        `#mod_ident`::module_exec(vm, &child)
+            .expect(concat!("failed to exec submodule ", `#py_name`));
         let child: ::rustpython_vm::PyObjectRef = child.into();
         vm.__module_set_attr(module, ctx.intern_str(`#py_name`), child).unwrap();
     }
 });

crates/codegen/src/ir.rs (1)

247-273: Second NOP-removal pass looks correct; consider inlining a use for HashSet.

The two-pass approach makes sense: remove_nops() (line 195) handles pre-existing NOPs before optimization passes, while this pass handles NOPs freshly created by convert_pseudo_ops and label_exception_targets. The set-based deduplication logic is sound — a NOP is kept only when no real instruction covers its line and no other NOP for that line was already retained.

Minor nit: std::collections::HashSet is spelled out three times. A local use would reduce noise.

♻️ Optional: local import for readability

+        use std::collections::HashSet;
         // Collect lines that have non-NOP instructions in this block
-        let non_nop_lines: std::collections::HashSet<_> = block
+        let non_nop_lines: HashSet<_> = block
             .instructions
             .iter()
             .filter(|ins| !matches!(ins.instr.real(), Some(Instruction::Nop)))
             .map(|ins| ins.location.line)
             .collect();
-        let mut kept_nop_lines: std::collections::HashSet<OneIndexed> =
-            std::collections::HashSet::new();
+        let mut kept_nop_lines: HashSet<OneIndexed> = HashSet::new();

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

890-895: Avoid holding the registry lock while locking per-thread frames.

thread_frames.lock() is held while slot.frames.lock() is acquired for each entry. This creates lock contention and prevents thread registration/cleanup during the snapshot. The faulthandler.rs timeout handler demonstrates the correct pattern: clone slot references under the registry lock, drop the registry lock, then lock individual slot frames.

♻️ Suggested refactor (drop registry lock before per-slot locks)

-        let registry = vm.state.thread_frames.lock();
-        registry
-            .iter()
-            .filter_map(|(id, slot)| slot.frames.lock().last().cloned().map(|f| (*id, f)))
-            .collect()
+        let registry = vm.state.thread_frames.lock();
+        let slots: Vec<_> = registry.iter().map(|(id, slot)| (*id, slot.clone())).collect();
+        drop(registry);
+        slots
+            .into_iter()
+            .filter_map(|(id, slot)| slot.frames.lock().last().cloned().map(|f| (id, f)))
+            .collect()

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

466-503: Shared prev_line between tracing and monitoring may suppress first Line event after mid-frame settrace() if still on the same line.

The code unconditionally updates prev_line at line 502 outside the monitoring_mask check. While this is intentional (per the comment on line 484 to prevent spurious LINE events when monitoring is enabled mid-function), it means if sys.settrace() is called mid-frame while still on the same line, the first Line event will not fire until the line changes. This design prioritizes shared state efficiency over system independence. Clarify whether this matches CPython 3.12+ behavior; if stricter separation is needed, track prev_trace_line independently for tracing while retaining prev_line for monitoring's mid-function safety.

crates/vm/src/stdlib/sys/monitoring.rs (4)

430-451: FIRING guard is not panic-safe — use a RAII drop guard instead.

If a Rust panic occurs inside the callback loop (lines 432–448), line 450 is never reached and FIRING stays true for the thread permanently, silently suppressing all future monitoring events on that thread.

Proposed fix using a drop guard

+struct FiringGuard;
+impl FiringGuard {
+    fn new() -> Self {
+        FIRING.with(|f| f.set(true));
+        Self
+    }
+}
+impl Drop for FiringGuard {
+    fn drop(&mut self) {
+        FIRING.with(|f| f.set(false));
+    }
+}
+
 // ... inside fire_event_inner:
-    FIRING.with(|f| f.set(true));
-    let result = (|| {
+    let _guard = FiringGuard::new();
+    let result = (|| {
         for (tool, cb) in callbacks {
             // ...
         }
         Ok(())
     })();
-    FIRING.with(|f| f.set(false));
     result

---

265-275: Replace magic event-ID literals with derived constants.

18, 8, 9 correspond to the bit positions of BRANCH, BRANCH_LEFT, and BRANCH_RIGHT respectively, but bare literals are fragile and will silently break if bit assignments change.

Proposed fix

You could define constants at the module level:

const BRANCH_EVENT_ID: usize = MonitoringEvents::BRANCH.bits().trailing_zeros() as usize;
const BRANCH_LEFT_EVENT_ID: usize = MonitoringEvents::BRANCH_LEFT.bits().trailing_zeros() as usize;
const BRANCH_RIGHT_EVENT_ID: usize = MonitoringEvents::BRANCH_RIGHT.bits().trailing_zeros() as usize;

Then replace all magic numbers in register_callback and the fire_* functions accordingly.

---

356-356: Remove decorative section separator.

As per coding guidelines, "Do not add decorative section separators (e.g. // -----------, // ===, /* *** */). Use /// doc-comments or short // comments only when they add value".

---

457-726: Consider a macro to reduce boilerplate across fire_* functions.

All 17 fire_* functions follow the same pattern: extract code_id, build FuncArgs, call fire_event_inner. The differences are the event ID, event bit constant, and the argument list. A declarative macro could eliminate the repetition while keeping each variant's distinct arguments clear.

This is a readability/maintainability suggestion — no functional concern.

crates/derive-impl/src/pymodule.rs (1)

197-248: Submodule initialization looks correct; consider a minor robustness note on the NameValue case.

The overall logic — detect nested #[pymodule] mods, skip sub modules, generate cfg-guarded init code — is clean and consistent with the rest of the file.

One small observation: at Line 213, syn::Meta::NameValue silently returns Ok(()), meaning a malformed #[pymodule = "..."] annotation would be silently ignored rather than producing a compile-time diagnostic. This is unlikely in practice but could cause confusion if someone misuses the attribute.

Optional: emit an error for the NameValue case

-                    _ => return Ok(()),
+                    syn::Meta::NameValue(nv) => {
+                        bail_span!(nv, "#[pymodule] does not support `=` syntax");
+                    }

crates/vm/src/stdlib/sys/mod.rs (1)

1072-1079: Inconsistency with non-threading _current_frames stub.

The non-threading _current_exceptions returns a dict with key 0 and the actual exception, while the non-threading _current_frames (line 1084) returns an empty dict. Consider aligning them — either both return meaningful data for the main thread or both return an empty dict. Currently CPython returns both with the main thread's data.

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

1109-1155: resume_gen_frame lacks scopeguard for recursion_depth decrement.

If f(frame) panics, recursion_depth at line 1153 won't be decremented, and the exception stack / frame chain won't be cleaned up either (lines 1141–1151). While panics from Python execution are rare, with_frame_exc at least protects the recursion depth via with_recursion's scopeguard::defer!. Consider wrapping the cleanup in a scopeguard or a drop guard for consistency.

Proposed fix — use scopeguard for cleanup

         self.recursion_depth.update(|d| d + 1);
 
         self.frames.borrow_mut().push(frame.clone());
         let frame_ptr: *const Frame = &***frame;
         let old_frame = crate::vm::thread::set_current_frame(frame_ptr);
         frame.previous.store(
             old_frame as *mut Frame,
             core::sync::atomic::Ordering::Relaxed,
         );
-        // Inline exception push without thread exception update
         self.exceptions.borrow_mut().stack.push(exc);
         let old_owner = frame.owner.swap(
             crate::frame::FrameOwner::Thread as i8,
             core::sync::atomic::Ordering::AcqRel,
         );
 
         let result = f(frame);
 
-        // SAFETY: frame_ptr is valid because self.frames holds a clone
-        unsafe { &*frame_ptr }
-            .owner
-            .store(old_owner, core::sync::atomic::Ordering::Release);
-        // Inline exception pop without thread exception update
-        self.exceptions
-            .borrow_mut()
-            .stack
-            .pop()
-            .expect("pop_exception() without nested exc stack");
-        crate::vm::thread::set_current_frame(old_frame);
-        let _popped = self.frames.borrow_mut().pop();
-
-        self.recursion_depth.update(|d| d - 1);
+        scopeguard::defer! {
+            // SAFETY: frame_ptr is valid because self.frames holds a clone
+            unsafe { &*frame_ptr }
+                .owner
+                .store(old_owner, core::sync::atomic::Ordering::Release);
+            self.exceptions
+                .borrow_mut()
+                .stack
+                .pop()
+                .expect("pop_exception() without nested exc stack");
+            crate::vm::thread::set_current_frame(old_frame);
+            let _popped = self.frames.borrow_mut().pop();
+            self.recursion_depth.update(|d| d - 1);
+        }
+
         result

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

2021-2056: Duplicated branch monitoring code — extract into a helper.

The exact same branch-monitoring pattern (compute src_offset/dest_offset, check mask, fire branch_left/branch_right depending on branch_taken) appears in:

1. PopJumpIfNone (Lines 2029-2056)
2. PopJumpIfNotNone (Lines 2067-2094)
3. pop_jump_if (Lines 3109-3136)
4. execute_for_iter (Lines 3150-3164, 3188-3202)

This violates DRY — extract a small helper like fire_branch_event(&self, vm, src_offset, dest_offset, branch_taken).

♻️ Sketch of helper

/// Fire BRANCH_LEFT or BRANCH_RIGHT monitoring event.
fn fire_branch_monitoring(
    &self,
    vm: &VirtualMachine,
    src_offset: u32,
    dest_offset: u32,
    branch_taken: bool,
) {
    let monitoring_mask = vm.state.monitoring_events.load();
    if monitoring_mask
        & (crate::stdlib::sys::monitoring::EVENT_BRANCH_LEFT
            | crate::stdlib::sys::monitoring::EVENT_BRANCH_RIGHT)
        != 0
    {
        if branch_taken {
            let _ = crate::stdlib::sys::monitoring::fire_branch_right(
                vm, self.code, src_offset, dest_offset,
            );
        } else {
            let _ = crate::stdlib::sys::monitoring::fire_branch_left(
                vm, self.code, src_offset, dest_offset,
            );
        }
    }
}

Then each call site becomes a single line:

-// Fire BRANCH monitoring events
-let monitoring_mask = vm.state.monitoring_events.load();
-if monitoring_mask
-    & (crate::stdlib::sys::monitoring::EVENT_BRANCH_LEFT
-        | crate::stdlib::sys::monitoring::EVENT_BRANCH_RIGHT)
-    != 0
-{
-    let dest_offset = if branch_taken { target.0 * 2 } else { self.lasti() * 2 };
-    if branch_taken {
-        let _ = crate::stdlib::sys::monitoring::fire_branch_right(
-            vm, self.code, src_offset, dest_offset,
-        );
-    } else {
-        let _ = crate::stdlib::sys::monitoring::fire_branch_left(
-            vm, self.code, src_offset, dest_offset,
-        );
-    }
-}
+let dest_offset = if branch_taken { target.0 * 2 } else { self.lasti() * 2 };
+self.fire_branch_monitoring(vm, src_offset, dest_offset, branch_taken);

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

Also applies to: 2059-2094, 3096-3137

---

2908-2966: execute_call monitoring: is_python_call check is shallow.

Line 2930 uses callable.downcast_ref::<PyFunction>().is_some() to decide whether to fire C_RETURN/C_RAISE. This misses cases where a PyFunction is wrapped (e.g., bound methods, functools.partial, descriptors). A bound method wrapping a Python function would still be classified as a "C call" and fire C_RETURN/C_RAISE events incorrectly. CPython checks Py_TYPE(callable) == &PyFunction_Type which is similarly shallow, so this may be acceptable for now, but it's worth noting.

---

2929-2944: call_arg0 computation always allocates the MISSING sentinel when no args present, even when CALL monitoring is disabled.

The MISSING sentinel is only fetched when monitoring_mask & EVENT_CALL != 0 (Line 2933), so the guard is correct. However, get_missing(vm) acquires the monitoring lock every time. For the hot path (CALL with no args but monitoring active), consider caching MISSING at VM init time to avoid repeated lock acquisition.

crates/vm/src/vm/thread.rs (2)

133-142: get_all_current_exceptions holds the registry lock while cloning all exceptions.

Line 137 acquires vm.state.thread_frames.lock() and then iterates all slots calling slot.exception.to_owned() (Line 140). If to_owned() involves cloning PyObjectRef (ref-count bump), this holds the registry lock for O(threads) clone operations. For typical thread counts this is fine, but consider collecting just the Arc<ThreadSlot> references first, then releasing the lock before reading exceptions, to minimize contention.

---

15-21: ThreadSlot fields are pub but the struct itself should probably restrict visibility.

Both frames and exception are pub, allowing any code to directly lock/swap them. Consider restricting to pub(crate) to match the existing visibility pattern in this module (e.g., pub(crate) static CURRENT_FRAME).

crates/vm/src/stdlib/sys/monitoring.rs (3)

248-278: Magic event IDs: hardcoded 18, 8, 9 for BRANCH/BRANCH_LEFT/BRANCH_RIGHT.

Lines 265, 267, 268, 272-274 use raw numeric event IDs. These should use named constants derived from the bitflags to prevent silent breakage if event positions change.

♻️ Proposed fix

+// Event ID constants (bit position = trailing_zeros of the corresponding bit)
+const EVENT_ID_BRANCH_LEFT: usize = MonitoringEvents::BRANCH_LEFT.bits().trailing_zeros() as usize;
+const EVENT_ID_BRANCH_RIGHT: usize = MonitoringEvents::BRANCH_RIGHT.bits().trailing_zeros() as usize;
+const EVENT_ID_BRANCH: usize = MonitoringEvents::BRANCH.bits().trailing_zeros() as usize;

 // In register_callback:
-        if event_id == 18 {
-            // BRANCH → BRANCH_LEFT (8) + BRANCH_RIGHT (9)
-            state.callbacks.insert((tool, 8), func.clone());
-            state.callbacks.insert((tool, 9), func);
+        if event_id == EVENT_ID_BRANCH {
+            state.callbacks.insert((tool, EVENT_ID_BRANCH_LEFT), func.clone());
+            state.callbacks.insert((tool, EVENT_ID_BRANCH_RIGHT), func);
         }

---

356-356: Remove decorative section separator.

Line 356 // ── Event dispatch ──... is a decorative separator. As per coding guidelines: "Do not add decorative section separators (e.g. // -----------, // ===, /* *** */). Use /// doc-comments or short // comments only when they add value."

---

84-107: MonitoringState uses HashMap for local_events, callbacks, and disabled — consider FxHashMap for better performance.

These maps are accessed under the monitoring lock on every event dispatch (fire_event_inner, Lines 407-418). The standard HashMap uses SipHash which is slower than FxHashMap for small integer keys (usize, usize). Since this is a runtime VM, using rustc_hash::FxHashMap (or similar) could reduce overhead when monitoring is active.

crates/codegen/src/ir.rs (1)

253-273: Consider using an import for std::collections::HashSet.

std::collections::HashSet is used three times inline (lines 254, 260, 261). A local use or a module-level import would reduce verbosity.

♻️ Suggested refactor

+        use std::collections::HashSet;
+
         // Collect lines that have non-NOP instructions in this block
-        let non_nop_lines: std::collections::HashSet<_> = block
+        let non_nop_lines: HashSet<_> = block
             .instructions
             .iter()
             .filter(|ins| !matches!(ins.instr.real(), Some(Instruction::Nop)))
             .map(|ins| ins.location.line)
             .collect();
-        let mut kept_nop_lines: std::collections::HashSet<OneIndexed> =
-            std::collections::HashSet::new();
+        let mut kept_nop_lines: HashSet<OneIndexed> = HashSet::new();

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

2692-2707: EXCEPTION_HANDLED hook placement looks correct

Please also confirm fmt/clippy and the bytecode clean-build command were run since instruction handling changed.

Commands to run locally:

cargo fmt
cargo clippy --all-targets --all-features
rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r

As per coding guidelines: "Follow the default rustfmt code style by running cargo fmt to format Rust code", "Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes", and "When modifying bytecode instructions, perform a clean build by running: rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r".

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

1148-1148: Triple deref (&***frame) is hard to follow — consider a helper or a brief inline comment.

frame is &FrameRef (&PyRef<Frame>), so &***frame dereferences through & → PyRef<Frame> → Py<Frame> → Frame then re-borrows to &Frame. It works, but is a readability hazard. A named binding or a brief // &FrameRef → &Frame comment would help future readers.

crates/vm/src/stdlib/sys/monitoring.rs (3)

456-726: Magic event-id literals scattered across all fire_* functions.

Every fire_* function passes a raw integer (e.g., 0, 1, 11, 16) as the event_id to fire_event_inner. A single typo would silently route callbacks to the wrong event with no compile-time protection. Consider defining named constants (e.g., const EVENT_ID_PY_START: usize = 0;) or deriving the id from the bit position of the corresponding EVENT_* constant (EVENT_PY_START.trailing_zeros() as usize).

Example using trailing_zeros

 pub fn fire_py_start(vm: &VirtualMachine, code: &PyRef<PyCode>, offset: u32) -> PyResult<()> {
     let code_id = code.get_id();
     let args = FuncArgs::from(vec![code.clone().into(), vm.ctx.new_int(offset).into()]);
-    fire_event_inner(vm, 0, EVENT_PY_START, code_id, offset, &args)
+    fire_event_inner(vm, EVENT_PY_START.trailing_zeros() as usize, EVENT_PY_START, code_id, offset, &args)
 }

Or, have fire_event_inner derive event_id from event_bit internally, eliminating the redundant parameter.

---

234-240: free_tool_id double-locks: clear_tool_id locks+unlocks, then line 237 locks again.

This is both a minor performance issue and the same TOCTOU pattern flagged on set_events. Consider inlining the clear logic or taking the lock once.

Sketch

 fn free_tool_id(tool_id: i32, vm: &VirtualMachine) -> PyResult<()> {
     let tool = check_valid_tool(tool_id, vm)?;
-    clear_tool_id(tool_id, vm)?;
     let mut state = vm.state.monitoring.lock();
+    if state.tool_names[tool].is_some() {
+        state.global_events[tool] = 0;
+        state.local_events.retain(|(local_tool, _), _| *local_tool != tool);
+        state.callbacks.retain(|(cb_tool, _), _| *cb_tool != tool);
+        state.disabled.retain(|&(_, _, t)| t != tool);
+    }
     state.tool_names[tool] = None;
+    update_events_mask(vm, &state);
     Ok(())
 }

---

84-107: MonitoringState fields are all pub — consider restricting visibility.

All fields of MonitoringState are public, allowing any code to mutate internal invariants (e.g., global_events, disabled) without going through the validation functions. Since this struct is behind a PyMutex, direct field access from outside this module could bypass the event normalization and mask update logic. Making fields pub(crate) or private with accessors would tighten the contract.

crates/codegen/src/ir.rs (1)

247-273: Consider using an imported HashSet instead of repeated inline paths.

std::collections::HashSet is spelled out in full three times within this block. A use import at the top of the file (or the function) would be slightly cleaner.

Also, a minor observation on correctness: this second NOP-removal pass (post-convert_pseudo_ops) correctly complements the earlier remove_nops pass (pre-pseudo-conversion). New NOPs introduced by convert_pseudo_ops (from SetupFinally, SetupWith, SetupCleanup, PopBlock) are properly filtered here against non_nop_lines, so line-marker uniqueness is preserved.

♻️ Suggested import cleanup

Add to the existing imports at the top of the file:

use std::collections::HashSet;

Then simplify the block:

-            let non_nop_lines: std::collections::HashSet<_> = block
+            let non_nop_lines: HashSet<_> = block
                 .instructions
                 .iter()
                 .filter(|ins| !matches!(ins.instr.real(), Some(Instruction::Nop)))
                 .map(|ins| ins.location.line)
                 .collect();
-            let mut kept_nop_lines: std::collections::HashSet<OneIndexed> =
-                std::collections::HashSet::new();
+            let mut kept_nop_lines: HashSet<OneIndexed> = HashSet::new();

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

103-112: Unsafe raw-pointer alias of &self.exception — sound but brittle.

The raw pointer on line 105 is used to sidestep the borrow checker: self is already borrowed, and the closure passed to resume_gen_frame needs mutable-like access to self.exception. The running flag logically guarantees exclusive access. This is sound provided resume_gen_frame invokes the closure synchronously (before the &self borrow expires) and never stores it.

Two observations:

1. The _old value discarded on line 110 should always be None (swapped in on line 104, with no intervening writes due to the running guard). If that invariant matters, a debug_assert!(_old.is_none()) would make it explicit and catch regressions.

2. Consider whether resume_gen_frame could be refactored to return the exception state separately, eliminating the need for the raw pointer entirely. That would remove both unsafe blocks here.

💡 Optional: add a debug assertion

             // SAFETY: exclusive access guaranteed by running flag
-            let _old = unsafe { (*exception_ptr).swap(vm.current_exception()) };
+            let old = unsafe { (*exception_ptr).swap(vm.current_exception()) };
+            debug_assert!(old.is_none(), "exception slot should have been None");
             result

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

2608-2692: Stateless incremental encoder/decoder: significant code duplication.

StatelessIncrementalEncoder::encode and StatelessIncrementalDecoder::decode share nearly identical logic (build args → call function → validate tuple → return first element). Consider extracting a shared helper to reduce duplication.

♻️ Suggested shared helper

+    fn call_stateless_codec(
+        func: &PyObjectRef,
+        input: PyObjectRef,
+        errors: &Option<PyStrRef>,
+        role: &str, // "encoder" or "decoder"
+        vm: &VirtualMachine,
+    ) -> PyResult {
+        let mut args: Vec<PyObjectRef> = vec![input];
+        if let Some(errors) = errors {
+            args.push(errors.to_owned().into());
+        }
+        let res = func.call(args, vm)?;
+        let tuple: PyTupleRef = res.try_into_value(vm)?;
+        if tuple.len() != 2 {
+            return Err(vm.new_type_error(format!(
+                "{role} must return a tuple (object, integer)"
+            )));
+        }
+        Ok(tuple[0].clone())
+    }

Then both encode and decode become one-liners delegating to this helper.