Make `OpArgType::from_op_arg` to return a `Result<Self, MarshalError>` by ShaharNaveh · Pull Request #6914 · RustPython/RustPython

ShaharNaveh

📝 Walkthrough

Walkthrough

A refactoring of error handling in bytecode instruction processing, converting return types from Option<T> to Result<T, MarshalError> across the OpArgType trait and Arg<T>::try_get method to provide more descriptive error information on invalid bytecode.

Changes

Cohort / File(s)	Summary
OpArgType trait refactoring `crates/compiler-core/src/bytecode/oparg.rs`	Changed `from_op_arg` return type from `Option<Self>` to `Result<Self, MarshalError>` in trait definition and all implementations (`ConvertValueOparg`, `u32`, `bool`, `Label`, `MakeFunctionFlags`, `BuildSliceArgCount`, `UnpackExArgs`, and macro-generated enums). Invalid bytecode conditions now return `Err(MarshalError::InvalidBytecode)` instead of `None`.
Arg type getter `crates/compiler-core/src/bytecode/instruction.rs`	Updated `Arg<T>::try_get` method signature to return `Result<T, MarshalError>` instead of `Option<T>`, delegating to the updated `T::from_op_arg` implementation.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

RustPython/RustPython#6703: This PR directly modifies the same OpArgType::from_op_arg trait method and Arg<T>::try_get implementation, converting their return types from Option to Result<_, MarshalError>.

Suggested reviewers

youknowone

Poem

🐰 From Options to Results, we now declare,
Error types flowing with much greater care,
MarshalError whispers of bytecode gone wrong,
No more silent Nones in our instruction song!
The bytecode now speaks what went awry, 🎯

🚥 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 pull request title accurately reflects the main change: converting OpArgType::from_op_arg to return Result<Self, MarshalError> instead of Option. This is clearly the primary objective of the changeset across both modified files.
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.}