-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix opcode args #6649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix opcode args #6649
Conversation
📝 WalkthroughWalkthroughThe opcode module's validity checks are simplified to rely on a unified validation approach, while expanding the set of recognized instructions in has_name and has_jump. Error handling for invalid stack arguments is improved with detailed type information, and boolean extraction is standardized. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/stdlib/src/opcode.rs (2)
56-132: Consider eliminating double conversion inhas_*functions.All the
has_*functions (lines 56-132) callSelf::is_valid(opcode)which internally performsInstruction::try_from(opcode as u8).is_ok(), then immediately callInstruction::try_from(opcode as u8)again in thematches!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, andhas_local.
158-176: Optional: Simplify redundant error handling foroparg.After the
fast_isinstancecheck on line 161 confirms the value is an int type, thedowncast_refon line 167 should always succeed. Theok_or_elseon 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
📒 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 runningcargo fmtto 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
PyBoolandPyNonealigns with the refactored boolean extraction logic instack_effectthat now usestry_to_bool.
178-185: Clean boolean extraction forjumpparameter.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 inhas_nameandhas_jumpare correctly classified.Verification against CPython 3.11 confirms:
has_nameadditions align with CPython'shasnameopcodes: ImportName, LoadAttr, LoadMethod, LoadNameAny (LOAD_NAME), and StoreLocal (STORE_NAME) all use co_names in CPython.has_jumpadditions are correctly classified: JumpIfTrueOrPop and Send match CPython'shasjrelopcodes, 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.
youknowone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, thank you for catching
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.