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 gil-refs in from_py_with #3967

Merged
merged 2 commits into from
Mar 19, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 34 additions & 5 deletions pyo3-macros-backend/src/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,38 @@ pub fn impl_arg_params(
let args_array = syn::Ident::new("output", Span::call_site());
let Ctx { pyo3_path } = ctx;

let from_py_with = spec
.signature
.arguments
.iter()
.enumerate()
.filter_map(|(i, arg)| {
let from_py_with = &arg.attrs.from_py_with.as_ref()?.value;
let from_py_with_holder =
syn::Ident::new(&format!("from_py_with_{}", i), Span::call_site());
Some(quote_spanned! { from_py_with.span() =>
let e = #pyo3_path::impl_::pymethods::Extractor::new();
let #from_py_with_holder = #pyo3_path::impl_::pymethods::inspect_fn(#from_py_with, &e);
e.extract_from_py_with();
})
})
.collect::<TokenStream>();

if !fastcall && is_forwarded_args(&spec.signature) {
// In the varargs convention, we can just pass though if the signature
// is (*args, **kwds).
let arg_convert = spec
.signature
.arguments
.iter()
.map(|arg| impl_arg_param(arg, &mut 0, &args_array, holders, ctx))
.enumerate()
.map(|(i, arg)| impl_arg_param(arg, i, &mut 0, &args_array, holders, ctx))
.collect::<Result<_>>()?;
return Ok((
quote! {
let _args = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_args);
let _kwargs = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr_or_opt(py, &_kwargs);
#from_py_with
},
arg_convert,
));
Expand Down Expand Up @@ -81,7 +100,8 @@ pub fn impl_arg_params(
.signature
.arguments
.iter()
.map(|arg| impl_arg_param(arg, &mut option_pos, &args_array, holders, ctx))
.enumerate()
.map(|(i, arg)| impl_arg_param(arg, i, &mut option_pos, &args_array, holders, ctx))
.collect::<Result<_>>()?;

let args_handler = if spec.signature.python_signature.varargs.is_some() {
Expand Down Expand Up @@ -136,6 +156,7 @@ pub fn impl_arg_params(
};
let mut #args_array = [::std::option::Option::None; #num_params];
let (_args, _kwargs) = #extract_expression;
#from_py_with
},
param_conversion,
))
Expand All @@ -145,6 +166,7 @@ pub fn impl_arg_params(
/// index and the index in option diverge when using py: Python
fn impl_arg_param(
arg: &FnArg<'_>,
pos: usize,
option_pos: &mut usize,
args_array: &syn::Ident,
holders: &mut Vec<TokenStream>,
Expand Down Expand Up @@ -222,14 +244,21 @@ fn impl_arg_param(
));
}

let tokens = if let Some(expr_path) = arg.attrs.from_py_with.as_ref().map(|attr| &attr.value) {
let tokens = if arg
.attrs
.from_py_with
.as_ref()
.map(|attr| &attr.value)
.is_some()
{
let from_py_with = syn::Ident::new(&format!("from_py_with_{}", pos), Span::call_site());
if let Some(default) = default {
quote_arg_span! {
#[allow(clippy::redundant_closure)]
#pyo3_path::impl_::extract_argument::from_py_with_with_default(
#arg_value.as_deref(),
#name_str,
#expr_path as fn(_) -> _,
#from_py_with as fn(_) -> _,
|| #default
)?
}
Expand All @@ -238,7 +267,7 @@ fn impl_arg_param(
#pyo3_path::impl_::extract_argument::from_py_with(
&#pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value),
#name_str,
#expr_path as fn(_) -> _,
#from_py_with as fn(_) -> _,
)?
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/impl_/pymethods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,10 @@ pub fn inspect_type<T>(t: T) -> (T, Extractor<T>) {
(t, Extractor::new())
}

pub fn inspect_fn<A, T>(f: fn(A) -> PyResult<T>, _: &Extractor<A>) -> fn(A) -> PyResult<T> {
f
}

pub struct Extractor<T>(NotAGilRef<T>);
pub struct NotAGilRef<T>(std::marker::PhantomData<T>);

Expand Down Expand Up @@ -616,10 +620,19 @@ impl<T: IsGilRef> Extractor<T> {
)
)]
pub fn extract_gil_ref(&self) {}
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor"
)
)]
pub fn extract_from_py_with(&self) {}
}

impl<T> NotAGilRef<T> {
pub fn extract_gil_ref(&self) {}
pub fn extract_from_py_with(&self) {}
pub fn is_python(&self) {}
}

Expand Down
2 changes: 1 addition & 1 deletion tests/test_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,7 @@ fn test_issue_2988() {
_data: Vec<i32>,
// The from_py_with here looks a little odd, we just need some way
// to encourage the macro to expand the from_py_with default path too
#[pyo3(from_py_with = "PyAny::extract")] _data2: Vec<i32>,
#[pyo3(from_py_with = "<Bound<'_, _> as PyAnyMethods>::extract")] _data2: Vec<i32>,
) {
}
}
10 changes: 6 additions & 4 deletions tests/test_pyfunction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,8 @@ fn test_functions_with_function_args() {
}

#[cfg(not(Py_LIMITED_API))]
fn datetime_to_timestamp(dt: &PyAny) -> PyResult<i64> {
let dt: &PyDateTime = dt.extract()?;
fn datetime_to_timestamp(dt: &Bound<'_, PyAny>) -> PyResult<i64> {
let dt = dt.downcast::<PyDateTime>()?;
let ts: f64 = dt.call_method0("timestamp")?.extract()?;

Ok(ts as i64)
Expand Down Expand Up @@ -170,7 +170,7 @@ fn test_function_with_custom_conversion_error() {

#[test]
fn test_from_py_with_defaults() {
fn optional_int(x: &PyAny) -> PyResult<Option<i32>> {
fn optional_int(x: &Bound<'_, PyAny>) -> PyResult<Option<i32>> {
if x.is_none() {
Ok(None)
} else {
Expand All @@ -185,7 +185,9 @@ fn test_from_py_with_defaults() {
}

#[pyfunction(signature = (len=0))]
fn from_py_with_default(#[pyo3(from_py_with = "PyAny::len")] len: usize) -> usize {
fn from_py_with_default(
#[pyo3(from_py_with = "<Bound<'_, _> as PyAnyMethods>::len")] len: usize,
davidhewitt marked this conversation as resolved.
Show resolved Hide resolved
) -> usize {
len
}

Expand Down
15 changes: 15 additions & 0 deletions tests/ui/deprecations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,21 @@ fn module_bound_by_value(m: Bound<'_, PyModule>) -> PyResult<()> {
Ok(())
}

fn extract_gil_ref(obj: &PyAny) -> PyResult<i32> {
obj.extract()
}

fn extract_bound(obj: &Bound<'_, PyAny>) -> PyResult<i32> {
obj.extract()
}

#[pyfunction]
fn pyfunction_from_py_with(
#[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32,
#[pyo3(from_py_with = "extract_bound")] _bound: i32,
) {
}

fn test_wrap_pyfunction(py: Python<'_>, m: &Bound<'_, PyModule>) {
// should lint
let _ = wrap_pyfunction!(double, py);
Expand Down
10 changes: 8 additions & 2 deletions tests/ui/deprecations.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,16 @@ error: use of deprecated method `pyo3::methods::Extractor::<T>::extract_gil_ref`
54 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
| ^

error: use of deprecated method `pyo3::methods::Extractor::<T>::extract_from_py_with`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor
--> tests/ui/deprecations.rs:87:27
|
87 | #[pyo3(from_py_with = "extract_gil_ref")] _gil_ref: i32,
| ^^^^^^^^^^^^^^^^^

error: use of deprecated method `pyo3::methods::Extractor::<pyo3::Python<'_>>::is_python`: use `wrap_pyfunction_bound!` instead
--> tests/ui/deprecations.rs:79:13
--> tests/ui/deprecations.rs:94:13
|
79 | let _ = wrap_pyfunction!(double, py);
94 | 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)
Loading