PyAnextAwaitable - #6427
Conversation
Sorry, something went wrong.
WalkthroughAdds qualname storage/accessors for Coro-derived objects, introduces a new PyAnextAwaitable type and registers it, updates builtins.anext to wrap awaitables when a default is provided, and tightens async-iterator protocol checks in get_aiter(). Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Builtin as builtins.anext
participant Obj as AsyncIterableObj
participant Awaitable as Awaitable (from __anext__)
participant Wrapper as PyAnextAwaitable
Caller->>Builtin: anext(obj, default?)
Builtin->>Obj: check __anext__ exists
alt no __anext__
Builtin-->>Caller: TypeError (not async iterator)
else has __anext__
Builtin->>Obj: call __anext__()
Obj-->>Awaitable: returns awaitable
alt default provided
Builtin->>Wrapper: PyAnextAwaitable::new(awaitable, default)
Builtin-->>Caller: return Wrapper
Caller->>Wrapper: await (calls __await__/send/throw)
Wrapper->>Awaitable: delegate send/throw/close
alt Awaitable raises StopAsyncIteration
Wrapper-->>Caller: raise StopIteration(default)
else Awaitable yields value
Wrapper-->>Caller: return value
end
else no default
Builtin-->>Caller: return Awaitable
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using cargo fmt --all. git pull origin asyncgen |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)crates/vm/src/protocol/object.rs (1)📜 Review detailscrates/vm/src/builtins/asyncgenerator.rs (2)106-110: Consider including the actual type name in the error message.
The error message uses a hardcoded string 'async_iterator' rather than the actual type name. CPython typically includes the actual type in such messages.
// Check that __aiter__ did not return a coroutine if iterator.downcast_ref::<PyCoroutine>().is_some() { - return Err(vm.new_type_error( - "'async_iterator' object cannot be interpreted as an async iterable; \ - perhaps you forgot to call aiter()?", - )); + return Err(vm.new_type_error(format!( + "'{}' object cannot be interpreted as an async iterable; \ + perhaps you forgot to call aiter()?", + iterator.class().name() + ))); }469-522: Redundant coroutine check at line 509.
The downcast_ref::<PyCoroutine>() check on line 509 is unreachable for actual coroutines because line 499 already handles the coroutine_type case and returns early. This check would only catch objects that have a __await__ method returning a PyCoroutine, which is already an error condition.
Consider clarifying the intent or removing the redundant check:
- // Check the result is an iterator, not a coroutine - if awaitable.downcast_ref::<PyCoroutine>().is_some() { - return Err(vm.new_type_error("__await__() returned a coroutine")); - } + // Check the __await__ result is not a coroutine (invalid usage) + if awaitable.class().is(vm.ctx.types.coroutine_type) { + return Err(vm.new_type_error("__await__() returned a coroutine")); + }
552-558: Consider propagating close errors instead of silently ignoring them.
The close() method silently ignores both errors from get_awaitable_iter() and from the inner close() call. While ignoring get_awaitable_iter errors may be intentional (the awaitable may already be exhausted), silently ignoring close() errors could hide legitimate issues.
#[pymethod] fn close(&self, vm: &VirtualMachine) -> PyResult<()> { if let Ok(awaitable) = self.get_awaitable_iter(vm) { - let _ = vm.call_method(&awaitable, "close", ()); + // Ignore errors from close() as is typical for cleanup methods, + // but the awaitable may not have a close() method + if let Some(close_method) = vm.get_method(awaitable.clone(), identifier!(vm, close)) { + let _ = close_method?.call((), vm); + } } Ok(()) }
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 835918a and 571539d.
⛔ Files ignored due to path filters (3)- Lib/test/test_asyncgen.py is excluded by !Lib/**
- Lib/test/test_generators.py is excluded by !Lib/**
- Lib/test/test_types.py is excluded by !Lib/**
- .cspell.dict/python-more.txt (1 hunks)
- crates/vm/src/builtins/asyncgenerator.rs (3 hunks)
- crates/vm/src/builtins/coroutine.rs (3 hunks)
- crates/vm/src/builtins/function.rs (1 hunks)
- crates/vm/src/builtins/generator.rs (2 hunks)
- crates/vm/src/coroutine.rs (3 hunks)
- crates/vm/src/protocol/object.rs (2 hunks)
- crates/vm/src/stdlib/builtins.rs (1 hunks)
- crates/vm/src/types/zoo.rs (2 hunks)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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
Files:
- crates/vm/src/stdlib/builtins.rs
- crates/vm/src/types/zoo.rs
- crates/vm/src/builtins/coroutine.rs
- crates/vm/src/builtins/function.rs
- crates/vm/src/builtins/generator.rs
- crates/vm/src/coroutine.rs
- crates/vm/src/protocol/object.rs
- crates/vm/src/builtins/asyncgenerator.rs
crates/vm/src/class.rs (1)crates/vm/src/builtins/coroutine.rs (2)
- init_builtin_type (39-48)
crates/vm/src/builtins/generator.rs (4)crates/vm/src/builtins/function.rs (2)crates/vm/src/coroutine.rs (5)
- new (35-39)
- __qualname__ (52-54)
- set___qualname__ (57-59)
- close (112-114)
- new (51-60)
- frame (180-182)
- name (184-186)
- qualname (192-194)
- close (150-170)
crates/vm/src/builtins/coroutine.rs (1)crates/vm/src/builtins/generator.rs (3)crates/vm/src/builtins/generator.rs (1)
- new (32-36)
- new (35-39)
crates/vm/src/builtins/asyncgenerator.rs (4)crates/vm/src/protocol/object.rs (1)crates/vm/src/builtins/coroutine.rs (3)
- new (36-41)
- new (455-460)
- __qualname__ (54-56)
- set___qualname__ (59-61)
crates/vm/src/builtins/property.rs (1)
- new (32-36)
- __qualname__ (49-51)
- set___qualname__ (54-56)
- setter (249-255)
crates/vm/src/builtins/asyncgenerator.rs (5)🔇 Additional comments (21)
- __aiter__ (89-91)
- class (25-27)
- class (156-158)
- class (206-208)
- __anext__ (94-96)
.cspell.dict/python-more.txt (1)crates/vm/src/types/zoo.rs (2)7-7: LGTM!
The dictionary entry is correctly alphabetized and corresponds to the new PyAnextAwaitable type introduced in this PR.
crates/vm/src/stdlib/builtins.rs (1)24-24: LGTM!
The anext_awaitable field is properly added to TypeZoo and follows the established pattern for type registration, grouped appropriately with other async generator related types.
143-143: LGTM!
Initialization correctly uses init_builtin_type() consistent with other type registrations in this file.
crates/vm/src/builtins/function.rs (1)543-561: LGTM!
The anext function implementation correctly:
- Validates that the object is an async iterator by checking for __anext__
- Calls __anext__ to get the awaitable
- Wraps the result in PyAnextAwaitable when a default value is provided
- Returns the raw awaitable when no default is given
The error message appropriately mirrors CPython's behavior for non-async-iterator objects.
crates/vm/src/protocol/object.rs (1)428-436: LGTM!
The updated constructor calls correctly pass both __name__() and __qualname__() to PyGenerator, PyCoroutine, and PyAsyncGen, aligning with the PR objective to propagate qualified names throughout the generator/coroutine hierarchy.
crates/vm/src/builtins/coroutine.rs (3)90-120: Enhanced async iterable protocol validation looks good.
The implementation properly:
- Checks for __aiter__ presence before calling
- Detects incorrectly defined async def __aiter__ methods that return coroutines
- Validates the result is an async iterator by checking for __anext__
crates/vm/src/coroutine.rs (3)32-36: LGTM!
The constructor update correctly propagates the new qualname parameter to Coro::new, maintaining consistency with PyGenerator::new and PyAsyncGen::new.
48-56: LGTM!
The __qualname__ getter and setter follow the same pattern as the existing __name__ accessors and correctly delegate to the inner Coro methods.
169-173: LGTM!
The close method correctly delegates to the inner coroutine's close method, completing the wrapper's lifecycle management API alongside the existing send and throw methods.
crates/vm/src/builtins/generator.rs (2)35-35: LGTM!
The new qualname field appropriately uses PyMutex<PyStrRef> for thread-safe access, consistent with the existing name field.
51-58: LGTM!
The constructor properly initializes the new qualname field alongside existing fields.
192-198: LGTM!
The qualname() getter and set_qualname() setter follow the established pattern of the name() and set_name() methods.
crates/vm/src/builtins/asyncgenerator.rs (7)35-39: LGTM!
The constructor update correctly propagates qualname to Coro::new, consistent with the parallel changes in PyCoroutine and PyAsyncGen.
51-59: LGTM!
The __qualname__ getter and setter correctly delegate to the inner Coro methods, following the same pattern as the __name__ accessors.
36-41: LGTM!
The constructor update correctly propagates qualname to Coro::new, consistent with the parallel changes in PyCoroutine and PyGenerator.
53-61: LGTM!
The __qualname__ getter and setter correctly delegate to the inner Coro methods, maintaining consistency with the other generator types.
437-460: LGTM!
The PyAnextAwaitable struct is well-documented and follows the established pattern for implementing Python types in RustPython.
561-569: LGTM!
The handle_result method correctly converts StopAsyncIteration to StopIteration(default_value), which is the core functionality needed for anext() with a default value.
572-577: LGTM!
The SelfIter and IterNext implementations follow the established pattern used by other async generator helper types.
583-583: LGTM!
The new PyAnextAwaitable type is properly registered in the init function.
524-549: get_awaitable_iter() is called on every send()/throw() invocation, but this is correct and necessary.
PyAnextAwaitable wraps arbitrary async iterators (unlike PyAsyncGenASend which handles only async generators directly). For the common case of wrapping async generators, get_awaitable_iter() returns immediately via an early check. For coroutines and other awaitables, the method must call __await__() fresh on each invocation per PEP 492 requirements — caching the result would violate the protocol by reusing an exhausted iterator. The current design correctly handles all awaitable types without unnecessary overhead.
Likely an incorrect or invalid review comment.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)crates/vm/src/coroutine.rs (1)📜 Review details200-207: Consider using qualname (not name) in Coro::repr() for CPython parity.
If RustPython aims to match CPython’s <coroutine object outer.<locals>.inner at ...> style, self.qualname is typically the right display name (and would respect __qualname__ mutation as well).
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 571539d and 455d3f6.
⛔ Files ignored due to path filters (3)- Lib/test/test_asyncgen.py is excluded by !Lib/**
- Lib/test/test_generators.py is excluded by !Lib/**
- Lib/test/test_types.py is excluded by !Lib/**
- .cspell.dict/python-more.txt (1 hunks)
- crates/vm/src/builtins/asyncgenerator.rs (3 hunks)
- crates/vm/src/builtins/coroutine.rs (3 hunks)
- crates/vm/src/builtins/function.rs (1 hunks)
- crates/vm/src/builtins/generator.rs (2 hunks)
- crates/vm/src/coroutine.rs (3 hunks)
- crates/vm/src/protocol/object.rs (2 hunks)
- crates/vm/src/stdlib/builtins.rs (1 hunks)
- crates/vm/src/types/zoo.rs (2 hunks)
- crates/vm/src/types/zoo.rs
- .cspell.dict/python-more.txt
- crates/vm/src/builtins/function.rs
- crates/vm/src/stdlib/builtins.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by 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
Files:
- crates/vm/src/protocol/object.rs
- crates/vm/src/builtins/coroutine.rs
- crates/vm/src/builtins/asyncgenerator.rs
- crates/vm/src/builtins/generator.rs
- crates/vm/src/coroutine.rs
crates/vm/src/builtins/asyncgenerator.rs (5)crates/vm/src/builtins/coroutine.rs (4)crates/vm/src/coroutine.rs (1)
- __aiter__ (89-91)
- class (25-27)
- class (156-158)
- class (206-208)
- __anext__ (94-96)
- name (184-186)
crates/vm/src/builtins/asyncgenerator.rs (7)crates/vm/src/coroutine.rs (4)crates/vm/src/builtins/function.rs (6)
- new (36-41)
- new (455-460)
- __qualname__ (54-56)
- set___qualname__ (59-61)
- close (277-279)
- close (401-403)
- close (553-558)
crates/vm/src/builtins/generator.rs (4)
- new (58-91)
- new (789-791)
- new (911-915)
- __qualname__ (567-569)
- __qualname__ (836-855)
- set___qualname__ (572-585)
crates/vm/src/coroutine.rs (5)
- new (35-39)
- __qualname__ (52-54)
- set___qualname__ (57-59)
- close (112-114)
- new (51-60)
- frame (180-182)
- name (184-186)
- qualname (192-194)
- close (150-170)
crates/vm/src/builtins/asyncgenerator.rs (2)⏰ 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). (9)crates/vm/src/builtins/coroutine.rs (1)
- new (36-41)
- new (455-460)
crates/vm/src/builtins/function.rs (3)
- new (32-36)
crates/vm/src/builtins/generator.rs (1)
- new (58-91)
- new (789-791)
- new (911-915)
- new (35-39)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
crates/vm/src/builtins/coroutine.rs (3)crates/vm/src/coroutine.rs (1)32-56: Qualname plumbing on PyCoroutine looks consistent with Coro and other generator-like types.
169-173: Wrapper close() delegation is a good parity improvement with generator wrappers.
32-36: All call sites have been properly updated to the new constructor signature.
The search for any remaining calls to PyCoroutine::new with the old 2-parameter signature found no matches, confirming that all call sites throughout the codebase (including tests and stdlib) have been updated to use the new 3-parameter signature (frame: FrameRef, name: PyStrRef, qualname: PyStrRef).
crates/vm/src/builtins/generator.rs (2)27-60: Coro qualname storage/accessors are straightforward and match the intended propagation pattern.
Also applies to: 192-198
crates/vm/src/builtins/asyncgenerator.rs (3)35-60: Generator qualname API mirrors coroutine changes cleanly.
35-39: All call sites of PyGenerator::new are properly updated. The constructor at lines 35-39 accepts three parameters (frame, name, qualname), and the single call site in crates/vm/src/builtins/function.rs:429 correctly passes all three arguments.
36-61: Async generator qualname changes align with the generator/coroutine pattern.
579-584: Type registration wiring looks correct (extend_class for anext_awaitable).
1-12: Please ensure cargo fmt + cargo clippy are clean for these protocol/runtime changes.
Also applies to: 579-584
Sorry, something went wrong.
| /// Awaitable wrapper for anext() builtin with default value. | ||
| /// When StopAsyncIteration is raised, it converts it to StopIteration(default). | ||
| #[pyclass(module = false, name = "anext_awaitable")] | ||
| #[derive(Debug)] | ||
| pub struct PyAnextAwaitable { | ||
| wrapped: PyObjectRef, | ||
| default_value: PyObjectRef, | ||
| } | ||
|
|
||
| impl PyPayload for PyAnextAwaitable { | ||
| #[inline] | ||
| fn class(ctx: &Context) -> &'static Py<PyType> { | ||
| ctx.types.anext_awaitable | ||
| } | ||
| } | ||
|
|
||
| #[pyclass(with(IterNext, Iterable))] | ||
| impl PyAnextAwaitable { | ||
| pub fn new(wrapped: PyObjectRef, default_value: PyObjectRef) -> Self { | ||
| Self { | ||
| wrapped, | ||
| default_value, | ||
| } | ||
| } | ||
|
|
||
| #[pymethod(name = "__await__")] | ||
| fn r#await(zelf: PyRef<Self>, _vm: &VirtualMachine) -> PyRef<Self> { | ||
| zelf | ||
| } | ||
|
|
||
| /// Get the awaitable iterator from wrapped object. | ||
| // = anextawaitable_getiter. | ||
| fn get_awaitable_iter(&self, vm: &VirtualMachine) -> PyResult { | ||
| use crate::builtins::PyCoroutine; | ||
| use crate::protocol::PyIter; | ||
|
|
||
| let wrapped = &self.wrapped; | ||
|
|
||
| // If wrapped is already an async_generator_asend, it's an iterator | ||
| if wrapped.class().is(vm.ctx.types.async_generator_asend) | ||
| || wrapped.class().is(vm.ctx.types.async_generator_athrow) | ||
| { | ||
| return Ok(wrapped.clone()); | ||
| } | ||
|
|
||
| // _PyCoro_GetAwaitableIter equivalent | ||
| let awaitable = if wrapped.class().is(vm.ctx.types.coroutine_type) { | ||
| // Coroutine - get __await__ later | ||
| wrapped.clone() | ||
| } else { | ||
| // Try to get __await__ method | ||
| if let Some(await_method) = vm.get_method(wrapped.clone(), identifier!(vm, __await__)) { | ||
| await_method?.call((), vm)? | ||
| } else { | ||
| return Err(vm.new_type_error(format!( | ||
| "object {} can't be used in 'await' expression", | ||
| wrapped.class().name() | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| // If awaitable is a coroutine, get its __await__ | ||
| if awaitable.class().is(vm.ctx.types.coroutine_type) { | ||
| let coro_await = vm.call_method(&awaitable, "__await__", ())?; | ||
| // Check that __await__ returned an iterator | ||
| if !PyIter::check(&coro_await) { | ||
| return Err(vm.new_type_error("__await__ returned a non-iterable")); | ||
| } | ||
| return Ok(coro_await); | ||
| } | ||
|
|
||
| // Check the result is an iterator, not a coroutine | ||
| if awaitable.downcast_ref::<PyCoroutine>().is_some() { | ||
| return Err(vm.new_type_error("__await__() returned a coroutine")); | ||
| } | ||
|
|
||
| // Check that the result is an iterator | ||
| if !PyIter::check(&awaitable) { | ||
| return Err(vm.new_type_error(format!( | ||
| "__await__() returned non-iterator of type '{}'", | ||
| awaitable.class().name() | ||
| ))); | ||
| } | ||
|
|
||
| Ok(awaitable) | ||
| } | ||
|
|
||
| #[pymethod] | ||
| fn send(&self, val: PyObjectRef, vm: &VirtualMachine) -> PyResult { | ||
| let awaitable = self.get_awaitable_iter(vm)?; | ||
| let result = vm.call_method(&awaitable, "send", (val,)); | ||
| self.handle_result(result, vm) | ||
| } | ||
|
|
||
| #[pymethod] | ||
| fn throw( | ||
| &self, | ||
| exc_type: PyObjectRef, | ||
| exc_val: OptionalArg, | ||
| exc_tb: OptionalArg, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let awaitable = self.get_awaitable_iter(vm)?; | ||
| let result = vm.call_method( | ||
| &awaitable, | ||
| "throw", | ||
| ( | ||
| exc_type, | ||
| exc_val.unwrap_or_none(vm), | ||
| exc_tb.unwrap_or_none(vm), | ||
| ), | ||
| ); | ||
| self.handle_result(result, vm) | ||
| } | ||
|
|
||
| #[pymethod] | ||
| fn close(&self, vm: &VirtualMachine) -> PyResult<()> { | ||
| if let Ok(awaitable) = self.get_awaitable_iter(vm) { | ||
| let _ = vm.call_method(&awaitable, "close", ()); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Convert StopAsyncIteration to StopIteration(default_value) | ||
| fn handle_result(&self, result: PyResult, vm: &VirtualMachine) -> PyResult { | ||
| match result { | ||
| Ok(value) => Ok(value), | ||
| Err(exc) if exc.fast_isinstance(vm.ctx.exceptions.stop_async_iteration) => { | ||
| Err(vm.new_stop_iteration(Some(self.default_value.clone()))) | ||
| } | ||
| Err(exc) => Err(exc), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl SelfIter for PyAnextAwaitable {} | ||
| impl IterNext for PyAnextAwaitable { | ||
| fn next(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyIterReturn> { | ||
| PyIterReturn::from_pyresult(zelf.send(vm.ctx.none(), vm), vm) | ||
| } | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical
PyAnextAwaitable is likely functionally wrong: it recreates the underlying __await__ iterator each .send() / .throw() call.
That breaks iterator state (and can repeat side effects) because __await__() is expected to be called once, and the returned iterator is then driven until completion. Cache the iterator inside PyAnextAwaitable (and mark “closed” after completion) like other awaitables in this file.
Minimal direction (illustrative diff sketch; adjust imports/types as needed):
pub struct PyAnextAwaitable {
wrapped: PyObjectRef,
default_value: PyObjectRef,
+ await_iter: crate::common::lock::PyMutex<Option<PyObjectRef>>,
+ state: AtomicCell<AwaitableState>,
}
impl PyAnextAwaitable {
pub fn new(wrapped: PyObjectRef, default_value: PyObjectRef) -> Self {
Self {
wrapped,
default_value,
+ await_iter: crate::common::lock::PyMutex::new(None),
+ state: AtomicCell::new(AwaitableState::Init),
}
}
- fn get_awaitable_iter(&self, vm: &VirtualMachine) -> PyResult {
+ fn get_awaitable_iter(&self, vm: &VirtualMachine) -> PyResult<PyObjectRef> {
+ if let AwaitableState::Closed = self.state.load() {
+ return Err(vm.new_runtime_error("cannot reuse already awaited anext()"));
+ }
+ if let Some(it) = self.await_iter.lock().clone() {
+ return Ok(it);
+ }
...
- Ok(awaitable)
+ *self.await_iter.lock() = Some(awaitable.clone());
+ self.state.store(AwaitableState::Iter);
+ Ok(awaitable)
}🤖 Prompt for AI AgentsCommittable suggestion skipped: line range outside the PR's diff.
crates/vm/src/builtins/asyncgenerator.rs around lines 437 to 577: the current implementation calls __await__ (or constructs the awaitable iterator) on every send/throw which recreates the iterator and breaks iterator state; change PyAnextAwaitable to hold an Option<PyObjectRef> field (e.g., awaitable: Option<PyObjectRef>) to cache the iterator, initialize that field the first time get_awaitable_iter is called, return the cached iterator on subsequent calls, and set it to None/mark closed when the iterator completes (StopAsyncIteration) or when close() is called; update get_awaitable_iter, send, throw and close to use and mutate this cached field appropriately, following the same ownership and VM error handling patterns used by other awaitable types in the file.
Sorry, something went wrong.
| #[pymethod] | ||
| fn send(&self, val: PyObjectRef, vm: &VirtualMachine) -> PyResult { | ||
| let awaitable = self.get_awaitable_iter(vm)?; | ||
| let result = vm.call_method(&awaitable, "send", (val,)); | ||
| self.handle_result(result, vm) | ||
| } | ||
|
|
||
| #[pymethod] | ||
| fn throw( | ||
| &self, | ||
| exc_type: PyObjectRef, | ||
| exc_val: OptionalArg, | ||
| exc_tb: OptionalArg, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult { | ||
| let awaitable = self.get_awaitable_iter(vm)?; | ||
| let result = vm.call_method( | ||
| &awaitable, | ||
| "throw", | ||
| ( | ||
| exc_type, | ||
| exc_val.unwrap_or_none(vm), | ||
| exc_tb.unwrap_or_none(vm), | ||
| ), | ||
| ); | ||
| self.handle_result(result, vm) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
PyAnextAwaitable assumes the await-iterator has send/throw; CPython allows plain iterators from __await__.
If RustPython’s await-driving machinery can handle plain iterators, PyAnextAwaitable.send() should fall back to advancing via __next__ (when val is None) if there’s no send, and throw() should propagate if there’s no throw—otherwise this wrapper will reject valid __await__ implementations.
Sorry, something went wrong.
| pub fn get_aiter(&self, vm: &VirtualMachine) -> PyResult { | ||
| if self.downcastable::<PyAsyncGen>() { | ||
| vm.call_special_method(self, identifier!(vm, __aiter__), ()) | ||
| } else { | ||
| Err(vm.new_type_error("wrong argument type")) | ||
| use crate::builtins::PyCoroutine; | ||
|
|
||
| // Check if object has __aiter__ method | ||
| let aiter_method = self.class().get_attr(identifier!(vm, __aiter__)); | ||
| let Some(_aiter_method) = aiter_method else { | ||
| return Err(vm.new_type_error(format!( | ||
| "'{}' object is not an async iterable", | ||
| self.class().name() | ||
| ))); | ||
| }; | ||
|
|
||
| // Call __aiter__ | ||
| let iterator = vm.call_special_method(self, identifier!(vm, __aiter__), ())?; | ||
|
|
||
| // Check that __aiter__ did not return a coroutine | ||
| if iterator.downcast_ref::<PyCoroutine>().is_some() { | ||
| return Err(vm.new_type_error( | ||
| "'async_iterator' object cannot be interpreted as an async iterable; \ | ||
| perhaps you forgot to call aiter()?", | ||
| )); | ||
| } | ||
|
|
||
| // Check that the result is an async iterator (has __anext__) | ||
| if !iterator.class().has_attr(identifier!(vm, __anext__)) { | ||
| return Err(vm.new_type_error(format!( | ||
| "'{}' object is not an async iterator", | ||
| iterator.class().name() | ||
| ))); | ||
| } | ||
|
|
||
| Ok(iterator) | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Async-iterator validation is too strict: __anext__ check should be on the object, not only on iterator.class().
iterator.class().has_attr(__anext__) can reject valid async iterators that provide __anext__ dynamically (e.g., via instance __getattr__). Prefer checking attribute access on the instance (iterator.as_object().has_attr(..., vm)?) or attempt to resolve/call __anext__ via the VM to match Python’s protocol behavior.
Suggested patch (illustrative):
- if !iterator.class().has_attr(identifier!(vm, __anext__)) {
+ if !iterator.as_object().has_attr(identifier!(vm, __anext__), vm)? {
return Err(vm.new_type_error(format!(
"'{}' object is not an async iterator",
iterator.class().name()
)));
}Committable suggestion skipped: line range outside the PR's diff.
Sorry, something went wrong.
| // Check if object has __aiter__ method | ||
| let aiter_method = self.class().get_attr(identifier!(vm, __aiter__)); | ||
| let Some(_aiter_method) = aiter_method else { | ||
| return Err(vm.new_type_error(format!( | ||
| "'{}' object is not an async iterable", | ||
| self.class().name() | ||
| ))); | ||
| }; | ||
|
|
||
| // Call __aiter__ | ||
| let iterator = vm.call_special_method(self, identifier!(vm, __aiter__), ())?; | ||
|
|
||
| // Check that __aiter__ did not return a coroutine | ||
| if iterator.downcast_ref::<PyCoroutine>().is_some() { | ||
| return Err(vm.new_type_error( | ||
| "'async_iterator' object cannot be interpreted as an async iterable; \ | ||
| perhaps you forgot to call aiter()?", | ||
| )); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
Coroutine-return check likely needs isinstance(coroutine_type) (not payload downcast) for completeness.
downcast_ref::<PyCoroutine>() may miss coroutine subclasses / alternative coroutine-like objects; if the intent is “returned a coroutine object”, prefer a type-based fast_isinstance(vm.ctx.types.coroutine_type) style check (or equivalent) for parity with other protocol checks.
🤖 Prompt for AI AgentsIn crates/vm/src/protocol/object.rs around lines 92 to 110, the code checks whether the result of __aiter__ is a coroutine by using downcast_ref::<PyCoroutine>(), which misses coroutine subclasses or alternative coroutine-like objects; replace that payload downcast with a type-based isinstance check (e.g., use the VM's fast/isinstance helper against vm.ctx.types.coroutine_type or equivalent) so the check catches coroutine subclasses and matches other protocol checks, keeping the same error path/message when the check is true.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.