GeezerVison is showing github.com — Newtype `LoadAttr` oparg by ShaharNaveh · Pull Request #7047 · RustPython/RustPython
Conversation
ShaharNaveh
commented
Feb 8, 2026
•
edited by coderabbitai
Bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Summary by CodeRabbit
- Refactor
- Improved internal code organization by refactoring attribute access operation handling to use a structured builder pattern, enhancing maintainability and consistency across the system. Observable behavior remains unchanged.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
coderabbitai
Bot
commented
Feb 8, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
📝 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
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 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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Mar 23, 2026
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|