clean up handling of default values in function signatures (#5677)

davidhewitt · web-flow · commit e8e5a504dca1 · 2025-12-05T20:33:38.000Z
* clean up handling of default values in function signatures

* fixups

* ensure trailing optional argument required
diff --git a/pyo3-macros-backend/src/introspection.rs b/pyo3-macros-backend/src/introspection.rs
@@ -11,7 +11,7 @@
 use crate::method::{FnArg, RegularArg};
 use crate::pyfunction::FunctionSignature;
 use crate::type_hint::PythonTypeHint;
-use crate::utils::PyO3CratePath;
+use crate::utils::{expr_to_python, PyO3CratePath};
 use proc_macro2::{Span, TokenStream};
 use quote::{format_ident, quote, ToTokens};
 use std::borrow::Cow;
@@ -291,10 +291,10 @@ fn argument_introspection_data<'a>(
     class_type: Option<&Type>,
 ) -> AttributedIntrospectionNode<'a> {
     let mut params: HashMap<_, _> = [("name", IntrospectionNode::String(name.into()))].into();
-    if desc.default_value.is_some() {
+    if let Some(expr) = &desc.default_value {
         params.insert(
             "default",
-            IntrospectionNode::String(desc.default_value().into()),
+            IntrospectionNode::String(expr_to_python(expr).into()),
         );
     }
 
diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs
@@ -9,7 +9,7 @@ use syn::{ext::IdentExt, spanned::Spanned, Ident, Result};
 
 use crate::pyfunction::{PyFunctionWarning, WarningFactory};
 use crate::pyversions::is_abi3_before;
-use crate::utils::{expr_to_python, Ctx};
+use crate::utils::Ctx;
 use crate::{
     attributes::{FromPyWithAttribute, TextSignatureAttribute, TextSignatureAttributeValue},
     params::{impl_arg_params, Holders},
@@ -31,28 +31,6 @@ pub struct RegularArg<'a> {
     pub annotation: Option<String>,
 }
 
-impl RegularArg<'_> {
-    pub fn default_value(&self) -> String {
-        if let Self {
-            default_value: Some(arg_default),
-            ..
-        } = self
-        {
-            expr_to_python(arg_default)
-        } else if let RegularArg {
-            option_wrapped_type: Some(..),
-            ..
-        } = self
-        {
-            // functions without a `#[pyo3(signature = (...))]` option
-            // will treat trailing `Option<T>` arguments as having a default of `None`
-            "None".to_string()
-        } else {
-            "...".to_string()
-        }
-    }
-}
-
 /// Pythons *args argument
 #[derive(Clone, Debug)]
 pub struct VarargsArg<'a> {
@@ -220,14 +198,6 @@ impl<'a> FnArg<'a> {
             }
         }
     }
-
-    pub fn default_value(&self) -> String {
-        if let Self::Regular(args) = self {
-            args.default_value()
-        } else {
-            "...".to_string()
-        }
-    }
 }
 
 fn handle_argument_error(pat: &syn::Pat) -> syn::Error {
diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs
@@ -90,16 +90,17 @@ pub fn impl_arg_params(
 
     let positional_parameter_names = &spec.signature.python_signature.positional_parameters;
     let positional_only_parameters = &spec.signature.python_signature.positional_only_parameters;
-    let required_positional_parameters = &spec
+    let required_positional_parameters = spec
         .signature
         .python_signature
-        .required_positional_parameters;
+        .required_positional_parameters();
     let keyword_only_parameters = spec
         .signature
         .python_signature
         .keyword_only_parameters
         .iter()
-        .map(|(name, required)| {
+        .map(|(name, default_value)| {
+            let required = default_value.is_none();
             quote! {
                 #pyo3_path::impl_::extract_argument::KeywordOnlyParameterDescription {
                     name: #name,
diff --git a/pyo3-macros-backend/src/pyfunction/signature.rs b/pyo3-macros-backend/src/pyfunction/signature.rs
@@ -1,6 +1,7 @@
 use crate::{
     attributes::{kw, KeywordAttribute},
-    method::{FnArg, RegularArg},
+    method::FnArg,
+    utils::expr_to_python,
 };
 use proc_macro2::{Span, TokenStream};
 use quote::ToTokens;
@@ -9,7 +10,7 @@ use syn::{
     parse::{Parse, ParseStream},
     punctuated::Punctuated,
     spanned::Spanned,
-    Token,
+    Expr, Token,
 };
 
 #[derive(Clone)]
@@ -270,10 +271,11 @@ impl ConstructorAttribute {
 pub struct PythonSignature {
     pub positional_parameters: Vec<String>,
     pub positional_only_parameters: usize,
-    pub required_positional_parameters: usize,
+    /// Vector of expressions representing positional defaults
+    pub default_positional_parameters: Vec<Expr>,
     pub varargs: Option<String>,
-    // Tuples of keyword name and whether it is required
-    pub keyword_only_parameters: Vec<(String, bool)>,
+    // Tuples of keyword name and optional default value
+    pub keyword_only_parameters: Vec<(String, Option<Expr>)>,
     pub kwargs: Option<String>,
 }
 
@@ -284,6 +286,13 @@ impl PythonSignature {
             && self.varargs.is_none()
             && self.kwargs.is_none()
     }
+
+    pub fn required_positional_parameters(&self) -> usize {
+        self.positional_parameters
+            .len()
+            .checked_sub(self.default_positional_parameters.len())
+            .expect("should always have positional defaults <= positional parameters")
+    }
 }
 
 #[derive(Clone)]
@@ -309,23 +318,24 @@ impl ParseState {
         &mut self,
         signature: &mut PythonSignature,
         name: String,
-        required: bool,
+        default_value: Option<Expr>,
         span: Span,
     ) -> syn::Result<()> {
         match self {
             ParseState::Positional | ParseState::PositionalAfterPosargs => {
                 signature.positional_parameters.push(name);
-                if required {
-                    signature.required_positional_parameters += 1;
-                    ensure_spanned!(
-                        signature.required_positional_parameters == signature.positional_parameters.len(),
-                        span => "cannot have required positional parameter after an optional parameter"
-                    );
+                if let Some(default_value) = default_value {
+                    signature.default_positional_parameters.push(default_value);
+                    // Now all subsequent positional parameters must also have defaults
+                } else if !signature.default_positional_parameters.is_empty() {
+                    bail_spanned!(span => "cannot have required positional parameter after an optional parameter")
                 }
                 Ok(())
             }
             ParseState::Keywords => {
-                signature.keyword_only_parameters.push((name, required));
+                signature
+                    .keyword_only_parameters
+                    .push((name, default_value));
                 Ok(())
             }
             ParseState::Done => {
@@ -475,7 +485,9 @@ impl<'a> FunctionSignature<'a> {
                     parse_state.add_argument(
                         &mut python_signature,
                         arg.ident.unraw().to_string(),
-                        arg.eq_and_default.is_none(),
+                        arg.eq_and_default
+                            .as_ref()
+                            .map(|(_, default)| default.clone()),
                         arg.span(),
                     )?;
                     let FnArg::Regular(fn_arg) = fn_arg else {
@@ -577,17 +589,6 @@ impl<'a> FunctionSignature<'a> {
                 continue;
             }
 
-            if let FnArg::Regular(RegularArg { .. }) = arg {
-                // This argument is required, all previous arguments must also have been required
-                assert_eq!(
-                    python_signature.required_positional_parameters,
-                    python_signature.positional_parameters.len(),
-                );
-
-                python_signature.required_positional_parameters =
-                    python_signature.positional_parameters.len() + 1;
-            }
-
             python_signature
                 .positional_parameters
                 .push(arg.name().unraw().to_string());
@@ -600,14 +601,6 @@ impl<'a> FunctionSignature<'a> {
         }
     }
 
-    fn default_value_for_parameter(&self, parameter: &str) -> String {
-        if let Some(fn_arg) = self.arguments.iter().find(|arg| arg.name() == parameter) {
-            fn_arg.default_value()
-        } else {
-            "...".to_string()
-        }
-    }
-
     pub fn text_signature(&self, self_argument: Option<&str>) -> String {
         let mut output = String::new();
         output.push('(');
@@ -630,14 +623,19 @@ impl<'a> FunctionSignature<'a> {
 
         let py_sig = &self.python_signature;
 
-        for (i, parameter) in py_sig.positional_parameters.iter().enumerate() {
+        let defaults = std::iter::repeat_n(None, py_sig.required_positional_parameters())
+            .chain(py_sig.default_positional_parameters.iter().map(Some));
+
+        for (i, (parameter, default)) in
+            std::iter::zip(&py_sig.positional_parameters, defaults).enumerate()
+        {
             maybe_push_comma(&mut output);
 
             output.push_str(parameter);
 
-            if i >= py_sig.required_positional_parameters {
+            if let Some(expr) = default {
                 output.push('=');
-                output.push_str(&self.default_value_for_parameter(parameter));
+                output.push_str(&expr_to_python(expr));
             }
 
             if py_sig.positional_only_parameters > 0 && i + 1 == py_sig.positional_only_parameters {
@@ -654,12 +652,12 @@ impl<'a> FunctionSignature<'a> {
             output.push('*');
         }
 
-        for (parameter, required) in &py_sig.keyword_only_parameters {
+        for (parameter, default) in &py_sig.keyword_only_parameters {
             maybe_push_comma(&mut output);
             output.push_str(parameter);
-            if !required {
+            if let Some(expr) = default {
                 output.push('=');
-                output.push_str(&self.default_value_for_parameter(parameter));
+                output.push_str(&expr_to_python(expr));
             }
         }
 
diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs
@@ -48,6 +48,32 @@ fn test_optional_bool() {
     });
 }
 
+#[test]
+fn test_trailing_optional_no_signature() {
+    // Since PyO3 0.24, trailing optional arguments are treated like any other required argument
+    // (previously would get an implicit default of `None`)
+
+    #[pyfunction]
+    fn trailing_optional(x: i32, y: Option<i32>) -> String {
+        format!("x={x:?} y={y:?}")
+    }
+
+    Python::attach(|py| {
+        let f = wrap_pyfunction!(trailing_optional)(py).unwrap();
+
+        py_assert!(py, f, "f(1, 2) == 'x=1 y=Some(2)'");
+        py_assert!(py, f, "f(2, None) == 'x=2 y=None'");
+
+        py_expect_exception!(
+            py,
+            f,
+            "f(3)",
+            PyTypeError,
+            "trailing_optional() missing 1 required positional argument: 'y'"
+        );
+    });
+}
+
 #[pyfunction]
 #[pyo3(signature=(arg))]
 fn required_optional_str(arg: Option<&str>) -> &str {
@@ -582,7 +608,10 @@ fn test_return_value_borrows_from_arguments() {
 
 #[test]
 fn test_some_wrap_arguments() {
-    // https://github.com/PyO3/pyo3/issues/3460
+    // Option<T> arguments get special treatment in pyfunction default values where it's
+    // valid to pass the inner type without wrapping in `Some()`.
+    //
+    // See also https://github.com/PyO3/pyo3/issues/3460
     const NONE: Option<u8> = None;
     #[pyfunction(signature = (a = 1, b = Some(2), c = None, d = NONE))]
     fn some_wrap_arguments(
diff --git a/tests/test_text_signature.rs b/tests/test_text_signature.rs
@@ -146,48 +146,45 @@ fn test_auto_test_signature_function() {
         let _ = (a, b, c);
     }
 
+    #[pyfunction]
+    fn trailing_optional_required(a: i32, b: Option<i32>, c: Option<i32>) {
+        // Since PyO3 0.24, trailing optional arguments are treated like any other required argument
+        // (previously would get an implicit default of `None`)
+        let _ = (a, b, c);
+    }
+
+    macro_rules! assert_text_signature {
+        ($py:expr, $func:ident, $expected:expr) => {
+            assert_eq!(
+                wrap_pyfunction!($func, $py)
+                    .unwrap()
+                    .getattr("__text_signature__")
+                    .unwrap()
+                    .cast_into::<pyo3::types::PyString>()
+                    .unwrap(),
+                $expected
+            );
+        };
+    }
+
     Python::attach(|py| {
-        let f = wrap_pyfunction!(my_function)(py).unwrap();
-        py_assert!(
-            py,
-            f,
-            "f.__text_signature__ == '(a, b, c)', f.__text_signature__"
-        );
+        assert_text_signature!(py, my_function, "(a, b, c)");
 
-        let f = wrap_pyfunction!(my_function_2)(py).unwrap();
-        py_assert!(
-            py,
-            f,
-            "f.__text_signature__ == '($module, a, b, c)', f.__text_signature__"
-        );
+        assert_text_signature!(py, my_function_2, "($module, a, b, c)");
 
-        let f = wrap_pyfunction!(my_function_3)(py).unwrap();
-        py_assert!(
-            py,
-            f,
-            "f.__text_signature__ == '(a, /, b=None, *, c=5)', f.__text_signature__"
-        );
+        assert_text_signature!(py, my_function_3, "(a, /, b=None, *, c=5)");
 
-        let f = wrap_pyfunction!(my_function_4)(py).unwrap();
-        py_assert!(
-            py,
-            f,
-            "f.__text_signature__ == '(a, /, b=None, *args, c, d=5, **kwargs)', f.__text_signature__"
-        );
+        assert_text_signature!(py, my_function_4, "(a, /, b=None, *args, c, d=5, **kwargs)");
 
-        let f = wrap_pyfunction!(my_function_5)(py).unwrap();
-        py_assert!(
+        assert_text_signature!(
             py,
-            f,
-            "f.__text_signature__ == '(a=1, /, b=None, c=1.5, d=5, e=\"pyo3\", f=\\'f\\', h=True)', f.__text_signature__"
+            my_function_5,
+            "(a=1, /, b=None, c=1.5, d=5, e=\"pyo3\", f='f', h=True)"
         );
 
-        let f = wrap_pyfunction!(my_function_6)(py).unwrap();
-        py_assert!(
-            py,
-            f,
-            "f.__text_signature__ == '(a, b=None, c=None)', f.__text_signature__"
-        );
+        assert_text_signature!(py, my_function_6, "(a, b=None, c=None)");
+
+        assert_text_signature!(py, trailing_optional_required, "(a, b, c)");
     });
 }
 
