diff --git a/newsfragments/2703.changed.md b/newsfragments/2703.changed.md new file mode 100644 index 00000000..bc67791e --- /dev/null +++ b/newsfragments/2703.changed.md @@ -0,0 +1 @@ +Deprecate required arguments after `Option` arguments to `#[pyfunction]` and `#[pymethods]` without also using `#[pyo3(signature)]` to specify whether the arguments should be required or have defaults. diff --git a/pyo3-macros-backend/src/deprecations.rs b/pyo3-macros-backend/src/deprecations.rs index 102fb196..9b168f77 100644 --- a/pyo3-macros-backend/src/deprecations.rs +++ b/pyo3-macros-backend/src/deprecations.rs @@ -1,11 +1,10 @@ use proc_macro2::{Span, TokenStream}; use quote::{quote_spanned, ToTokens}; -// Clippy complains all these variants have the same prefix "Py"... -#[allow(clippy::enum_variant_names)] pub enum Deprecation { PyFunctionArguments, PyMethodArgsAttribute, + RequiredArgumentAfterOption, } impl Deprecation { @@ -13,6 +12,7 @@ impl Deprecation { let string = match self { Deprecation::PyFunctionArguments => "PYFUNCTION_ARGUMENTS", Deprecation::PyMethodArgsAttribute => "PYMETHODS_ARGS_ATTRIBUTE", + Deprecation::RequiredArgumentAfterOption => "REQUIRED_ARGUMENT_AFTER_OPTION", }; syn::Ident::new(string, span) } diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 3b85fc56..93da4040 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -312,7 +312,7 @@ impl<'a> FnSpec<'a> { } else if let Some(deprecated_args) = deprecated_args { FunctionSignature::from_arguments_and_deprecated_args(arguments, deprecated_args)? } else { - FunctionSignature::from_arguments(arguments) + FunctionSignature::from_arguments(arguments, &mut deprecations) }; let text_signature_string = match &fn_type { diff --git a/pyo3-macros-backend/src/pyfunction.rs b/pyo3-macros-backend/src/pyfunction.rs index 6ce47409..199dda73 100644 --- a/pyo3-macros-backend/src/pyfunction.rs +++ b/pyo3-macros-backend/src/pyfunction.rs @@ -366,7 +366,7 @@ pub fn impl_wrap_pyfunction( deprecated_args, signature, text_signature, - deprecations, + mut deprecations, krate, } = options; @@ -404,7 +404,7 @@ pub fn impl_wrap_pyfunction( } else if let Some(deprecated_args) = deprecated_args { FunctionSignature::from_arguments_and_deprecated_args(arguments, deprecated_args)? } else { - FunctionSignature::from_arguments(arguments) + FunctionSignature::from_arguments(arguments, &mut deprecations) }; let ty = method::get_return_info(&func.sig.output); diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs index 0e4e3b2b..3a852745 100644 --- a/pyo3-macros-backend/src/pyfunction/signature.rs +++ b/pyo3-macros-backend/src/pyfunction/signature.rs @@ -12,6 +12,7 @@ use syn::{ use crate::{ attributes::{kw, KeywordAttribute}, + deprecations::{Deprecation, Deprecations}, method::{FnArg, FnType}, pyfunction::Argument, }; @@ -530,7 +531,7 @@ impl<'a> FunctionSignature<'a> { } /// Without `#[pyo3(signature)]` or `#[args]` - just take the Rust function arguments as positional. - pub fn from_arguments(mut arguments: Vec>) -> Self { + pub fn from_arguments(mut arguments: Vec>, deprecations: &mut Deprecations) -> Self { let mut python_signature = PythonSignature::default(); for arg in &arguments { // Python<'_> arguments don't show in Python signature @@ -540,6 +541,13 @@ impl<'a> FunctionSignature<'a> { if arg.optional.is_none() { // This argument is required + if python_signature.required_positional_parameters + != python_signature.positional_parameters.len() + { + // A previous argument was not required + deprecations.push(Deprecation::RequiredArgumentAfterOption, arg.name.span()); + } + python_signature.required_positional_parameters = python_signature.positional_parameters.len() + 1; } diff --git a/pytests/src/datetime.rs b/pytests/src/datetime.rs index ba48705d..da5e5a4c 100644 --- a/pytests/src/datetime.rs +++ b/pytests/src/datetime.rs @@ -37,6 +37,7 @@ fn make_time<'p>( } #[pyfunction] +#[pyo3(signature = (hour, minute, second, microsecond, tzinfo, fold))] fn time_with_fold<'p>( py: Python<'p>, hour: u8, diff --git a/src/impl_/deprecations.rs b/src/impl_/deprecations.rs index 1cda64d4..901bea50 100644 --- a/src/impl_/deprecations.rs +++ b/src/impl_/deprecations.rs @@ -11,3 +11,9 @@ pub const PYFUNCTION_ARGUMENTS: () = (); note = "the `#[args]` attribute for `#[methods]` is being replaced by `#[pyo3(signature)]`" )] pub const PYMETHODS_ARGS_ATTRIBUTE: () = (); + +#[deprecated( + since = "0.18.0", + note = "required arguments after an `Option<_>` argument are ambiguous and being phased out\n= help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters" +)] +pub const REQUIRED_ARGUMENT_AFTER_OPTION: () = (); diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index ec7afb46..46403da9 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -477,6 +477,7 @@ fn use_pyfunction() { } #[test] +#[allow(deprecated)] fn required_argument_after_option() { #[pyfunction] pub fn foo(x: Option, y: i32) -> i32 { diff --git a/tests/test_text_signature.rs b/tests/test_text_signature.rs index 63ed8161..da40add7 100644 --- a/tests/test_text_signature.rs +++ b/tests/test_text_signature.rs @@ -121,12 +121,12 @@ fn test_function() { #[test] fn test_auto_test_signature_function() { #[pyfunction] - fn my_function(a: i32, b: Option, c: i32) { + fn my_function(a: i32, b: i32, c: i32) { let _ = (a, b, c); } #[pyfunction(pass_module)] - fn my_function_2(module: &PyModule, a: i32, b: Option, c: i32) { + fn my_function_2(module: &PyModule, a: i32, b: i32, c: i32) { let _ = (module, a, b, c); } @@ -173,7 +173,7 @@ fn test_auto_test_signature_method() { #[pymethods] impl MyClass { - fn method(&self, a: i32, b: Option, c: i32) { + fn method(&self, a: i32, b: i32, c: i32) { let _ = (a, b, c); } @@ -196,12 +196,12 @@ fn test_auto_test_signature_method() { } #[staticmethod] - fn staticmethod(a: i32, b: Option, c: i32) { + fn staticmethod(a: i32, b: i32, c: i32) { let _ = (a, b, c); } #[classmethod] - fn classmethod(cls: &PyType, a: i32, b: Option, c: i32) { + fn classmethod(cls: &PyType, a: i32, b: i32, c: i32) { let _ = (cls, a, b, c); } } @@ -239,7 +239,7 @@ fn test_auto_test_signature_method() { #[test] fn test_auto_test_signature_opt_out() { #[pyfunction(text_signature = None)] - fn my_function(a: i32, b: Option, c: i32) { + fn my_function(a: i32, b: i32, c: i32) { let _ = (a, b, c); } @@ -254,7 +254,7 @@ fn test_auto_test_signature_opt_out() { #[pymethods] impl MyClass { #[pyo3(text_signature = None)] - fn method(&self, a: i32, b: Option, c: i32) { + fn method(&self, a: i32, b: i32, c: i32) { let _ = (a, b, c); } @@ -265,13 +265,13 @@ fn test_auto_test_signature_opt_out() { #[staticmethod] #[pyo3(text_signature = None)] - fn staticmethod(a: i32, b: Option, c: i32) { + fn staticmethod(a: i32, b: i32, c: i32) { let _ = (a, b, c); } #[classmethod] #[pyo3(text_signature = None)] - fn classmethod(cls: &PyType, a: i32, b: Option, c: i32) { + fn classmethod(cls: &PyType, a: i32, b: i32, c: i32) { let _ = (cls, a, b, c); } } diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index f5fb7a5d..f36ddf8c 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -5,6 +5,9 @@ use pyo3::prelude::*; #[pyfunction(_opt = "None", x = "5")] fn function_with_args(_opt: Option, _x: i32) {} +#[pyfunction] +fn function_with_required_after_option(_opt: Option, _x: i32) {} + #[pyclass] struct MyClass; @@ -12,8 +15,12 @@ struct MyClass; impl MyClass { #[args(_opt = "None", x = "5")] fn function_with_args(&self, _opt: Option, _x: i32) {} + + #[args(_has_default = 1)] + fn default_arg_before_required_deprecated(&self, _has_default: isize, _required: isize) {} } fn main() { + function_with_required_after_option(None, 0); function_with_args(None, 0); } diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index bbe16515..e99f8b05 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -10,8 +10,21 @@ note: the lint level is defined here 1 | #![deny(deprecated)] | ^^^^^^^^^^ +error: use of deprecated constant `pyo3::impl_::deprecations::REQUIRED_ARGUMENT_AFTER_OPTION`: required arguments after an `Option<_>` argument are ambiguous and being phased out + = help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters + --> tests/ui/deprecations.rs:9:59 + | +9 | fn function_with_required_after_option(_opt: Option, _x: i32) {} + | ^^ + error: use of deprecated constant `pyo3::impl_::deprecations::PYMETHODS_ARGS_ATTRIBUTE`: the `#[args]` attribute for `#[methods]` is being replaced by `#[pyo3(signature)]` - --> tests/ui/deprecations.rs:13:7 + --> tests/ui/deprecations.rs:16:7 | -13 | #[args(_opt = "None", x = "5")] +16 | #[args(_opt = "None", x = "5")] + | ^^^^ + +error: use of deprecated constant `pyo3::impl_::deprecations::PYMETHODS_ARGS_ATTRIBUTE`: the `#[args]` attribute for `#[methods]` is being replaced by `#[pyo3(signature)]` + --> tests/ui/deprecations.rs:19:7 + | +19 | #[args(_has_default = 1)] | ^^^^