Skip to content

Resume opcode to hold ResumeType#7465

Merged
youknowone merged 2 commits intoRustPython:mainfrom
ShaharNaveh:resume-type-oparg
Mar 19, 2026
Merged

Resume opcode to hold ResumeType#7465
youknowone merged 2 commits intoRustPython:mainfrom
ShaharNaveh:resume-type-oparg

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Mar 19, 2026

Summary by CodeRabbit

  • Refactor
    • Improved type safety in bytecode instruction handling by refining argument type representation for resume operations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 19, 2026

📝 Walkthrough

Walkthrough

Refactored the Instruction::Resume opcode's operand type from Arg<u32> to Arg<oparg::ResumeType>, replacing the auto-generated ResumeType enum with a manually defined variant that includes a catch-all Other(u32) case, and updated all emission sites to pass enum values directly.

Changes

Cohort / File(s) Summary
ResumeType Enum Redefinition
crates/compiler-core/src/bytecode/oparg.rs
Replaced auto-generated ResumeType with manual enum definition featuring named variants (AtFuncStart, AfterYield, AfterYieldFrom, AfterAwait) plus Other(u32) catch-all; added bidirectional From<u32> and From<ResumeType> for u32 conversions, Display trait, and OpArgType implementation.
Instruction Type Update
crates/compiler-core/src/bytecode/instruction.rs
Changed Instruction::Resume { context: Arg<u32> } field type to Arg<oparg::ResumeType>, updating the typed interpretation of the RESUME opcode operand.
Emission Site Updates
crates/codegen/src/compile.rs
Updated four Resume instruction emission sites to pass ResumeType enum variants directly instead of converting them via u32::from(...), affecting yield-from/await sequences and generator-expression resume paths.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A rabbit hops through bytecode neat,
No more raw u32s to compete—
Resume now carries types so fine,
Each variant dancing in perfect line,
Enums and conversions align! 🎯

🚥 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 summarizes the main change: updating the Resume opcode's context field from u32 to hold ResumeType enum values directly.
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 unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

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.

@ShaharNaveh
Copy link
Contributor Author

ShaharNaveh commented Mar 19, 2026

@youknowone I'm thinking to create a derive macro called: #[newtype_oparg] that can be defined on either struct or enums. this will help streamline the oparg definition while being able to handle slightly less straight forward cases like overriding the fmt::Display for specific enum variants like, like we do here:

impl fmt::Display for ConvertValueOparg {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let out = match self {
Self::Str => "1 (str)",
Self::Repr => "2 (repr)",
Self::Ascii => "3 (ascii)",
// We should never reach this. `FVC_NONE` are being handled by `Instruction::FormatSimple`
Self::None => "",
};
write!(f, "{out}")
}
}

and

impl fmt::Display for SpecialMethod {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let method_name = match self {
Self::Enter => "__enter__",
Self::Exit => "__exit__",
Self::AEnter => "__aenter__",
Self::AExit => "__aexit__",
};
write!(f, "{method_name}")
}
}

(we have more places)

Also with generate the From<u32> and From<OPARG_ENUM> for u32 for enums with a fallback variant, like here with "Other(u32)". Instruction::GetAwaitable oparg can also use this pattern


lmk what you think:)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode/oparg.rs (1)

313-317: Consider showing variant names in Display for better diagnostics.

The current Display impl shows only the numeric value. For disassembly output, showing variant names could improve readability.

🔧 Optional enhancement for Display
 impl core::fmt::Display for ResumeType {
     fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        u32::from(*self).fmt(f)
+        match self {
+            Self::AtFuncStart => write!(f, "0 (AtFuncStart)"),
+            Self::AfterYield => write!(f, "1 (AfterYield)"),
+            Self::AfterYieldFrom => write!(f, "2 (AfterYieldFrom)"),
+            Self::AfterAwait => write!(f, "3 (AfterAwait)"),
+            Self::Other(v) => write!(f, "{v}"),
+        }
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 313 - 317, The
Display impl for ResumeType currently prints only its numeric value
(u32::from(*self).fmt(f)); change it to print human-friendly variant names for
diagnostics by matching on ResumeType (in the impl core::fmt::Display for
ResumeType, fn fmt) and write the variant identifier strings (e.g., "None",
"Yield", etc.) to the formatter, optionally falling back to the numeric value
for unknown/other variants to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/compiler-core/src/bytecode/oparg.rs`:
- Around line 313-317: The Display impl for ResumeType currently prints only its
numeric value (u32::from(*self).fmt(f)); change it to print human-friendly
variant names for diagnostics by matching on ResumeType (in the impl
core::fmt::Display for ResumeType, fn fmt) and write the variant identifier
strings (e.g., "None", "Yield", etc.) to the formatter, optionally falling back
to the numeric value for unknown/other variants to preserve behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a841bb7a-52f4-4af9-85c5-31e89e5a26d1

📥 Commits

Reviewing files that changed from the base of the PR and between b11b8e6 and adae369.

📒 Files selected for processing (3)
  • crates/codegen/src/compile.rs
  • crates/compiler-core/src/bytecode/instruction.rs
  • crates/compiler-core/src/bytecode/oparg.rs

@youknowone
Copy link
Member

@ShaharNaveh sounds good to me. Could you show me an example how it will look like once the macro is introduced?

@youknowone youknowone enabled auto-merge (squash) March 19, 2026 13:36
@ShaharNaveh
Copy link
Contributor Author

ShaharNaveh commented Mar 19, 2026

@ShaharNaveh sounds good to me. Could you show me an example how it will look like once the macro is introduced?

I was thinking something like:

#[newtype_oparg]
struct VarNum(u32);

#[newtype_oparg]
enum SpecialMethod {
  /// `__enter__` for sync context manager
  #[oparg(display = "__enter__")]
  Enter = 0,
  /// `__exit__` for sync context manager
  #[oparg(display = "__exit__")]
  Exit = 1,
  ...
}

#[newtype_oparg]
enum ResumeType {
  AfterYield = 1
  AfterYieldFrom = 2,
  #[oparg(catch_all)]
  Other(u32)
}

@youknowone youknowone merged commit 53fa525 into RustPython:main Mar 19, 2026
15 checks passed
@ShaharNaveh
Copy link
Contributor Author

@youknowone loks good?

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