From d75d4bdf8150da4f3d6f4eaee5ee9262034eaa38 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 19 Dec 2023 18:42:41 +0100 Subject: [PATCH 1/5] Fix some holdouts from using argument holders for lifetime extensions. --- pyo3-macros-backend/src/method.rs | 76 ++++++++++++++++++++--------- pyo3-macros-backend/src/params.rs | 9 ++-- pyo3-macros-backend/src/pymethod.rs | 21 +++++--- 3 files changed, 70 insertions(+), 36 deletions(-) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index cb025149..7050be23 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -103,12 +103,18 @@ impl FnType { } } - pub fn self_arg(&self, cls: Option<&syn::Type>, error_mode: ExtractErrorMode) -> TokenStream { + pub fn self_arg( + &self, + cls: Option<&syn::Type>, + error_mode: ExtractErrorMode, + holders: &mut Vec, + ) -> TokenStream { match self { FnType::Getter(st) | FnType::Setter(st) | FnType::Fn(st) => { let mut receiver = st.receiver( cls.expect("no class given for Fn with a \"self\" receiver"), error_mode, + holders, ); syn::Token![,](Span::call_site()).to_tokens(&mut receiver); receiver @@ -161,7 +167,12 @@ impl ExtractErrorMode { } impl SelfType { - pub fn receiver(&self, cls: &syn::Type, error_mode: ExtractErrorMode) -> TokenStream { + pub fn receiver( + &self, + cls: &syn::Type, + error_mode: ExtractErrorMode, + holders: &mut Vec, + ) -> TokenStream { // Due to use of quote_spanned in this function, need to bind these idents to the // main macro callsite. let py = syn::Ident::new("py", Span::call_site()); @@ -173,10 +184,15 @@ impl SelfType { } else { syn::Ident::new("extract_pyclass_ref", *span) }; + let holder = syn::Ident::new(&format!("holder_{}", holders.len()), *span); + holders.push(quote_spanned! { *span => + #[allow(clippy::let_unit_value)] + let mut #holder = _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT; + }); error_mode.handle_error(quote_spanned! { *span => _pyo3::impl_::extract_argument::#method::<#cls>( #py.from_borrowed_ptr::<_pyo3::PyAny>(#slf), - &mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT }, + &mut #holder, ) }) } @@ -457,9 +473,6 @@ impl<'a> FnSpec<'a> { ident: &proc_macro2::Ident, cls: Option<&syn::Type>, ) -> Result { - let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise); - let func_name = &self.name; - let mut cancel_handle_iter = self .signature .arguments @@ -473,7 +486,9 @@ impl<'a> FnSpec<'a> { } } - let rust_call = |args: Vec| { + let rust_call = |args: Vec, holders: &mut Vec| { + let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise, holders); + let call = if self.asyncness.is_some() { let throw_callback = if cancel_handle.is_some() { quote! { Some(__throw_callback) } @@ -486,14 +501,22 @@ impl<'a> FnSpec<'a> { None => quote!(None), }; let future = match self.tp { - FnType::Fn(SelfType::Receiver { mutable: false, .. }) => quote! {{ - let __guard = _pyo3::impl_::coroutine::RefGuard::<#cls>::new(py.from_borrowed_ptr::<_pyo3::types::PyAny>(_slf))?; - async move { function(&__guard, #(#args),*).await } - }}, - FnType::Fn(SelfType::Receiver { mutable: true, .. }) => quote! {{ - let mut __guard = _pyo3::impl_::coroutine::RefMutGuard::<#cls>::new(py.from_borrowed_ptr::<_pyo3::types::PyAny>(_slf))?; - async move { function(&mut __guard, #(#args),*).await } - }}, + FnType::Fn(SelfType::Receiver { mutable: false, .. }) => { + holders.pop().unwrap(); // does not actually use holder created by `self_arg` + + quote! {{ + let __guard = _pyo3::impl_::coroutine::RefGuard::<#cls>::new(py.from_borrowed_ptr::<_pyo3::types::PyAny>(_slf))?; + async move { function(&__guard, #(#args),*).await } + }} + } + FnType::Fn(SelfType::Receiver { mutable: true, .. }) => { + holders.pop().unwrap(); // does not actually use holder created by `self_arg` + + quote! {{ + let mut __guard = _pyo3::impl_::coroutine::RefMutGuard::<#cls>::new(py.from_borrowed_ptr::<_pyo3::types::PyAny>(_slf))?; + async move { function(&mut __guard, #(#args),*).await } + }} + } _ => quote! { function(#self_arg #(#args),*) }, }; let mut call = quote! {{ @@ -519,6 +542,7 @@ impl<'a> FnSpec<'a> { quotes::map_result_into_ptr(quotes::ok_wrap(call)) }; + let func_name = &self.name; let rust_name = if let Some(cls) = cls { quote!(#cls::#func_name) } else { @@ -527,6 +551,7 @@ impl<'a> FnSpec<'a> { Ok(match self.convention { CallingConvention::Noargs => { + let mut holders = Vec::new(); let args = self .signature .arguments @@ -541,7 +566,7 @@ impl<'a> FnSpec<'a> { } }) .collect(); - let call = rust_call(args); + let call = rust_call(args, &mut holders); quote! { unsafe fn #ident<'py>( @@ -549,13 +574,15 @@ impl<'a> FnSpec<'a> { _slf: *mut _pyo3::ffi::PyObject, ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { let function = #rust_name; // Shadow the function name to avoid #3017 + #( #holders )* #call } } } CallingConvention::Fastcall => { - let (arg_convert, args) = impl_arg_params(self, cls, true)?; - let call = rust_call(args); + let mut holders = Vec::new(); + let (arg_convert, args) = impl_arg_params(self, cls, true, &mut holders)?; + let call = rust_call(args, &mut holders); quote! { unsafe fn #ident<'py>( py: _pyo3::Python<'py>, @@ -566,13 +593,15 @@ impl<'a> FnSpec<'a> { ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert + #( #holders )* #call } } } CallingConvention::Varargs => { - let (arg_convert, args) = impl_arg_params(self, cls, false)?; - let call = rust_call(args); + let mut holders = Vec::new(); + let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders)?; + let call = rust_call(args, &mut holders); quote! { unsafe fn #ident<'py>( py: _pyo3::Python<'py>, @@ -582,13 +611,15 @@ impl<'a> FnSpec<'a> { ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert + #( #holders )* #call } } } CallingConvention::TpNew => { - let (arg_convert, args) = impl_arg_params(self, cls, false)?; - let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise); + let mut holders = Vec::new(); + let (arg_convert, args) = impl_arg_params(self, cls, false, &mut holders)?; + let self_arg = self.tp.self_arg(cls, ExtractErrorMode::Raise, &mut holders); let call = quote! { #rust_name(#self_arg #(#args),*) }; quote! { unsafe fn #ident( @@ -600,6 +631,7 @@ impl<'a> FnSpec<'a> { use _pyo3::callback::IntoPyCallbackOutput; let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert + #( #holders )* let result = #call; let initializer: _pyo3::PyClassInitializer::<#cls> = result.convert(py)?; let cell = initializer.create_cell_from_subtype(py, _slf)?; diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index d8a0dc74..1ef31867 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -29,24 +29,23 @@ pub fn impl_arg_params( spec: &FnSpec<'_>, self_: Option<&syn::Type>, fastcall: bool, + holders: &mut Vec, ) -> Result<(TokenStream, Vec)> { let args_array = syn::Ident::new("output", Span::call_site()); if !fastcall && is_forwarded_args(&spec.signature) { // In the varargs convention, we can just pass though if the signature // is (*args, **kwds). - let mut holders = Vec::new(); let arg_convert = spec .signature .arguments .iter() - .map(|arg| impl_arg_param(arg, &mut 0, &args_array, &mut holders)) + .map(|arg| impl_arg_param(arg, &mut 0, &args_array, holders)) .collect::>()?; return Ok(( quote! { let _args = py.from_borrowed_ptr::<_pyo3::types::PyTuple>(_args); let _kwargs: ::std::option::Option<&_pyo3::types::PyDict> = py.from_borrowed_ptr_or_opt(_kwargs); - #( #holders )* }, arg_convert, )); @@ -75,12 +74,11 @@ pub fn impl_arg_params( let num_params = positional_parameter_names.len() + keyword_only_parameters.len(); let mut option_pos = 0; - let mut holders = Vec::new(); let param_conversion = spec .signature .arguments .iter() - .map(|arg| impl_arg_param(arg, &mut option_pos, &args_array, &mut holders)) + .map(|arg| impl_arg_param(arg, &mut option_pos, &args_array, holders)) .collect::>()?; let args_handler = if spec.signature.python_signature.varargs.is_some() { @@ -134,7 +132,6 @@ pub fn impl_arg_params( keyword_only_parameters: &[#(#keyword_only_parameters),*], }; let mut #args_array = [::std::option::Option::None; #num_params]; - #( #holders )* let (_args, _kwargs) = #extract_expression; }, param_conversion, diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index b87f4644..239bd96a 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -481,9 +481,10 @@ fn impl_call_setter( cls: &syn::Type, spec: &FnSpec<'_>, self_type: &SelfType, + holders: &mut Vec, ) -> syn::Result { let (py_arg, args) = split_off_python_arg(&spec.signature.arguments); - let slf = self_type.receiver(cls, ExtractErrorMode::Raise); + let slf = self_type.receiver(cls, ExtractErrorMode::Raise, holders); if args.is_empty() { bail_spanned!(spec.name.span() => "setter function expected to have one argument"); @@ -511,6 +512,7 @@ pub fn impl_py_setter_def( ) -> Result { let python_name = property_type.null_terminated_python_name()?; let doc = property_type.doc(); + let mut holders = Vec::new(); let setter_impl = match property_type { PropertyType::Descriptor { field_index, field, .. @@ -519,7 +521,7 @@ pub fn impl_py_setter_def( mutable: true, span: Span::call_site(), } - .receiver(cls, ExtractErrorMode::Raise); + .receiver(cls, ExtractErrorMode::Raise, &mut holders); if let Some(ident) = &field.ident { // named struct field quote!({ #slf.#ident = _val; }) @@ -531,7 +533,7 @@ pub fn impl_py_setter_def( } PropertyType::Function { spec, self_type, .. - } => impl_call_setter(cls, spec, self_type)?, + } => impl_call_setter(cls, spec, self_type, &mut holders)?, }; let wrapper_ident = match property_type { @@ -575,7 +577,7 @@ pub fn impl_py_setter_def( _pyo3::exceptions::PyAttributeError::new_err("can't delete attribute") })?; let _val = _pyo3::FromPyObject::extract(_value)?; - + #( #holders )* _pyo3::callback::convert(py, #setter_impl) } }; @@ -601,9 +603,10 @@ fn impl_call_getter( cls: &syn::Type, spec: &FnSpec<'_>, self_type: &SelfType, + holders: &mut Vec, ) -> syn::Result { let (py_arg, args) = split_off_python_arg(&spec.signature.arguments); - let slf = self_type.receiver(cls, ExtractErrorMode::Raise); + let slf = self_type.receiver(cls, ExtractErrorMode::Raise, holders); ensure_spanned!( args.is_empty(), args[0].ty.span() => "getter function can only have one argument (of type pyo3::Python)" @@ -627,6 +630,7 @@ pub fn impl_py_getter_def( let python_name = property_type.null_terminated_python_name()?; let doc = property_type.doc(); + let mut holders = Vec::new(); let body = match property_type { PropertyType::Descriptor { field_index, field, .. @@ -635,7 +639,7 @@ pub fn impl_py_getter_def( mutable: false, span: Span::call_site(), } - .receiver(cls, ExtractErrorMode::Raise); + .receiver(cls, ExtractErrorMode::Raise, &mut holders); let field_token = if let Some(ident) = &field.ident { // named struct field ident.to_token_stream() @@ -651,7 +655,7 @@ pub fn impl_py_getter_def( PropertyType::Function { spec, self_type, .. } => { - let call = impl_call_getter(cls, spec, self_type)?; + let call = impl_call_getter(cls, spec, self_type, &mut holders)?; quote! { _pyo3::callback::convert(py, #call) } @@ -692,6 +696,7 @@ pub fn impl_py_getter_def( py: _pyo3::Python<'_>, _slf: *mut _pyo3::ffi::PyObject ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> { + #( #holders )* #body } }; @@ -1154,7 +1159,7 @@ fn generate_method_body( holders: &mut Vec, return_mode: Option<&ReturnMode>, ) -> Result { - let self_arg = spec.tp.self_arg(Some(cls), extract_error_mode); + let self_arg = spec.tp.self_arg(Some(cls), extract_error_mode, holders); let rust_name = spec.name; let args = extract_proto_arguments(spec, arguments, extract_error_mode, holders)?; let call = quote! { _pyo3::callback::convert(py, #cls::#rust_name(#self_arg #(#args),*)) }; From ca7d90dcf3b764c133023bc8558338bd14b16835 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Mon, 18 Dec 2023 16:26:33 +0100 Subject: [PATCH 2/5] Replace IterNextOutput by autoref-based specialization to allow returning arbitrary values --- pyo3-macros-backend/src/pymethod.rs | 31 ++++++++--- pytests/src/awaitable.rs | 17 +++--- pytests/src/pyclasses.rs | 9 ++-- src/coroutine.rs | 26 +++------ src/impl_/pymethods.rs | 82 +++++++++++++++++++++++++++++ src/lib.rs | 1 + src/pyclass.rs | 36 ++++--------- tests/test_proto_methods.rs | 8 +-- 8 files changed, 141 insertions(+), 69 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 239bd96a..2a730fd2 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -792,9 +792,11 @@ pub const __RICHCMP__: SlotDef = SlotDef::new("Py_tp_richcompare", "richcmpfunc" const __GET__: SlotDef = SlotDef::new("Py_tp_descr_get", "descrgetfunc") .arguments(&[Ty::MaybeNullObject, Ty::MaybeNullObject]); const __ITER__: SlotDef = SlotDef::new("Py_tp_iter", "getiterfunc"); -const __NEXT__: SlotDef = SlotDef::new("Py_tp_iternext", "iternextfunc").return_conversion( - TokenGenerator(|| quote! { _pyo3::class::iter::IterNextOutput::<_, _> }), -); +const __NEXT__: SlotDef = SlotDef::new("Py_tp_iternext", "iternextfunc") + .return_specialized_conversion( + TokenGenerator(|| quote! { IterBaseKind, IterOptionKind, IterResultOptionKind }), + TokenGenerator(|| quote! { iter_tag }), + ); const __AWAIT__: SlotDef = SlotDef::new("Py_am_await", "unaryfunc"); const __AITER__: SlotDef = SlotDef::new("Py_am_aiter", "unaryfunc"); const __ANEXT__: SlotDef = SlotDef::new("Py_am_anext", "unaryfunc").return_conversion( @@ -1003,17 +1005,23 @@ fn extract_object( enum ReturnMode { ReturnSelf, Conversion(TokenGenerator), + SpecializedConversion(TokenGenerator, TokenGenerator), } impl ReturnMode { fn return_call_output(&self, call: TokenStream) -> TokenStream { match self { ReturnMode::Conversion(conversion) => quote! { - let _result: _pyo3::PyResult<#conversion> = #call; + let _result: _pyo3::PyResult<#conversion> = _pyo3::callback::convert(py, #call); _pyo3::callback::convert(py, _result) }, + ReturnMode::SpecializedConversion(traits, tag) => quote! { + let _result = #call; + use _pyo3::impl_::pymethods::{#traits}; + (&_result).#tag().convert(py, _result) + }, ReturnMode::ReturnSelf => quote! { - let _result: _pyo3::PyResult<()> = #call; + let _result: _pyo3::PyResult<()> = _pyo3::callback::convert(py, #call); _result?; _pyo3::ffi::Py_XINCREF(_raw_slf); ::std::result::Result::Ok(_raw_slf) @@ -1062,6 +1070,15 @@ impl SlotDef { self } + const fn return_specialized_conversion( + mut self, + traits: TokenGenerator, + tag: TokenGenerator, + ) -> Self { + self.return_mode = Some(ReturnMode::SpecializedConversion(traits, tag)); + self + } + const fn extract_error_mode(mut self, extract_error_mode: ExtractErrorMode) -> Self { self.extract_error_mode = extract_error_mode; self @@ -1162,11 +1179,11 @@ fn generate_method_body( let self_arg = spec.tp.self_arg(Some(cls), extract_error_mode, holders); let rust_name = spec.name; let args = extract_proto_arguments(spec, arguments, extract_error_mode, holders)?; - let call = quote! { _pyo3::callback::convert(py, #cls::#rust_name(#self_arg #(#args),*)) }; + let call = quote! { #cls::#rust_name(#self_arg #(#args),*) }; Ok(if let Some(return_mode) = return_mode { return_mode.return_call_output(call) } else { - call + quote! { _pyo3::callback::convert(py, #call) } }) } diff --git a/pytests/src/awaitable.rs b/pytests/src/awaitable.rs index 1f798aa4..0cc17333 100644 --- a/pytests/src/awaitable.rs +++ b/pytests/src/awaitable.rs @@ -5,7 +5,8 @@ //! when awaited, see guide examples related to pyo3-asyncio for ways //! to suspend tasks and await results. -use pyo3::{prelude::*, pyclass::IterNextOutput}; +use pyo3::exceptions::PyStopIteration; +use pyo3::prelude::*; #[pyclass] #[derive(Debug)] @@ -30,13 +31,13 @@ impl IterAwaitable { pyself } - fn __next__(&mut self, py: Python<'_>) -> PyResult> { + fn __next__(&mut self, py: Python<'_>) -> PyResult { match self.result.take() { Some(res) => match res { - Ok(v) => Ok(IterNextOutput::Return(v)), + Ok(v) => Err(PyStopIteration::new_err(v)), Err(err) => Err(err), }, - _ => Ok(IterNextOutput::Yield(py.None().into())), + _ => Ok(py.None().into()), } } } @@ -66,15 +67,13 @@ impl FutureAwaitable { pyself } - fn __next__( - mut pyself: PyRefMut<'_, Self>, - ) -> PyResult, PyObject>> { + fn __next__(mut pyself: PyRefMut<'_, Self>) -> PyResult> { match pyself.result { Some(_) => match pyself.result.take().unwrap() { - Ok(v) => Ok(IterNextOutput::Return(v)), + Ok(v) => Err(PyStopIteration::new_err(v)), Err(err) => Err(err), }, - _ => Ok(IterNextOutput::Yield(pyself)), + _ => Ok(pyself), } } } diff --git a/pytests/src/pyclasses.rs b/pytests/src/pyclasses.rs index 46c8523c..326893d1 100644 --- a/pytests/src/pyclasses.rs +++ b/pytests/src/pyclasses.rs @@ -1,5 +1,4 @@ -use pyo3::exceptions::PyValueError; -use pyo3::iter::IterNextOutput; +use pyo3::exceptions::{PyStopIteration, PyValueError}; use pyo3::prelude::*; use pyo3::types::PyType; @@ -28,12 +27,12 @@ impl PyClassIter { Default::default() } - fn __next__(&mut self) -> IterNextOutput { + fn __next__(&mut self) -> PyResult { if self.count < 5 { self.count += 1; - IterNextOutput::Yield(self.count) + Ok(self.count) } else { - IterNextOutput::Return("Ended") + Err(PyStopIteration::new_err("Ended")) } } } diff --git a/src/coroutine.rs b/src/coroutine.rs index 6380b4e0..7dd73cbb 100644 --- a/src/coroutine.rs +++ b/src/coroutine.rs @@ -14,7 +14,6 @@ use crate::{ coroutine::{cancel::ThrowCallback, waker::AsyncioWaker}, exceptions::{PyAttributeError, PyRuntimeError, PyStopIteration}, panic::PanicException, - pyclass::IterNextOutput, types::{PyIterator, PyString}, IntoPy, Py, PyAny, PyErr, PyObject, PyResult, Python, }; @@ -68,11 +67,7 @@ impl Coroutine { } } - fn poll( - &mut self, - py: Python<'_>, - throw: Option, - ) -> PyResult> { + fn poll(&mut self, py: Python<'_>, throw: Option) -> PyResult { // raise if the coroutine has already been run to completion let future_rs = match self.future { Some(ref mut fut) => fut, @@ -100,7 +95,7 @@ impl Coroutine { match panic::catch_unwind(panic::AssertUnwindSafe(poll)) { Ok(Poll::Ready(res)) => { self.close(); - return Ok(IterNextOutput::Return(res?)); + return Err(PyStopIteration::new_err(res?)); } Err(err) => { self.close(); @@ -115,19 +110,12 @@ impl Coroutine { if let Some(future) = PyIterator::from_object(future).unwrap().next() { // future has not been leaked into Python for now, and Rust code can only call // `set_result(None)` in `Wake` implementation, so it's safe to unwrap - return Ok(IterNextOutput::Yield(future.unwrap().into())); + return Ok(future.unwrap().into()); } } // if waker has been waken during future polling, this is roughly equivalent to // `await asyncio.sleep(0)`, so just yield `None`. - Ok(IterNextOutput::Yield(py.None().into())) - } -} - -pub(crate) fn iter_result(result: IterNextOutput) -> PyResult { - match result { - IterNextOutput::Yield(ob) => Ok(ob), - IterNextOutput::Return(ob) => Err(PyStopIteration::new_err(ob)), + Ok(py.None().into()) } } @@ -153,11 +141,11 @@ impl Coroutine { } fn send(&mut self, py: Python<'_>, _value: &PyAny) -> PyResult { - iter_result(self.poll(py, None)?) + self.poll(py, None) } fn throw(&mut self, py: Python<'_>, exc: PyObject) -> PyResult { - iter_result(self.poll(py, Some(exc))?) + self.poll(py, Some(exc)) } fn close(&mut self) { @@ -170,7 +158,7 @@ impl Coroutine { self_ } - fn __next__(&mut self, py: Python<'_>) -> PyResult> { + fn __next__(&mut self, py: Python<'_>) -> PyResult { self.poll(py, None) } } diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index ff2857ec..3cc77341 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -1,3 +1,4 @@ +use crate::callback::IntoPyCallbackOutput; use crate::gil::LockGIL; use crate::impl_::panic::PanicTrap; use crate::internal_tricks::extract_c_string; @@ -7,6 +8,7 @@ use std::ffi::CStr; use std::fmt; use std::os::raw::{c_int, c_void}; use std::panic::{catch_unwind, AssertUnwindSafe}; +use std::ptr::null_mut; /// Python 3.8 and up - __ipow__ has modulo argument correctly populated. #[cfg(Py_3_8)] @@ -299,3 +301,83 @@ pub(crate) fn get_name(name: &'static str) -> PyResult> { pub(crate) fn get_doc(doc: &'static str) -> PyResult> { extract_c_string(doc, "function doc cannot contain NUL byte.") } + +// Autoref-based specialization for handling `__next__` returning `Option` + +pub struct IterBaseTag; + +impl IterBaseTag { + #[inline] + pub fn convert(self, py: Python<'_>, value: Value) -> PyResult + where + Value: IntoPyCallbackOutput, + { + value.convert(py) + } +} + +pub trait IterBaseKind { + #[inline] + fn iter_tag(&self) -> IterBaseTag { + IterBaseTag + } +} + +impl IterBaseKind for &Value {} + +pub struct IterOptionTag; + +impl IterOptionTag { + #[inline] + pub fn convert( + self, + py: Python<'_>, + value: Option, + ) -> PyResult<*mut ffi::PyObject> + where + Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + { + match value { + Some(value) => value.convert(py), + None => Ok(null_mut()), + } + } +} + +pub trait IterOptionKind { + #[inline] + fn iter_tag(&self) -> IterOptionTag { + IterOptionTag + } +} + +impl IterOptionKind for Option {} + +pub struct IterResultOptionTag; + +impl IterResultOptionTag { + #[inline] + pub fn convert( + self, + py: Python<'_>, + value: PyResult>, + ) -> PyResult<*mut ffi::PyObject> + where + Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + { + match value { + Ok(Some(value)) => value.convert(py), + Ok(None) => Ok(null_mut()), + Err(err) => Err(err), + } + } +} + +pub trait IterResultOptionKind { + #[inline] + fn iter_tag(&self) -> IterResultOptionTag { + IterResultOptionTag + } +} + +impl IterResultOptionKind for PyResult> {} diff --git a/src/lib.rs b/src/lib.rs index 985ec0aa..f787b23a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -362,6 +362,7 @@ pub mod class { /// For compatibility reasons this has not yet been removed, however will be done so /// once is resolved. pub mod iter { + #[allow(deprecated)] pub use crate::pyclass::{IterNextOutput, PyIterNextOutput}; } diff --git a/src/pyclass.rs b/src/pyclass.rs index 23affb4b..b1c49be2 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -91,6 +91,7 @@ impl CompareOp { /// Usage example: /// /// ```rust +/// # #![allow(deprecated)] /// use pyo3::prelude::*; /// use pyo3::iter::IterNextOutput; /// @@ -122,6 +123,7 @@ impl CompareOp { /// } /// } /// ``` +#[deprecated(since = "0.21.0", note = "Use `Option` or `PyStopIteration` instead.")] pub enum IterNextOutput { /// The value yielded by the iterator. Yield(T), @@ -130,38 +132,22 @@ pub enum IterNextOutput { } /// Alias of `IterNextOutput` with `PyObject` yield & return values. +#[deprecated(since = "0.21.0", note = "Use `Option` or `PyStopIteration` instead.")] +#[allow(deprecated)] pub type PyIterNextOutput = IterNextOutput; -impl IntoPyCallbackOutput<*mut ffi::PyObject> for PyIterNextOutput { - fn convert(self, _py: Python<'_>) -> PyResult<*mut ffi::PyObject> { - match self { - IterNextOutput::Yield(o) => Ok(o.into_ptr()), - IterNextOutput::Return(opt) => Err(crate::exceptions::PyStopIteration::new_err((opt,))), - } - } -} - -impl IntoPyCallbackOutput for IterNextOutput +#[allow(deprecated)] +impl IntoPyCallbackOutput<*mut ffi::PyObject> for IterNextOutput where T: IntoPy, U: IntoPy, { - fn convert(self, py: Python<'_>) -> PyResult { + fn convert(self, py: Python<'_>) -> PyResult<*mut ffi::PyObject> { match self { - IterNextOutput::Yield(o) => Ok(IterNextOutput::Yield(o.into_py(py))), - IterNextOutput::Return(o) => Ok(IterNextOutput::Return(o.into_py(py))), - } - } -} - -impl IntoPyCallbackOutput for Option -where - T: IntoPy, -{ - fn convert(self, py: Python<'_>) -> PyResult { - match self { - Some(o) => Ok(PyIterNextOutput::Yield(o.into_py(py))), - None => Ok(PyIterNextOutput::Return(py.None().into())), + IterNextOutput::Yield(o) => Ok(o.into_py(py).into_ptr()), + IterNextOutput::Return(o) => { + Err(crate::exceptions::PyStopIteration::new_err(o.into_py(py))) + } } } } diff --git a/tests/test_proto_methods.rs b/tests/test_proto_methods.rs index b3503451..caaae751 100644 --- a/tests/test_proto_methods.rs +++ b/tests/test_proto_methods.rs @@ -658,10 +658,10 @@ impl OnceFuture { fn __iter__(slf: PyRef<'_, Self>) -> PyRef<'_, Self> { slf } - fn __next__(mut slf: PyRefMut<'_, Self>) -> Option { - if !slf.polled { - slf.polled = true; - Some(slf.future.clone()) + fn __next__<'py>(&'py mut self, py: Python<'py>) -> Option<&'py PyAny> { + if !self.polled { + self.polled = true; + Some(self.future.as_ref(py)) } else { None } From 83697f0c6292af3f07c6f783b29160f6a096fd41 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 19 Dec 2023 10:27:53 +0100 Subject: [PATCH 3/5] Also replace IterANextOutput by autoref-based specialization to allow returning arbitrary values --- pyo3-macros-backend/src/pymethod.rs | 5 +- src/impl_/pymethods.rs | 81 +++++++++++++++++++++++++++++ src/lib.rs | 1 + src/pyclass.rs | 43 ++++++--------- 4 files changed, 101 insertions(+), 29 deletions(-) diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 2a730fd2..8a97c712 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -799,8 +799,9 @@ const __NEXT__: SlotDef = SlotDef::new("Py_tp_iternext", "iternextfunc") ); const __AWAIT__: SlotDef = SlotDef::new("Py_am_await", "unaryfunc"); const __AITER__: SlotDef = SlotDef::new("Py_am_aiter", "unaryfunc"); -const __ANEXT__: SlotDef = SlotDef::new("Py_am_anext", "unaryfunc").return_conversion( - TokenGenerator(|| quote! { _pyo3::class::pyasync::IterANextOutput::<_, _> }), +const __ANEXT__: SlotDef = SlotDef::new("Py_am_anext", "unaryfunc").return_specialized_conversion( + TokenGenerator(|| quote! { AsyncIterBaseKind, AsyncIterOptionKind, AsyncIterResultOptionKind }), + TokenGenerator(|| quote! { async_iter_tag }), ); const __LEN__: SlotDef = SlotDef::new("Py_mp_length", "lenfunc").ret_ty(Ty::PySsizeT); const __CONTAINS__: SlotDef = SlotDef::new("Py_sq_contains", "objobjproc") diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index 3cc77341..aeac6518 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -1,4 +1,5 @@ use crate::callback::IntoPyCallbackOutput; +use crate::exceptions::PyStopAsyncIteration; use crate::gil::LockGIL; use crate::impl_::panic::PanicTrap; use crate::internal_tricks::extract_c_string; @@ -381,3 +382,83 @@ pub trait IterResultOptionKind { } impl IterResultOptionKind for PyResult> {} + +// Autoref-based specialization for handling `__anext__` returning `Option` + +pub struct AsyncIterBaseTag; + +impl AsyncIterBaseTag { + #[inline] + pub fn convert(self, py: Python<'_>, value: Value) -> PyResult + where + Value: IntoPyCallbackOutput, + { + value.convert(py) + } +} + +pub trait AsyncIterBaseKind { + #[inline] + fn async_iter_tag(&self) -> AsyncIterBaseTag { + AsyncIterBaseTag + } +} + +impl AsyncIterBaseKind for &Value {} + +pub struct AsyncIterOptionTag; + +impl AsyncIterOptionTag { + #[inline] + pub fn convert( + self, + py: Python<'_>, + value: Option, + ) -> PyResult<*mut ffi::PyObject> + where + Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + { + match value { + Some(value) => value.convert(py), + None => Err(PyStopAsyncIteration::new_err(())), + } + } +} + +pub trait AsyncIterOptionKind { + #[inline] + fn async_iter_tag(&self) -> AsyncIterOptionTag { + AsyncIterOptionTag + } +} + +impl AsyncIterOptionKind for Option {} + +pub struct AsyncIterResultOptionTag; + +impl AsyncIterResultOptionTag { + #[inline] + pub fn convert( + self, + py: Python<'_>, + value: PyResult>, + ) -> PyResult<*mut ffi::PyObject> + where + Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + { + match value { + Ok(Some(value)) => value.convert(py), + Ok(None) => Err(PyStopAsyncIteration::new_err(())), + Err(err) => Err(err), + } + } +} + +pub trait AsyncIterResultOptionKind { + #[inline] + fn async_iter_tag(&self) -> AsyncIterResultOptionTag { + AsyncIterResultOptionTag + } +} + +impl AsyncIterResultOptionKind for PyResult> {} diff --git a/src/lib.rs b/src/lib.rs index f787b23a..091be583 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -351,6 +351,7 @@ pub mod class { /// For compatibility reasons this has not yet been removed, however will be done so /// once is resolved. pub mod pyasync { + #[allow(deprecated)] pub use crate::pyclass::{IterANextOutput, PyIterANextOutput}; } diff --git a/src/pyclass.rs b/src/pyclass.rs index b1c49be2..eb4a5595 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -155,6 +155,10 @@ where /// Output of `__anext__`. /// /// +#[deprecated( + since = "0.21.0", + note = "Use `Option` or `PyStopAsyncIteration` instead." +)] pub enum IterANextOutput { /// An expression which the generator yielded. Yield(T), @@ -163,40 +167,25 @@ pub enum IterANextOutput { } /// An [IterANextOutput] of Python objects. +#[deprecated( + since = "0.21.0", + note = "Use `Option` or `PyStopAsyncIteration` instead." +)] +#[allow(deprecated)] pub type PyIterANextOutput = IterANextOutput; -impl IntoPyCallbackOutput<*mut ffi::PyObject> for PyIterANextOutput { - fn convert(self, _py: Python<'_>) -> PyResult<*mut ffi::PyObject> { - match self { - IterANextOutput::Yield(o) => Ok(o.into_ptr()), - IterANextOutput::Return(opt) => { - Err(crate::exceptions::PyStopAsyncIteration::new_err((opt,))) - } - } - } -} - -impl IntoPyCallbackOutput for IterANextOutput +#[allow(deprecated)] +impl IntoPyCallbackOutput<*mut ffi::PyObject> for IterANextOutput where T: IntoPy, U: IntoPy, { - fn convert(self, py: Python<'_>) -> PyResult { + fn convert(self, py: Python<'_>) -> PyResult<*mut ffi::PyObject> { match self { - IterANextOutput::Yield(o) => Ok(IterANextOutput::Yield(o.into_py(py))), - IterANextOutput::Return(o) => Ok(IterANextOutput::Return(o.into_py(py))), - } - } -} - -impl IntoPyCallbackOutput for Option -where - T: IntoPy, -{ - fn convert(self, py: Python<'_>) -> PyResult { - match self { - Some(o) => Ok(PyIterANextOutput::Yield(o.into_py(py))), - None => Ok(PyIterANextOutput::Return(py.None().into())), + IterANextOutput::Yield(o) => Ok(o.into_py(py).into_ptr()), + IterANextOutput::Return(o) => Err(crate::exceptions::PyStopAsyncIteration::new_err( + o.into_py(py), + )), } } } From a605308cee1615b9ba6c1f807b2699a826c6431e Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 19 Dec 2023 18:54:56 +0100 Subject: [PATCH 4/5] Add change log and migration guide entries. --- guide/src/migration.md | 119 +++++++++++++++++++++++++++++++++- newsfragments/3661.changed.md | 1 + 2 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 newsfragments/3661.changed.md diff --git a/guide/src/migration.md b/guide/src/migration.md index 0c55c15c..b002daf9 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -74,7 +74,124 @@ Python::with_gil(|py| { }); ``` -### `PyType::name` is now `PyType::qualname` +### `Iter(A)NextOutput` are deprecated + +The `__next__` and `__anext__` magic methods can now return any type convertible into Python objects directly just like all other `#[pymethods]`. The `IterNextOutput` used by `__next__` and `IterANextOutput` used by `__anext__` are subsequently deprecated. Most importantly, this change allows returning an awaitable from `__anext__` without non-sensically wrapping it into `Yield` or `Some`. Only the return types `Option` and `PyResult>` are still handled in a special manner where `Some(val)` yields `val` and `None` stops iteration. + +Starting with an implementation of a Python iterator using `IterNextOutput`, e.g. + +```rust +#![allow(deprecated)] +use pyo3::prelude::*; +use pyo3::iter::IterNextOutput; + +#[pyclass] +struct PyClassIter { + count: usize, +} + +#[pymethods] +impl PyClassIter { + fn __next__(&mut self) -> IterNextOutput { + if self.count < 5 { + self.count += 1; + IterNextOutput::Yield(self.count) + } else { + IterNextOutput::Return("done") + } + } +} +``` + +If returning `"done"` via `StopIteration` is not really required, this should be written as + +```rust +use pyo3::prelude::*; + +#[pyclass] +struct PyClassIter { + count: usize, +} + +#[pymethods] +impl PyClassIter { + fn __next__(&mut self) -> Option { + if self.count < 5 { + self.count += 1; + Some(self.count) + } else { + None + } + } +} +``` + +This form also has additional benefits: It has already worked in previous PyO3 versions, it matches the signature of Rust's [`Iterator` trait](https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html) and it allows using a fast path in CPython which completely avoids the cost of raising a `StopIteration` exception. Note that using [`Option::transpose`](https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.transpose) and the `PyResult>` variant, this form can also be used to wrap fallible iterators. + +Alternatively, the implementation can also be done as it would in Python itself, i.e. by "raising" a `StopIteration` exception + +```rust +use pyo3::prelude::*; +use pyo3::exceptions::PyStopIteration; + +#[pyclass] +struct PyClassIter { + count: usize, +} + +#[pymethods] +impl PyClassIter { + fn __next__(&mut self) -> PyResult { + if self.count < 5 { + self.count += 1; + Ok(self.count) + } else { + Err(PyStopIteration::new_err("done")) + } + } +} +``` + +Finally, an asynchronous iterator can directly return an awaitable without confusing wrapping + +```rust +use pyo3::prelude::*; + +#[pyclass] +struct PyClassAwaitable { + number: usize, +} + +#[pymethods] +impl PyClassAwaitable { + fn __next__(&self) -> usize { + self.number + } + + fn __await__(slf: Py) -> Py { + slf + } +} + +#[pyclass] +struct PyClassAsyncIter { + number: usize, +} + +#[pymethods] +impl PyClassAsyncIter { + fn __anext__(&mut self) -> PyClassAwaitable { + self.number += 1; + PyClassAwaitable { number: self.number } + } + + fn __aiter__(slf: Py) -> Py { + slf + } +} +``` + +### `PyType::name` has been renamed to `PyType::qualname` `PyType::name` has been renamed to `PyType::qualname` to indicate that it does indeed return the [qualified name](https://docs.python.org/3/glossary.html#term-qualified-name), matching the `__qualname__` attribute. The newly added `PyType::name` yields the full name including the module name now which corresponds to `__module__.__name__` on the level of attributes. diff --git a/newsfragments/3661.changed.md b/newsfragments/3661.changed.md new file mode 100644 index 00000000..8245a6f1 --- /dev/null +++ b/newsfragments/3661.changed.md @@ -0,0 +1 @@ +The `Iter(A)NextOutput` types are now deprecated and `__(a)next__` can directly return anything which can be converted into Python objects, i.e. awaitables do not need to be wrapped into `IterANextOutput` or `Option` any more. `Option` can still be used as well and returning `None` will trigger the fast path for `__next__`, stopping iteration without having to raise a `StopIteration` exception. From 5528895f3ea58fae5df479fc46dbcc240503a69b Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Wed, 20 Dec 2023 13:12:16 +0100 Subject: [PATCH 5/5] Relax the error type in the Result, E>> specializations for __(a)next__. --- guide/src/migration.md | 4 ++-- src/impl_/pymethods.rs | 22 +++++++++++++--------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/guide/src/migration.md b/guide/src/migration.md index b002daf9..4380dc4c 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -76,7 +76,7 @@ Python::with_gil(|py| { ### `Iter(A)NextOutput` are deprecated -The `__next__` and `__anext__` magic methods can now return any type convertible into Python objects directly just like all other `#[pymethods]`. The `IterNextOutput` used by `__next__` and `IterANextOutput` used by `__anext__` are subsequently deprecated. Most importantly, this change allows returning an awaitable from `__anext__` without non-sensically wrapping it into `Yield` or `Some`. Only the return types `Option` and `PyResult>` are still handled in a special manner where `Some(val)` yields `val` and `None` stops iteration. +The `__next__` and `__anext__` magic methods can now return any type convertible into Python objects directly just like all other `#[pymethods]`. The `IterNextOutput` used by `__next__` and `IterANextOutput` used by `__anext__` are subsequently deprecated. Most importantly, this change allows returning an awaitable from `__anext__` without non-sensically wrapping it into `Yield` or `Some`. Only the return types `Option` and `Result, E>` are still handled in a special manner where `Some(val)` yields `val` and `None` stops iteration. Starting with an implementation of a Python iterator using `IterNextOutput`, e.g. @@ -126,7 +126,7 @@ impl PyClassIter { } ``` -This form also has additional benefits: It has already worked in previous PyO3 versions, it matches the signature of Rust's [`Iterator` trait](https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html) and it allows using a fast path in CPython which completely avoids the cost of raising a `StopIteration` exception. Note that using [`Option::transpose`](https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.transpose) and the `PyResult>` variant, this form can also be used to wrap fallible iterators. +This form also has additional benefits: It has already worked in previous PyO3 versions, it matches the signature of Rust's [`Iterator` trait](https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html) and it allows using a fast path in CPython which completely avoids the cost of raising a `StopIteration` exception. Note that using [`Option::transpose`](https://doc.rust-lang.org/stable/std/option/enum.Option.html#method.transpose) and the `Result, E>` variant, this form can also be used to wrap fallible iterators. Alternatively, the implementation can also be done as it would in Python itself, i.e. by "raising" a `StopIteration` exception diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs index aeac6518..f2d816bb 100644 --- a/src/impl_/pymethods.rs +++ b/src/impl_/pymethods.rs @@ -3,7 +3,9 @@ use crate::exceptions::PyStopAsyncIteration; use crate::gil::LockGIL; use crate::impl_::panic::PanicTrap; use crate::internal_tricks::extract_c_string; -use crate::{ffi, PyAny, PyCell, PyClass, PyObject, PyResult, PyTraverseError, PyVisit, Python}; +use crate::{ + ffi, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult, PyTraverseError, PyVisit, Python, +}; use std::borrow::Cow; use std::ffi::CStr; use std::fmt; @@ -358,18 +360,19 @@ pub struct IterResultOptionTag; impl IterResultOptionTag { #[inline] - pub fn convert( + pub fn convert( self, py: Python<'_>, - value: PyResult>, + value: Result, Error>, ) -> PyResult<*mut ffi::PyObject> where Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + Error: Into, { match value { Ok(Some(value)) => value.convert(py), Ok(None) => Ok(null_mut()), - Err(err) => Err(err), + Err(err) => Err(err.into()), } } } @@ -381,7 +384,7 @@ pub trait IterResultOptionKind { } } -impl IterResultOptionKind for PyResult> {} +impl IterResultOptionKind for Result, Error> {} // Autoref-based specialization for handling `__anext__` returning `Option` @@ -438,18 +441,19 @@ pub struct AsyncIterResultOptionTag; impl AsyncIterResultOptionTag { #[inline] - pub fn convert( + pub fn convert( self, py: Python<'_>, - value: PyResult>, + value: Result, Error>, ) -> PyResult<*mut ffi::PyObject> where Value: IntoPyCallbackOutput<*mut ffi::PyObject>, + Error: Into, { match value { Ok(Some(value)) => value.convert(py), Ok(None) => Err(PyStopAsyncIteration::new_err(())), - Err(err) => Err(err), + Err(err) => Err(err.into()), } } } @@ -461,4 +465,4 @@ pub trait AsyncIterResultOptionKind { } } -impl AsyncIterResultOptionKind for PyResult> {} +impl AsyncIterResultOptionKind for Result, Error> {}