From fdb29cc31fc8dc0327738e28c8363253c8b6811f Mon Sep 17 00:00:00 2001 From: Icxolu <10486322+Icxolu@users.noreply.github.com> Date: Sat, 2 Nov 2024 11:06:54 +0100 Subject: [PATCH] fix unintentional `unsafe_op_in_unsafe_fn` trigger (#4674) * fix unintentional `unsafe_op_in_unsafe_fn` trigger * add newsfragment --- newsfragments/4674.fixed.md | 1 + pyo3-macros-backend/src/method.rs | 30 ++++++++++++---------- pyo3-macros-backend/src/module.rs | 2 +- pyo3-macros-backend/src/params.rs | 5 ++-- pyo3-macros-backend/src/pymethod.rs | 39 +++++++++++++++-------------- tests/ui/forbid_unsafe.rs | 1 + 6 files changed, 43 insertions(+), 35 deletions(-) create mode 100644 newsfragments/4674.fixed.md diff --git a/newsfragments/4674.fixed.md b/newsfragments/4674.fixed.md new file mode 100644 index 00000000000..6245a6f734a --- /dev/null +++ b/newsfragments/4674.fixed.md @@ -0,0 +1 @@ +Fixes unintentional `unsafe_op_in_unsafe_fn` trigger by adjusting macro hygiene. \ No newline at end of file diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 019fb5e644b..f99e64562b7 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -270,9 +270,9 @@ impl FnType { ::std::convert::Into::into( #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &*(&#slf as *const _ as *const *mut _)) .downcast_unchecked::<#pyo3_path::types::PyType>() - ), + ) }; - Some(ret) + Some(quote! { unsafe { #ret }, }) } FnType::FnModule(span) => { let py = syn::Ident::new("py", Span::call_site()); @@ -283,9 +283,9 @@ impl FnType { ::std::convert::Into::into( #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &*(&#slf as *const _ as *const *mut _)) .downcast_unchecked::<#pyo3_path::types::PyModule>() - ), + ) }; - Some(ret) + Some(quote! { unsafe { #ret }, }) } FnType::FnNew | FnType::FnStatic | FnType::ClassAttribute => None, } @@ -332,6 +332,8 @@ impl SelfType { let py = syn::Ident::new("py", Span::call_site()); let slf = syn::Ident::new("_slf", Span::call_site()); let Ctx { pyo3_path, .. } = ctx; + let bound_ref = + quote! { unsafe { #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf) } }; match self { SelfType::Receiver { span, mutable } => { let method = if *mutable { @@ -344,7 +346,7 @@ impl SelfType { error_mode.handle_error( quote_spanned! { *span => #pyo3_path::impl_::extract_argument::#method::<#cls>( - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf).0, + #bound_ref.0, &mut #holder, ) }, @@ -355,7 +357,7 @@ impl SelfType { let pyo3_path = pyo3_path.to_tokens_spanned(*span); error_mode.handle_error( quote_spanned! { *span => - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(#py, &#slf).downcast::<#cls>() + #bound_ref.downcast::<#cls>() .map_err(::std::convert::Into::<#pyo3_path::PyErr>::into) .and_then( #[allow(unknown_lints, clippy::unnecessary_fallible_conversions)] // In case slf is Py (unknown_lints can be removed when MSRV is 1.75+) @@ -665,14 +667,14 @@ impl<'a> FnSpec<'a> { FnType::Fn(SelfType::Receiver { mutable: false, .. }) => { quote! {{ #(let #arg_names = #args;)* - let __guard = #pyo3_path::impl_::coroutine::RefGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))?; + let __guard = unsafe { #pyo3_path::impl_::coroutine::RefGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))? }; async move { function(&__guard, #(#arg_names),*).await } }} } FnType::Fn(SelfType::Receiver { mutable: true, .. }) => { quote! {{ #(let #arg_names = #args;)* - let mut __guard = #pyo3_path::impl_::coroutine::RefMutGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))?; + let mut __guard = unsafe { #pyo3_path::impl_::coroutine::RefMutGuard::<#cls>::new(&#pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_slf))? }; async move { function(&mut __guard, #(#arg_names),*).await } }} } @@ -862,11 +864,13 @@ impl<'a> FnSpec<'a> { _args: *mut #pyo3_path::ffi::PyObject, ) -> *mut #pyo3_path::ffi::PyObject { - #pyo3_path::impl_::trampoline::noargs( - _slf, - _args, - #wrapper - ) + unsafe { + #pyo3_path::impl_::trampoline::noargs( + _slf, + _args, + #wrapper + ) + } } trampoline }, diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 7d2c72dbdfb..5aaf7740461 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -449,7 +449,7 @@ fn module_initialization( #[doc(hidden)] #[export_name = #pyinit_symbol] pub unsafe extern "C" fn __pyo3_init() -> *mut #pyo3_path::ffi::PyObject { - #pyo3_path::impl_::trampoline::module_init(|py| _PYO3_DEF.make_module(py)) + unsafe { #pyo3_path::impl_::trampoline::module_init(|py| _PYO3_DEF.make_module(py)) } } }); } diff --git a/pyo3-macros-backend/src/params.rs b/pyo3-macros-backend/src/params.rs index ccf725d3760..67054458c98 100644 --- a/pyo3-macros-backend/src/params.rs +++ b/pyo3-macros-backend/src/params.rs @@ -79,7 +79,7 @@ pub fn impl_arg_params( .collect(); return ( quote! { - let _args = #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &_args); + let _args = unsafe { #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 }, @@ -301,9 +301,10 @@ pub(crate) fn impl_regular_arg_param( } } else { let holder = holders.push_holder(arg.name.span()); + let unwrap = quote! {unsafe { #pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value) }}; quote_arg_span! { #pyo3_path::impl_::extract_argument::extract_argument( - #pyo3_path::impl_::extract_argument::unwrap_required_argument(#arg_value), + #unwrap, &mut #holder, #name_str )? diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index d825609cd77..1254a8d510b 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -1182,25 +1182,26 @@ fn extract_object( let Ctx { pyo3_path, .. } = ctx; let name = arg.name().unraw().to_string(); - let extract = - if let Some(from_py_with) = arg.from_py_with().map(|from_py_with| &from_py_with.value) { - quote! { - #pyo3_path::impl_::extract_argument::from_py_with( - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &#source_ptr).0, - #name, - #from_py_with as fn(_) -> _, - ) - } - } else { - let holder = holders.push_holder(Span::call_site()); - quote! { - #pyo3_path::impl_::extract_argument::extract_argument( - #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &#source_ptr).0, - &mut #holder, - #name - ) - } - }; + let extract = if let Some(from_py_with) = + arg.from_py_with().map(|from_py_with| &from_py_with.value) + { + quote! { + #pyo3_path::impl_::extract_argument::from_py_with( + unsafe { #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &#source_ptr).0 }, + #name, + #from_py_with as fn(_) -> _, + ) + } + } else { + let holder = holders.push_holder(Span::call_site()); + quote! { + #pyo3_path::impl_::extract_argument::extract_argument( + unsafe { #pyo3_path::impl_::pymethods::BoundRef::ref_from_ptr(py, &#source_ptr).0 }, + &mut #holder, + #name + ) + } + }; let extracted = extract_error_mode.handle_error(extract, ctx); quote!(#extracted) diff --git a/tests/ui/forbid_unsafe.rs b/tests/ui/forbid_unsafe.rs index 9b62886b650..660f5fa36c0 100644 --- a/tests/ui/forbid_unsafe.rs +++ b/tests/ui/forbid_unsafe.rs @@ -1,4 +1,5 @@ #![forbid(unsafe_code)] +#![forbid(unsafe_op_in_unsafe_fn)] use pyo3::*;