◐ Shell
reader mode source ↗
Skip to content

Use num_enum crate for oparg types#6980

Merged
youknowone merged 10 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-oparg-num-enum
Feb 6, 2026
Merged

Use num_enum crate for oparg types#6980
youknowone merged 10 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-oparg-num-enum

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Feb 3, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Chores

    • Standardized opcode/argument conversions across the bytecode layer for greater consistency.
    • Improved error handling for invalid bytecode, reducing ambiguous failures and making errors more actionable.
    • Updated textual formatting for several bytecode entities, making diagnostics and messages clearer.
  • New Features

    • Exposed and stabilized many bytecode argument types, labels, operator flags and enums with explicit numeric representations to improve serialization, interoperability and tooling.

@coderabbitai

coderabbitai Bot commented Feb 3, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Replaced bespoke op-arg conversion helpers with Into/TryFrom (num_enum-derived) and added workspace dependency num_enum. Many op-arg enums were converted to explicit repr(u8) types, new op-arg types were introduced, and Arg conversion/error paths were updated.

Changes

Cohort / File(s) Summary
Dependency Addition
crates/compiler-core/Cargo.toml
Added workspace dependency num_enum = { workspace = true }.
Instruction Arg Conversion
crates/compiler-core/src/bytecode/instruction.rs
Switched Arg construction/access to use Into<u32> / TryFrom<u32>; updated error mapping to MarshalError::InvalidBytecode and unchecked conversions to use try_from(...).unwrap_unchecked().
OpArg Types & Trait
crates/compiler-core/src/bytecode/oparg.rs
Changed OpArgType trait to Copy + Into<u32> + TryFrom<u32>; replaced macro-generated enums with explicit repr(u8) enums deriving num_enum traits; added new public types (Label, intrinsics, RaiseKind, ResumeType, flags, etc.), conversions, Display impls, and a helper for invalid-bytecode errors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 I found a crate named num_enum today,

Into and TryFrom hopped into play.
Enums donned u8 coats, tidy and bright,
Labels and flags danced into the light,
A rabbit cheers — bytecode leaps in flight.

🚥 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: refactoring oparg types to use the num_enum crate, which is clearly demonstrated by the dependency addition and the widespread trait implementation changes.
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 docstrings
🧪 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide 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).

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Sorry! forgot to finish the comment. I had to investigte the background a little bit.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Hide details View details @youknowone youknowone merged commit 956f471 into RustPython:main Feb 6, 2026
13 checks passed
This was referenced Mar 11, 2026
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