From 34b361aa911546929278c8b12aa44e00fa412660 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Fri, 12 Apr 2024 18:26:41 +0200 Subject: [PATCH 1/4] deprecate "trailing optional arguments" implicit default behaviour --- guide/src/async-await.md | 1 + guide/src/function/signature.md | 13 ++++++++++ pyo3-macros-backend/src/params.rs | 38 ++++++++++++++++++++++++++--- pyo3-macros-backend/src/pymethod.rs | 1 + pytests/src/datetime.rs | 3 +++ src/impl_/deprecations.rs | 10 ++++++++ src/tests/hygiene/pymethods.rs | 1 + tests/test_arithmetics.rs | 1 + tests/test_mapping.rs | 2 ++ tests/test_methods.rs | 11 +++++++++ tests/test_pyfunction.rs | 2 ++ tests/test_sequence.rs | 1 + tests/test_text_signature.rs | 1 + tests/ui/deprecations.rs | 8 ++++++ tests/ui/deprecations.stderr | 30 ++++++++++++++--------- 15 files changed, 108 insertions(+), 15 deletions(-) diff --git a/guide/src/async-await.md b/guide/src/async-await.md index 06fa1580ad7..27574181804 100644 --- a/guide/src/async-await.md +++ b/guide/src/async-await.md @@ -12,6 +12,7 @@ use futures::channel::oneshot; use pyo3::prelude::*; #[pyfunction] +#[pyo3(signature=(seconds, result=None))] async fn sleep(seconds: f64, result: Option) -> Option { let (tx, rx) = oneshot::channel(); thread::spawn(move || { diff --git a/guide/src/function/signature.md b/guide/src/function/signature.md index b276fc457fb..936fa95e261 100644 --- a/guide/src/function/signature.md +++ b/guide/src/function/signature.md @@ -121,9 +121,22 @@ num=-1 ## Trailing optional arguments +
+ +⚠️ Warning: This behaviour is phased out 🛠️ + +The special casing of trailing optional arguments is deprecated. In a future `pyo3` version, arguments of type `Option<..>` will share the same behaviour as other arguments, they are required unless a default is set using `#[pyo3(signature = (...))]`. + +This is done to better align the Python and Rust definition of such functions and make it more intuitive to rewrite them from Python in Rust. Specifically `def some_fn(a: int, b: Optional[int]): ...` will not automatically default `b` to `none`, but requires an explicit default if desired, where as in current `pyo3` it is handled the other way around. + +During the migration window a `#[pyo3(signature = (...))]` will be required to silence the deprecation warning. After support for trailing optional arguments is fully removed, the signature attribute can be removed if all arguments should be required. +
+ + As a convenience, functions without a `#[pyo3(signature = (...))]` option will treat trailing `Option` arguments as having a default of `None`. In the example below, PyO3 will create `increment` with a signature of `increment(x, amount=None)`. ```rust +#![allow(deprecated)] use pyo3::prelude::*; /// Returns a copy of `x` increased by `amount`. diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index cab9d2a7d29..469b59a10fa 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -134,7 +134,16 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| impl_arg_param(arg, i, &mut 0, holders, ctx)) + .map(|(i, arg)| { + impl_arg_param( + arg, + spec.signature.attribute.is_some(), + i, + &mut 0, + holders, + ctx, + ) + }) .collect(); return ( quote! { @@ -174,7 +183,16 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, holders, ctx)) + .map(|(i, arg)| { + impl_arg_param( + arg, + spec.signature.attribute.is_some(), + i, + &mut option_pos, + holders, + ctx, + ) + }) .collect(); let args_handler = if spec.signature.python_signature.varargs.is_some() { @@ -237,6 +255,7 @@ pub fn impl_arg_params( fn impl_arg_param( arg: &FnArg<'_>, + has_signature_attr: bool, pos: usize, option_pos: &mut usize, holders: &mut Holders, @@ -250,7 +269,14 @@ fn impl_arg_param( let from_py_with = format_ident!("from_py_with_{}", pos); let arg_value = quote!(#args_array[#option_pos].as_deref()); *option_pos += 1; - let tokens = impl_regular_arg_param(arg, from_py_with, arg_value, holders, ctx); + let tokens = impl_regular_arg_param( + arg, + has_signature_attr, + from_py_with, + arg_value, + holders, + ctx, + ); check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx) } FnArg::VarArgs(arg) => { @@ -285,6 +311,7 @@ fn impl_arg_param( /// index and the index in option diverge when using py: Python pub(crate) fn impl_regular_arg_param( arg: &RegularArg<'_>, + has_signature_attr: bool, from_py_with: syn::Ident, arg_value: TokenStream, // expected type: Option<&'a Bound<'py, PyAny>> holders: &mut Holders, @@ -335,6 +362,11 @@ pub(crate) fn impl_regular_arg_param( } } else if arg.option_wrapped_type.is_some() { let holder = holders.push_holder(arg.name.span()); + let arg_value = if !has_signature_attr { + quote_arg_span! { #pyo3_path::impl_::deprecations::deprecate_implicit_option(#arg_value) } + } else { + quote!(#arg_value) + }; quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_optional_argument( #arg_value, diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 3e8c2980700..4c44504ac64 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -630,6 +630,7 @@ pub fn impl_py_setter_def( let tokens = impl_regular_arg_param( arg, + spec.signature.attribute.is_some(), ident, quote!(::std::option::Option::Some(_value.into())), &mut holders, diff --git a/pytests/src/datetime.rs b/pytests/src/datetime.rs index d0de99ae406..e26782d04f7 100644 --- a/pytests/src/datetime.rs +++ b/pytests/src/datetime.rs @@ -25,6 +25,7 @@ fn date_from_timestamp(py: Python<'_>, timestamp: i64) -> PyResult( py: Python<'py>, hour: u8, @@ -101,6 +102,7 @@ fn get_delta_tuple<'py>(delta: &Bound<'py, PyDelta>) -> Bound<'py, PyTuple> { #[allow(clippy::too_many_arguments)] #[pyfunction] +#[pyo3(signature=(year, month, day, hour, minute, second, microsecond, tzinfo=None))] fn make_datetime<'py>( py: Python<'py>, year: i32, @@ -159,6 +161,7 @@ fn get_datetime_tuple_fold<'py>(dt: &Bound<'py, PyDateTime>) -> Bound<'py, PyTup } #[pyfunction] +#[pyo3(signature=(ts, tz=None))] fn datetime_from_timestamp<'py>( py: Python<'py>, ts: f64, diff --git a/src/impl_/deprecations.rs b/src/impl_/deprecations.rs index 650e01ce729..a924383f779 100644 --- a/src/impl_/deprecations.rs +++ b/src/impl_/deprecations.rs @@ -73,3 +73,13 @@ impl std::ops::Deref for OptionGilRefs { &self.0 } } + +#[deprecated( + since = "0.22.0", + note = "Implicit default for trailing optional arguments is phased out. Add an explicit \ + `#[pyo3(signature = (...))]` attribute a to silence this warning. In a future \ + pyo3 version `Option<..>` arguments will be treated the same as any other argument." +)] +pub fn deprecate_implicit_option(t: T) -> T { + t +} diff --git a/src/tests/hygiene/pymethods.rs b/src/tests/hygiene/pymethods.rs index 020f983be31..95d670c63a6 100644 --- a/src/tests/hygiene/pymethods.rs +++ b/src/tests/hygiene/pymethods.rs @@ -309,6 +309,7 @@ impl Dummy { 0 } + #[pyo3(signature=(ndigits=::std::option::Option::None))] fn __round__(&self, ndigits: ::std::option::Option) -> u32 { 0 } diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index b1efcd0ac55..007f42a79e8 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -35,6 +35,7 @@ impl UnaryArithmetic { Self::new(self.inner.abs()) } + #[pyo3(signature=(_ndigits=None))] fn __round__(&self, _ndigits: Option) -> Self { Self::new(self.inner.round()) } diff --git a/tests/test_mapping.rs b/tests/test_mapping.rs index 675dd14d63f..784ab8845cd 100644 --- a/tests/test_mapping.rs +++ b/tests/test_mapping.rs @@ -21,6 +21,7 @@ struct Mapping { #[pymethods] impl Mapping { #[new] + #[pyo3(signature=(elements=None))] fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult { if let Some(pylist) = elements { let mut elems = HashMap::with_capacity(pylist.len()); @@ -59,6 +60,7 @@ impl Mapping { } } + #[pyo3(signature=(key, default=None))] fn get(&self, py: Python<'_>, key: &str, default: Option) -> Option { self.index .get(key) diff --git a/tests/test_methods.rs b/tests/test_methods.rs index c1610be3dd6..37f3b2d8bd6 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -187,6 +187,7 @@ impl MethSignature { fn get_optional2(&self, test: Option) -> Option { test } + #[pyo3(signature=(_t1 = None, t2 = None, _t3 = None))] fn get_optional_positional( &self, _t1: Option, @@ -745,11 +746,13 @@ impl MethodWithPyClassArg { fn inplace_add_pyref(&self, mut other: PyRefMut<'_, MethodWithPyClassArg>) { other.value += self.value; } + #[pyo3(signature=(other = None))] fn optional_add(&self, other: Option<&MethodWithPyClassArg>) -> MethodWithPyClassArg { MethodWithPyClassArg { value: self.value + other.map(|o| o.value).unwrap_or(10), } } + #[pyo3(signature=(other = None))] fn optional_inplace_add(&self, other: Option<&mut MethodWithPyClassArg>) { if let Some(other) = other { other.value += self.value; @@ -851,6 +854,7 @@ struct FromSequence { #[pymethods] impl FromSequence { #[new] + #[pyo3(signature=(seq = None))] fn new(seq: Option<&Bound<'_, PySequence>>) -> PyResult { if let Some(seq) = seq { Ok(FromSequence { @@ -1026,6 +1030,7 @@ macro_rules! issue_1506 { issue_1506!( #[pymethods] impl Issue1506 { + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506( &self, _py: Python<'_>, @@ -1035,6 +1040,7 @@ issue_1506!( ) { } + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_mut( &mut self, _py: Python<'_>, @@ -1044,6 +1050,7 @@ issue_1506!( ) { } + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_custom_receiver( _slf: Py, _py: Python<'_>, @@ -1053,6 +1060,7 @@ issue_1506!( ) { } + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_custom_receiver_explicit( _slf: Py, _py: Python<'_>, @@ -1063,6 +1071,7 @@ issue_1506!( } #[new] + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_new( _py: Python<'_>, _arg: &Bound<'_, PyAny>, @@ -1081,6 +1090,7 @@ issue_1506!( fn issue_1506_setter(&self, _py: Python<'_>, _value: i32) {} #[staticmethod] + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_static( _py: Python<'_>, _arg: &Bound<'_, PyAny>, @@ -1090,6 +1100,7 @@ issue_1506!( } #[classmethod] + #[pyo3(signature = (_arg, _args, _kwargs=None))] fn issue_1506_class( _cls: &Bound<'_, PyType>, _py: Python<'_>, diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 8a57e2707c9..6eaf1a8f11b 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -216,6 +216,7 @@ struct ValueClass { } #[pyfunction] +#[pyo3(signature=(str_arg, int_arg, tuple_arg, option_arg = None, struct_arg = None))] fn conversion_error( str_arg: &str, int_arg: i64, @@ -542,6 +543,7 @@ fn test_some_wrap_arguments() { #[test] fn test_reference_to_bound_arguments() { #[pyfunction] + #[pyo3(signature = (x, y = None))] fn reference_args<'py>( x: &Bound<'py, PyAny>, y: Option<&Bound<'py, PyAny>>, diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 3715affc05b..9627f06ca75 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -17,6 +17,7 @@ struct ByteSequence { #[pymethods] impl ByteSequence { #[new] + #[pyo3(signature=(elements = None))] fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult { if let Some(pylist) = elements { let mut elems = Vec::with_capacity(pylist.len()); diff --git a/tests/test_text_signature.rs b/tests/test_text_signature.rs index a9f5a041596..3899878bd56 100644 --- a/tests/test_text_signature.rs +++ b/tests/test_text_signature.rs @@ -142,6 +142,7 @@ fn test_auto_test_signature_function() { } #[pyfunction] + #[pyo3(signature=(a, b=None, c=None))] fn my_function_6(a: i32, b: Option, c: Option) { let _ = (a, b, c); } diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index ef0b06652e4..f59a8167352 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -114,8 +114,16 @@ fn pyfunction_from_py_with( fn pyfunction_gil_ref(_any: &PyAny) {} #[pyfunction] +#[pyo3(signature = (_any))] fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} +#[pyfunction] +#[pyo3(signature = (_i, _any=None))] +fn pyfunction_option_1(_i: u32, _any: Option) {} + +#[pyfunction] +fn pyfunction_option_2(_i: u32, _any: Option) {} + #[derive(Debug, FromPyObject)] pub struct Zap { #[pyo3(item)] diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index dc21b595743..5c78bdea67f 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -10,6 +10,12 @@ note: the lint level is defined here 1 | #![deny(deprecated)] | ^^^^^^^^^^ +error: use of deprecated function `pyo3::deprecations::deprecate_implicit_option`: Implicit default for trailing optional arguments is phased out. Add an explicit `#[pyo3(signature = (...))]` attribute a to silence this warning. In a future pyo3 version `Option<..>` arguments will be treated the same as any other argument. + --> tests/ui/deprecations.rs:125:39 + | +125 | fn pyfunction_option_2(_i: u32, _any: Option) {} + | ^^^^^^ + error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor --> tests/ui/deprecations.rs:42:44 | @@ -77,39 +83,39 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg` | ^ error: use of deprecated method `pyo3::deprecations::OptionGilRefs::>::function_arg`: use `Option<&Bound<'_, T>>` instead for this function argument - --> tests/ui/deprecations.rs:117:36 + --> tests/ui/deprecations.rs:118:36 | -117 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} +118 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} | ^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:124:27 + --> tests/ui/deprecations.rs:132:27 | -124 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] +132 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:134:27 + --> tests/ui/deprecations.rs:142:27 | -134 | #[pyo3(from_py_with = "PyAny::len")] usize, +142 | #[pyo3(from_py_with = "PyAny::len")] usize, | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:140:31 + --> tests/ui/deprecations.rs:148:31 | -140 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), +148 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:147:27 + --> tests/ui/deprecations.rs:155:27 | -147 | #[pyo3(from_py_with = "extract_gil_ref")] +155 | #[pyo3(from_py_with = "extract_gil_ref")] | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:160:13 + --> tests/ui/deprecations.rs:168:13 | -160 | let _ = wrap_pyfunction!(double, py); +168 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info) From 2fa9a2ae49d489a23ff945d179bb6d0952fd1946 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sun, 14 Apr 2024 17:57:15 +0200 Subject: [PATCH 2/4] add newsfragment --- newsfragments/4078.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4078.changed.md diff --git a/newsfragments/4078.changed.md b/newsfragments/4078.changed.md new file mode 100644 index 00000000000..45f160f5556 --- /dev/null +++ b/newsfragments/4078.changed.md @@ -0,0 +1 @@ +deprecate implicit default for trailing optional arguments From adc2b5a3d92efda46e2f14af9bbc7d89509ea2d3 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 9 May 2024 12:07:01 +0200 Subject: [PATCH 3/4] generate individual deprecation messages per function --- guide/src/function/signature.md | 2 +- pyo3-macros-backend/src/deprecations.rs | 53 +++++++++++++++++- pyo3-macros-backend/src/method.rs | 7 +++ pyo3-macros-backend/src/params.rs | 38 +------------ pyo3-macros-backend/src/pymethod.rs | 5 +- src/impl_/deprecations.rs | 10 ---- tests/test_pyfunction.rs | 1 + tests/ui/deprecations.rs | 18 ++++++ tests/ui/deprecations.stderr | 74 +++++++++++++++---------- 9 files changed, 132 insertions(+), 76 deletions(-) diff --git a/guide/src/function/signature.md b/guide/src/function/signature.md index 936fa95e261..69949220be6 100644 --- a/guide/src/function/signature.md +++ b/guide/src/function/signature.md @@ -123,7 +123,7 @@ num=-1
-⚠️ Warning: This behaviour is phased out 🛠️ +⚠️ Warning: This behaviour is being phased out 🛠️ The special casing of trailing optional arguments is deprecated. In a future `pyo3` version, arguments of type `Option<..>` will share the same behaviour as other arguments, they are required unless a default is set using `#[pyo3(signature = (...))]`. diff --git a/pyo3-macros-backend/src/deprecations.rs b/pyo3-macros-backend/src/deprecations.rs index 3f1f34144f6..4db40cc86f7 100644 --- a/pyo3-macros-backend/src/deprecations.rs +++ b/pyo3-macros-backend/src/deprecations.rs @@ -1,4 +1,7 @@ -use crate::utils::Ctx; +use crate::{ + method::{FnArg, FnSpec}, + utils::Ctx, +}; use proc_macro2::{Span, TokenStream}; use quote::{quote_spanned, ToTokens}; @@ -45,3 +48,51 @@ impl<'ctx> ToTokens for Deprecations<'ctx> { } } } + +pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> TokenStream { + if spec.signature.attribute.is_none() + && spec.signature.arguments.iter().any(|arg| { + if let FnArg::Regular(arg) = arg { + arg.option_wrapped_type.is_some() + } else { + false + } + }) + { + use std::fmt::Write; + let mut deprecation_msg = String::from( + "This function has implicit defaults for the trailing `Option` arguments. \ + These implicit defaults are being phased out. Add `#[pyo3(signature = (", + ); + spec.signature.arguments.iter().for_each(|arg| { + match arg { + FnArg::Regular(arg) => { + if arg.option_wrapped_type.is_some() { + write!(deprecation_msg, "{}=None, ", arg.name) + } else { + write!(deprecation_msg, "{}, ", arg.name) + } + } + FnArg::VarArgs(arg) => write!(deprecation_msg, "{}, ", arg.name), + FnArg::KwArgs(arg) => write!(deprecation_msg, "{}, ", arg.name), + FnArg::Py(_) | FnArg::CancelHandle(_) => Ok(()), + } + .expect("writing to `String` should not fail"); + }); + + //remove trailing space and comma + deprecation_msg.pop(); + deprecation_msg.pop(); + + deprecation_msg + .push_str(")]` to this function to silence this warning and keep the current behavior"); + quote_spanned! { spec.name.span() => + #[deprecated(note = #deprecation_msg)] + #[allow(dead_code)] + const SIGNATURE: () = (); + const _: () = SIGNATURE; + } + } else { + TokenStream::new() + } +} diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index e1a025b819e..c0e38bf8416 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -5,6 +5,7 @@ use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote, quote_spanned, ToTokens}; use syn::{ext::IdentExt, spanned::Spanned, Ident, Result}; +use crate::deprecations::deprecate_trailing_option_default; use crate::utils::Ctx; use crate::{ attributes::{FromPyWithAttribute, TextSignatureAttribute, TextSignatureAttributeValue}, @@ -708,6 +709,8 @@ impl<'a> FnSpec<'a> { quote!(#func_name) }; + let deprecation = deprecate_trailing_option_default(self); + Ok(match self.convention { CallingConvention::Noargs => { let mut holders = Holders::new(); @@ -730,6 +733,7 @@ impl<'a> FnSpec<'a> { py: #pyo3_path::Python<'py>, _slf: *mut #pyo3_path::ffi::PyObject, ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + #deprecation let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 #init_holders @@ -754,6 +758,7 @@ impl<'a> FnSpec<'a> { _nargs: #pyo3_path::ffi::Py_ssize_t, _kwnames: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + #deprecation let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert @@ -778,6 +783,7 @@ impl<'a> FnSpec<'a> { _args: *mut #pyo3_path::ffi::PyObject, _kwargs: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { + #deprecation let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert @@ -805,6 +811,7 @@ impl<'a> FnSpec<'a> { _kwargs: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { use #pyo3_path::callback::IntoPyCallbackOutput; + #deprecation let _slf_ref = &_slf; let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 469b59a10fa..cab9d2a7d29 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -134,16 +134,7 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| { - impl_arg_param( - arg, - spec.signature.attribute.is_some(), - i, - &mut 0, - holders, - ctx, - ) - }) + .map(|(i, arg)| impl_arg_param(arg, i, &mut 0, holders, ctx)) .collect(); return ( quote! { @@ -183,16 +174,7 @@ pub fn impl_arg_params( .arguments .iter() .enumerate() - .map(|(i, arg)| { - impl_arg_param( - arg, - spec.signature.attribute.is_some(), - i, - &mut option_pos, - holders, - ctx, - ) - }) + .map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, holders, ctx)) .collect(); let args_handler = if spec.signature.python_signature.varargs.is_some() { @@ -255,7 +237,6 @@ pub fn impl_arg_params( fn impl_arg_param( arg: &FnArg<'_>, - has_signature_attr: bool, pos: usize, option_pos: &mut usize, holders: &mut Holders, @@ -269,14 +250,7 @@ fn impl_arg_param( let from_py_with = format_ident!("from_py_with_{}", pos); let arg_value = quote!(#args_array[#option_pos].as_deref()); *option_pos += 1; - let tokens = impl_regular_arg_param( - arg, - has_signature_attr, - from_py_with, - arg_value, - holders, - ctx, - ); + let tokens = impl_regular_arg_param(arg, from_py_with, arg_value, holders, ctx); check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx) } FnArg::VarArgs(arg) => { @@ -311,7 +285,6 @@ fn impl_arg_param( /// index and the index in option diverge when using py: Python pub(crate) fn impl_regular_arg_param( arg: &RegularArg<'_>, - has_signature_attr: bool, from_py_with: syn::Ident, arg_value: TokenStream, // expected type: Option<&'a Bound<'py, PyAny>> holders: &mut Holders, @@ -362,11 +335,6 @@ pub(crate) fn impl_regular_arg_param( } } else if arg.option_wrapped_type.is_some() { let holder = holders.push_holder(arg.name.span()); - let arg_value = if !has_signature_attr { - quote_arg_span! { #pyo3_path::impl_::deprecations::deprecate_implicit_option(#arg_value) } - } else { - quote!(#arg_value) - }; quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_optional_argument( #arg_value, diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 4c44504ac64..208735f2619 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1,6 +1,7 @@ use std::borrow::Cow; use crate::attributes::{NameAttribute, RenamingRule}; +use crate::deprecations::deprecate_trailing_option_default; use crate::method::{CallingConvention, ExtractErrorMode, PyArg}; use crate::params::{check_arg_for_gil_refs, impl_regular_arg_param, Holders}; use crate::utils::Ctx; @@ -630,7 +631,6 @@ pub fn impl_py_setter_def( let tokens = impl_regular_arg_param( arg, - spec.signature.attribute.is_some(), ident, quote!(::std::option::Option::Some(_value.into())), &mut holders, @@ -638,7 +638,10 @@ pub fn impl_py_setter_def( ); let extract = check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx); + + let deprecation = deprecate_trailing_option_default(spec); quote! { + #deprecation #from_py_with let _val = #extract; } diff --git a/src/impl_/deprecations.rs b/src/impl_/deprecations.rs index a924383f779..650e01ce729 100644 --- a/src/impl_/deprecations.rs +++ b/src/impl_/deprecations.rs @@ -73,13 +73,3 @@ impl std::ops::Deref for OptionGilRefs { &self.0 } } - -#[deprecated( - since = "0.22.0", - note = "Implicit default for trailing optional arguments is phased out. Add an explicit \ - `#[pyo3(signature = (...))]` attribute a to silence this warning. In a future \ - pyo3 version `Option<..>` arguments will be treated the same as any other argument." -)] -pub fn deprecate_implicit_option(t: T) -> T { - t -} diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 6eaf1a8f11b..4a90f3f9d99 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -182,6 +182,7 @@ fn test_from_py_with_defaults() { // issue 2280 combination of from_py_with and Option did not compile #[pyfunction] + #[pyo3(signature = (int=None))] fn from_py_with_option(#[pyo3(from_py_with = "optional_int")] int: Option) -> i32 { int.unwrap_or(0) } diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index f59a8167352..fc9e8687cae 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -39,6 +39,9 @@ impl MyClass { #[setter] fn set_bar_bound(&self, _value: &Bound<'_, PyAny>) {} + #[setter] + fn set_option(&self, _value: Option) {} + fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { true } @@ -103,6 +106,10 @@ fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult { obj.extract() } +fn extract_options(obj: &Bound<'_, PyAny>) -> PyResult> { + obj.extract() +} + #[pyfunction] fn pyfunction_from_py_with( #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, @@ -124,6 +131,17 @@ fn pyfunction_option_1(_i: u32, _any: Option) {} #[pyfunction] fn pyfunction_option_2(_i: u32, _any: Option) {} +#[pyfunction] +fn pyfunction_option_3(_i: u32, _any: Option, _foo: Option) {} + +#[pyfunction] +fn pyfunction_option_4( + _i: u32, + #[pyo3(from_py_with = "extract_options")] _any: Option, + _foo: Option, +) { +} + #[derive(Debug, FromPyObject)] pub struct Zap { #[pyo3(item)] diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index 5c78bdea67f..b11c0058ce2 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -10,16 +10,34 @@ note: the lint level is defined here 1 | #![deny(deprecated)] | ^^^^^^^^^^ -error: use of deprecated function `pyo3::deprecations::deprecate_implicit_option`: Implicit default for trailing optional arguments is phased out. Add an explicit `#[pyo3(signature = (...))]` attribute a to silence this warning. In a future pyo3 version `Option<..>` arguments will be treated the same as any other argument. - --> tests/ui/deprecations.rs:125:39 +error: use of deprecated constant `MyClass::__pymethod_set_set_option__::SIGNATURE`: This function has implicit defaults for the trailing `Option` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_value=None)]` to this function to silence this warning and keep the current behavior + --> tests/ui/deprecations.rs:43:8 + | +43 | fn set_option(&self, _value: Option) {} + | ^^^^^^^^^^ + +error: use of deprecated constant `__pyfunction_pyfunction_option_2::SIGNATURE`: This function has implicit defaults for the trailing `Option` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None)]` to this function to silence this warning and keep the current behavior + --> tests/ui/deprecations.rs:132:4 + | +132 | fn pyfunction_option_2(_i: u32, _any: Option) {} + | ^^^^^^^^^^^^^^^^^^^ + +error: use of deprecated constant `__pyfunction_pyfunction_option_3::SIGNATURE`: This function has implicit defaults for the trailing `Option` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None, _foo=None)]` to this function to silence this warning and keep the current behavior + --> tests/ui/deprecations.rs:135:4 + | +135 | fn pyfunction_option_3(_i: u32, _any: Option, _foo: Option) {} + | ^^^^^^^^^^^^^^^^^^^ + +error: use of deprecated constant `__pyfunction_pyfunction_option_4::SIGNATURE`: This function has implicit defaults for the trailing `Option` arguments. These implicit defaults are being phased out. Add `#[pyo3(signature = (_i, _any=None, _foo=None)]` to this function to silence this warning and keep the current behavior + --> tests/ui/deprecations.rs:138:4 | -125 | fn pyfunction_option_2(_i: u32, _any: Option) {} - | ^^^^^^ +138 | fn pyfunction_option_4( + | ^^^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:42:44 + --> tests/ui/deprecations.rs:45:44 | -42 | fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { +45 | fn __eq__(&self, #[pyo3(from_py_with = "extract_gil_ref")] _other: i32) -> bool { | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument @@ -53,69 +71,69 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg` | ^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:61:44 + --> tests/ui/deprecations.rs:64:44 | -61 | fn pyfunction_with_module_gil_ref(_module: &PyModule) -> PyResult<&str> { +64 | fn pyfunction_with_module_gil_ref(_module: &PyModule) -> PyResult<&str> { | ^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:71:19 + --> tests/ui/deprecations.rs:74:19 | -71 | fn module_gil_ref(_m: &PyModule) -> PyResult<()> { +74 | fn module_gil_ref(_m: &PyModule) -> PyResult<()> { | ^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:76:57 + --> tests/ui/deprecations.rs:79:57 | -76 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, _m: &PyModule) -> PyResult<()> { +79 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, _m: &PyModule) -> PyResult<()> { | ^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:108:27 + --> tests/ui/deprecations.rs:115:27 | -108 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, +115 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32, | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg`: use `&Bound<'_, T>` instead for this function argument - --> tests/ui/deprecations.rs:114:29 + --> tests/ui/deprecations.rs:121:29 | -114 | fn pyfunction_gil_ref(_any: &PyAny) {} +121 | fn pyfunction_gil_ref(_any: &PyAny) {} | ^ error: use of deprecated method `pyo3::deprecations::OptionGilRefs::>::function_arg`: use `Option<&Bound<'_, T>>` instead for this function argument - --> tests/ui/deprecations.rs:118:36 + --> tests/ui/deprecations.rs:125:36 | -118 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} +125 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} | ^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:132:27 + --> tests/ui/deprecations.rs:150:27 | -132 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] +150 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:142:27 + --> tests/ui/deprecations.rs:160:27 | -142 | #[pyo3(from_py_with = "PyAny::len")] usize, +160 | #[pyo3(from_py_with = "PyAny::len")] usize, | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:148:31 + --> tests/ui/deprecations.rs:166:31 | -148 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), +166 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:155:27 + --> tests/ui/deprecations.rs:173:27 | -155 | #[pyo3(from_py_with = "extract_gil_ref")] +173 | #[pyo3(from_py_with = "extract_gil_ref")] | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:168:13 + --> tests/ui/deprecations.rs:186:13 | -168 | let _ = wrap_pyfunction!(double, py); +186 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info) From 6cf6532fd0e40166cb1d3f8b73ea049db70d4569 Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Thu, 9 May 2024 23:54:45 +0200 Subject: [PATCH 4/4] add migration guide entry --- guide/src/migration.md | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/guide/src/migration.md b/guide/src/migration.md index 2317f85185e..875407317bf 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -3,6 +3,41 @@ This guide can help you upgrade code through breaking changes from one PyO3 version to the next. For a detailed list of all changes, see the [CHANGELOG](changelog.md). +## from 0.21.* to 0.22 + +### Deprecation of implicit default for trailing optional arguments +
+Click to expand + +With `pyo3` 0.22 the implicit `None` default for trailing `Option` type argument is deprecated. To migrate, place a `#[pyo3(signature = (...))]` attribute on affected functions or methods and specify the desired behavior. +The migration warning specifies the corresponding signature to keep the current behavior. With 0.23 the signature will be required for any function containing `Option` type parameters to prevent accidental +and unnoticed changes in behavior. With 0.24 this restriction will be lifted again and `Option` type arguments will be treated as any other argument _without_ special handling. + +Before: + +```rust +# #![allow(deprecated, dead_code)] +# use pyo3::prelude::*; +#[pyfunction] +fn increment(x: u64, amount: Option) -> u64 { + x + amount.unwrap_or(1) +} +``` + +After: + +```rust +# #![allow(dead_code)] +# use pyo3::prelude::*; +#[pyfunction] +#[pyo3(signature = (x, amount=None))] +fn increment(x: u64, amount: Option) -> u64 { + x + amount.unwrap_or(1) +} +``` + +
+ ## from 0.20.* to 0.21
Click to expand