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

6796-6853: Run the required clean build for these bytecode-instruction changes.

Given instruction emission/targets changed here, please run the repository’s clean-build step before final validation to avoid stale artifacts masking issues.

rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r

As per coding guidelines "When modifying bytecode instructions, perform a clean build by running: rm -r target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r".

🤖 Prompt for AI Agents

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

In `@crates/codegen/src/compile.rs` around lines 6796 - 6853, After changing
bytecode emission/targets in compile_yield_from_sequence (and related
Instruction/PseudoInstruction usage like Send, SetupFinally, YieldValue, Resume,
CleanupThrow, EndSend), perform a clean build to avoid stale artifacts: remove
target/debug build caches and .pyc files then rebuild by running: rm -r
target/debug/build/rustpython-* && find . | grep -E "\.pyc$" | xargs rm -r; then
run the repository build/test commands to validate the changes.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

3557-3575: ⚠️ Potential issue | 🟠 Major

Guard relative jump arithmetic against u32 wrap-around.

Line 3564 and Line 3574 use unchecked arithmetic, and the same pattern is still present at Line 1360, Line 2434, Line 2660, Line 2671, and Line 2729+ sites. With malformed bytecode, this can wrap/underflow lasti and redirect execution to invalid instruction indices.

🐛 Suggested direction

- fn jump_relative_forward(&mut self, delta: u32, caches: u32) {
-     let target = self.lasti() + caches + delta;
-     self.update_lasti(|i| *i = target);
- }
+ fn jump_relative_forward(
+     &mut self,
+     delta: u32,
+     caches: u32,
+     vm: &VirtualMachine,
+ ) -> PyResult<()> {
+     let target = self
+         .lasti()
+         .checked_add(caches)
+         .and_then(|v| v.checked_add(delta))
+         .ok_or_else(|| vm.new_system_error("relative forward jump overflow".to_owned()))?;
+     self.update_lasti(|i| *i = target);
+     Ok(())
+ }

- fn jump_relative_backward(&mut self, delta: u32, caches: u32) {
-     let target = self.lasti() + caches - delta;
-     self.update_lasti(|i| *i = target);
- }
+ fn jump_relative_backward(
+     &mut self,
+     delta: u32,
+     caches: u32,
+     vm: &VirtualMachine,
+ ) -> PyResult<()> {
+     let target = self
+         .lasti()
+         .checked_add(caches)
+         .and_then(|v| v.checked_sub(delta))
+         .ok_or_else(|| vm.new_system_error("relative backward jump underflow".to_owned()))?;
+     self.update_lasti(|i| *i = target);
+     Ok(())
+ }

#!/bin/bash
# Read-only verification: list unchecked relative jump math sites and checked arithmetic usage.
rg -n 'lasti\(\)\s*\+\s*1\s*\+\s*u32::from\(arg\)|lasti\(\)\s*\+\s*u32::from\(arg\)|lasti\(\)\s*\+\s*1\s*-\s*u32::from\(arg\)|\+\s*caches\s*\+\s*delta|\+\s*caches\s*-\s*delta' crates/vm/src/frame.rs
rg -n 'checked_add|checked_sub' crates/vm/src/frame.rs

Also applies to: 1358-1361, 2433-2434, 2660-2672, 2728-2773

🤖 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 3557 - 3575, Guard all relative-jump
arithmetic against u32 wrap/underflow by replacing unchecked
addition/subtraction in jump_relative_forward and jump_relative_backward (and
the other sites with the same pattern) with checked arithmetic (e.g.,
u32::checked_add / u32::checked_sub); compute target via checked ops, and if
they return None, stop and handle the invalid target (propagate an
Err/trap/panic consistent with the frame's error model) instead of calling
update_lasti with a wrapped value; ensure the same pattern is applied to the
other occurrences (the similar add/sub uses that set lasti via update_lasti) so
all malformed bytecode cannot produce wrapped/invalid instruction indices.

crates/jit/src/instructions.rs (1)

184-204: Prefer deriving cache widths from Instruction::cache_entries() to prevent drift.

While the current hard-coded cache widths (0 and 1) are correct, duplicating this metadata creates maintenance risk. If cache_entries() is updated, the JIT's target calculation could silently diverge. Consider refactoring instruction_target to query the opcode metadata directly, ensuring JIT labels remain synchronized with compiler-core.

🤖 Prompt for AI Agents

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

In `@crates/jit/src/instructions.rs` around lines 184 - 204, The match in
instruction_target currently uses hard-coded cache widths (0 and 1) when calling
Self::jump_target_forward/backward; replace those magic numbers by querying the
opcode's cache width from the instruction metadata (use
Instruction::cache_entries() or the equivalent method on the matched variant)
and pass that value into jump_target_forward/jump_target_backward so the JIT
follows the canonical cache entry count; update all arms that currently pass 0
or 1 (e.g., Instruction::JumpForward, JumpBackward, JumpBackwardNoInterrupt,
PopJumpIfFalse/True/IfNone/IfNotNone, ForIter, Send) to compute width =
instruction.cache_entries() (or the per-variant entry count) and use width or
width - 1 where semantics require, preserving the existing offset adjustments.

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

201-233: Consider making unchecked jump arithmetic explicit with checked operations or comments.

Lines 202, 214, 224, 232, and 253 use unchecked addition and subtraction to compute jump targets. While subsequent bounds checks (j < stacks.len()) prevent out-of-bounds access, the pattern silently skips processing jump targets that wrap (e.g., when oparg exceeds next_i on backward jumps). This defensive behavior is safe but implicit. Either use checked_add/checked_sub to be explicit about handling invalid jumps, or add a comment clarifying that out-of-bounds targets are intentionally skipped during stack analysis.

🤖 Prompt for AI Agents

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

In `@crates/vm/src/builtins/frame.rs` around lines 201 - 233, The jump target
arithmetic currently uses plain add/sub (e.g., computing j = next_i + oparg as
usize and j = next_i - oparg as usize inside the match arms for
Instruction::Send, Instruction::JumpForward, Instruction::JumpBackward /
JumpBackwardNoInterrupt and the earlier case that pops a value) which can
silently wrap and be implicitly ignored by the subsequent j < stacks.len()
checks; update these sites to use explicit checked arithmetic
(checked_add/checked_sub on usize or convert oparg safely) and handle the None
case by skipping the target, or add a short clarifying comment at each
calculation (mentioning j, next_i, oparg, and stacks.len()) that wrapping is
intentionally treated as an invalid/ignored target—pick one approach and apply
it consistently to all occurrences in the file.

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

7918-7922: Async inlined-comprehension loop cleanup currently diverges from the async-for protocol.

This branch emits GetANext + yield_from without SetupFinally/PopBlock, then emits EndAsyncFor plus an extra PopTop. That differs from the non-inlined async path and can mis-handle stack/exception cleanup if PEP 709 inlining is enabled later.

♻️ Suggested alignment with the non-inlined async pattern

             if generator.is_async {
+                emit!(
+                    self,
+                    PseudoInstruction::SetupFinally {
+                        target: after_block
+                    }
+                );
                 emit!(self, Instruction::GetANext);
                 self.emit_load_const(ConstantData::None);
                 end_async_for_target = self.compile_yield_from_sequence(true)?;
+                emit!(self, PseudoInstruction::PopBlock);
                 self.compile_store(&generator.target)?;
             } else {
                 emit!(
                     self,
                     Instruction::ForIter {
@@
             self.switch_to_block(after_block);
             if is_async {
                 self.emit_end_async_for(end_async_for_target);
-                // Pop the iterator
-                emit!(self, Instruction::PopTop);
             } else {
                 // END_FOR + POP_ITER pattern (CPython 3.14)
                 emit!(self, Instruction::EndFor);
                 emit!(self, Instruction::PopIter);
             }

Also applies to: 7958-7963

🤖 Prompt for AI Agents

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

In `@crates/codegen/src/compile.rs` around lines 7918 - 7922, The async
inlined-comprehension branch (where generator.is_async is checked and the code
emits Instruction::GetANext, self.compile_yield_from_sequence(true), and
self.compile_store(&generator.target)) is missing the SetupFinally/PopBlock
pairing before yielding and leaves an extra PopTop/EndAsyncFor sequence that
diverges from the non-inlined async-for protocol; update this branch (and the
analogous block around the other compile_yield_from_sequence call) to mirror the
non-inlined pattern by emitting a SetupFinally before GetANext/yield_from,
ensuring PopBlock is emitted after the yield sequence, and then using
EndAsyncFor without leaving the stray PopTop — i.e., wrap the GetANext +
compile_yield_from_sequence(true) call with the same
SetupFinally/PopBlock/EndAsyncFor pairing used by the non-inlined async-for path
so stack/exception cleanup matches.

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

581-592: Extract duplicated “fallthrough cache-skip” logic into one helper.

The same post-dispatch cache-skip block is repeated three times. Centralizing it reduces drift risk when cache semantics evolve.

♻️ Refactor sketch

+ #[inline]
+ fn skip_caches_if_fallthrough(&mut self, op: Instruction, lasti_before: u32) {
+     if self.lasti() == lasti_before {
+         let base = op.to_base().unwrap_or(op);
+         let caches = base.cache_entries();
+         if caches > 0 {
+             self.update_lasti(|i| *i += caches as u32);
+         }
+     }
+ }

Then call this helper in the three current call sites.

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: 2868-2876, 2907-2916

🤖 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 581 - 592, The post-dispatch
"fallthrough cache-skip" block duplicated around calls to
self.execute_instruction should be extracted into a single helper (e.g., a
private method on Frame like skip_fallthrough_caches(&mut self, op: Instruction)
or skip_fallthrough_caches(&mut self, base_op: Instruction)) that captures the
logic: record lasti_before = self.lasti(), check self.lasti() == lasti_before,
derive base_op = op.to_base().unwrap_or(op), get caches =
base_op.cache_entries(), and if caches > 0 call self.update_lasti(|i| *i +=
caches as u32); replace the three duplicated blocks with calls to this helper
immediately after the corresponding execute_instruction(op, arg, &mut
do_extend_arg, vm) invocations so behavior is identical.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

408-411: ⚠️ Potential issue | 🟡 Minor

Make END_ skipping conditional for ForIter/Send.*

Line 410 always adds + 1, which can skip past the real target when END_FOR/END_SEND is not actually at that slot.

Suggested fix

-                Instruction::ForIter { .. } | Instruction::Send { .. } => {
-                    // Skip over END_FOR/END_SEND
-                    Some(after_caches + delta + 1)
-                }
+                Instruction::ForIter { .. } => {
+                    let t = after_caches + delta;
+                    let is_end_for = matches!(
+                        code.code.instructions.get(t).map(|u| u.op.to_base().unwrap_or(u.op)),
+                        Some(Instruction::EndFor)
+                    );
+                    Some(if is_end_for { t + 1 } else { t })
+                }
+                Instruction::Send { .. } => {
+                    let t = after_caches + delta;
+                    let is_end_send = matches!(
+                        code.code.instructions.get(t).map(|u| u.op.to_base().unwrap_or(u.op)),
+                        Some(Instruction::EndSend)
+                    );
+                    Some(if is_end_send { t + 1 } else { t })
+                }

🤖 Prompt for AI Agents

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

In `@crates/vm/src/stdlib/sys/monitoring.rs` around lines 408 - 411, The branch
handling Instruction::ForIter and Instruction::Send always returns
Some(after_caches + delta + 1), which incorrectly skips one slot even when the
following opcode isn't END_FOR/END_SEND; change this to conditionally add +1 by
looking up the instruction at index after_caches + delta (using the same
bytecode/instruction buffer visible in the surrounding function) and: if that
instruction is Instruction::EndFor (for ForIter) or Instruction::EndSend (for
Send) return Some(after_caches + delta + 1), otherwise return Some(after_caches
+ delta); keep referencing the Instruction enum and the local variables
after_caches and delta to locate and implement this conditional logic.

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

2534-2566: Document unchecked arithmetic assumption in helper functions.

The jump_relative_forward and jump_relative_backward helpers use unchecked arithmetic, which the doc comments justify as "compiler guarantees valid targets." While this is reasonable for well-formed bytecode, consider adding a debug assertion to catch malformed bytecode during development.

♻️ Optional: Add debug assertions for safety

     #[inline]
     fn jump_relative_forward(&mut self, delta: u32, caches: u32) {
+        debug_assert!(
+            self.lasti().checked_add(caches).and_then(|v| v.checked_add(delta)).is_some(),
+            "jump_relative_forward overflow"
+        );
         let target = self.lasti() + caches + delta;
         self.update_lasti(|i| *i = target);
     }

     #[inline]
     fn jump_relative_backward(&mut self, delta: u32, caches: u32) {
+        debug_assert!(
+            self.lasti().checked_add(caches).and_then(|v| v.checked_sub(delta)).is_some(),
+            "jump_relative_backward underflow"
+        );
         let target = self.lasti() + caches - delta;
         self.update_lasti(|i| *i = target);
     }

🤖 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 2534 - 2566, The helpers
jump_relative_forward and jump_relative_backward use unchecked arithmetic based
on the assumption that the compiler or caller ensures valid targets; add debug
assertions in each helper (e.g., debug_assert! or debug_assert_eq!) that
validate the computed target is within the code bounds and that the resulting
lasti/offset is non-negative and aligns with instruction-width expectations
before performing the unchecked arithmetic, and add a short comment documenting
the invariant being checked so malformed bytecode is caught in debug builds
while release keeps the unchecked ops.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

1349-1352: ⚠️ Potential issue | 🟠 Major

Guard relative jump arithmetic with checked operations (still unresolved).

This repeats a previously reported issue: relative target math is still unchecked in helper and direct paths. Malformed bytecode can wrap/underflow lasti and corrupt control-flow before a later panic.

#!/bin/bash
# Verify unresolved unchecked relative target arithmetic in frame execution.
# Expected after fix: no matches.
rg -n 'self\.lasti\(\)\s*\+\s*1\s*\+\s*u32::from\(arg\)|self\.lasti\(\)\s*\+\s*1\s*-\s*u32::from\(arg\)|self\.lasti\(\)\s*\+\s*caches\s*[+-]\s*delta' crates/vm/src/frame.rs

Suggested fix direction

- fn jump_relative_forward(&mut self, delta: u32, caches: u32) {
-     let target = self.lasti() + caches + delta;
-     self.update_lasti(|i| *i = target);
- }
+ fn jump_relative_forward(
+     &mut self,
+     delta: u32,
+     caches: u32,
+     vm: &VirtualMachine,
+ ) -> FrameResult {
+     let target = self
+         .lasti()
+         .checked_add(caches)
+         .and_then(|v| v.checked_add(delta))
+         .ok_or_else(|| vm.new_system_error("relative jump overflow".to_owned()))?;
+     self.update_lasti(|i| *i = target);
+     Ok(None)
+ }

Apply the same checked pattern to backward targets and direct relative computations.

Also applies to: 2428-2429, 2655-2667, 2724-2763, 3538-3557

🤖 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 1349 - 1352, The relative jump
computation in Instruction::ForIter (and similar sites) performs unchecked
arithmetic like bytecode::Label(self.lasti() + 1 + u32::from(arg)), allowing
wrap/underflow; update these sites (e.g., inside the match arm that calls
self.execute_for_iter(vm, target) and other locations noted) to use checked
arithmetic (checked_add/checked_sub or checked_{add,sub}_then_convert) and
return an Err/raise a VM error if the computed target overflows or is out of
range before constructing bytecode::Label; ensure the same checked pattern is
applied to backward targets and direct relative computations referenced in
execute_for_iter and other helper paths so all relative jumps validate bounds
before use.

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

5126-5126: ⚠️ Potential issue | 🟠 Major

Remove Instruction::NotTaken from async-iteration SEND flow.

NOT_TAKEN here is attached to async iteration control flow (SEND path), not a boolean POP_JUMP_IF_* branch, so monitoring semantics become incorrect.

Suggested fix

-            emit!(self, Instruction::NotTaken);

Based on learnings: In RustPython's codegen ir.rs, the is_conditional_jump function should only match POP_JUMP_IF_* instruction variants; ForIter and Send should not be included because NOT_TAKEN is for boolean-test conditional jumps only.

🤖 Prompt for AI Agents

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

In `@crates/codegen/src/compile.rs` at line 5126, The emitted
Instruction::NotTaken is wrongly applied to the async-iteration SEND path;
remove the emit!(self, Instruction::NotTaken) call from the SEND/SEND-related
async-iteration flow so that NOT_TAKEN is only used for boolean POP_JUMP_IF_*
branches. Locate the SEND/Send handling in compile.rs (the emit!(...,
Instruction::NotTaken) call) and delete that emission, and also ensure the
is_conditional_jump matcher (or equivalent logic) only considers POP_JUMP_IF_*
variants (not ForIter or Send) so NOT_TAKEN stays exclusive to boolean-test
conditional jumps.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

1568-1596: Refactor duplicated jump-direction logic in pseudo jump resolution.

Both pseudo-jump arms perform the same forward/backward check. Extract the direction once and reuse it to reduce duplication.

♻️ Proposed refactor

             let target_pos = block_order[target.idx()];
+            let is_forward = target_pos > source_pos;
             info.instr = match info.instr {
                 AnyInstruction::Pseudo(PseudoInstruction::Jump { .. }) => {
-                    if target_pos > source_pos {
+                    if is_forward {
                         Instruction::JumpForward {
                             target: Arg::marker(),
                         }
                         .into()
                     } else {
                         Instruction::JumpBackward {
                             target: Arg::marker(),
                         }
                         .into()
                     }
                 }
                 AnyInstruction::Pseudo(PseudoInstruction::JumpNoInterrupt { .. }) => {
-                    if target_pos > source_pos {
+                    if is_forward {
                         Instruction::JumpForward {
                             target: Arg::marker(),
                         }
                         .into()
                     } else {
                         Instruction::JumpBackwardNoInterrupt {
                             target: Arg::marker(),
                         }
                         .into()
                     }
                 }
                 other => other,
             };

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/codegen/src/ir.rs` around lines 1568 - 1596, The match on info.instr
duplicates the target_pos > source_pos check for both PseudoInstruction::Jump
and PseudoInstruction::JumpNoInterrupt; instead compute a boolean (e.g.
is_forward = target_pos > source_pos) before or at the top of the match and then
use that to select the appropriate Instruction variant. Update the arms for
AnyInstruction::Pseudo(PseudoInstruction::Jump { .. }) and
AnyInstruction::Pseudo(PseudoInstruction::JumpNoInterrupt { .. }) to use the
computed is_forward and construct either Instruction::JumpForward { target:
Arg::marker() }.into() or the corresponding backward variant
(Instruction::JumpBackward{...} or Instruction::JumpBackwardNoInterrupt{...})
without repeating the comparison, and assign the result back to info.instr.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro