Skip to content

Commit

Permalink
deprecate "trailing optional arguments" implicit default behaviour
Browse files Browse the repository at this point in the history
  • Loading branch information
Icxolu committed Apr 14, 2024
1 parent cc7e16f commit 75b8bc2
Show file tree
Hide file tree
Showing 15 changed files with 108 additions and 15 deletions.
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 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
38 changes: 35 additions & 3 deletions pyo3-macros-backend/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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! {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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,
Expand All @@ -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) => {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -309,6 +336,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,
Expand Down
1 change: 1 addition & 0 deletions pyo3-macros-backend/src/pymethod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,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,
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
10 changes: 10 additions & 0 deletions src/impl_/deprecations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
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 @@ -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>,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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<'_>,
Expand All @@ -1042,6 +1047,7 @@ issue_1506!(
) {
}

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

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

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

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

#[classmethod]
#[pyo3(signature = (_arg, _args, _kwargs=None))]
fn issue_1506_class(
_cls: &Bound<'_, PyType>,
_py: Python<'_>,
Expand Down
2 changes: 2 additions & 0 deletions tests/test_pyfunction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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>>,
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
8 changes: 8 additions & 0 deletions tests/ui/deprecations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
30 changes: 18 additions & 12 deletions tests/ui/deprecations.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand Down Expand Up @@ -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)

0 comments on commit 75b8bc2

Please sign in to comment.