experimental-inspect: Iterator __next__ not optional - #6274
Conversation
| /// The Python type an iterator's `__next__` produces. | ||
| /// | ||
| /// PyO3 lets `__next__` be written as `-> Option<T>` (or `-> PyResult<Option<T>>`), where `None` | ||
| /// means "raise `StopIteration`" rather than "yield `None`". At the Python level that is not a | ||
| /// returned value at all, so the stub has to say `T`: a stub saying `T | None` makes the loop | ||
| /// variable optional and stops the class satisfying `Iterator[T]`. | ||
| /// | ||
| /// The unwrapping is done with an inherent const on the concrete shapes, which takes priority over | ||
| /// the trait const used for everything else. Everything that is not an `Option` — an iterator whose | ||
| /// `__next__` always yields and signals exhaustion by raising — falls through to | ||
| /// [`PyReturnType`] unchanged. | ||
| pub struct IterNextOutput<T>(PhantomData<T>); | ||
|
|
||
| /// The fallback used when the return type is not an `Option`. | ||
| pub trait IterNextOutputFallback: iter_next::Sealed { | ||
| /// The function return type | ||
| const OUTPUT_TYPE: PyStaticExpr; | ||
| } | ||
|
|
||
| mod iter_next { | ||
| pub trait Sealed {} | ||
| impl<T> Sealed for super::IterNextOutput<T> {} | ||
| } |
There was a problem hiding this comment.
It seems unfortunate that this machinery is not built on the same structures handling IterNextOutput in impl_/pymethods.rs
Having it build in the same machinery would avoid any accidental future divergence. We might need to move that machinery to use const-generic specialization, however. Not sure.
Comparing to that machinery immediately shows that __anext__ has the same problem.
|
Hey @davidhewitt, thanks for your review! Unfortunately I won't have much time until beginning of next week, and I also am not enough of an expert yet in this codebase to accurately judge with which alternative implementation to proceed. The best I came up with is the following exploration:
Then, there are 3 different solutions I explored branching of the test harness above:
Solutions A & B clear up some code duplication, while C basically keeps existing things as-is. => If this input is helpful, you could tell me which direction looks promising. Also feel free to commit and/or use this PR as much as you like, if needed (I saw you wanted to have this included in the next release). If you don't find time and above commits understandibly are a bit too much to review, I could have a more thorough and concentrated look next week! |
|
I prefer solution A please |
…anext__` shapes Route A: one wrapper carrying both the conversion and the type hint Review pass on the `IterNextOutput` wrapper pytests: assert the generated `__next__` / `__anext__` hints
3741b0e to
ffad12d
Compare
|
Implemented the test harness + solution A |
|
Hey @davidhewitt, anything I can do here to support? :) I'm also shamelessly pinging for my other PRs (#6242, #6271) that should hopefully suffice to make |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Thanks and sorry for the delay, this generally looks great, a few thoughts and comments.
- Use `StaticIdent` for the hard-coded wrapper / fallback names instead of `&str` + `format_ident!` (and `TokenGenerator` on the slot side). - Drop the `asyncness` guard around the `__next__` / `__anext__` return type: `async fn` does not compile for *any* slot method today (the slot body hands the future straight to `IntoPyCallbackOutput`), so the guard was unreachable. - Note in `impl_/pymethods.rs` that `am_anext` has no null-without-error convention, so `StopAsyncIteration` has to be raised explicitly.
|
@davidhewitt addressed your remarks, CI failures are due to something fixed in #6337 I believe. |
|
seems like the WASI job has broken; I'm looking into it. |
`PyClassOptionAsyncIter.__anext__` signals exhaustion the way the `am_anext` slot has to: it raises `StopAsyncIteration` synchronously, since the slot has no "returned null, no error set" convention. GraalPy < 25.1 lets such a synchronous raise escape `async for` instead of ending the loop. That is not specific to native classes -- a plain Python class with a non-`async def __anext__` reproduces it -- and it is already fixed in GraalPy 25.1, so gate the test on the version rather than on the implementation alone.
|
Two test failures:
|
2 participants
PyO3 lets
__next__be written as-> Option<T>, whereNonemeans "raiseStopIteration". Introspection resolved that through the ordinary return-type path, so the stub said:Which is wrong in a way that costs more than
Incompletewould. mypy takes__next__'s return type as the iteration item type, so everyfor x in itbindsint | None, and the class stops satisfyingIterator[int].