vm/src/builtins/function.rs (2)

403-408: Use the cloned code for consistency (avoid TOCTOU and extra lock)

You already cloned code above. Re-locking for flags can observe a different code object and adds overhead. Use code.flags.

-        let locals = if self
-            .code
-            .lock()
-            .flags
-            .contains(bytecode::CodeFlags::NEW_LOCALS)
-        {
+        let locals = if code.flags.contains(bytecode::CodeFlags::NEW_LOCALS) {

---

463-466: Invalidate JIT when __code__ changes

Changing __code__ should drop any cached compiled code; otherwise, stale JIT may run with mismatched code/flags.

     #[pygetset(setter)]
     fn set___code__(&self, code: PyRef<PyCode>) {
         *self.code.lock() = code;
+        #[cfg(feature = "jit")]
+        {
+            // If available, clear cached compiled code. Adjust to actual API of OnceCell.
+            let _ = self.jitted_code.take();
+        }
     }

Please verify crate::common::lock::OnceCell exposes take() (or an equivalent method). If not, we may need a small wrapper or to recreate the OnceCell via interior mutability.

vm/src/builtins/code.rs (2)

608-618: Unsafe co_code byte view: consider safe encode path

Current approach assumes CodeUnit is exactly 2 bytes and tightly packed. If that invariant ever changes, this breaks.

Optionally, re-encode via iterator for safety.

-        // SAFETY: CodeUnit is #[repr(C)] with size 2, so we can safely transmute to bytes
-        let bytes = unsafe {
-            std::slice::from_raw_parts(
-                self.code.instructions.as_ptr() as *const u8,
-                self.code.instructions.len() * 2,
-            )
-        };
-        vm.ctx.new_bytes(bytes.to_vec())
+        // Safer: rebuild from op/arg pairs (avoids layout assumptions)
+        let mut out = Vec::with_capacity(self.code.instructions.len() * 2);
+        for cu in self.code.instructions.iter() {
+            out.push(u8::from(cu.op));
+            out.push(cu.arg.0);
+        }
+        vm.ctx.new_bytes(out)

---

1037-1055: parse_bytecode(): minor diagnostics improvement (optional)

Consider returning a PyErr with precise position/opcode for easier debugging rather than None.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

vm/src/builtins/function.rs (2)

99-105: Avoid holding the code lock across the entire argument fill

let code = &*self.code.lock(); keeps the lock for the whole function scope. Clone the PyRef<PyCode> and release the lock early.

Apply:

-        let code = &*self.code.lock();
+        let code = self.code.lock().clone(); // PyRef<PyCode>
+        let code = &*code; // drop lock; use local clone

Keeps critical section small and reduces contention.

---

401-409: Reuse the cloned code instead of re-locking for NEW_LOCALS check

You already cloned code above; use code.flags here to avoid an extra lock and potential inconsistency.

Apply:

-        let locals = if self
-            .code
-            .lock()
-            .flags
-            .contains(bytecode::CodeFlags::NEW_LOCALS)
-        {
+        let locals = if code.flags.contains(bytecode::CodeFlags::NEW_LOCALS) {

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

vm/src/builtins/code.rs (2)

507-515: Compute cell2arg in constructor; closures capturing args currently broken

cell2arg is left as None, so Frame.fill_locals_from_args won’t populate cells for arguments captured by closures. Compute the mapping from each cellvar to its argument index (or -1). This is a correctness bug affecting closures created from this constructor.

Apply:

-            cell2arg: None, // TODO: reuse `fn cell2arg`
+            // Map each cellvar to its argument index or -1 if not an argument
+            cell2arg: Some(
+                cellvars
+                    .iter()
+                    .map(|cv| {
+                        varnames
+                            .iter()
+                            .position(|vn| vn.as_str() == cv.as_str())
+                            .map(|i| i as i32)
+                            .unwrap_or(-1)
+                    })
+                    .collect::<Vec<_>>()
+                    .into_boxed_slice(),
+            ),

---

977-1006: Locations must match instruction count; rebuild when co_code changes

locations is cloned even if instructions length changes. This breaks invariants and downstream consumers. Recompute locations to instructions.len() using first_line_number when co_code is replaced.

Apply:

-        let new_code = CodeObject {
+        // Ensure locations length matches instructions length
+        let locations = if instructions.len() == self.code.instructions.len() {
+            self.code.locations.clone()
+        } else {
+            let row = first_line_number.unwrap_or(OneIndexed::MIN);
+            vec![
+                rustpython_compiler_core::SourceLocation {
+                    // NOTE: confirm field names below in core crate
+                    row,
+                    column: OneIndexed::from_zero_indexed(0),
+                };
+                instructions.len()
+            ]
+            .into_boxed_slice()
+        };
+
+        let new_code = CodeObject {
             flags: CodeFlags::from_bits_truncate(flags),
@@
-            instructions,
-            // FIXME: invalid locations. Actually locations is a duplication of linetable.
-            // It can be removed once we move every other code to use linetable only.
-            locations: self.code.locations.clone(),
+            instructions,
+            locations,

vm/src/builtins/function.rs (2)

99-160: Optional: shrink code lock scope in fill_locals_from_args

The code lock is held across argument parsing. Copy small scalars (counts/flags) then drop the lock to reduce contention. Behavior unchanged.

Sketch:

• Lock, read arg_count, kwonlyarg_count, posonlyarg_count, flags, varnames ref (clone Arc/Ptr if cheap), then release.
• Proceed with locals filling.

Also applies to: 196-205, 272-303

---

348-363: Optional: validate closure length in set_function_attribute

When setting CLOSURE via MakeFunctionFlags, consider validating tuple length equals code.freevars.len() to match new checks and fail fast.

Example:

-            self.closure = Some(closure_tuple.try_into_typed::<PyCell>(vm)?);
+            let typed = closure_tuple.try_into_typed::<PyCell>(vm)?;
+            if typed.len() != self.code.lock().freevars.len() {
+                return Err(vm.new_value_error("closure size mismatch with freevars"));
+            }
+            self.closure = Some(typed);

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

vm/src/builtins/code.rs (3)

507-508: cell2arg left as None breaks closures capturing arguments.

This issue has been previously identified. Setting cell2arg to None prevents closures from properly capturing function arguments. The cell2arg mapping must be computed to map each cellvar to its corresponding argument index.

Based on past review comments, the fix is to compute the mapping:

-            cell2arg: None, // TODO: reuse `fn cell2arg`
+            cell2arg: Some(
+                cellvars
+                    .iter()
+                    .map(|cv| {
+                        varnames
+                            .iter()
+                            .position(|vn| vn.as_str() == cv.as_str())
+                            .map(|i| i as i32)
+                            .unwrap_or(-1)
+                    })
+                    .collect::<Vec<_>>()
+                    .into_boxed_slice(),
+            ),

---

1001-1005: cell2arg not recomputed when varnames/cellvars change.

This issue was previously identified. When varnames or cellvars are modified via replace(), the cell2arg mapping must be recomputed to maintain correctness. Currently, it always clones the old mapping.

Based on past review comments, the fix is:

             cellvars,
             freevars,
-            cell2arg: self.code.cell2arg.clone(),
+            cell2arg: if matches!(co_varnames, OptionalArg::Present(_))
+                || matches!(co_cellvars, OptionalArg::Present(_))
+            {
+                Some(
+                    cellvars
+                        .iter()
+                        .map(|cv| {
+                            varnames
+                                .iter()
+                                .position(|vn| vn.as_str() == cv.as_str())
+                                .map(|i| i as i32)
+                                .unwrap_or(-1)
+                        })
+                        .collect::<Vec<_>>()
+                        .into_boxed_slice(),
+                )
+            } else {
+                self.code.cell2arg.clone()
+            },
             linetable,
             exceptiontable,

---

989-991: Locations remain inconsistent when instructions change.

The FIXME comment acknowledges that locations is kept from the old code object even when instructions may have changed. While a past review comment about this was marked as addressed in commit fca521c, the current code still has the issue. When co_code is replaced with a different number of instructions, the locations vector length won't match instructions.len().

The fix should ensure locations length matches instructions length:

+        // Ensure locations length matches instructions length
+        let locations = if instructions.len() == self.code.instructions.len() {
+            self.code.locations.clone()
+        } else {
+            let row = first_line_number.unwrap_or(OneIndexed::MIN);
+            vec![
+                rustpython_compiler_core::SourceLocation {
+                    line: row,
+                    character_offset: OneIndexed::from_zero_indexed(0),
+                };
+                instructions.len()
+            ]
+            .into_boxed_slice()
+        };
+
         let new_code = CodeObject {
             flags: CodeFlags::from_bits_truncate(flags),
             posonlyarg_count,
             arg_count,
             kwonlyarg_count,
             source_path: source_path.as_object().as_interned_str(vm).unwrap(),
             first_line_number,
             obj_name: obj_name.as_object().as_interned_str(vm).unwrap(),
             qualname: qualname.as_object().as_interned_str(vm).unwrap(),
 
             max_stackdepth,
             instructions,
-            // FIXME: invalid locations. Actually locations is a duplication of linetable.
-            // It can be removed once we move every other code to use linetable only.
-            locations: self.code.locations.clone(),
+            locations,

vm/src/builtins/function/jit.rs (2)

67-107: Consider reducing the lock scope for better concurrency.

The code lock is held for the entire duration of annotation processing and iteration. Consider extracting only the necessary metadata (arg_names, flags) into local variables, then releasing the lock before the annotation processing loop. This would improve concurrency when multiple threads JIT functions simultaneously.

Apply this pattern:

 pub fn get_jit_arg_types(func: &Py<PyFunction>, vm: &VirtualMachine) -> PyResult<Vec<JitType>> {
-    let code = func.code.lock();
-    let arg_names = code.arg_names();
+    let (arg_names, has_varargs_or_varkeywords) = {
+        let code = func.code.lock();
+        let arg_names = code.arg_names();
+        let flags = code.flags.intersects(CodeFlags::HAS_VARARGS | CodeFlags::HAS_VARKEYWORDS);
+        (arg_names.to_owned(), flags)
+    };
 
-    if code
-        .flags
-        .intersects(CodeFlags::HAS_VARARGS | CodeFlags::HAS_VARKEYWORDS)
-    {
+    if has_varargs_or_varkeywords {
         return Err(new_jit_error(
             "Can't jit functions with variable number of arguments".to_owned(),
             vm,
         ));
     }

Note: You'd need to own the arg_names data or adjust the extraction strategy based on the Arguments struct definition.

---

161-221: Consider reducing the lock scope for better performance.

The code lock is held for approximately 60 lines while processing arguments, defaults, and keyword defaults. This lock duration could impact concurrent JIT compilation attempts. Consider extracting only the necessary metadata upfront, then releasing the lock before the argument processing logic.

Additionally, the explicit drop(code) at line 221 is unnecessary since the lock would be automatically released at function exit. While harmless, it adds clutter.

-    drop(code);
-
     jit_args.into_args().ok_or(ArgsError::NotAllArgsPassed)

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

vm/src/builtins/code.rs (3)

507-507: cell2arg left as None breaks closures capturing arguments.

This issue has already been identified in previous reviews. When creating code via the constructor, closures will not get argument cells populated correctly since Frame.fill_locals_from_args relies on cell2arg. The mapping should be computed by matching each cellvar to its corresponding argument index in varnames.

---

986-988: Locations can become inconsistent when co_code changes.

The FIXME comment correctly identifies that keeping old locations while changing instruction count breaks invariants. This issue was previously identified and the code should recompute locations to match instructions.len() when the instruction count changes.

---

1000-1000: cell2arg not recomputed when varnames/cellvars change.

This issue has already been identified in previous reviews. When co_varnames or co_cellvars are provided in replace(), the cell2arg mapping should be recomputed to reflect the new variable-to-cell mappings, otherwise closures will have incorrect argument cells.

vm/src/builtins/code.rs (1)

1036-1047: LGTM! Pragmatic bytecode validation helper.

The helper function correctly validates bytecode length (must be even for 2-byte instructions) and delegates to parse_instructions_from_bytes. The comment acknowledges that using MarshalError is temporary, which is reasonable for this refactoring stage.

For future consideration: You may want to introduce a dedicated error type for bytecode validation in the VM layer to avoid coupling with marshal errors, but the current approach is acceptable.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro