Skip to content

Commit

Permalink
makes trailing optional arguments a hard error (see PyO3#2935)
Browse files Browse the repository at this point in the history
  • Loading branch information
Icxolu committed Nov 26, 2024
1 parent 0fb3623 commit a1ad577
Show file tree
Hide file tree
Showing 15 changed files with 60 additions and 265 deletions.
78 changes: 1 addition & 77 deletions guide/src/function/signature.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 = "...")]`

Expand Down Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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::*;
Expand Down
1 change: 1 addition & 0 deletions newsfragments/4729.removed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
removes implicit default of trailing optional arguments (see #2935)
54 changes: 0 additions & 54 deletions pyo3-macros-backend/src/deprecations.rs

This file was deleted.

1 change: 0 additions & 1 deletion pyo3-macros-backend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
mod utils;

mod attributes;
mod deprecations;
mod frompyobject;
mod intopyobject;
mod konst;
Expand Down
9 changes: 1 addition & 8 deletions pyo3-macros-backend/src/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use proc_macro2::{Span, TokenStream};
use quote::{format_ident, quote, quote_spanned, ToTokens};
use syn::{ext::IdentExt, spanned::Spanned, Ident, Result};

use crate::deprecations::deprecate_trailing_option_default;
use crate::pyversions::is_abi3_before;
use crate::utils::{Ctx, LitCStr};
use crate::{
Expand Down Expand Up @@ -474,7 +473,7 @@ impl<'a> FnSpec<'a> {
let signature = if let Some(signature) = signature {
FunctionSignature::from_arguments_and_attribute(arguments, signature)?
} else {
FunctionSignature::from_arguments(arguments)?
FunctionSignature::from_arguments(arguments)
};

let convention = if matches!(fn_type, FnType::FnNew | FnType::FnNewClass(_)) {
Expand Down Expand Up @@ -745,8 +744,6 @@ impl<'a> FnSpec<'a> {
quote!(#func_name)
};

let deprecation = deprecate_trailing_option_default(self);

Ok(match self.convention {
CallingConvention::Noargs => {
let mut holders = Holders::new();
Expand All @@ -767,7 +764,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;
Expand All @@ -789,7 +785,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
Expand All @@ -811,7 +806,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
Expand All @@ -836,7 +830,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
Expand Down
52 changes: 25 additions & 27 deletions pyo3-macros-backend/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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());
Expand Down
6 changes: 3 additions & 3 deletions pyo3-macros-backend/src/pyclass.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1660,7 +1660,7 @@ fn complex_enum_struct_variant_new<'a>(
constructor.into_signature(),
)?
} else {
crate::pyfunction::FunctionSignature::from_arguments(args)?
crate::pyfunction::FunctionSignature::from_arguments(args)
};

let spec = FnSpec {
Expand Down Expand Up @@ -1714,7 +1714,7 @@ fn complex_enum_tuple_variant_new<'a>(
constructor.into_signature(),
)?
} else {
crate::pyfunction::FunctionSignature::from_arguments(args)?
crate::pyfunction::FunctionSignature::from_arguments(args)
};

let spec = FnSpec {
Expand All @@ -1737,7 +1737,7 @@ fn complex_enum_variant_field_getter<'a>(
field_span: Span,
ctx: &Ctx,
) -> Result<MethodAndMethodDef> {
let signature = crate::pyfunction::FunctionSignature::from_arguments(vec![])?;
let signature = crate::pyfunction::FunctionSignature::from_arguments(vec![]);

let self_type = crate::method::SelfType::TryFromBoundRef(field_span);

Expand Down
2 changes: 1 addition & 1 deletion pyo3-macros-backend/src/pyfunction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ pub fn impl_wrap_pyfunction(
let signature = if let Some(signature) = signature {
FunctionSignature::from_arguments_and_attribute(arguments, signature)?
} else {
FunctionSignature::from_arguments(arguments)?
FunctionSignature::from_arguments(arguments)
};

let spec = method::FnSpec {
Expand Down
20 changes: 7 additions & 13 deletions pyo3-macros-backend/src/pyfunction/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,25 +459,19 @@ impl<'a> FunctionSignature<'a> {
}

/// Without `#[pyo3(signature)]` or `#[args]` - just take the Rust function arguments as positional.
pub fn from_arguments(arguments: Vec<FnArg<'a>>) -> syn::Result<Self> {
pub fn from_arguments(arguments: Vec<FnArg<'a>>) -> Self {
let mut python_signature = PythonSignature::default();
for arg in &arguments {
// Python<'_> arguments don't show in Python signature
if matches!(arg, FnArg::Py(..) | FnArg::CancelHandle(..)) {
continue;
}

if let FnArg::Regular(RegularArg {
ty,
option_wrapped_type: None,
..
}) = arg
{
if let FnArg::Regular(RegularArg { .. }) = 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(),
ty.span() => "required arguments after an `Option<_>` argument are ambiguous\n\
= help: add a `#[pyo3(signature)]` annotation on this function to unambiguously specify the default values for all optional parameters"
assert_eq!(
python_signature.required_positional_parameters,
python_signature.positional_parameters.len(),
);

python_signature.required_positional_parameters =
Expand All @@ -489,11 +483,11 @@ impl<'a> FunctionSignature<'a> {
.push(arg.name().unraw().to_string());
}

Ok(Self {
Self {
arguments,
python_signature,
attribute: None,
})
}
}

fn default_value_for_parameter(&self, parameter: &str) -> String {
Expand Down
Loading

0 comments on commit a1ad577

Please sign in to comment.