Implement minimal builtins.anext() #6226
Conversation
issue : RustPython#3609 Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Builtin as anext() Builtin
participant Iterator as Async Iterator
participant Awaitable as __anext__ Awaitable
User->>Builtin: anext(aiter, [default_value])
Builtin->>Iterator: Access __anext__ method
Iterator->>Awaitable: Return awaitable
Awaitable-->>Builtin: Awaitable object
alt default_value provided
Note over Builtin: TODO: Handle default value<br/>(incomplete)
end
Builtin-->>User: Return awaitable
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
|
@youknowone can you provide some direction to implement PyAnextAwaitable. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vm/src/stdlib/builtins.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Format Rust code with the default rustfmt style (runcargo fmt)
Run clippy and fix any warnings or lints introduced by your changes
Follow Rust best practices for error handling and memory management
Files:
vm/src/stdlib/builtins.rs
{vm,stdlib}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use RustPython macros (
pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
vm/src/stdlib/builtins.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
| #[pyfunction] | ||
| fn anext( | ||
| aiter: PyObjectRef, | ||
| default_value: OptionalArg<PyObjectRef>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let awaitable = vm.call_method(&aiter, "__anext__", ())?; | ||
|
|
||
| if default_value.is_missing() { | ||
| Ok(awaitable) | ||
| } else { | ||
| // TODO: Implement CPython like PyAnextAwaitable to properly handle the default value. | ||
| Ok(awaitable) | ||
| } | ||
| } |
There was a problem hiding this comment.
Incomplete default value handling makes the function only partially functional.
The default_value parameter is accepted but completely ignored—both branches return the same awaitable regardless of whether a default is provided. This causes the function to fail the second use case described in the PR objectives: when the async iterator is exhausted and a default value is provided, RustPython raises StopAsyncIteration instead of returning the default like CPython does.
While the TODO comment acknowledges this limitation, having a function signature that accepts a parameter it cannot honor is problematic because:
- Users expect CPython-compatible behavior
- The silent acceptance of
default_valuecreates a false sense of functionality - It's better to fail fast than fail silently
Consider one of these approaches before merging:
Option 1 (preferred if feasible): Complete the implementation by adding PyAnextAwaitable to wrap the awaitable and handle the default value case.
Option 2: Reject the default parameter explicitly until support is implemented:
#[pyfunction]
fn anext(
aiter: PyObjectRef,
default_value: OptionalArg<PyObjectRef>,
vm: &VirtualMachine,
) -> PyResult {
let awaitable = vm.call_method(&aiter, "__anext__", ())?;
- if default_value.is_missing() {
- Ok(awaitable)
- } else {
- // TODO: Implement CPython like PyAnextAwaitable to properly handle the default value.
- Ok(awaitable)
+ if let OptionalArg::Present(_) = default_value {
+ Err(vm.new_not_implemented_error(
+ "anext() with default value is not yet implemented".to_owned()
+ ))
+ } else {
+ Ok(awaitable)
}
}Option 3: Defer merging this PR until the default value handling is fully implemented.
📝 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.
| #[pyfunction] | |
| fn anext( | |
| aiter: PyObjectRef, | |
| default_value: OptionalArg<PyObjectRef>, | |
| vm: &VirtualMachine, | |
| ) -> PyResult { | |
| let awaitable = vm.call_method(&aiter, "__anext__", ())?; | |
| if default_value.is_missing() { | |
| Ok(awaitable) | |
| } else { | |
| // TODO: Implement CPython like PyAnextAwaitable to properly handle the default value. | |
| Ok(awaitable) | |
| } | |
| } | |
| #[pyfunction] | |
| fn anext( | |
| aiter: PyObjectRef, | |
| default_value: OptionalArg<PyObjectRef>, | |
| vm: &VirtualMachine, | |
| ) -> PyResult { | |
| let awaitable = vm.call_method(&aiter, "__anext__", ())?; | |
| if let OptionalArg::Present(_) = default_value { | |
| Err(vm.new_not_implemented_error( | |
| "anext() with default value is not yet implemented".to_owned() | |
| )) | |
| } else { | |
| Ok(awaitable) | |
| } | |
| } |
|
Thanks! By looking the CI result, the test is failing because this patch fixed a few tests: Remove For PyAnextAwaitable, I'd start to defining the type on |
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
issue : #3609
Currently passes
Failed case ,need PyAnextAwaitable
Summary by CodeRabbit
anextbuiltin function to retrieve the next item from asynchronous iterators with optional default value support.