Skip to content

Bytecode enum named oparg#7294

Merged
youknowone merged 26 commits intoRustPython:mainfrom
ShaharNaveh:bytecode-enum-named-data
Mar 3, 2026
Merged

Bytecode enum named oparg#7294
youknowone merged 26 commits intoRustPython:mainfrom
ShaharNaveh:bytecode-enum-named-data

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 1, 2026

Align oparg names with https://docs.python.org/3.14/library/dis.html

No logic changes, only semantic changes

Summary by CodeRabbit

  • Refactor
    • Standardized and renamed instruction operand fields and pseudo‑instruction shapes across the VM, compiler, and JIT; metadata, display/formatting, and opcode mappings updated for consistency while preserving runtime behavior.
  • Tests
    • Updated test patterns and helpers to match the revised instruction shapes to ensure existing behavior remains unchanged.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 1, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04b7c4a and 0ae8b08.

📒 Files selected for processing (1)
  • crates/vm/src/frame.rs

📝 Walkthrough

Walkthrough

This PR renames and restructures many bytecode Instruction and PseudoInstruction operand fields (e.g., idxconsti/namei/i/var_num, targetdelta, sizecount, nargsargc) and converts many tuple-like variants to struct-like variants across compiler-core, codegen, JIT, VM, stdlib, and tests.

Changes

Cohort / File(s) Summary
Core Instruction Definitions
crates/compiler-core/src/bytecode/instruction.rs
Major public API changes: many Instruction/PseudoInstruction variants restructured and renamed (e.g., idxconsti/namei/i, targetdelta, sizecount, nargsargc); display, metadata, and instrumented/base mappings updated.
Codegen IR & Optimizations
crates/codegen/src/ir.rs
Updated IR transformations, peephole patterns, and pseudo-op lowering to use renamed fields (e.g., LoadConst.consti, LoadFast.var_num, jumps use delta).
JIT Implementation & Tests
crates/jit/src/instructions.rs, crates/jit/tests/common.rs
Adjusted JIT instruction handling, operand extraction, and test helpers to new field names (argc, consti, namei, var_num, count, var_nums).
VM Execution & Builtins
crates/vm/src/frame.rs, crates/vm/src/builtins/frame.rs
Interpreter and builtin frame match arms updated to struct-style variants and renamed operand fields throughout execution and deopt paths.
Stdlib Opcode Utilities
crates/stdlib/src/_opcode.rs
Pattern matches converted from tuple-like to struct-like variants for name/free/local cases to align with restructured Instructions.
Spell dictionary
.cspell.dict/cpython.txt
Added lexical token consti.

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

  • youknowone

Poem

"I nibble bytes and hop through code,
var_num and namei on the road.
consti counts my rosy treats,
delta marks the jumping beats.
argc gathers all the loads." 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Bytecode enum named oparg' is vague and does not clearly convey the main purpose of the changeset, which involves renaming multiple instruction field names (idx→namei/consti, size→count, nargs→argc, etc.) to align with Python dis documentation. Consider a more descriptive title such as 'Align bytecode instruction field names with Python dis documentation' or 'Rename instruction operand fields for consistency with Python dis.'
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ShaharNaveh ShaharNaveh marked this pull request as ready for review March 2, 2026 09:29
@ShaharNaveh ShaharNaveh requested a review from youknowone March 2, 2026 09:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | 🔴 Critical

Guard SWAP depth before index arithmetic to prevent panic

len - 1 - index.get(arg) as usize can underflow and panic on malformed bytecode. Return JitCompileError::BadBytecode instead 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: Decode RaiseVarargs kind 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: Align Copy comments with current opcode naming.

Line [1330]–Line [1332] still mention CopyItem { index: ... }; the active variant here is Instruction::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

📥 Commits

Reviewing files that changed from the base of the PR and between baba1f9 and cf22b78.

📒 Files selected for processing (8)
  • crates/codegen/src/compile.rs
  • crates/codegen/src/ir.rs
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/jit/src/instructions.rs
  • crates/jit/tests/common.rs
  • crates/stdlib/src/_opcode.rs
  • crates/vm/src/builtins/frame.rs
  • crates/vm/src/frame.rs

@youknowone
Copy link
Member

@ShaharNaveh I am sorry, i had to merge this first. it has conflict on frame.rs

@ShaharNaveh
Copy link
Contributor Author

@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

@ShaharNaveh ShaharNaveh requested a review from youknowone March 3, 2026 12:56
@youknowone youknowone enabled auto-merge (squash) March 3, 2026 15:46
@youknowone youknowone merged commit e9001ec into RustPython:main Mar 3, 2026
63 of 68 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants