Actionable comments posted: 0

🧹 Nitpick comments (2)

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

56-132: Consider eliminating double conversion in has_* functions.

All the has_* functions (lines 56-132) call Self::is_valid(opcode) which internally performs Instruction::try_from(opcode as u8).is_ok(), then immediately call Instruction::try_from(opcode as u8) again in the matches! macro. This double conversion is inefficient.

Consider inlining the range check and performing a single conversion:

🔎 Example refactor for has_const

 pub fn has_const(opcode: i32) -> bool {
-    Self::is_valid(opcode)
-        && matches!(
-            Instruction::try_from(opcode as u8),
-            Ok(Instruction::LoadConst { .. } | Instruction::ReturnConst { .. })
-        )
+    (0..=255).contains(&opcode)
+        && matches!(
+            Instruction::try_from(opcode as u8),
+            Ok(Instruction::LoadConst { .. } | Instruction::ReturnConst { .. })
+        )
 }

Apply the same pattern to has_name, has_jump, has_free, and has_local.

---

158-176: Optional: Simplify redundant error handling for oparg.

After the fast_isinstance check on line 161 confirms the value is an int type, the downcast_ref on line 167 should always succeed. The ok_or_else on lines 168-173 provides defensive error handling with the same error message, but is likely redundant.

🔎 Simplified approach

 let oparg = args
     .oparg
     .map(|v| {
         if !v.fast_isinstance(vm.ctx.types.int_type) {
             return Err(vm.new_type_error(format!(
                 "'{}' object cannot be interpreted as an integer",
                 v.class().name()
             )));
         }
         v.downcast_ref::<PyInt>()
-            .ok_or_else(|| {
-                vm.new_type_error(format!(
-                    "'{}' object cannot be interpreted as an integer",
-                    v.class().name()
-                ))
-            })?
+            .expect("downcast should succeed after isinstance check")
             .try_to_primitive::<u32>(vm)
     })
     .unwrap_or(Ok(0))?;

Alternatively, keep the defensive check if there are VM edge cases where isinstance and downcast might diverge.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75838e5 and b2ca650.

📒 Files selected for processing (1)

• crates/stdlib/src/opcode.rs

🧰 Additional context used 📓 Path-based instructions (1) **/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

• crates/stdlib/src/opcode.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Check Rust code with clippy
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run tests under miri

🔇 Additional comments (3)

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

7-7: Import cleanup looks appropriate.

The removal of PyBool and PyNone aligns with the refactored boolean extraction logic in stack_effect that now uses try_to_bool.

---

178-185: Clean boolean extraction for jump parameter.

The use of try_to_bool(vm) with a clear error message is a clean, idiomatic approach that properly handles boolean conversion through the VM's standard method.

---

67-83: The expanded opcode lists in has_name and has_jump are correctly classified.

Verification against CPython 3.11 confirms:

• has_name additions align with CPython's hasname opcodes: ImportName, LoadAttr, LoadMethod, LoadNameAny (LOAD_NAME), and StoreLocal (STORE_NAME) all use co_names in CPython.
• has_jump additions are correctly classified: JumpIfTrueOrPop and Send match CPython's hasjrel opcodes, and JumpIfNotExcMatch is properly identified as a jump instruction.

All opcodes exist in the Instruction enum and are semantically aligned with their CPython counterparts despite different naming conventions.

oops, thank you for catching