makes trailing optional arguments a hard error (see #2935)

Author: Icxolu

guide/src/function/signature.md

@@ -2,7 +2,7 @@
 
 The `#[pyfunction]` attribute also accepts parameters to control how the generated Python function accepts arguments. Just like in Python, arguments can be positional-only, keyword-only, or accept either. `*args` lists and `**kwargs` dicts can also be accepted. These parameters also work for `#[pymethods]` which will be introduced in the [Python Classes](../class.md) section of the guide.
 
-Like Python, by default PyO3 accepts all arguments as either positional or keyword arguments. Most arguments are required by default, except for trailing `Option<_>` arguments, which are [implicitly given a default of `None`](#trailing-optional-arguments). This behaviour can be configured by the `#[pyo3(signature = (...))]` option which allows writing a signature in Python syntax.
+Like Python, by default PyO3 accepts all arguments as either positional or keyword arguments. All arguments are required by default. This behaviour can be configured by the `#[pyo3(signature = (...))]` option which allows writing a signature in Python syntax.
 
 This section of the guide goes into detail about use of the `#[pyo3(signature = (...))]` option and its related option `#[pyo3(text_signature = "...")]`
 
@@ -118,82 +118,6 @@ num=-1
 > }
 > ```
 
-## Trailing optional arguments
-
-<div class="warning">
-
-⚠️ 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.
-</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`.
-///
-/// If `amount` is unspecified or `None`, equivalent to `x + 1`.
-#[pyfunction]
-fn increment(x: u64, amount: Option<u64>) -> 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<T>` 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>) -> 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<T>` argument is surrounded by arguments which aren't `Option<T>`.
-
 ## 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.

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]
@@ -872,7 +872,7 @@ Python::with_gil(|py| -> PyResult<()> {
 <details>
 <summary><small>Click to expand</small></summary>
 
-[Trailing `Option<T>` arguments](./function/signature.md#trailing-optional-arguments) have an automatic default of `None`. To avoid unwanted changes when modifying function signatures, in PyO3 0.18 it was deprecated to have a required argument after an `Option<T>` argument without using `#[pyo3(signature = (...))]` to specify the intended defaults. In PyO3 0.20, this becomes a hard error.
+Trailing `Option<T>` arguments have an automatic default of `None`. To avoid unwanted changes when modifying function signatures, in PyO3 0.18 it was deprecated to have a required argument after an `Option<T>` argument without using `#[pyo3(signature = (...))]` to specify the intended defaults. In PyO3 0.20, this becomes a hard error.
 
 Before:
 
@@ -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::*;
 

newsfragments/4729.changed.md

@@ -0,0 +1 @@
+makes trailing optional arguments a hard error (see #2935)

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<T>` arguments \n\
-             = note: these implicit defaults are being phased out \n\
+            "this function uses trailing `Option<T>` 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(())
     }
 }

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

pyo3-macros-backend/src/params.rs

@@ -245,10 +245,7 @@ pub(crate) fn impl_regular_arg_param(
     // Option<T> 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());

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

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

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<T>` 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<T>` 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<i32>) {}
    |    ^^^^^^^^^^^^^^^^^^^
-   |
-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<T>` 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<T>` 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<i32>, _foo: Option<String>) {}
    |    ^^^^^^^^^^^^^^^^^^^
 
-error: use of deprecated constant `__pyfunction_pyfunction_option_4::SIGNATURE`: this function has implicit defaults for the trailing `Option<T>` 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<T>` 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)

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<i32>, _x: i32) {}
-
 #[pyfunction]
 #[pyo3(signature=(*args))]
 fn function_with_optional_args(args: Option<Bound<'_, PyTuple>>) {