Newtype `var_num` oparg by ShaharNaveh · Pull Request #7400 · RustPython/RustPython
📝 Walkthrough
Walkthrough
This PR introduces a new oparg::VarNum typed newtype to replace generic numeric indices for variable references throughout the compiler. Methods, instruction variants, and indexing operations are updated across codegen, bytecode, and runtime modules to use this more type-safe representation for variable number arguments.
Changes
| Cohort / File(s) | Summary |
|---|---|
Type System Foundation crates/compiler-core/src/bytecode/oparg.rs |
Introduces VarNum(u32) newtype via macro-based pattern alongside refactored ConstIdx. Adds from_u32, as_u32, and as_usize accessors with trait impls (From, OpArgType) for both types. |
Bytecode Indexing & Traits crates/compiler-core/src/bytecode.rs |
Adds Index and IndexMut implementations for slices using oparg::VarNum. Updates InstrDisplayContext trait and CodeObject impl to accept VarNum instead of usize for variable name lookups. Adjusts internal name mapping logic in map_bag. |
Instruction Variant Definitions crates/compiler-core/src/bytecode/instruction.rs |
Changes var_num field type from Arg<NameIdx> to Arg<oparg::VarNum> in six instruction variants: DeleteFast, LoadFast, LoadFastAndClear, LoadFastBorrow, LoadFastCheck, StoreFast. Updates disassembly formatting logic to convert arguments for display. |
Code Generation crates/codegen/src/compile.rs, crates/codegen/src/ir.rs |
Updates get_local_var_index and varname methods to return oparg::VarNum instead of u32/NameIdx. Updates _name_inner return type to u32 with conversions to VarNum at call sites. Changes get_varname signature in IR context to accept VarNum with .as_usize() indexing. |
Runtime & JIT crates/jit/src/instructions.rs, crates/vm/src/frame.rs |
Updates store_variable signature to accept oparg::VarNum. Refactors pattern bindings in VM frame dispatch to use var_num directly instead of nested idx binding, updating all .get(arg) call sites to use the new binding. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
- Newtype ConstIdx, Constants #7358: Introduces typed oparg newtypes (
VarNum,ConstIdx) and updates indexing/instruction variants to use typed indices for better type safety. - Move
OpArgto its own file #6703: Expands bytecode oparg surface with new typed argument patterns and updates instruction/codegen modules to consume them. - Use
num_enumcrate for oparg types #6980: Modifies oparg type definitions and instruction argument types with similar refactoring of how indices are represented and used.
Suggested reviewers
- youknowone
Poem
🐰 A rabbit hops through types so neat,
Where VarNum makes indices complete!
From plain ol' u32 we say goodbye,
Type safety's here—no more guessing why! ✨
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 44.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Newtype var_num oparg' directly reflects the main change: introducing a newtype for var_num in the oparg module to improve type safety in variable indexing across the codebase. |
✏️ 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.
Comment @coderabbitai help to get the list of available commands and usage tips.