Add raise_if_stop! macro for protocol/iter.rs
#5885
Conversation
WalkthroughA new macro, Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
vm/src/stdlib/itertools.rs
Outdated
| @@ -1,5 +1,14 @@ | |||
| pub(crate) use decl::make_module; | |||
|
|
|||
| macro_rules! handle_pyiter_return { | |||
There was a problem hiding this comment.
This macro is expected to be defined on vm/src/protocol/iter.rs because it is mainly a function of PyIterReturn.
The name can be simplified as what it means in Python.
| macro_rules! handle_pyiter_return { | |
| macro_rules! raise_stop { |
I like this idea 👍
handle_pyiter_return! macro for itertoolsraise_stop! macro for protocol/iter.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
vm/src/protocol/iter.rs (1)
282-289: Consider error propagation in the macro.The macro doesn't handle potential errors that might occur when evaluating the input expression. If the input expression can fail, those errors should be propagated.
Consider this alternative implementation that handles potential errors in the input:
macro_rules! raise_stop { ($input:expr) => { - match $input { + match $input? { $crate::protocol::PyIterReturn::Return(obj) => obj, $crate::protocol::PyIterReturn::StopIteration(v) => { return Ok($crate::protocol::PyIterReturn::StopIteration(v)) } } }; }However, verify this change aligns with the intended usage patterns in
vm/src/stdlib/itertools.rs.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vm/src/protocol/iter.rs(1 hunks)vm/src/stdlib/itertools.rs(30 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vm/src/stdlib/itertools.rs
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
vm/src/protocol/iter.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
vm/src/protocol/iter.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.851Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (1)
vm/src/protocol/iter.rs (1)
280-290: Macro implementation follows Rust best practices.The macro correctly uses
$crate::prefix for absolute path resolution and implements the expected pattern matching logic forPyIterReturnhandling. The early return pattern is appropriate for iterator implementations.
vm/src/protocol/iter.rs
Outdated
| #[macro_export] | ||
| macro_rules! raise_stop { | ||
| ($input:expr) => { | ||
| match $input { | ||
| $crate::protocol::PyIterReturn::Return(obj) => obj, | ||
| $crate::protocol::PyIterReturn::StopIteration(v) => { | ||
| return Ok($crate::protocol::PyIterReturn::StopIteration(v)) | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the macro's usage constraints.
The macro assumes it's used within a function that returns PyResult<PyIterReturn>, but this constraint isn't documented. Consider adding documentation to clarify the expected usage context.
Add documentation above the macro:
+/// Macro to handle `PyIterReturn` values in iterator implementations.
+///
+/// Extracts the object from `PyIterReturn::Return(obj)` or performs early return
+/// for `PyIterReturn::StopIteration(v)`. This macro should only be used within
+/// functions that return `PyResult<PyIterReturn>`.
+///
+/// # Example
+/// ```rust
+/// fn iterator_next(&self, vm: &VirtualMachine) -> PyResult<PyIterReturn> {
+/// let value = raise_stop!(some_pyiter_return_value);
+/// // Process the extracted value...
+/// }
+/// ```
#[macro_export]
macro_rules! raise_stop {🤖 Prompt for AI Agents
In vm/src/protocol/iter.rs around lines 280 to 290, the raise_stop! macro lacks
documentation specifying that it must be used within a function returning
PyResult<PyIterReturn>. Add a doc comment above the macro definition explaining
this usage constraint and provide a brief example showing how to use the macro
inside such a function to clarify its expected context.
Naming inconsistency with PR objectives.
The PR title mentions handle_pyiter_return! macro, but the implemented macro is named raise_stop!. Please ensure the naming is consistent with the intended functionality and PR objectives.
Consider renaming the macro to match the PR title:
-macro_rules! raise_stop {
+macro_rules! handle_pyiter_return {Or update the PR title if raise_stop! is the preferred name.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[macro_export] | |
| macro_rules! raise_stop { | |
| ($input:expr) => { | |
| match $input { | |
| $crate::protocol::PyIterReturn::Return(obj) => obj, | |
| $crate::protocol::PyIterReturn::StopIteration(v) => { | |
| return Ok($crate::protocol::PyIterReturn::StopIteration(v)) | |
| } | |
| } | |
| }; | |
| } | |
| #[macro_export] | |
| macro_rules! handle_pyiter_return { | |
| ($input:expr) => { | |
| match $input { | |
| $crate::protocol::PyIterReturn::Return(obj) => obj, | |
| $crate::protocol::PyIterReturn::StopIteration(v) => { | |
| return Ok($crate::protocol::PyIterReturn::StopIteration(v)) | |
| } | |
| } | |
| }; | |
| } |
🤖 Prompt for AI Agents
In vm/src/protocol/iter.rs around lines 280 to 290, the macro is named
raise_stop! but the PR title refers to handle_pyiter_return!. Rename the macro
from raise_stop! to handle_pyiter_return! to maintain consistency with the PR
objectives, or alternatively update the PR title to reflect the current macro
name if that is preferred.
vm/src/stdlib/itertools.rs
Outdated
| PyIterReturn::Return(obj) => obj, | ||
| PyIterReturn::StopIteration(v) => return Ok(PyIterReturn::StopIteration(v)), | ||
| }; | ||
| let obj = raise_stop!(zelf.iterable.next(vm)?); |
There was a problem hiding this comment.
uh, my fault. now returning a value from raise_stop! looks awkward.
| let obj = raise_stop!(zelf.iterable.next(vm)?); | |
| let obj = raise_if_stop!(zelf.iterable.next(vm)?); |
I will add if in the middle
There was a problem hiding this comment.
No worries it's just a simple: :%s/raise_stop/raise_if_stop/g 😉
raise_stop! macro for protocol/iter.rsraise_if_stop! macro for protocol/iter.rs
Summary by CodeRabbit
No changes to user-facing features or functionality.