Use num_enum crate for oparg types#6980
Conversation
📝 WalkthroughWalkthroughReplaced bespoke op-arg conversion helpers with Into/TryFrom (num_enum-derived) and added workspace dependency Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Sorry, something went wrong.
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 288-303: The TryFrom<u32> impl for MakeFunctionFlags currently
uses a silent truncating cast (value as u8); change MakeFunctionFlags::try_from
to perform a checked conversion (use u8::try_from(value) and map any Err to
MarshalError::InvalidBytecode) and then call Self::from_bits on the resulting
u8, returning the same InvalidBytecode error if from_bits returns None; update
references in the impl TryFrom<u32> for MakeFunctionFlags to remove the direct
cast and use the checked conversion pattern used elsewhere (e.g., instruction
parsing) so high-bit values are rejected instead of truncated.
🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode/instruction.rs (1)
1271-1276: Update the safety note to match the current conversion.The safety contract still references the old conversion name; adjusting the wording avoids confusion for future readers.
📝 Suggested doc tweak
- /// T::from_op_arg(self) must succeed + /// The conversion from `OpArg` to `T` must succeed.
Sorry, something went wrong.
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 567-580: Replace the infallible From<u32> for UnpackExArgs with a
checked TryFrom<u32> implementation that validates the top two bytes are zero
and returns an appropriate error on non-zero high bits; keep the existing
From<UnpackExArgs> for u32 as-is and ensure UnpackExArgs still implements
OpArgType (update any call sites to use TryFrom::try_from where needed).
Specifically, implement TryFrom<u32> for UnpackExArgs by calling
value.to_le_bytes(), verifying that bytes[2] and bytes[3] are zero, and
constructing Self { before: bytes[0], after: bytes[1] } on success, otherwise
return an error consistent with the crate's error type pattern (matching how
MakeFunctionFlags handles malformed input).
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
Sorry! forgot to finish the comment. I had to investigte the background a little bit.
Sorry, something went wrong.
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 186-195: ResumeType is missing the standard enum boilerplate used
elsewhere: add the impl_oparg_enum_traits! macro invocation for ResumeType and
provide the OpArgType impl for ResumeType (matching the pattern used by
RaiseKind, IntrinsicFunction1, IntrinsicFunction2, etc.) so it can be treated as
an instruction argument; locate the ResumeType enum and insert
impl_oparg_enum_traits!(ResumeType) and the corresponding impl OpArgType for
ResumeType following the same structure as the other enums in this file.
Sorry, something went wrong.
956f471
into
RustPython:main
Feb 6, 2026
Summary by CodeRabbit
Chores
New Features