PyAnextAwaitable #6427
PyAnextAwaitable #6427
Conversation
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)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin asyncgen |
7fe0cdb to
0199879
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/vm/src/protocol/object.rs (1)
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() + ))); }crates/vm/src/builtins/asyncgenerator.rs (2)
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 thecoroutine_typecase and returns early. This check would only catch objects that have a__await__method returning aPyCoroutine, 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 fromget_awaitable_iter()and from the innerclose()call. While ignoringget_awaitable_itererrors may be intentional (the awaitable may already be exhausted), silently ignoringclose()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(()) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/test_asyncgen.pyis excluded by!Lib/**Lib/test/test_generators.pyis excluded by!Lib/**Lib/test/test_types.pyis excluded by!Lib/**
📒 Files selected for processing (9)
.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)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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.rscrates/vm/src/types/zoo.rscrates/vm/src/builtins/coroutine.rscrates/vm/src/builtins/function.rscrates/vm/src/builtins/generator.rscrates/vm/src/coroutine.rscrates/vm/src/protocol/object.rscrates/vm/src/builtins/asyncgenerator.rs
🧬 Code graph analysis (5)
crates/vm/src/types/zoo.rs (1)
crates/vm/src/class.rs (1)
init_builtin_type(39-48)
crates/vm/src/builtins/coroutine.rs (2)
crates/vm/src/builtins/generator.rs (4)
new(35-39)__qualname__(52-54)set___qualname__(57-59)close(112-114)crates/vm/src/coroutine.rs (5)
new(51-60)frame(180-182)name(184-186)qualname(192-194)close(150-170)
crates/vm/src/builtins/function.rs (2)
crates/vm/src/builtins/coroutine.rs (1)
new(32-36)crates/vm/src/builtins/generator.rs (1)
new(35-39)
crates/vm/src/builtins/generator.rs (3)
crates/vm/src/builtins/asyncgenerator.rs (4)
new(36-41)new(455-460)__qualname__(54-56)set___qualname__(59-61)crates/vm/src/builtins/coroutine.rs (3)
new(32-36)__qualname__(49-51)set___qualname__(54-56)crates/vm/src/builtins/property.rs (1)
setter(249-255)
crates/vm/src/protocol/object.rs (1)
crates/vm/src/builtins/asyncgenerator.rs (5)
__aiter__(89-91)class(25-27)class(156-158)class(206-208)__anext__(94-96)
🔇 Additional comments (21)
.cspell.dict/python-more.txt (1)
7-7: LGTM!The dictionary entry is correctly alphabetized and corresponds to the new
PyAnextAwaitabletype introduced in this PR.crates/vm/src/types/zoo.rs (2)
24-24: LGTM!The
anext_awaitablefield is properly added toTypeZooand 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/stdlib/builtins.rs (1)
543-561: LGTM!The
anextfunction implementation correctly:
- Validates that the object is an async iterator by checking for
__anext__- Calls
__anext__to get the awaitable- Wraps the result in
PyAnextAwaitablewhen 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/builtins/function.rs (1)
428-436: LGTM!The updated constructor calls correctly pass both
__name__()and__qualname__()toPyGenerator,PyCoroutine, andPyAsyncGen, aligning with the PR objective to propagate qualified names throughout the generator/coroutine hierarchy.crates/vm/src/protocol/object.rs (1)
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/builtins/coroutine.rs (3)
32-36: LGTM!The constructor update correctly propagates the new
qualnameparameter toCoro::new, maintaining consistency withPyGenerator::newandPyAsyncGen::new.
48-56: LGTM!The
__qualname__getter and setter follow the same pattern as the existing__name__accessors and correctly delegate to the innerCoromethods.
169-173: LGTM!The
closemethod correctly delegates to the inner coroutine'sclosemethod, completing the wrapper's lifecycle management API alongside the existingsendandthrowmethods.crates/vm/src/coroutine.rs (3)
35-35: LGTM!The new
qualnamefield appropriately usesPyMutex<PyStrRef>for thread-safe access, consistent with the existingnamefield.
51-58: LGTM!The constructor properly initializes the new
qualnamefield alongside existing fields.
192-198: LGTM!The
qualname()getter andset_qualname()setter follow the established pattern of thename()andset_name()methods.crates/vm/src/builtins/generator.rs (2)
35-39: LGTM!The constructor update correctly propagates
qualnametoCoro::new, consistent with the parallel changes inPyCoroutineandPyAsyncGen.
51-59: LGTM!The
__qualname__getter and setter correctly delegate to the innerCoromethods, following the same pattern as the__name__accessors.crates/vm/src/builtins/asyncgenerator.rs (7)
36-41: LGTM!The constructor update correctly propagates
qualnametoCoro::new, consistent with the parallel changes inPyCoroutineandPyGenerator.
53-61: LGTM!The
__qualname__getter and setter correctly delegate to the innerCoromethods, maintaining consistency with the other generator types.
437-460: LGTM!The
PyAnextAwaitablestruct is well-documented and follows the established pattern for implementing Python types in RustPython.
561-569: LGTM!The
handle_resultmethod correctly convertsStopAsyncIterationtoStopIteration(default_value), which is the core functionality needed foranext()with a default value.
572-577: LGTM!The
SelfIterandIterNextimplementations follow the established pattern used by other async generator helper types.
583-583: LGTM!The new
PyAnextAwaitabletype is properly registered in theinitfunction.
524-549:get_awaitable_iter()is called on everysend()/throw()invocation, but this is correct and necessary.
PyAnextAwaitablewraps arbitrary async iterators (unlikePyAsyncGenASendwhich 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.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
crates/vm/src/coroutine.rs (1)
200-207: Consider usingqualname(notname) inCoro::repr()for CPython parity.If RustPython aims to match CPython’s
<coroutine object outer.<locals>.inner at ...>style,self.qualnameis typically the right display name (and would respect__qualname__mutation as well).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/test_asyncgen.pyis excluded by!Lib/**Lib/test/test_generators.pyis excluded by!Lib/**Lib/test/test_types.pyis excluded by!Lib/**
📒 Files selected for processing (9)
.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)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/vm/src/types/zoo.rs
- .cspell.dict/python-more.txt
- crates/vm/src/builtins/function.rs
- crates/vm/src/stdlib/builtins.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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.rscrates/vm/src/builtins/coroutine.rscrates/vm/src/builtins/asyncgenerator.rscrates/vm/src/builtins/generator.rscrates/vm/src/coroutine.rs
🧬 Code graph analysis (3)
crates/vm/src/protocol/object.rs (2)
crates/vm/src/builtins/asyncgenerator.rs (5)
__aiter__(89-91)class(25-27)class(156-158)class(206-208)__anext__(94-96)crates/vm/src/coroutine.rs (1)
name(184-186)
crates/vm/src/builtins/coroutine.rs (4)
crates/vm/src/builtins/asyncgenerator.rs (7)
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/function.rs (6)
new(58-91)new(789-791)new(911-915)__qualname__(567-569)__qualname__(836-855)set___qualname__(572-585)crates/vm/src/builtins/generator.rs (4)
new(35-39)__qualname__(52-54)set___qualname__(57-59)close(112-114)crates/vm/src/coroutine.rs (5)
new(51-60)frame(180-182)name(184-186)qualname(192-194)close(150-170)
crates/vm/src/coroutine.rs (4)
crates/vm/src/builtins/asyncgenerator.rs (2)
new(36-41)new(455-460)crates/vm/src/builtins/coroutine.rs (1)
new(32-36)crates/vm/src/builtins/function.rs (3)
new(58-91)new(789-791)new(911-915)crates/vm/src/builtins/generator.rs (1)
new(35-39)
⏰ 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)
- 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
🔇 Additional comments (9)
crates/vm/src/builtins/coroutine.rs (3)
32-56: Qualname plumbing on PyCoroutine looks consistent with Coro and other generator-like types.
169-173: Wrapperclose()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::newwith 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/coroutine.rs (1)
27-60: Coro qualname storage/accessors are straightforward and match the intended propagation pattern.Also applies to: 192-198
crates/vm/src/builtins/generator.rs (2)
35-60: Generator qualname API mirrors coroutine changes cleanly.
35-39: All call sites ofPyGenerator::neware properly updated. The constructor at lines 35-39 accepts three parameters (frame,name,qualname), and the single call site incrates/vm/src/builtins/function.rs:429correctly passes all three arguments.crates/vm/src/builtins/asyncgenerator.rs (3)
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 ensurecargo fmt+cargo clippyare clean for these protocol/runtime changes.Also applies to: 579-584
| /// 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.
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)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
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.
| #[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.
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.
| 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.
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.
| // 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.
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 Agents
In 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.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.