deprecate "trailing optional arguments" implicit default behaviour

Author: Icxolu

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<PyObject>) -> Option<PyObject> {
     let (tx, rx) = oneshot::channel();
     thread::spawn(move || {

guide/src/function/signature.md

@@ -121,9 +121,22 @@ num=-1
 
 ## Trailing optional arguments
 
+<div class="warning">
+
+⚠️ 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.
+</div>
+
+
 As a convenience, functions without a `#[pyo3(signature = (...))]` option will treat trailing `Option<T>` 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`.

pyo3-macros-backend/src/params.rs

@@ -108,7 +108,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! {
@@ -148,7 +157,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() {
@@ -211,6 +229,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,
@@ -224,7 +243,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) => {
@@ -259,6 +285,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,
@@ -315,6 +342,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,

pyo3-macros-backend/src/pymethod.rs

@@ -614,6 +614,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,

pytests/src/datetime.rs

@@ -25,6 +25,7 @@ fn date_from_timestamp(py: Python<'_>, timestamp: i64) -> PyResult<Bound<'_, PyD
 }
 
 #[pyfunction]
+#[pyo3(signature=(hour, minute, second, microsecond, tzinfo=None))]
 fn make_time<'py>(
     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,

src/impl_/deprecations.rs

@@ -85,3 +85,13 @@ impl<T> std::ops::Deref for OptionGilRefs<T> {
         &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 {
+    t
+}

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>) -> u32 {
         0
     }

tests/test_arithmetics.rs

@@ -35,6 +35,7 @@ impl UnaryArithmetic {
         Self::new(self.inner.abs())
     }
 
+    #[pyo3(signature=(_ndigits=None))]
     fn __round__(&self, _ndigits: Option<u32>) -> Self {
         Self::new(self.inner.round())
     }

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<Self> {
         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<PyObject>) -> Option<PyObject> {
         self.index
             .get(key)

tests/test_methods.rs

@@ -194,6 +194,7 @@ impl MethSignature {
     fn get_optional2(&self, test: Option<i32>) -> Option<i32> {
         test
     }
+    #[pyo3(signature=(_t1 = None, t2 = None, _t3 = None))]
     fn get_optional_positional(
         &self,
         _t1: Option<i32>,
@@ -752,11 +753,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;
@@ -858,6 +861,7 @@ struct FromSequence {
 #[pymethods]
 impl FromSequence {
     #[new]
+    #[pyo3(signature=(seq = None))]
     fn new(seq: Option<&Bound<'_, PySequence>>) -> PyResult<Self> {
         if let Some(seq) = seq {
             Ok(FromSequence {
@@ -1033,6 +1037,7 @@ macro_rules! issue_1506 {
 issue_1506!(
     #[pymethods]
     impl Issue1506 {
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506(
             &self,
             _py: Python<'_>,
@@ -1042,6 +1047,7 @@ issue_1506!(
         ) {
         }
 
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506_mut(
             &mut self,
             _py: Python<'_>,
@@ -1051,6 +1057,7 @@ issue_1506!(
         ) {
         }
 
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506_custom_receiver(
             _slf: Py<Self>,
             _py: Python<'_>,
@@ -1060,6 +1067,7 @@ issue_1506!(
         ) {
         }
 
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506_custom_receiver_explicit(
             _slf: Py<Issue1506>,
             _py: Python<'_>,
@@ -1070,6 +1078,7 @@ issue_1506!(
         }
 
         #[new]
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506_new(
             _py: Python<'_>,
             _arg: &Bound<'_, PyAny>,
@@ -1088,6 +1097,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>,
@@ -1097,6 +1107,7 @@ issue_1506!(
         }
 
         #[classmethod]
+        #[pyo3(signature = (_arg, _args, _kwargs=None))]
         fn issue_1506_class(
             _cls: &Bound<'_, PyType>,
             _py: Python<'_>,

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>>,

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<Self> {
         if let Some(pylist) = elements {
             let mut elems = Vec::with_capacity(pylist.len());

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<i32>, c: Option<i32>) {
         let _ = (a, b, c);
     }

tests/ui/deprecations.rs

@@ -108,8 +108,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<i32>) {}
+
+#[pyfunction]
+fn pyfunction_option_2(_i: u32, _any: Option<i32>) {}
+
 #[derive(Debug, FromPyObject)]
 pub struct Zap {
     #[pyo3(item)]

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:119:39
+    |
+119 | fn pyfunction_option_2(_i: u32, _any: Option<i32>) {}
+    |                                       ^^^^^^
+
 error: use of deprecated struct `pyo3::PyCell`: `PyCell` was merged into `Bound`, use that instead; see the migration guide for more info
   --> tests/ui/deprecations.rs:23:30
    |
@@ -77,39 +83,39 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::function_arg`
     |                             ^
 
 error: use of deprecated method `pyo3::deprecations::OptionGilRefs::<std::option::Option<T>>::function_arg`: use `Option<&Bound<'_, T>>` instead for this function argument
-   --> tests/ui/deprecations.rs:111:36
+   --> tests/ui/deprecations.rs:112:36
     |
-111 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {}
+112 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {}
     |                                    ^^^^^^
 
 error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
-   --> tests/ui/deprecations.rs:118:27
+   --> tests/ui/deprecations.rs:126:27
     |
-118 |     #[pyo3(from_py_with = "PyAny::len", item("my_object"))]
+126 |     #[pyo3(from_py_with = "PyAny::len", item("my_object"))]
     |                           ^^^^^^^^^^^^
 
 error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
-   --> tests/ui/deprecations.rs:128:27
+   --> tests/ui/deprecations.rs:136:27
     |
-128 |     #[pyo3(from_py_with = "PyAny::len")] usize,
+136 |     #[pyo3(from_py_with = "PyAny::len")] usize,
     |                           ^^^^^^^^^^^^
 
 error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
-   --> tests/ui/deprecations.rs:134:31
+   --> tests/ui/deprecations.rs:142:31
     |
-134 |     Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32),
+142 |     Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32),
     |                               ^^^^^^^^^^^^^^^^^
 
 error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
-   --> tests/ui/deprecations.rs:141:27
+   --> tests/ui/deprecations.rs:149:27
     |
-141 |     #[pyo3(from_py_with = "extract_gil_ref")]
+149 |     #[pyo3(from_py_with = "extract_gil_ref")]
     |                           ^^^^^^^^^^^^^^^^^
 
 error: use of deprecated method `pyo3::deprecations::GilRefs::<pyo3::Python<'_>>::is_python`: use `wrap_pyfunction_bound!` instead
-   --> tests/ui/deprecations.rs:154:13
+   --> tests/ui/deprecations.rs:162:13
     |
-154 |     let _ = wrap_pyfunction!(double, py);
+162 |     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)