Merge #2703

2703: deprecate required argument after `Option<T>` without signature r=davidhewitt a=davidhewitt

This PR is a follow-up to #2702 to make required arguments after `Option<T>` arguments require a `#[pyo3(signature)]` annotation to remove the possible ambiguity on whether the developer actually wanted to have a required option (which is what PyO3 is currently forced to assume).

Co-authored-by: David Hewitt <[email protected]>

Author: bors[bot]

newsfragments/2703.changed.md

@@ -0,0 +1 @@
+Deprecate required arguments after `Option<T>` arguments to `#[pyfunction]` and `#[pymethods]` without also using `#[pyo3(signature)]` to specify whether the arguments should be required or have defaults.

pyo3-macros-backend/src/deprecations.rs

@@ -1,18 +1,18 @@
 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 {
     fn ident(&self, span: Span) -> syn::Ident {
         let string = match self {
             Deprecation::PyFunctionArguments => "PYFUNCTION_ARGUMENTS",
             Deprecation::PyMethodArgsAttribute => "PYMETHODS_ARGS_ATTRIBUTE",
+            Deprecation::RequiredArgumentAfterOption => "REQUIRED_ARGUMENT_AFTER_OPTION",
         };
         syn::Ident::new(string, span)
     }

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 {

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);

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<FnArg<'a>>) -> Self {
+    pub fn from_arguments(mut arguments: Vec<FnArg<'a>>, 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;
             }

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,

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: () = ();

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<i32>, y: i32) -> i32 {

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<i32>, 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<i32>, 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<i32>, 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<i32>, c: i32) {
+        fn staticmethod(a: i32, b: i32, c: i32) {
             let _ = (a, b, c);
         }
 
         #[classmethod]
-        fn classmethod(cls: &PyType, a: i32, b: Option<i32>, 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<i32>, 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<i32>, 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<i32>, 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<i32>, c: i32) {
+        fn classmethod(cls: &PyType, a: i32, b: i32, c: i32) {
             let _ = (cls, a, b, c);
         }
     }

tests/ui/deprecations.rs

@@ -5,15 +5,22 @@ use pyo3::prelude::*;
 #[pyfunction(_opt = "None", x = "5")]
 fn function_with_args(_opt: Option<i32>, _x: i32) {}
 
+#[pyfunction]
+fn function_with_required_after_option(_opt: Option<i32>, _x: i32) {}
+
 #[pyclass]
 struct MyClass;
 
 #[pymethods]
 impl MyClass {
     #[args(_opt = "None", x = "5")]
     fn function_with_args(&self, _opt: Option<i32>, _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);
 }