Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deprecate implicit default for trailing optional arguments #4078

Merged
merged 4 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions guide/src/async-await.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 || {
Expand Down
13 changes: 13 additions & 0 deletions guide/src/function/signature.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,22 @@ 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`.
Expand Down
35 changes: 35 additions & 0 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,41 @@
This guide can help you upgrade code through breaking changes from one PyO3 version to the next.
For a detailed list of all changes, see the [CHANGELOG](changelog.md).

## from 0.21.* to 0.22

### Deprecation of implicit default for trailing optional arguments
<details open>
<summary><small>Click to expand</small></summary>

With `pyo3` 0.22 the implicit `None` default for trailing `Option<T>` type argument is deprecated. To migrate, place a `#[pyo3(signature = (...))]` attribute on affected functions or methods and specify the desired behavior.
The migration warning specifies the corresponding signature to keep the current behavior. With 0.23 the signature will be required for any function containing `Option<T>` type parameters to prevent accidental
and unnoticed changes in behavior. With 0.24 this restriction will be lifted again and `Option<T>` type arguments will be treated as any other argument _without_ special handling.

Icxolu marked this conversation as resolved.
Show resolved Hide resolved
Before:

```rust
# #![allow(deprecated, dead_code)]
# use pyo3::prelude::*;
#[pyfunction]
fn increment(x: u64, amount: Option<u64>) -> u64 {
x + amount.unwrap_or(1)
}
```

After:

```rust
# #![allow(dead_code)]
# use pyo3::prelude::*;
#[pyfunction]
#[pyo3(signature = (x, amount=None))]
fn increment(x: u64, amount: Option<u64>) -> u64 {
x + amount.unwrap_or(1)
}
```

</details>

## from 0.20.* to 0.21
<details open>
<summary><small>Click to expand</small></summary>
Expand Down
1 change: 1 addition & 0 deletions newsfragments/4078.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
deprecate implicit default for trailing optional arguments
53 changes: 52 additions & 1 deletion pyo3-macros-backend/src/deprecations.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::utils::Ctx;
use crate::{
method::{FnArg, FnSpec},
utils::Ctx,
};
use proc_macro2::{Span, TokenStream};
use quote::{quote_spanned, ToTokens};

Expand Down Expand Up @@ -45,3 +48,51 @@ impl<'ctx> ToTokens for Deprecations<'ctx> {
}
}
}

pub(crate) fn deprecate_trailing_option_default(spec: &FnSpec<'_>) -> TokenStream {
if spec.signature.attribute.is_none()
&& spec.signature.arguments.iter().any(|arg| {
if let FnArg::Regular(arg) = arg {
arg.option_wrapped_type.is_some()
} else {
false
}
})
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved
{
use std::fmt::Write;
let mut deprecation_msg = String::from(
"This function has implicit defaults for the trailing `Option<T>` arguments. \
These implicit defaults are being phased out. Add `#[pyo3(signature = (",
);
spec.signature.arguments.iter().for_each(|arg| {
match arg {
FnArg::Regular(arg) => {
if arg.option_wrapped_type.is_some() {
write!(deprecation_msg, "{}=None, ", arg.name)
} else {
write!(deprecation_msg, "{}, ", arg.name)
}
}
FnArg::VarArgs(arg) => write!(deprecation_msg, "{}, ", arg.name),
FnArg::KwArgs(arg) => write!(deprecation_msg, "{}, ", arg.name),
FnArg::Py(_) | FnArg::CancelHandle(_) => Ok(()),
}
.expect("writing to `String` should not fail");
});

//remove trailing space and comma
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;
}
} else {
TokenStream::new()
}
}
7 changes: 7 additions & 0 deletions pyo3-macros-backend/src/method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ 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::utils::Ctx;
use crate::{
attributes::{FromPyWithAttribute, TextSignatureAttribute, TextSignatureAttributeValue},
Expand Down Expand Up @@ -708,6 +709,8 @@ 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 @@ -730,6 +733,7 @@ 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 _slf_ref = &_slf;
let function = #rust_name; // Shadow the function name to avoid #3017
#init_holders
Expand All @@ -754,6 +758,7 @@ 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 _slf_ref = &_slf;
let function = #rust_name; // Shadow the function name to avoid #3017
#arg_convert
Expand All @@ -778,6 +783,7 @@ 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 _slf_ref = &_slf;
let function = #rust_name; // Shadow the function name to avoid #3017
#arg_convert
Expand Down Expand Up @@ -805,6 +811,7 @@ impl<'a> FnSpec<'a> {
_kwargs: *mut #pyo3_path::ffi::PyObject
) -> #pyo3_path::PyResult<*mut #pyo3_path::ffi::PyObject> {
use #pyo3_path::callback::IntoPyCallbackOutput;
#deprecation
let _slf_ref = &_slf;
let function = #rust_name; // Shadow the function name to avoid #3017
#arg_convert
Expand Down
4 changes: 4 additions & 0 deletions pyo3-macros-backend/src/pymethod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::borrow::Cow;

use crate::attributes::{NameAttribute, RenamingRule};
use crate::deprecations::deprecate_trailing_option_default;
use crate::method::{CallingConvention, ExtractErrorMode, PyArg};
use crate::params::{check_arg_for_gil_refs, impl_regular_arg_param, Holders};
use crate::utils::Ctx;
Expand Down Expand Up @@ -637,7 +638,10 @@ pub fn impl_py_setter_def(
);
let extract =
check_arg_for_gil_refs(tokens, holders.push_gil_refs_checker(arg.ty.span()), ctx);

let deprecation = deprecate_trailing_option_default(spec);
quote! {
#deprecation
#from_py_with
let _val = #extract;
}
Expand Down
3 changes: 3 additions & 0 deletions pytests/src/datetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/tests/hygiene/pymethods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ impl Dummy {
0
}

#[pyo3(signature=(ndigits=::std::option::Option::None))]
fn __round__(&self, ndigits: ::std::option::Option<u32>) -> u32 {
0
}
Expand Down
1 change: 1 addition & 0 deletions tests/test_arithmetics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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())
}
Expand Down
2 changes: 2 additions & 0 deletions tests/test_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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)
Expand Down
11 changes: 11 additions & 0 deletions tests/test_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,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>,
Expand Down Expand Up @@ -745,11 +746,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;
Expand Down Expand Up @@ -851,6 +854,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 {
Expand Down Expand Up @@ -1026,6 +1030,7 @@ macro_rules! issue_1506 {
issue_1506!(
#[pymethods]
impl Issue1506 {
#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506(
&self,
_py: Python<'_>,
Expand All @@ -1035,6 +1040,7 @@ issue_1506!(
) {
}

#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_mut(
&mut self,
_py: Python<'_>,
Expand All @@ -1044,6 +1050,7 @@ issue_1506!(
) {
}

#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_custom_receiver(
_slf: Py<Self>,
_py: Python<'_>,
Expand All @@ -1053,6 +1060,7 @@ issue_1506!(
) {
}

#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_custom_receiver_explicit(
_slf: Py<Issue1506>,
_py: Python<'_>,
Expand All @@ -1063,6 +1071,7 @@ issue_1506!(
}

#[new]
#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_new(
_py: Python<'_>,
_arg: &Bound<'_, PyAny>,
Expand All @@ -1081,6 +1090,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>,
Expand All @@ -1090,6 +1100,7 @@ issue_1506!(
}

#[classmethod]
#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_class(
_cls: &Bound<'_, PyType>,
_py: Python<'_>,
Expand Down
3 changes: 3 additions & 0 deletions tests/test_pyfunction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ fn test_from_py_with_defaults() {

// issue 2280 combination of from_py_with and Option<T> did not compile
#[pyfunction]
#[pyo3(signature = (int=None))]
fn from_py_with_option(#[pyo3(from_py_with = "optional_int")] int: Option<i32>) -> i32 {
int.unwrap_or(0)
}
Expand Down Expand Up @@ -216,6 +217,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,
Expand Down Expand Up @@ -542,6 +544,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>>,
Expand Down
1 change: 1 addition & 0 deletions tests/test_sequence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
1 change: 1 addition & 0 deletions tests/test_text_signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down