diff --git a/guide/src/function/signature.md b/guide/src/function/signature.md index 8ebe74456a1..1aea8cdf962 100644 --- a/guide/src/function/signature.md +++ b/guide/src/function/signature.md @@ -118,82 +118,6 @@ num=-1 > } > ``` -## Trailing optional arguments - -
- -⚠️ 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 = (...))]`. - -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`. -/// -/// If `amount` is unspecified or `None`, equivalent to `x + 1`. -#[pyfunction] -fn increment(x: u64, amount: Option) -> u64 { - x + amount.unwrap_or(1) -} -# -# fn main() -> PyResult<()> { -# Python::with_gil(|py| { -# let fun = pyo3::wrap_pyfunction!(increment, py)?; -# -# let inspect = PyModule::import(py, "inspect")?.getattr("signature")?; -# let sig: String = inspect -# .call1((fun,))? -# .call_method0("__str__")? -# .extract()?; -# -# #[cfg(Py_3_8)] // on 3.7 the signature doesn't render b, upstream bug? -# assert_eq!(sig, "(x, amount=None)"); -# -# Ok(()) -# }) -# } -``` - -To make trailing `Option` arguments required, but still accept `None`, add a `#[pyo3(signature = (...))]` annotation. For the example above, this would be `#[pyo3(signature = (x, amount))]`: - -```rust -# use pyo3::prelude::*; -#[pyfunction] -#[pyo3(signature = (x, amount))] -fn increment(x: u64, amount: Option) -> u64 { - x + amount.unwrap_or(1) -} -# -# fn main() -> PyResult<()> { -# Python::with_gil(|py| { -# let fun = pyo3::wrap_pyfunction!(increment, py)?; -# -# let inspect = PyModule::import(py, "inspect")?.getattr("signature")?; -# let sig: String = inspect -# .call1((fun,))? -# .call_method0("__str__")? -# .extract()?; -# -# #[cfg(Py_3_8)] // on 3.7 the signature doesn't render b, upstream bug? -# assert_eq!(sig, "(x, amount)"); -# -# Ok(()) -# }) -# } -``` - -To help avoid confusion, PyO3 requires `#[pyo3(signature = (...))]` when an `Option` argument is surrounded by arguments which aren't `Option`. - ## Making the function signature available to Python The function signature is exposed to Python via the `__text_signature__` attribute. PyO3 automatically generates this for every `#[pyfunction]` and all `#[pymethods]` directly from the Rust function, taking into account any override done with the `#[pyo3(signature = (...))]` option. diff --git a/guide/src/migration.md b/guide/src/migration.md index 8a8f3694f6d..1e6aced5acd 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -282,7 +282,7 @@ and unnoticed changes in behavior. With 0.24 this restriction will be lifted aga Before: -```rust +```rust,ignore # #![allow(deprecated, dead_code)] # use pyo3::prelude::*; #[pyfunction] @@ -1095,7 +1095,7 @@ Starting with PyO3 0.18, this is deprecated and a future PyO3 version will requi Before, x in the below example would be required to be passed from Python code: -```rust,compile_fail +```rust,compile_fail,ignore # #![allow(dead_code)] # use pyo3::prelude::*; diff --git a/newsfragments/4729.changed.md b/newsfragments/4729.changed.md new file mode 100644 index 00000000000..abb093e66f4 --- /dev/null +++ b/newsfragments/4729.changed.md @@ -0,0 +1 @@ +makes trailing optional arguments a hard error (see #2935) \ No newline at end of file diff --git a/pyo3-macros-backend/src/deprecations.rs b/pyo3-macros-backend/src/deprecations.rs index df48c9da417..6631ebf287f 100644 --- a/pyo3-macros-backend/src/deprecations.rs +++ b/pyo3-macros-backend/src/deprecations.rs @@ -1,22 +1,24 @@ -use crate::method::{FnArg, FnSpec}; -use proc_macro2::TokenStream; -use quote::quote_spanned; +use crate::method::{FnArg, FnSpec, RegularArg}; -pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> TokenStream { +pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> syn::Result<()> { if spec.signature.attribute.is_none() && spec.tp.signature_attribute_allowed() - && spec.signature.arguments.iter().any(|arg| { - if let FnArg::Regular(arg) = arg { - arg.option_wrapped_type.is_some() - } else { - false - } + && spec.signature.arguments.iter().last().map_or(false, |arg| { + matches!( + arg, + FnArg::Regular(RegularArg { + option_wrapped_type: Some(..), + .. + }), + ) }) { use std::fmt::Write; let mut deprecation_msg = String::from( - "this function has implicit defaults for the trailing `Option` arguments \n\ - = note: these implicit defaults are being phased out \n\ + "this function uses trailing `Option` arguments \n\ + = note: these used to be implicitly default to `None` \n\ + = note: this behaviour is phased out \n\ + = note: in the future this error will be lifted and trailing `Option`s be treated as any other argument \n\ = help: add `#[pyo3(signature = (", ); spec.signature.arguments.iter().for_each(|arg| { @@ -39,16 +41,9 @@ pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> TokenStrea 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; - } + deprecation_msg.push_str("))]` to this function to keep the previous behavior"); + Err(syn::Error::new(spec.name.span(), deprecation_msg)) } else { - TokenStream::new() + Ok(()) } } diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index f99e64562b7..c7ac4f52753 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -745,7 +745,7 @@ impl<'a> FnSpec<'a> { quote!(#func_name) }; - let deprecation = deprecate_trailing_option_default(self); + deprecate_trailing_option_default(self)?; Ok(match self.convention { CallingConvention::Noargs => { @@ -767,7 +767,6 @@ 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 function = #rust_name; // Shadow the function name to avoid #3017 #init_holders let result = #call; @@ -789,7 +788,6 @@ 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 function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #init_holders @@ -811,7 +809,6 @@ 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 function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #init_holders @@ -836,7 +833,6 @@ impl<'a> FnSpec<'a> { _kwargs: *mut #pyo3_path::ffi::PyObject ) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> { use #pyo3_path::impl_::callback::IntoPyCallbackOutput; - #deprecation let function = #rust_name; // Shadow the function name to avoid #3017 #arg_convert #init_holders diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index 67054458c98..9517d35b25c 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -245,10 +245,7 @@ pub(crate) fn impl_regular_arg_param( // Option arguments have special treatment: the default should be specified _without_ the // Some() wrapper. Maybe this should be changed in future?! if arg.option_wrapped_type.is_some() { - default = Some(default.map_or_else( - || quote!(::std::option::Option::None), - |tokens| some_wrap(tokens, ctx), - )); + default = default.map(|tokens| some_wrap(tokens, ctx)); } if arg.from_py_with.is_some() { @@ -273,31 +270,32 @@ pub(crate) fn impl_regular_arg_param( )? } } - } else if arg.option_wrapped_type.is_some() { - let holder = holders.push_holder(arg.name.span()); - quote_arg_span! { - #pyo3_path::impl_::extract_argument::extract_optional_argument( - #arg_value, - &mut #holder, - #name_str, - #[allow(clippy::redundant_closure)] - { - || #default - } - )? - } } else if let Some(default) = default { let holder = holders.push_holder(arg.name.span()); - quote_arg_span! { - #pyo3_path::impl_::extract_argument::extract_argument_with_default( - #arg_value, - &mut #holder, - #name_str, - #[allow(clippy::redundant_closure)] - { - || #default - } - )? + if arg.option_wrapped_type.is_some() { + quote_arg_span! { + #pyo3_path::impl_::extract_argument::extract_optional_argument( + #arg_value, + &mut #holder, + #name_str, + #[allow(clippy::redundant_closure)] + { + || #default + } + )? + } + } else { + quote_arg_span! { + #pyo3_path::impl_::extract_argument::extract_argument_with_default( + #arg_value, + &mut #holder, + #name_str, + #[allow(clippy::redundant_closure)] + { + || #default + } + )? + } } } else { let holder = holders.push_holder(arg.name.span()); diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index 0a2d861d2b1..d7268f5f7b6 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -467,12 +467,7 @@ impl<'a> FunctionSignature<'a> { continue; } - if let FnArg::Regular(RegularArg { - ty, - option_wrapped_type: None, - .. - }) = arg - { + if let FnArg::Regular(RegularArg { ty, .. }) = arg { // This argument is required, all previous arguments must also have been required ensure_spanned!( python_signature.required_positional_parameters == python_signature.positional_parameters.len(), diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index 560c3c9dcc1..f3b5ce79383 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -685,9 +685,8 @@ pub fn impl_py_setter_def( ctx, ); - let deprecation = deprecate_trailing_option_default(spec); + deprecate_trailing_option_default(spec)?; quote! { - #deprecation #from_py_with let _val = #extract; } diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index 6236dc55631..52a51cd11ad 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -1,28 +1,28 @@ -error: use of deprecated constant `__pyfunction_pyfunction_option_2::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments - = note: these implicit defaults are being phased out - = help: add `#[pyo3(signature = (_i, _any=None))]` to this function to silence this warning and keep the current behavior +error: this function uses trailing `Option` arguments + = note: these used to be implicitly default to `None` + = note: this behaviour is phased out + = note: in the future this error will be lifted and trailing `Option`s be treated as any other argument + = help: add `#[pyo3(signature = (_i, _any=None))]` to this function to keep the previous behavior --> tests/ui/deprecations.rs:15:4 | 15 | fn pyfunction_option_2(_i: u32, _any: Option) {} | ^^^^^^^^^^^^^^^^^^^ - | -note: the lint level is defined here - --> tests/ui/deprecations.rs:1:9 - | -1 | #![deny(deprecated)] - | ^^^^^^^^^^ -error: use of deprecated constant `__pyfunction_pyfunction_option_3::SIGNATURE`: this function has implicit defaults for the trailing `Option` arguments - = note: these implicit defaults are being phased out - = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to silence this warning and keep the current behavior +error: this function uses trailing `Option` arguments + = note: these used to be implicitly default to `None` + = note: this behaviour is phased out + = note: in the future this error will be lifted and trailing `Option`s be treated as any other argument + = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to keep the previous behavior --> tests/ui/deprecations.rs:18:4 | 18 | 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 - = note: these implicit defaults are being phased out - = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to silence this warning and keep the current behavior +error: this function uses trailing `Option` arguments + = note: these used to be implicitly default to `None` + = note: this behaviour is phased out + = note: in the future this error will be lifted and trailing `Option`s be treated as any other argument + = help: add `#[pyo3(signature = (_i, _any=None, _foo=None))]` to this function to keep the previous behavior --> tests/ui/deprecations.rs:21:4 | 21 | fn pyfunction_option_4( @@ -34,4 +34,9 @@ error: use of deprecated method `pyo3::impl_::pyclass::Deprecation::autogenerate 28 | #[pyclass] | ^^^^^^^^^^ | +note: the lint level is defined here + --> tests/ui/deprecations.rs:1:9 + | +1 | #![deny(deprecated)] + | ^^^^^^^^^^ = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/tests/ui/invalid_pyfunctions.rs b/tests/ui/invalid_pyfunctions.rs index 1c0c45d6b95..cb59808ecc7 100644 --- a/tests/ui/invalid_pyfunctions.rs +++ b/tests/ui/invalid_pyfunctions.rs @@ -13,9 +13,6 @@ fn wildcard_argument(_: i32) {} #[pyfunction] fn destructured_argument((_a, _b): (i32, i32)) {} -#[pyfunction] -fn function_with_required_after_option(_opt: Option, _x: i32) {} - #[pyfunction] #[pyo3(signature=(*args))] fn function_with_optional_args(args: Option>) { diff --git a/tests/ui/invalid_pyfunctions.stderr b/tests/ui/invalid_pyfunctions.stderr index ab35b086b94..4d86bf3ab7f 100644 --- a/tests/ui/invalid_pyfunctions.stderr +++ b/tests/ui/invalid_pyfunctions.stderr @@ -22,13 +22,6 @@ error: destructuring in arguments is not supported 14 | fn destructured_argument((_a, _b): (i32, i32)) {} | ^^^^^^^^ -error: required arguments after an `Option<_>` argument are ambiguous - = help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters - --> tests/ui/invalid_pyfunctions.rs:17:63 - | -17 | fn function_with_required_after_option(_opt: Option, _x: i32) {} - | ^^^ - error: args cannot be optional --> tests/ui/invalid_pyfunctions.rs:21:32 |