Make OpArgType::from_op_arg to return a Result<Self, MarshalError>#6914
Merged
youknowone merged 1 commit intoRustPython:mainfrom Jan 31, 2026
Merged
Make OpArgType::from_op_arg to return a Result<Self, MarshalError>#6914youknowone merged 1 commit intoRustPython:mainfrom
OpArgType::from_op_arg to return a Result<Self, MarshalError>#6914youknowone merged 1 commit intoRustPython:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughA refactoring of error handling in bytecode instruction processing, converting return types from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
ShaharNaveh
changed the title
to_oparg to return a Result<Self, MarshalError>OpArgType::from_op_arg to return a Result<Self, MarshalError>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 640-645: from_op_arg for UnpackExArgs currently truncates high
bytes via to_le_bytes; instead validate that the upper two bytes of x are zero
before constructing UnpackExArgs to avoid accepting invalid bytecode: check (x
>> 16) == 0 (or inspect bytes[2..]) and return an appropriate MarshalError if
non-zero, otherwise proceed to extract before and after and return
Ok(Self{before, after}); reference UnpackExArgs, OpArgType::from_op_arg, and
MarshalError to locate where to add the check and error return.
Comment on lines
640
to
645
| impl OpArgType for UnpackExArgs { | ||
| #[inline(always)] | ||
| fn from_op_arg(x: u32) -> Option<Self> { | ||
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | ||
| let [before, after, ..] = x.to_le_bytes(); | ||
| Some(Self { before, after }) | ||
| Ok(Self { before, after }) | ||
| } |
Contributor
There was a problem hiding this comment.
Validate upper bytes for UnpackExArgs to avoid accepting invalid bytecode.
Right now values with non-zero high bytes are silently truncated, which defeats the new error reporting.
🛠️ Suggested fix
fn from_op_arg(x: u32) -> Result<Self, MarshalError> {
- let [before, after, ..] = x.to_le_bytes();
- Ok(Self { before, after })
+ let [before, after, hi1, hi2] = x.to_le_bytes();
+ if hi1 != 0 || hi2 != 0 {
+ return Err(MarshalError::InvalidBytecode);
+ }
+ Ok(Self { before, after })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl OpArgType for UnpackExArgs { | |
| #[inline(always)] | |
| fn from_op_arg(x: u32) -> Option<Self> { | |
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | |
| let [before, after, ..] = x.to_le_bytes(); | |
| Some(Self { before, after }) | |
| Ok(Self { before, after }) | |
| } | |
| impl OpArgType for UnpackExArgs { | |
| #[inline(always)] | |
| fn from_op_arg(x: u32) -> Result<Self, MarshalError> { | |
| let [before, after, hi1, hi2] = x.to_le_bytes(); | |
| if hi1 != 0 || hi2 != 0 { | |
| return Err(MarshalError::InvalidBytecode); | |
| } | |
| Ok(Self { before, after }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 640 - 645,
from_op_arg for UnpackExArgs currently truncates high bytes via to_le_bytes;
instead validate that the upper two bytes of x are zero before constructing
UnpackExArgs to avoid accepting invalid bytecode: check (x >> 16) == 0 (or
inspect bytes[2..]) and return an appropriate MarshalError if non-zero,
otherwise proceed to extract before and after and return Ok(Self{before,
after}); reference UnpackExArgs, OpArgType::from_op_arg, and MarshalError to
locate where to add the check and error return.
youknowone
approved these changes
Jan 31, 2026
This was referenced Feb 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This will allow us to use
num_enumcrate with a custom error, I'll do it in a followup PRSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.