Skip to content

Newtype oparg align methods#7403

Merged
youknowone merged 3 commits intoRustPython:mainfrom
ShaharNaveh:newtype-oparg-align
Mar 12, 2026
Merged

Newtype oparg align methods#7403
youknowone merged 3 commits intoRustPython:mainfrom
ShaharNaveh:newtype-oparg-align

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 12, 2026

Summary by CodeRabbit

  • Refactor
    • Internal code structure improvements across multiple modules for better maintainability and consistency.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 12, 2026

📝 Walkthrough

Walkthrough

This PR refactors the Label oparg type from a tuple struct with direct field access to a newtype with constructor and accessor methods. Multiple oparg types (Label, StoreFastLoadFast, ConstIdx, VarNum, LoadAttr, LoadSuperAttr) are reimplemented using a macro-driven newtype pattern. Call sites across codegen, compiler-core, jit, and vm are systematically updated to use Label::new() constructors and as_u32()/as_usize() accessors instead of tuple field access.

Changes

Cohort / File(s) Summary
Oparg Type Refactoring
crates/compiler-core/src/bytecode/oparg.rs
Introduces newtype_oparg macro to generate uniform newtypes for oparg types. Replaces ad-hoc Label/StoreFastLoadFast implementations with macro-generated versions. Adds ConstIdx, VarNum, LoadAttr, LoadSuperAttr newtypes and LoadSuperAttrBuilder. All types now include constructor, conversion, and Display impls via macro.
Label Constructor Method Addition
crates/compiler-core/src/bytecode.rs
Adds Label::new(u32) constructor in CodeObject::display_inner call site.
Label API Updates - Codegen & JIT
crates/codegen/src/ir.rs, crates/jit/src/instructions.rs
Replace Label tuple construction Label(value) with Label::new(value) and field access .0 with as_u32() / as_usize() accessors in jump target calculations.
Label API Updates - VM
crates/vm/src/frame.rs
Systematically update all Label construction and access patterns across jump operations, exception handling, and iterator instructions to use Label::new() constructors and as_u32() / as_usize() accessors.
Instruction Formatting
crates/compiler-core/src/bytecode/instruction.rs
Adjust fmt_dis macro to format argument without explicit u32 conversion in display output.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A Label once lived tuple-free,
With constructor methods, oh what glee!
As_u32() reads its heart so true,
A macro-born newtype, shiny and new!
From codegen to VM, changes all align,
A refactor so neat, it's divine!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main refactoring work: converting oparg types to newtypes with a uniform constructor/accessor method interface.
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 12, 2026 12:46
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.

🧹 Nitpick comments (2)
crates/vm/src/frame.rs (1)

6835-6846: Deduplicate the FOR_ITER exhaustion target adjustment.

for_iter_jump_on_exhausted() now re-implements the same END_FOR skip logic as for_iter_jump_target(). Reusing the helper would keep this control-flow rule in one place.

♻️ Proposed cleanup
     /// Handle iterator exhaustion in specialized FOR_ITER handlers.
     /// Skips END_FOR if present at target and jumps.
     fn for_iter_jump_on_exhausted(&mut self, target: bytecode::Label) {
-        let target_idx = target.as_usize();
-        let jump_target = if let Some(unit) = self.code.instructions.get(target_idx) {
-            if matches!(
-                unit.op,
-                bytecode::Instruction::EndFor | bytecode::Instruction::InstrumentedEndFor
-            ) {
-                bytecode::Label::new(target.as_u32() + 1)
-            } else {
-                target
-            }
-        } else {
-            target
-        };
-        self.jump(jump_target);
+        self.jump(self.for_iter_jump_target(target));
     }

Also applies to: 8817-8832

🤖 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 6835 - 6846, The FOR_ITER exhaustion
target adjustment logic is duplicated between for_iter_jump_target and
for_iter_jump_on_exhausted; refactor so the rule lives in one place by making
for_iter_jump_on_exhausted call for_iter_jump_target (or extract a shared helper
used by both). Locate the duplicate logic in functions for_iter_jump_target and
for_iter_jump_on_exhausted and remove the repeated END_FOR/InstrumentedEndFor
check from for_iter_jump_on_exhausted, delegating to for_iter_jump_target to
perform the END_FOR skip and return the adjusted bytecode::Label.
crates/compiler-core/src/bytecode/oparg.rs (1)

702-705: From<$name> for usize bakes in a pointer-width assumption.

std intentionally does not provide From<u32> for usize; this macro reintroduces the same potentially truncating cast for every oparg wrapper. I'd keep as_usize() as the explicit conversion and only add a trait impl here if you want a checked TryFrom.

Suggested change
-        impl From<$name> for usize {
-            fn from(value: $name) -> Self {
-                value.as_usize()
-            }
-        }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 702 - 705, The impl
From<$name> for usize bakes in a pointer-width assumption and can truncate on
some platforms; remove that impl and keep callers using the explicit as_usize()
method on the wrapper ($name.as_usize()). If you need a fallible/checked
conversion, add a TryFrom<$name> for usize implementation instead (implement
TryFrom::try_from for $name that performs a safe/checked conversion and returns
a Result), but do not reintroduce the infallible From<$name> for usize.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 702-705: The impl From<$name> for usize bakes in a pointer-width
assumption and can truncate on some platforms; remove that impl and keep callers
using the explicit as_usize() method on the wrapper ($name.as_usize()). If you
need a fallible/checked conversion, add a TryFrom<$name> for usize
implementation instead (implement TryFrom::try_from for $name that performs a
safe/checked conversion and returns a Result), but do not reintroduce the
infallible From<$name> for usize.

In `@crates/vm/src/frame.rs`:
- Around line 6835-6846: The FOR_ITER exhaustion target adjustment logic is
duplicated between for_iter_jump_target and for_iter_jump_on_exhausted; refactor
so the rule lives in one place by making for_iter_jump_on_exhausted call
for_iter_jump_target (or extract a shared helper used by both). Locate the
duplicate logic in functions for_iter_jump_target and for_iter_jump_on_exhausted
and remove the repeated END_FOR/InstrumentedEndFor check from
for_iter_jump_on_exhausted, delegating to for_iter_jump_target to perform the
END_FOR skip and return the adjusted bytecode::Label.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: e1a75ba9-493f-4eb0-99ba-b6eefb796523

📥 Commits

Reviewing files that changed from the base of the PR and between 331029e and 8acdc3e.

📒 Files selected for processing (6)
  • crates/codegen/src/ir.rs
  • crates/compiler-core/src/bytecode.rs
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/compiler-core/src/bytecode/oparg.rs
  • crates/jit/src/instructions.rs
  • crates/vm/src/frame.rs

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

👍

@youknowone youknowone merged commit bb36783 into RustPython:main Mar 12, 2026
15 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