Skip to content

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Feb 6, 2026

Discussed at #6980 (comment)
Pretty much reverts #6980.

Summary by CodeRabbit

  • Chores

    • Removed unused internal dependency
    • Internal code structure improvements and standardization
  • Refactor

    • Simplified internal type conversion patterns across the compiler

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 6, 2026

📝 Walkthrough

Walkthrough

This PR refactors bytecode enum handling and casting patterns across the compiler. It replaces explicit u32 casts with standard conversion functions (u32::from()), removes the num_enum dependency, adds Debug derives to bytecode types, macro-generates oparg enum definitions and trait implementations, and removes const qualifiers from eval_ord methods. All changes maintain functional equivalence while modernizing the codebase.

Changes

Cohort / File(s) Summary
Casting & Conversion Modernization
crates/codegen/src/compile.rs, crates/vm/src/types/slot.rs
Updated explicit u32 and u8 casts to use u32::from() and u8::from() for ResumeType and comparison operators. In compile.rs, also adjusted oparg calculation for BUILD_INTERPOLATION. Removed const qualifier from eval_ord methods in slot.rs.
Bytecode Debug Support
crates/compiler-core/src/bytecode.rs
Added Debug derive to CodeUnit and CodeUnits structs to enable debug formatting alongside existing Copy/Clone and Clone derives respectively.
Enum Macro Refactoring
crates/compiler-core/src/bytecode/oparg.rs
Introduced oparg_enum! and impl_oparg_enum macros to auto-generate TryFrom, From, and OpArgType trait implementations. Refactored all public enums (ConvertValueOparg, ResumeType, RaiseKind, IntrinsicFunction1/2, Intrinsic, BinaryOperator, ComparisonOperator, Invert, SpecialMethod, CommonConstant, BuildSliceArgCount) to use macro-based definitions, eliminating repetitive manual trait impls.
Dependency Cleanup
crates/compiler-core/Cargo.toml
Removed workspace dependency on num_enum from [dependencies] section.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 Hoppy hops through casting code,
Macros bloom along the road!
Enums dance in templates bright,
From-conversions feel so right! ✨
Debug fields now shine with care,
Refactored with rabbit flair!

🚥 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 'Bytecode oparg optimization' directly and accurately reflects the main objective of the PR: optimizing bytecode operand arguments through macro-based enum generation and the removal of the num_enum dependency.
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.

$vis enum $name {
$(
$(#[$variant_meta])*
$variant, // Do assign value to variant.
Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Feb 6, 2026

Choose a reason for hiding this comment

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

@youknowone This breaks RustPython. if I put $variant = $value then everything works just fine.
I suspect that there's some unsafe transmuting somewhere, do you have an idea how can I debug this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After lots of debugging I've figured that the only problematic enum is ComparisonOperator. I'm not sure about why yet, but it's not related to an unsafe transmutation like I initially thought

Copy link
Member

Choose a reason for hiding this comment

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

ComparisonOperator seems to support bit operation. It doesn't have 0 like other enums

@ShaharNaveh ShaharNaveh marked this pull request as ready for review February 7, 2026 07:00
@youknowone youknowone enabled auto-merge (squash) February 7, 2026 10:11
@youknowone youknowone disabled auto-merge February 7, 2026 10:14
@youknowone youknowone merged commit f817ab8 into RustPython:main Feb 7, 2026
13 checks passed
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