Actionable comments posted: 7

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

2679-2687: Extract duplicated deopt blocks into helpers.

These branches all perform the same deopt sequence (replace_op + write_adaptive_counter). A small helper per opcode family would reduce repetition and drift risk.

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: 2719-2726, 2751-2758, 2884-2894, 2922-2932

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 2679 - 2687, Several branches perform
the same deopt sequence (calling self.code.instructions.replace_op(...) followed
by write_adaptive_counter(...)) and should be consolidated: add a small helper
on the Frame (or Instructions) such as deopt_replace_and_backoff(&mut self,
instr_idx: usize, new_op: Instruction, cache_base: usize) that calls
self.code.instructions.replace_op(instr_idx, new_op) and then
self.code.instructions.write_adaptive_counter(cache_base,
ADAPTIVE_BACKOFF_VALUE); then replace the duplicated blocks (e.g., the places
that call replace_op with Instruction::LoadAttr { idx: Arg::marker() } and
similar variants) to compute the differing value (the Instruction variant and
cache_base) and call the helper instead to remove duplication and keep the same
behavior.

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

Inline comments:
In `@crates/compiler-core/src/bytecode.rs`:
- Around line 555-571: The function quicken holds a shared borrow (let units =
unsafe { &*self.0.get() }) across the loop while calling write_adaptive_counter
which performs a mutable borrow, causing aliasing UB; fix by limiting the shared
borrow's lifetime so you only read what you need (e.g., copy units[i].op, call
op.cache_entries() and op.is_instrumented() into locals) while the shared borrow
is active, then drop that borrow (end the unsafe &*self.0.get() scope) before
invoking write_adaptive_counter(cache_base, ADAPTIVE_WARMUP_VALUE); in short,
ensure quicken does not keep units alive across the mutable call to
write_adaptive_counter.

In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 515-629: deoptimize() currently falls through for JumpBackwardJit
and JumpBackwardNoJit which leaves them specialized instead of mapping back to a
base opcode; add explicit match arms for Self::JumpBackwardJit and
Self::JumpBackwardNoJit to return the appropriate base (e.g. Self::JumpBackward
{ target: Arg::marker() } or Self::JumpBackwardNoInterrupt { target:
Arg::marker() } depending on intended semantics), similar to other
specialization branches, so marshaling via original_bytes() and PEP 669
monitoring see only base opcodes; reference deoptimize(), JumpBackwardJit,
JumpBackwardNoJit, JumpBackward, JumpBackwardNoInterrupt and to_base() when
making this change and verify mapping choice against NoJit semantics.

In `@crates/vm/src/builtins/function.rs`:
- Around line 611-621: can_specialize_call currently allows exact-args fast-path
for code objects that may rely on module globals when NEWLOCALS is unset,
changing semantics; update can_specialize_call to also require the
bytecode::CodeFlags::NEWLOCALS flag (i.e., only return true when NEWLOCALS is
set) so invoke_exact_args and invoke_with_locals remain equivalent, and apply
the same NEWLOCALS guard to the similar specialization logic in the other block
referenced around lines 627-660 (where the same flags and arg count checks are
performed).

In `@crates/vm/src/builtins/type.rs`:
- Around line 202-210: The current assign_version_tag uses
NEXT_TYPE_VERSION.fetch_add and treats the returned previous value v as the
assigned tag, which fails on wraparound because tags can become 0 or be reused;
change logic to compute the new tag as v.wrapping_add(1) (call it new_v), check
if new_v == 0 (wrap occurred) and in that case store 0 into self.tp_version_tag
and return 0, otherwise store new_v with Ordering::Release and return new_v;
update references in assign_version_tag to use new_v instead of v and keep
atomic ordering semantics.

In `@crates/vm/src/frame.rs`:
- Around line 2702-2704: The instance-dict lookup currently swallows errors via
`.ok().flatten()` which hides lookup exceptions and can wrongly take the fast
cached path; change the `shadowed` computation (the `if let Some(dict) =
owner.dict() { dict.get_item_opt(attr_name, vm).ok().flatten().is_some() } else
{ ... }` code) to let lookup errors propagate instead of being dropped — e.g.
call `dict.get_item_opt(attr_name, vm)?` and check `.is_some()` (or explicitly
match `Ok(Some(_)) / Ok(None) / Err(e) => return Err(e)`), so attribute lookup
failures from `get_item_opt` bubble up rather than being ignored.

---

Outside diff comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 1923-1945: The descriptor-path in setattro uses an early return
when calling the descriptor (via descr_set and descriptor(&attr,
zelf.to_owned().into(), value, vm)), so zelf.modified() is never executed for
descriptor-backed setters; change that return into capturing the descriptor call
result (e.g. let res = descriptor(...) ), call zelf.modified() before returning,
and then return res (or propagate the error) so that any mutations performed by
descriptor setters still trigger the inline-cache invalidation; ensure this
touches the block using zelf.get_class_attr, descr_set, descriptor(...), and the
existing zelf.modified() call.

---

Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 2679-2687: Several branches perform the same deopt sequence
(calling self.code.instructions.replace_op(...) followed by
write_adaptive_counter(...)) and should be consolidated: add a small helper on
the Frame (or Instructions) such as deopt_replace_and_backoff(&mut self,
instr_idx: usize, new_op: Instruction, cache_base: usize) that calls
self.code.instructions.replace_op(instr_idx, new_op) and then
self.code.instructions.write_adaptive_counter(cache_base,
ADAPTIVE_BACKOFF_VALUE); then replace the duplicated blocks (e.g., the places
that call replace_op with Instruction::LoadAttr { idx: Arg::marker() } and
similar variants) to compute the differing value (the Instruction variant and
cache_base) and call the helper instead to remove duplication and keep the same
behavior.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 7b89d82 and 821e648.

• .cspell.dict/cpython.txt
• .cspell.json
• crates/codegen/src/ir.rs
• crates/compiler-core/src/bytecode.rs
• crates/compiler-core/src/bytecode/instruction.rs
• crates/compiler-core/src/marshal.rs
• crates/vm/src/builtins/code.rs
• crates/vm/src/builtins/function.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/frame.rs
• crates/vm/src/object/core.rs
• crates/vm/src/stdlib/sys/monitoring.rs

Actionable comments posted: 4

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

2722-2738: ⚠️ Potential issue | 🟠 Major

Propagate dict lookup errors instead of retrying via slow path.

The Err(_) arm deoptimizes and re-runs lookup through load_attr_slow, which drops the original error and can duplicate side effects.

💡 Proposed fix

-                            Err(_) => {
-                                // Dict lookup error → deoptimize to safe path
-                                unsafe {
-                                    self.code.instructions.replace_op(
-                                        instr_idx,
-                                        Instruction::LoadAttr { idx: Arg::marker() },
-                                    );
-                                    self.code
-                                        .instructions
-                                        .write_adaptive_counter(cache_base, ADAPTIVE_BACKOFF_VALUE);
-                                }
-                                return self.load_attr_slow(vm, oparg);
-                            }
+                            Err(e) => {
+                                // Dict lookup error should propagate; keep deopt for future runs.
+                                unsafe {
+                                    self.code.instructions.replace_op(
+                                        instr_idx,
+                                        Instruction::LoadAttr { idx: Arg::marker() },
+                                    );
+                                    self.code
+                                        .instructions
+                                        .write_adaptive_counter(cache_base, ADAPTIVE_BACKOFF_VALUE);
+                                }
+                                return Err(e);
+                            }

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 2722 - 2738, The dict lookup Err branch
currently swallows the original error, deoptimizes, and re-runs via
load_attr_slow; instead capture and propagate the original error from
dict.get_item_opt(attr_name, vm) so callers see the original failure and side
effects are not duplicated. Replace the Err(_) arm in the match over
dict.get_item_opt with Err(err) => return Err(err) (or use the ? operator)
rather than calling self.code.instructions.replace_op / write_adaptive_counter
and return self.load_attr_slow; leave the deoptimization path only for non-error
conditions where you intentionally want to fall back to the safe path.

crates/vm/src/builtins/type.rs (3)

54-56: Avoid exposing tp_version_tag: AtomicU32 as a public writable field (easy invariant break).

Making the raw atomic public means any module can store arbitrary values (including non-0 “invalid” states) and bypass invalidation rules. Prefer pub(crate) + accessor(s), or keep it private and expose only load_tp_version_tag() / invalidate_tp_version_tag() / assign_version_tag().

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/type.rs` around lines 54 - 56, The field
tp_version_tag: AtomicU32 is currently public and allows external code to break
invariants by writing arbitrary values; change its visibility to non-public
(make it private or pub(crate)) and add controlled accessors such as
load_tp_version_tag(&self) -> u32, assign_version_tag(&self, new: u32) to set a
valid tag, and invalidate_tp_version_tag(&self) to set it to 0, ensuring writers
use AtomicU32 methods internally and enforcing any validation/ordering
semantics; update call sites that reference tp_version_tag directly to use these
new methods.

---

219-228: modified() should avoid unwrap() + consider iterative traversal to prevent deep recursion.

sub.downcast_ref::<PyType>().unwrap() will panic if subclasses ever contains a non-PyType (even transiently). Also, recursive subclass traversal can stack overflow in pathological hierarchies. An iterative stack/VecDeque walk is safer.

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/type.rs` around lines 219 - 228, In modified(), avoid
unwrap and recursion: replace the recursive call to
sub.downcast_ref::<PyType>().unwrap().modified() with an iterative traversal
using a stack/VecDeque of upgraded PyType refs; for each upgraded object, call
downcast_ref::<PyType>() and if it returns Some, store its tp_version_tag with
Ordering::Release and push its subclasses' weak refs (upgrading them when
needed) onto the deque, otherwise skip that entry (do not panic). Keep the
initial tp_version_tag.store(0, Ordering::Release) behavior for self and ensure
you only call modified-equivalent logic on successfully downcasted PyType
instances.

---

202-217: Simplify assign_version_tag() with fetch_update to reduce spin-loop complexity.

The current CAS loop pattern is correct, but fetch_update reduces boilerplate and makes the code less error-prone:

Proposed simplification

 pub fn assign_version_tag(&self) -> u32 {
-    loop {
-        let current = NEXT_TYPE_VERSION.load(Ordering::Relaxed);
-        let Some(next) = current.checked_add(1) else {
-            return 0; // Overflow: version space exhausted
-        };
-        if NEXT_TYPE_VERSION
-            .compare_exchange_weak(current, next, Ordering::Relaxed, Ordering::Relaxed)
-            .is_ok()
-        {
-            self.tp_version_tag.store(current, Ordering::Release);
-            return current;
-        }
-    }
+    let assigned = NEXT_TYPE_VERSION
+        .fetch_update(Ordering::Relaxed, Ordering::Relaxed, |current| current.checked_add(1))
+        .ok()
+        .unwrap_or(0);
+
+    if assigned == 0 {
+        return 0;
+    }
+    self.tp_version_tag.store(assigned, Ordering::Release);
+    assigned
 }

Note: Overflow (returning 0) is intentional—unversioned types don't use inline caching per the guard checks in frame.rs, and the backoff mechanism prevents re-attempting on overflow.

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/type.rs` around lines 202 - 217, Replace the explicit
CAS spin-loop in assign_version_tag() with AtomicU32::fetch_update on
NEXT_TYPE_VERSION: call fetch_update(Relaxed, Relaxed, |current|
current.checked_add(1).ok_or(current)) and handle the Result—on Ok(prev) store
prev into self.tp_version_tag with Ordering::Release and return prev; on Err(_)
treat as overflow and return 0. Keep the same ordering semantics and the
overflow behavior (return 0) and reference NEXT_TYPE_VERSION and tp_version_tag
in your change.

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

2699-2707: Extract duplicated LOAD_ATTR deopt sequence into a helper.

The same deopt/backoff block is repeated in multiple branches; consolidating it will reduce drift risk.

♻️ Refactor sketch

+    #[inline]
+    fn deoptimize_load_attr(&mut self, instr_idx: usize, cache_base: usize) {
+        unsafe {
+            self.code
+                .instructions
+                .replace_op(instr_idx, Instruction::LoadAttr { idx: Arg::marker() });
+            self.code
+                .instructions
+                .write_adaptive_counter(cache_base, ADAPTIVE_BACKOFF_VALUE);
+        }
+    }
...
-                unsafe {
-                    self.code
-                        .instructions
-                        .replace_op(instr_idx, Instruction::LoadAttr { idx: Arg::marker() });
-                    self.code
-                        .instructions
-                        .write_adaptive_counter(cache_base, ADAPTIVE_BACKOFF_VALUE);
-                }
+                self.deoptimize_load_attr(instr_idx, cache_base);

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: 2755-2762, 2787-2794

🤖 Prompt for AI Agents

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

In `@crates/vm/src/frame.rs` around lines 2699 - 2707, Duplicate deopt/backoff
logic for LOAD_ATTR should be extracted into a single helper to avoid
repetition: create a method (e.g., deopt_load_attr or write_load_attr_deopt) on
the same type as self that takes the differing values (instr_idx and
cache_base), and move the unsafe block that calls
self.code.instructions.replace_op(... Instruction::LoadAttr { idx: Arg::marker()
}) and self.code.instructions.write_adaptive_counter(cache_base,
ADAPTIVE_BACKOFF_VALUE) into that helper; then replace each repeated block with
a call to this helper passing instr_idx and cache_base. Ensure the helper
preserves the unsafe semantics and references Instruction::LoadAttr,
Arg::marker(), write_adaptive_counter, and ADAPTIVE_BACKOFF_VALUE.

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

Inline comments:
In `@crates/vm/src/builtins/function.rs`:
- Around line 602-663: invoke_exact_args currently assumes preconditions and can
OOB panic; add debug-only guards to assert those preconditions (or make the
function non-public if only internal). In the body of invoke_exact_args (and/or
at its start) add debug_asserts: debug_assert_eq!(args.len(), code.arg_count as
usize); debug_assert!(code.flags.contains(bytecode::CodeFlags::NEWLOCALS));
debug_assert_eq!(code.kwonlyarg_count, 0); and
debug_assert!(!code.flags.intersects(bytecode::CodeFlags::VARARGS |
bytecode::CodeFlags::VARKEYWORDS | bytecode::CodeFlags::GENERATOR |
bytecode::CodeFlags::COROUTINE)); this will prevent OOB writes during
development (or alternatively change pub fn invoke_exact_args to pub(crate) if
it is not externally used).
- Around line 684-686: The cache invalidation currently sets func_version to 0
in setters like set___code__, set___defaults__, and set___kwdefaults__, which
permanently disables re-validation; instead either (A) update those setters to
assign a fresh non-zero version by using FUNC_VERSION_COUNTER.fetch_add(1,
Relaxed) when invalidating func_version so subsequent validations can succeed,
or (B) add a helper fn assign_func_version(&self) -> u32 that generates/sets a
non-zero version (using FUNC_VERSION_COUNTER) and call that helper from the
hot-path cache validation code whenever func_version == 0 so the function can be
re-validated; update references to func_version, set___code__, set___defaults__,
set___kwdefaults__, and add/use assign_func_version or FUNC_VERSION_COUNTER
accordingly.

In `@crates/vm/src/builtins/type.rs`:
- Around line 843-845: When updating a class's __bases__ you already call
zelf.modified() for IC invalidation, but you must also remove the class from the
old bases' subclasses lists and add it to the new bases' subclasses to avoid
stale subclass entries; update the code path that changes __bases__ (the
function that mutates __bases__) to iterate old_bases and call something like
old_base.subclasses.remove(zelf) and then for each new_base add zelf to
new_base.subclasses, then call zelf.modified() and ensure any TODO about
removing from old bases' subclasses is implemented accordingly.
- Line 58: The global counter NEXT_TYPE_VERSION currently uses checked_add in
assign_version_tag which will stop advancing once it hits u32::MAX and then
always return 0; change the overflow policy to a defined behavior (e.g., wrap
back to 1) by replacing the checked_add logic with an atomic update that
advances the counter and, on overflow, resets it to 1; implement this using
AtomicU32::fetch_update or a loop with compare_exchange on NEXT_TYPE_VERSION in
assign_version_tag so that when the current value is u32::MAX you store 1,
otherwise you store current + 1, ensuring the counter never permanently becomes
0 and the doc comment (“all caches invalidated”) is upheld.

---

Duplicate comments:
In `@crates/vm/src/frame.rs`:
- Around line 2722-2738: The dict lookup Err branch currently swallows the
original error, deoptimizes, and re-runs via load_attr_slow; instead capture and
propagate the original error from dict.get_item_opt(attr_name, vm) so callers
see the original failure and side effects are not duplicated. Replace the Err(_)
arm in the match over dict.get_item_opt with Err(err) => return Err(err) (or use
the ? operator) rather than calling self.code.instructions.replace_op /
write_adaptive_counter and return self.load_attr_slow; leave the deoptimization
path only for non-error conditions where you intentionally want to fall back to
the safe path.

---

Nitpick comments:
In `@crates/vm/src/builtins/type.rs`:
- Around line 54-56: The field tp_version_tag: AtomicU32 is currently public and
allows external code to break invariants by writing arbitrary values; change its
visibility to non-public (make it private or pub(crate)) and add controlled
accessors such as load_tp_version_tag(&self) -> u32, assign_version_tag(&self,
new: u32) to set a valid tag, and invalidate_tp_version_tag(&self) to set it to
0, ensuring writers use AtomicU32 methods internally and enforcing any
validation/ordering semantics; update call sites that reference tp_version_tag
directly to use these new methods.
- Around line 219-228: In modified(), avoid unwrap and recursion: replace the
recursive call to sub.downcast_ref::<PyType>().unwrap().modified() with an
iterative traversal using a stack/VecDeque of upgraded PyType refs; for each
upgraded object, call downcast_ref::<PyType>() and if it returns Some, store its
tp_version_tag with Ordering::Release and push its subclasses' weak refs
(upgrading them when needed) onto the deque, otherwise skip that entry (do not
panic). Keep the initial tp_version_tag.store(0, Ordering::Release) behavior for
self and ensure you only call modified-equivalent logic on successfully
downcasted PyType instances.
- Around line 202-217: Replace the explicit CAS spin-loop in
assign_version_tag() with AtomicU32::fetch_update on NEXT_TYPE_VERSION: call
fetch_update(Relaxed, Relaxed, |current| current.checked_add(1).ok_or(current))
and handle the Result—on Ok(prev) store prev into self.tp_version_tag with
Ordering::Release and return prev; on Err(_) treat as overflow and return 0.
Keep the same ordering semantics and the overflow behavior (return 0) and
reference NEXT_TYPE_VERSION and tp_version_tag in your change.

In `@crates/vm/src/frame.rs`:
- Around line 2699-2707: Duplicate deopt/backoff logic for LOAD_ATTR should be
extracted into a single helper to avoid repetition: create a method (e.g.,
deopt_load_attr or write_load_attr_deopt) on the same type as self that takes
the differing values (instr_idx and cache_base), and move the unsafe block that
calls self.code.instructions.replace_op(... Instruction::LoadAttr { idx:
Arg::marker() }) and self.code.instructions.write_adaptive_counter(cache_base,
ADAPTIVE_BACKOFF_VALUE) into that helper; then replace each repeated block with
a call to this helper passing instr_idx and cache_base. Ensure the helper
preserves the unsafe semantics and references Instruction::LoadAttr,
Arg::marker(), write_adaptive_counter, and ADAPTIVE_BACKOFF_VALUE.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Reviewing files that changed from the base of the PR and between 821e648 and 79ca9c1.

• .cspell.dict/cpython.txt
• .cspell.json
• crates/codegen/src/ir.rs
• crates/compiler-core/src/bytecode.rs
• crates/compiler-core/src/bytecode/instruction.rs
• crates/compiler-core/src/marshal.rs
• crates/vm/src/builtins/code.rs
• crates/vm/src/builtins/function.rs
• crates/vm/src/builtins/type.rs
• crates/vm/src/frame.rs
• crates/vm/src/object/core.rs
• crates/vm/src/stdlib/sys/monitoring.rs

• crates/compiler-core/src/marshal.rs
• .cspell.dict/cpython.txt
• crates/codegen/src/ir.rs
• crates/vm/src/builtins/code.rs
• crates/vm/src/stdlib/sys/monitoring.rs
• crates/vm/src/object/core.rs
• .cspell.json