crates/compiler-core/src/bytecode/instruction.rs (1)

873-884: fmt_dis for PseudoInstruction will panic if invoked.

The unimplemented!() macro will cause a runtime panic if any code path attempts to display a pseudo instruction for debugging/disassembly. Since InstructionMetadata is implemented for both RealInstruction and PseudoInstruction, and Instruction::fmt_dis delegates via the inst_either! macro, calling display() on an Instruction::Pseudo variant will panic.

Consider providing a basic implementation:

♻️ Suggested implementation

     fn fmt_dis(
         &self,
-        _arg: OpArg,
-        _f: &mut fmt::Formatter<'_>,
-        _ctx: &impl InstrDisplayContext,
+        arg: OpArg,
+        f: &mut fmt::Formatter<'_>,
+        _ctx: &impl InstrDisplayContext,
         _expand_code_objects: bool,
-        _pad: usize,
+        pad: usize,
         _level: usize,
     ) -> fmt::Result {
-        unimplemented!()
+        match self {
+            Self::Jump { target } => write!(f, "{:pad$}({})", "JUMP", target.get(arg)),
+            Self::JumpNoInterrupt { target } => write!(f, "{:pad$}({})", "JUMP_NO_INTERRUPT", target.get(arg)),
+            Self::LoadAttrMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ATTR_METHOD", idx.get(arg)),
+            Self::LoadSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_SUPER_METHOD", idx.get(arg)),
+            Self::LoadZeroSuperAttr { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_ATTR", idx.get(arg)),
+            Self::LoadZeroSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_METHOD", idx.get(arg)),
+            Self::PopBlock => write!(f, "POP_BLOCK"),
+            Self::SetupCleanup => write!(f, "SETUP_CLEANUP"),
+            Self::SetupFinally => write!(f, "SETUP_FINALLY"),
+            Self::SetupWith => write!(f, "SETUP_WITH"),
+            Self::StoreFastMaybeNull => write!(f, "STORE_FAST_MAYBE_NULL"),
+            Self::Reserved258 => write!(f, "RESERVED_258"),
+        }
     }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: RustPython is a Python 3 interpreter written in Rust implementing Python 3.13.0+ compatibility with complete Python environment in Rust (not CPython bindings), clean implementation without compatibility hacks, and cross-platform support including WebAssembly compilation

crates/compiler-core/src/bytecode/instruction.rs (2)

982-986: expect_real panics in const context - message won't be visible.

expect_real uses Option::expect which panics with a message, but since this is a const fn, the panic message may not be displayed in all contexts. Consider whether unwrap() suffices or if a non-const version with better diagnostics is needed for runtime usage.

---

908-912: Actual memory footprint of Instruction enum is unverified.

RealInstruction and PseudoInstruction are correctly sized at 1 and 2 bytes respectively, but there is no size assertion for the Instruction enum itself. Without verification of actual compiled size, the claim that it "will likely be 4 bytes" is speculative. More importantly, there's no evidence in the codebase that memory footprint is a performance concern for bytecode arrays—suggesting a union-based approach trades type safety for unverified space savings and would require clear performance metrics to justify.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

crates/compiler-core/src/bytecode/instruction.rs (1)

876-887: fmt_dis for PseudoInstruction still panics unconditionally.

This concern was raised in a previous review. The unimplemented!() will panic if Instruction::Pseudo is formatted via InstructionMetadata. Consider providing a minimal implementation or using todo!("PseudoInstruction display not yet implemented") for a clearer error message.

crates/compiler-core/src/bytecode/instruction.rs (1)

829-845: Range-based transmute assumes contiguous discriminants.

The TryFrom<u16> implementation relies on all discriminants being contiguous (256-267). Currently this holds, but if future changes introduce gaps (e.g., removing Reserved258), the transmute would accept invalid values.

Consider either:

1. Adding a comment documenting this invariant
2. Using an explicit match or lookup similar to RealInstruction's custom_ops approach

📝 Suggested documentation

 impl TryFrom<u16> for PseudoInstruction {
     type Error = MarshalError;
 
     #[inline]
     fn try_from(value: u16) -> Result<Self, MarshalError> {
+        // SAFETY: All discriminants from Jump (256) to StoreFastMaybeNull (267)
+        // must be contiguous for this range check to be valid.
         let start = u16::from(Self::Jump {
             target: Arg::marker(),
         });

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

crates/compiler-core/src/bytecode/instruction.rs (2)

847-857: JumpNoInterrupt still missing from label_arg and unconditional_branch.

The previous review flagged that PseudoInstruction::JumpNoInterrupt has a target: Arg<Label> field but is not handled in either method. This is inconsistent with how RealInstruction::JumpBackwardNoInterrupt is handled (lines 357, 377). Label resolution and control-flow analysis will miss this instruction's jump target.

🐛 Proposed fix

     fn label_arg(&self) -> Option<Arg<Label>> {
         match self {
-            Self::Jump { target: l } => Some(*l),
+            Self::Jump { target: l } | Self::JumpNoInterrupt { target: l } => Some(*l),
             _ => None,
         }
     }

     fn unconditional_branch(&self) -> bool {
-        matches!(self, Self::Jump { .. },)
+        matches!(self, Self::Jump { .. } | Self::JumpNoInterrupt { .. })
     }

---

876-887: fmt_dis for PseudoInstruction still panics unconditionally.

The previous review flagged this unimplemented!(). If Instruction::Pseudo is ever formatted via InstructionMetadata (e.g., during debug output in max_stackdepth), this will panic. Consider at minimum using todo!("PseudoInstruction display not yet implemented") for a clearer error message, or provide a basic implementation.

💡 Minimal implementation suggestion

     fn fmt_dis(
         &self,
-        _arg: OpArg,
-        _f: &mut fmt::Formatter<'_>,
+        arg: OpArg,
+        f: &mut fmt::Formatter<'_>,
         _ctx: &impl InstrDisplayContext,
         _expand_code_objects: bool,
-        _pad: usize,
+        pad: usize,
         _level: usize,
     ) -> fmt::Result {
-        unimplemented!()
+        match self {
+            Self::Jump { target } => write!(f, "{:pad$}({})", "JUMP", target.get(arg)),
+            Self::JumpNoInterrupt { target } => write!(f, "{:pad$}({})", "JUMP_NO_INTERRUPT", target.get(arg)),
+            Self::LoadAttrMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ATTR_METHOD", idx.get(arg)),
+            Self::LoadSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_SUPER_METHOD", idx.get(arg)),
+            Self::LoadZeroSuperAttr { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_ATTR", idx.get(arg)),
+            Self::LoadZeroSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_METHOD", idx.get(arg)),
+            Self::PopBlock => write!(f, "POP_BLOCK"),
+            _ => write!(f, "{:?}", self),
+        }
     }

crates/compiler-core/src/bytecode/instruction.rs (1)

279-279: Consider tracking the TODO for LoadClosure migration.

The TODO indicates LoadClosure should be moved to PseudoInstruction. This appears to be a deliberate deferral for a future refactoring pass. Consider creating an issue to track this.

Would you like me to open an issue to track the LoadClosure migration to pseudo instructions?

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: RustPython is a Python 3 interpreter written in Rust implementing Python 3.13.0+ compatibility with complete Python environment in Rust (not CPython bindings), clean implementation without compatibility hacks, and cross-platform support including WebAssembly compilation

Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When modifying bytecode instructions, perform a full clean build by running `rm -r target/debug/build/rustpython-* && find . | grep -E '\.pyc$' | xargs rm -r`

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

crates/compiler-core/src/bytecode/instruction.rs (2)

847-857: JumpNoInterrupt still missing from label_arg and unconditional_branch.

PseudoInstruction::JumpNoInterrupt has a target: Arg<Label> field, but it's not handled in label_arg (caught by _ => None) and not matched in unconditional_branch. This could cause label resolution or control-flow analysis to miss this instruction's jump target.

🐛 Proposed fix

     fn label_arg(&self) -> Option<Arg<Label>> {
         match self {
-            Self::Jump { target: l } => Some(*l),
+            Self::Jump { target: l } | Self::JumpNoInterrupt { target: l } => Some(*l),
             _ => None,
         }
     }

     fn unconditional_branch(&self) -> bool {
-        matches!(self, Self::Jump { .. },)
+        matches!(self, Self::Jump { .. } | Self::JumpNoInterrupt { .. })
     }

---

876-886: fmt_dis for PseudoInstruction panics unconditionally.

This unimplemented!() will panic if Instruction::Pseudo is ever formatted via the InstructionMetadata trait (e.g., during debug/disassembly output). At minimum, use todo!() for a clearer message, or provide a basic implementation.

Minimal implementation suggestion

     fn fmt_dis(
         &self,
-        _arg: OpArg,
-        _f: &mut fmt::Formatter<'_>,
+        arg: OpArg,
+        f: &mut fmt::Formatter<'_>,
         _ctx: &impl InstrDisplayContext,
         _expand_code_objects: bool,
-        _pad: usize,
+        pad: usize,
         _level: usize,
     ) -> fmt::Result {
-        unimplemented!()
+        match self {
+            Self::Jump { target } => write!(f, "{:pad$}({})", "JUMP", target.get(arg)),
+            Self::JumpNoInterrupt { target } => write!(f, "{:pad$}({})", "JUMP_NO_INTERRUPT", target.get(arg)),
+            Self::LoadAttrMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ATTR_METHOD", idx.get(arg)),
+            Self::LoadSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_SUPER_METHOD", idx.get(arg)),
+            Self::LoadZeroSuperAttr { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_ATTR", idx.get(arg)),
+            Self::LoadZeroSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_METHOD", idx.get(arg)),
+            Self::PopBlock => write!(f, "POP_BLOCK"),
+            _ => write!(f, "PSEUDO({:?})", self),
+        }
     }

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

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

644-650: Fix SWAP semantics (off-by-one).

SWAP 1 should swap TOS with itself (no-op), but len - 1 - index swaps with the second-from-top.

Proposed fix

             Instruction::Swap { index } => {
                 let len = self.stack.len();
                 let i = len - 1;
-                let j = len - 1 - index.get(arg) as usize;
+                let n = index.get(arg) as usize; // 1-based
+                if n == 0 || n > len {
+                    return Err(JitCompileError::BadBytecode);
+                }
+                let j = len - n;
                 self.stack.swap(i, j);
                 Ok(())
             }

crates/compiler-core/src/bytecode/instruction.rs (2)

785-786: LOAD_CLOSURE disassembly omits its index/name.
This makes traces/disassembly much harder to interpret.

Proposed change

-            Self::LoadClosure(_) => w!(LOAD_CLOSURE),
+            Self::LoadClosure(idx) => w!(LOAD_CLOSURE, cell_name = idx),

---

852-892: Pseudo jump metadata + formatting are incomplete (will miss labels / can panic).

• JumpNoInterrupt should participate in label_arg() and unconditional_branch() like Jump.
• fmt_dis currently panics via unimplemented!() if pseudos ever get formatted.

Suggested fix

 impl InstructionMetadata for PseudoInstruction {
     fn label_arg(&self) -> Option<Arg<Label>> {
         match self {
-            Self::Jump { target: l } => Some(*l),
+            Self::Jump { target: l } | Self::JumpNoInterrupt { target: l } => Some(*l),
             _ => None,
         }
     }

     fn unconditional_branch(&self) -> bool {
-        matches!(self, Self::Jump { .. },)
+        matches!(self, Self::Jump { .. } | Self::JumpNoInterrupt { .. })
     }

     fn fmt_dis(/* ... */) -> fmt::Result {
-        unimplemented!()
+        // At minimum, avoid panicking during debug/disassembly paths.
+        todo!("PseudoInstruction disassembly not implemented yet")
     }
 }

crates/stdlib/src/opcode.rs (1)

82-99: Predicate classification gaps: missing real jump ops + LoadSuperAttr name usage.

• has_jump should include Instruction::JumpForward/JumpBackward/JumpBackwardNoInterrupt.
• has_name should include Instruction::LoadSuperAttr { .. }.

Proposed fix

         pub fn has_name(opcode: i32) -> bool {
             matches!(
                 Self::try_from(opcode).map(|op| op.inner()),
                 Ok(AnyInstruction::Real(
                     Instruction::DeleteAttr { .. }
                         | Instruction::DeleteGlobal(_)
                         | Instruction::DeleteName(_)
                         | Instruction::ImportFrom { .. }
                         | Instruction::ImportName { .. }
                         | Instruction::LoadAttr { .. }
                         | Instruction::LoadGlobal(_)
                         | Instruction::LoadName(_)
+                        | Instruction::LoadSuperAttr { .. }
                         | Instruction::StoreAttr { .. }
                         | Instruction::StoreGlobal(_)
                         | Instruction::StoreName(_)
                 ) | AnyInstruction::Pseudo(PseudoInstruction::LoadAttrMethod { .. }))
             )
         }

         pub fn has_jump(opcode: i32) -> bool {
             matches!(
                 Self::try_from(opcode).map(|op| op.inner()),
                 Ok(AnyInstruction::Real(
                     Instruction::Break { .. }
                         | Instruction::Continue { .. }
                         | Instruction::ForIter { .. }
+                        | Instruction::JumpForward { .. }
+                        | Instruction::JumpBackward { .. }
+                        | Instruction::JumpBackwardNoInterrupt { .. }
                         | Instruction::JumpIfFalseOrPop { .. }
                         | Instruction::JumpIfNotExcMatch(_)
                         | Instruction::JumpIfTrueOrPop { .. }
                         | Instruction::PopJumpIfFalse { .. }
                         | Instruction::PopJumpIfTrue { .. }
                         | Instruction::Send { .. }
                 ) | AnyInstruction::Pseudo(PseudoInstruction::Jump { .. }))
             )
         }

Also applies to: 103-118

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

199-268: JumpNoInterrupt is still a runtime panic + has no lowering path.

Right now:

• It’s grouped into the unimplemented!() placeholder bucket.
• The later “convert JUMP pseudo to real” block only handles PseudoInstruction::Jump.

If codegen ever emits JumpNoInterrupt, compilation will panic in finalize_code.

Direction for a fix

• Treat JumpNoInterrupt like Jump in the early pseudo pass (do not unimplemented!() it).
• Lower it alongside Jump, likely to Instruction::JumpBackwardNoInterrupt when target_offset <= current_offset (and decide what to do for forward jumps: either forbid via assertion or map to JumpForward).

-                    PseudoInstruction::JumpNoInterrupt { .. }
-                    | PseudoInstruction::Reserved258
+                    PseudoInstruction::JumpNoInterrupt { .. } => {
+                        // handled later like Jump (lowering step)
+                    }
+                    PseudoInstruction::Reserved258
                     | PseudoInstruction::SetupCleanup
                     | PseudoInstruction::SetupFinally
                     | PseudoInstruction::SetupWith
                     | PseudoInstruction::StoreFastMaybeNull => {
                         unimplemented!("Got a placeholder pseudo instruction ({instr:?})")
                     }

And in the lowering match:

-                        AnyInstruction::Pseudo(PseudoInstruction::Jump { .. })
+                        AnyInstruction::Pseudo(PseudoInstruction::Jump { .. } | PseudoInstruction::JumpNoInterrupt { .. })
                             if target != BlockIdx::NULL =>
                         {
                             let target_offset = block_to_offset[target.idx()].0;
                             if target_offset > current_offset {
-                                Instruction::JumpForward { target: Arg::marker() }
+                                Instruction::JumpForward { target: Arg::marker() } // or forbid for JumpNoInterrupt
                             } else {
-                                Instruction::JumpBackward { target: Arg::marker() }
+                                if matches!(info.instr, AnyInstruction::Pseudo(PseudoInstruction::JumpNoInterrupt { .. })) {
+                                    Instruction::JumpBackwardNoInterrupt { target: Arg::marker() }
+                                } else {
+                                    Instruction::JumpBackward { target: Arg::marker() }
+                                }
                             }
                         }

Also applies to: 305-323

crates/compiler-core/src/bytecode/instruction.rs (2)

288-294: Unsafe transmute is OK here, but keep the safety story airtight.

Given #[repr(u8)] + size_of::<Instruction>() == 1 + explicit value validation in TryFrom<u8>, this looks fine. Consider adding a short note that all payloads are ZST (Arg<T>) so there’s no invalid-data concern beyond discriminant validity.

Also applies to: 349-350

---

296-354: TryFrom<u8> whitelist looks correct; consider reducing “special cases” drift.

The explicit custom_ops list avoids gaps (good). The lingering LoadClosure exception is a maintenance footgun—if it must stay for now, I’d at least add a comment pointing to the tracking issue/PR that will move it to pseudos.

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

277-297: emit! macro refactor is clean; consider adding one “label arg” example in comments/tests.

The new macro shapes (struct/tuple/no-arg) map well onto emit_arg/emit_no_arg. Since label emission now depends on the EmitArg<bytecode::Label> for BlockIdx impl, a tiny comment or targeted test snapshot for a label-bearing instruction would make regressions easier to spot.

---

6103-6166: Comprehension init_collection AnyInstruction plumbing works, but call sites got noisier (marker() + .into()).

Right now every list/set/dict comp has to build an AnyInstruction manually with OpArgMarker::marker() and .into(). If feasible, consider making compile_comprehension generic over I: Into<AnyInstruction> for init_collection to keep call sites closer to the pre-refactor style.

Also applies to: 6522-6714

---

6744-6932: Inlined comprehension cleanup path: ensure it’s correct when init_collection is None (generator case).

The exception cleanup block currently assumes a “collection” exists when it does Swap { index: 2 } + PopTop (comment says “Pop incomplete collection”). If init_collection is None, the stack shape differs and this cleanup may pop the iterator or otherwise mis-restore locals. Even if PEP 709 is currently disabled, tightening this now will prevent future footguns.

---

6955-6983: Generic emission plumbing (Into<AnyInstruction>) is a solid direction; watch for monomorphization bloat in hot paths.

_emit/emit_arg now monomorphize per instruction constructor closure type. It’s probably fine for codegen, but if compile times regress, consider moving some generic helpers behind a smaller set of concrete wrappers (or accept AnyInstruction at specific choke points).

Also applies to: 7454-7478

Configuration used: Path: .coderabbit.yml