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.

Sign up for GitHub

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

Jump to bottom

pyfunction: fix compile error for Option<&T> argument with a default #2630

Merged
merged 1 commit into from Sep 21, 2022
Merged

pyfunction: fix compile error for Option<&T> argument with a default #2630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 newsfragments/2630.fixed.md
View file
Open in desktop
@@ -0,0 +1 @@
Fix compile error since 0.17.0 with `Option<&SomePyClass>` argument with a default.
116 changes: 49 additions & 67 deletions pyo3-macros-backend/src/params.rs
View file
Open in desktop
@@ -1,7 +1,6 @@
// Copyright (c) 2017-present PyO3 Project and Contributors

use crate::{
attributes::FromPyWithAttribute,
method::{FnArg, FnSpec},
pyfunction::Argument,
};
Expand Down Expand Up @@ -222,84 +221,67 @@ fn impl_arg_param(
_pyo3::impl_::extract_argument::extract_optional_argument(
_kwargs.map(::std::convert::AsRef::as_ref),
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str
#name_str,
|| None
)?
});
}

let arg_value = quote_arg_span!(#args_array[#option_pos]);
*option_pos += 1;

let tokens = if let Some(FromPyWithAttribute {
value: expr_path, ..
}) = &arg.attrs.from_py_with
{
match (spec.default_value(name), arg.optional.is_some()) {
(Some(default), true) if default.to_string() != "None" => {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || Some(#default))?
}
}
(Some(default), _) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || #default)?
}
let mut default = spec.default_value(name);

// Option<T> arguments have special treatment: the default should be specified _without_ the
// Some() wrapper. Maybe this should be changed in future?!
if arg.optional.is_some() {
default = Some(match &default {
Some(expression) if expression.to_string() != "None" => {
quote!(::std::option::Option::Some(#expression))
}
(None, true) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || None)?
}
_ => quote!(::std::option::Option::None),
})
}

let tokens = if let Some(expr_path) = arg.attrs.from_py_with.as_ref().map(|attr| &attr.value) {
if let Some(default) = default {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with_with_default(#arg_value, #name_str, #expr_path, || #default)?
}
(None, false) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with(
_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value),
#name_str,
#expr_path,
)?
}
} else {
quote_arg_span! {
_pyo3::impl_::extract_argument::from_py_with(
_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value),
#name_str,
#expr_path,
)?
}
}
} else if arg.optional.is_some() {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_optional_argument(
#arg_value,
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str,
|| #default
)?
}
} else if let Some(default) = default {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_argument_with_default(
#arg_value,
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str,
|| #default
)?
}
} else {
match (spec.default_value(name), arg.optional.is_some()) {
(Some(default), true) if default.to_string() != "None" => {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_argument_with_default(
#arg_value,
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str,
|| Some(#default)
)?
}
}
(Some(default), _) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_argument_with_default(
#arg_value,
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str,
|| #default
)?
}
}
(None, true) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_optional_argument(
#arg_value,
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str
)?
}
}
(None, false) => {
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_argument(
_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value),
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str
)?
}
}
quote_arg_span! {
_pyo3::impl_::extract_argument::extract_argument(
_pyo3::impl_::extract_argument::unwrap_required_argument(#arg_value),
&mut { _pyo3::impl_::extract_argument::FunctionArgumentHolder::INIT },
#name_str
)?
}
};
Ok(tokens)
Expand Down
12 changes: 5 additions & 7 deletions src/impl_/extract_argument.rs
View file
Open in desktop
Expand Up @@ -91,26 +91,28 @@ where
}
}

/// Alternative to [`extract_argument`] used for `Option<T>` arguments (because they are implicitly treated
/// as optional if at the end of the positional parameters).
/// Alternative to [`extract_argument`] used for `Option<T>` arguments. This is necessary because Option<&T>
/// does not implement `PyFunctionArgument` for `T: PyClass`.
#[doc(hidden)]
pub fn extract_optional_argument<'a, 'py, T>(
obj: Option<&'py PyAny>,
holder: &'a mut T::Holder,
arg_name: &str,
default: fn() -> Option<T>,
) -> PyResult<Option<T>>
where
T: PyFunctionArgument<'a, 'py>,
{
match obj {
Some(obj) => {
if obj.is_none() {
// Explicit `None` will result in None being used as the function argument
Ok(None)
} else {
extract_argument(obj, holder, arg_name).map(Some)
}
}
None => Ok(None),
_ => Ok(default()),
}
}

Expand All @@ -132,16 +134,12 @@ where
}

/// Alternative to [`extract_argument`] used when the argument has a `#[pyo3(from_py_with)]` annotation.
///
/// # Safety
/// - `obj` must not be None (this helper is only used for required function arguments).
#[doc(hidden)]
pub fn from_py_with<'py, T>(
obj: &'py PyAny,
arg_name: &str,
extractor: fn(&'py PyAny) -> PyResult<T>,
) -> PyResult<T> {
// Safety: obj is not None (see safety
match extractor(obj) {
Ok(value) => Ok(value),
Err(e) => Err(argument_extraction_error(obj.py(), arg_name, e)),
Expand Down
25 changes: 25 additions & 0 deletions tests/test_methods.rs
View file
Open in desktop
Expand Up @@ -1029,3 +1029,28 @@ pymethods!(
fn issue_1696(&self, _x: &InstanceMethod) {}
}
);

#[test]
fn test_option_pyclass_arg() {
// Option<&PyClass> argument with a default set in a signature regressed to a compile
// error in PyO3 0.17.0 - this test it continues to be accepted.

#[pyclass]
struct SomePyClass {}

#[pyfunction(arg = "None")]
fn option_class_arg(arg: Option<&SomePyClass>) -> Option<SomePyClass> {
arg.map(|_| SomePyClass {})
}

Python::with_gil(|py| {
let f = wrap_pyfunction!(option_class_arg, py).unwrap();
assert!(f.call0().unwrap().is_none());
let obj = Py::new(py, SomePyClass {}).unwrap();
assert!(f
.call1((obj,))
.unwrap()
.extract::<Py<SomePyClass>>()
.is_ok());
})
}