Newtype `LoadAttr` oparg by ShaharNaveh · Pull Request #7047 · RustPython/RustPython

ShaharNaveh

📝 Walkthrough

Walkthrough

The PR refactors LOAD_ATTR operand handling by introducing a structured LoadAttr oparg type with builder pattern, removing standalone encode/decode helper functions, and updating all call sites to use the new builder-based construction.

Changes

Cohort / File(s)	Summary
New Oparg Type `crates/compiler-core/src/bytecode/oparg.rs`	Introduces public `LoadAttr` struct with builder pattern, storing a u32 internally with methods to extract name_idx (via right-shift) and is_method flag (least significant bit), plus `LoadAttrBuilder` for ergonomic construction.
Instruction and Export Updates `crates/compiler-core/src/bytecode/instruction.rs`, `crates/compiler-core/src/bytecode.rs`	Changes LoadAttr instruction variant from `Arg<NameIdx>` to `Arg<LoadAttr>`, removes public helper functions `encode_load_attr_arg` and `decode_load_attr_arg`, updates public re-exports to remove the old helpers and add LoadAttr to oparg exports.
Call Site Updates `crates/codegen/src/compile.rs`, `crates/vm/src/frame.rs`	Updates LOAD_ATTR emission in codegen to use `LoadAttr::builder().name_idx(...)` pattern instead of calling encode helper; updates VM frame handler to accept `LoadAttr` parameter and use `oparg.name_idx()` and `oparg.is_method()` accessors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Move OpArg to its own file #6703: Reorganized oparg into its own module and established re-export surface, providing the foundation for exposing the new LoadAttr type.
Bytecode pseudo opcodes #6715: Previously introduced encode_/decode_load_attr_arg helpers that this PR replaces with a structured LoadAttr type.
Pseudo ops #6678: Modified LOAD_ATTR operand encoding/decoding via helper functions, addressing similar concerns that this PR resolves via a builder pattern.

Suggested reviewers

youknowone

Poem

🐰 A builder rises where helpers once stood,
LoadAttr now structured—just as it should!
No more bit-twiddling, the oparg shines bright,
With names and methods packed oh-so-right! 🎉

🚥 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 describes the main change: introducing a newtype wrapper for the LoadAttr oparg, replacing raw u32 encoding with a structured LoadAttr type throughout the codebase.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

crates/compiler-core/src/bytecode/oparg.rs (1)
797-820: Missing From<LoadAttrBuilder> for LoadAttr impl for consistency with LoadSuperAttr.

LoadSuperAttrBuilder implements From<LoadSuperAttrBuilder> for LoadSuperAttr (line 752–756), enabling ergonomic .into() conversions. Consider adding the equivalent for LoadAttrBuilder.
♻️ Proposed addition after line 820
+impl From<LoadAttrBuilder> for LoadAttr {
+    fn from(builder: LoadAttrBuilder) -> Self {
+        builder.build()
+    }
+}

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.}