Bytecode enum named oparg#7294
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR renames and restructures many bytecode Instruction and PseudoInstruction operand fields (e.g., Changes
Sequence Diagram(s)(Skipped — changes are renames/restructuring without new multi-component control flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/jit/src/instructions.rs (1)
730-734:⚠️ Potential issue | 🔴 CriticalGuard
SWAPdepth before index arithmetic to prevent panic
len - 1 - index.get(arg) as usizecan underflow and panic on malformed bytecode. ReturnJitCompileError::BadBytecodeinstead of panicking.Suggested fix
Instruction::Swap { i: index } => { let len = self.stack.len(); - let i = len - 1; - let j = len - 1 - index.get(arg) as usize; - self.stack.swap(i, j); + let depth = index.get(arg) as usize; + if depth >= len { + return Err(JitCompileError::BadBytecode); + } + self.stack.swap(len - 1, len - 1 - depth); Ok(()) }🤖 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 730 - 734, The Swap handling currently computes j as len - 1 - index.get(arg) as usize which can underflow and panic; update Instruction::Swap in the match arm to first validate index.get(arg) against the current stack length (self.stack.len()) and return Err(JitCompileError::BadBytecode) when the index is out of range or would underflow, and only then compute i/j and call self.stack.swap(i, j); ensure you use index.get(arg) once (or bind it to a local) to avoid double evaluation and reference JitCompileError::BadBytecode for the error path.
🧹 Nitpick comments (2)
crates/vm/src/frame.rs (2)
759-776: DecodeRaiseVarargskind once and reuse it.Line [759] and Line [775] decode
argc.get(arg)separately for the same opcode path. Please extract once and reuse in both predicates.♻️ Proposed refactor
- let is_reraise = match op { - Instruction::RaiseVarargs { argc } => matches!( - argc.get(arg), - bytecode::RaiseKind::BareRaise | bytecode::RaiseKind::ReraiseFromStack - ), - Instruction::Reraise { .. } - | Instruction::EndAsyncFor - | Instruction::CleanupThrow => true, - _ => false, - }; + let raise_kind = match op { + Instruction::RaiseVarargs { argc } => Some(argc.get(arg)), + _ => None, + }; + + let is_reraise = matches!( + raise_kind, + Some( + bytecode::RaiseKind::BareRaise + | bytecode::RaiseKind::ReraiseFromStack + ) + ) || matches!( + op, + Instruction::Reraise { .. } + | Instruction::EndAsyncFor + | Instruction::CleanupThrow + ); @@ - let is_new_raise = matches!( - op, - Instruction::RaiseVarargs { argc } - if matches!( - argc.get(arg), - bytecode::RaiseKind::Raise | bytecode::RaiseKind::RaiseCause - ) - ); + let is_new_raise = matches!( + raise_kind, + Some(bytecode::RaiseKind::Raise | bytecode::RaiseKind::RaiseCause) + );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/vm/src/frame.rs` around lines 759 - 776, Extract the result of argc.get(arg) for the RaiseVarargs opcode into a local binding (e.g., let raise_kind = argc.get(arg)) before the two matches, then replace both occurrences of argc.get(arg) in the first predicate and in the is_new_raise match arm with that raise_kind variable; update the matching arms under Instruction::RaiseVarargs { argc } and the is_new_raise computation to use raise_kind so the kind is decoded once and reused.
1330-1333: AlignCopycomments with current opcode naming.Line [1330]–Line [1332] still mention
CopyItem { index: ... }; the active variant here isInstruction::Copy { i }. Updating this avoids VM debugging confusion.🤖 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 1330 - 1333, Update the comment to reference the current opcode name Instruction::Copy and its field i instead of the outdated CopyItem { index: ... } so it matches the code. Specifically, adjust the comment immediately above the line using let idx = i.get(arg) as usize; to say that Instruction::Copy { i } with i.get(arg) is 1-indexed (Copy with index 1 copies TOS, index 2 copies second from top) to avoid confusion during VM debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/jit/src/instructions.rs`:
- Around line 730-734: The Swap handling currently computes j as len - 1 -
index.get(arg) as usize which can underflow and panic; update Instruction::Swap
in the match arm to first validate index.get(arg) against the current stack
length (self.stack.len()) and return Err(JitCompileError::BadBytecode) when the
index is out of range or would underflow, and only then compute i/j and call
self.stack.swap(i, j); ensure you use index.get(arg) once (or bind it to a
local) to avoid double evaluation and reference JitCompileError::BadBytecode for
the error path.
---
Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 759-776: Extract the result of argc.get(arg) for the RaiseVarargs
opcode into a local binding (e.g., let raise_kind = argc.get(arg)) before the
two matches, then replace both occurrences of argc.get(arg) in the first
predicate and in the is_new_raise match arm with that raise_kind variable;
update the matching arms under Instruction::RaiseVarargs { argc } and the
is_new_raise computation to use raise_kind so the kind is decoded once and
reused.
- Around line 1330-1333: Update the comment to reference the current opcode name
Instruction::Copy and its field i instead of the outdated CopyItem { index: ...
} so it matches the code. Specifically, adjust the comment immediately above the
line using let idx = i.get(arg) as usize; to say that Instruction::Copy { i }
with i.get(arg) is 1-indexed (Copy with index 1 copies TOS, index 2 copies
second from top) to avoid confusion during VM debugging.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode/instruction.rscrates/jit/src/instructions.rscrates/jit/tests/common.rscrates/stdlib/src/_opcode.rscrates/vm/src/builtins/frame.rscrates/vm/src/frame.rs
|
@ShaharNaveh I am sorry, i had to merge this first. it has conflict on frame.rs |
No worries, I'll fix the merge conflicts soon |
Align oparg names with https://docs.python.org/3.14/library/dis.html
No logic changes, only semantic changes
Summary by CodeRabbit