From c93ee00130f915397186f49d68a74c64c2f49d49 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Thu, 10 Feb 2022 08:15:13 +0000 Subject: [PATCH] refactor: inline handle_panic into macro output --- CHANGELOG.md | 2 +- pyo3-macros-backend/src/method.rs | 24 ++++++--- pyo3-macros-backend/src/module.rs | 3 +- pyo3-macros-backend/src/pymethod.rs | 18 ++++--- src/callback.rs | 6 ++- src/impl_/pyclass.rs | 75 +++++++++++++++++------------ src/impl_/pymodule.rs | 16 ++++-- tests/ui/static_ref.stderr | 26 +++------- 8 files changed, 98 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 585ec0dc..627dcf5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `__getitem__`, `__setitem__` and `__delitem__` in `#[pymethods]` now implement both a Python mapping and sequence by default. [#2065](https://github.com/PyO3/pyo3/pull/2065) - Improve performance and error messages for `#[derive(FromPyObject)]` for enums. [#2068](https://github.com/PyO3/pyo3/pull/2068) - Reduce generated LLVM code size (to improve compile times) for: - - internal `handle_panic` helper [#2074](https://github.com/PyO3/pyo3/pull/2074) + - internal `handle_panic` helper [#2074](https://github.com/PyO3/pyo3/pull/2074) [#2158](https://github.com/PyO3/pyo3/pull/2158) - `#[pyfunction]` and `#[pymethods]` argument extraction [#2075](https://github.com/PyO3/pyo3/pull/2075) [#2085](https://github.com/PyO3/pyo3/pull/2085) - `#[pyclass]` type object creation [#2076](https://github.com/PyO3/pyo3/pull/2076) [#2081](https://github.com/PyO3/pyo3/pull/2081) [#2157](https://github.com/PyO3/pyo3/pull/2157) - `__ipow__` now supports modulo argument on Python 3.8+. [#2083](https://github.com/PyO3/pyo3/pull/2083) diff --git a/pyo3-macros-backend/src/method.rs b/pyo3-macros-backend/src/method.rs index 8d03d75c..bf34070f 100644 --- a/pyo3-macros-backend/src/method.rs +++ b/pyo3-macros-backend/src/method.rs @@ -490,10 +490,12 @@ impl<'a> FnSpec<'a> { { use #krate as _pyo3; #deprecations - _pyo3::callback::handle_panic(|#py| { + let gil = _pyo3::GILPool::new(); + let #py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion #rust_call - }) + })) } } } @@ -508,11 +510,13 @@ impl<'a> FnSpec<'a> { { use #krate as _pyo3; #deprecations - _pyo3::callback::handle_panic(|#py| { + let gil = _pyo3::GILPool::new(); + let #py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion #arg_convert #rust_call - }) + })) } } } @@ -526,11 +530,13 @@ impl<'a> FnSpec<'a> { { use #krate as _pyo3; #deprecations - _pyo3::callback::handle_panic(|#py| { + let gil = _pyo3::GILPool::new(); + let #py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #self_conversion #arg_convert #rust_call - }) + })) } } } @@ -546,13 +552,15 @@ impl<'a> FnSpec<'a> { use #krate as _pyo3; #deprecations use _pyo3::callback::IntoPyCallbackOutput; - _pyo3::callback::handle_panic(|#py| { + let gil = _pyo3::GILPool::new(); + let #py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #arg_convert let result = #rust_call; let initializer: _pyo3::PyClassInitializer::<#cls> = result.convert(#py)?; let cell = initializer.create_cell_from_subtype(#py, subtype)?; ::std::result::Result::Ok(cell as *mut _pyo3::ffi::PyObject) - }) + })) } } } diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index ecfcc9f4..738b69fb 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -80,8 +80,7 @@ pub fn pymodule_impl( /// This autogenerated function is called by the python interpreter when importing /// the module. pub unsafe extern "C" fn #cb_name() -> *mut #krate::ffi::PyObject { - use #krate::{self as _pyo3, IntoPyPointer}; - _pyo3::callback::handle_panic(|_py| ::std::result::Result::Ok(#module_def_name.make_module(_py)?.into_ptr())) + unsafe { #module_def_name.module_init() } } #[doc(hidden)] diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index c914b4d0..4ec87956 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/pyo3-macros-backend/src/pymethod.rs @@ -304,7 +304,9 @@ pub fn impl_py_setter_def(cls: &syn::Type, property_type: PropertyType) -> Resul _value: *mut _pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void ) -> ::std::os::raw::c_int { - _pyo3::callback::handle_panic(|_py| { + let gil = _pyo3::GILPool::new(); + let _py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(_py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #slf let _value = _py .from_borrowed_ptr_or_opt(_value) @@ -314,7 +316,7 @@ pub fn impl_py_setter_def(cls: &syn::Type, property_type: PropertyType) -> Resul let _val = _pyo3::FromPyObject::extract(_value)?; _pyo3::callback::convert(_py, #setter_impl) - }) + })) } __wrap }), @@ -383,10 +385,12 @@ pub fn impl_py_getter_def(cls: &syn::Type, property_type: PropertyType) -> Resul _slf: *mut _pyo3::ffi::PyObject, _: *mut ::std::os::raw::c_void ) -> *mut _pyo3::ffi::PyObject { - _pyo3::callback::handle_panic(|_py| { + let gil = _pyo3::GILPool::new(); + let _py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(_py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #slf _pyo3::callback::convert(_py, #getter_impl) - }) + })) } __wrap }), @@ -880,9 +884,11 @@ impl SlotDef { unsafe extern "C" fn __wrap(_raw_slf: *mut _pyo3::ffi::PyObject, #(#method_arguments),*) -> #ret_ty { let _slf = _raw_slf; #before_call_method - _pyo3::callback::handle_panic(|#py| { + let gil = _pyo3::GILPool::new(); + let #py = gil.python(); + _pyo3::callback::panic_result_into_callback_output(#py, ::std::panic::catch_unwind(move || -> _pyo3::PyResult<_> { #body - }) + })) } _pyo3::ffi::PyType_Slot { slot: _pyo3::ffi::#slot, diff --git a/src/callback.rs b/src/callback.rs index 4a8598f3..4b54de81 100644 --- a/src/callback.rs +++ b/src/callback.rs @@ -245,7 +245,11 @@ where panic_result_into_callback_output(py, panic::catch_unwind(move || -> PyResult<_> { body(py) })) } -fn panic_result_into_callback_output( +/// Converts the output of std::panic::catch_unwind into a Python function output, either by raising a Python +/// exception or by unwrapping the contained success output. +#[doc(hidden)] +#[inline] +pub fn panic_result_into_callback_output( py: Python, panic_result: Result, Box>, ) -> R diff --git a/src/impl_/pyclass.rs b/src/impl_/pyclass.rs index 1f3174a0..7b1c8ef0 100644 --- a/src/impl_/pyclass.rs +++ b/src/impl_/pyclass.rs @@ -260,14 +260,19 @@ macro_rules! define_pyclass_setattr_slot { use ::std::option::Option::*; use $crate::callback::IntoPyCallbackOutput; use $crate::impl_::pyclass::*; - $crate::callback::handle_panic(|py| { - let collector = PyClassImplCollector::<$cls>::new(); - if let Some(value) = ::std::ptr::NonNull::new(value) { - collector.$set(py, _slf, attr, value).convert(py) - } else { - collector.$del(py, _slf, attr).convert(py) - } - }) + let gil = $crate::GILPool::new(); + let py = gil.python(); + $crate::callback::panic_result_into_callback_output( + py, + ::std::panic::catch_unwind(move || -> $crate::PyResult<_> { + let collector = PyClassImplCollector::<$cls>::new(); + if let Some(value) = ::std::ptr::NonNull::new(value) { + collector.$set(py, _slf, attr, value).convert(py) + } else { + collector.$del(py, _slf, attr).convert(py) + } + }), + ) } $crate::ffi::PyType_Slot { slot: $crate::ffi::$slot, @@ -367,17 +372,22 @@ macro_rules! define_pyclass_binary_operator_slot { _slf: *mut $crate::ffi::PyObject, _other: *mut $crate::ffi::PyObject, ) -> *mut $crate::ffi::PyObject { - $crate::callback::handle_panic(|py| { - use $crate::impl_::pyclass::*; - let collector = PyClassImplCollector::<$cls>::new(); - let lhs_result = collector.$lhs(py, _slf, _other)?; - if lhs_result == $crate::ffi::Py_NotImplemented() { - $crate::ffi::Py_DECREF(lhs_result); - collector.$rhs(py, _other, _slf) - } else { - ::std::result::Result::Ok(lhs_result) - } - }) + let gil = $crate::GILPool::new(); + let py = gil.python(); + $crate::callback::panic_result_into_callback_output( + py, + ::std::panic::catch_unwind(move || -> $crate::PyResult<_> { + use $crate::impl_::pyclass::*; + let collector = PyClassImplCollector::<$cls>::new(); + let lhs_result = collector.$lhs(py, _slf, _other)?; + if lhs_result == $crate::ffi::Py_NotImplemented() { + $crate::ffi::Py_DECREF(lhs_result); + collector.$rhs(py, _other, _slf) + } else { + ::std::result::Result::Ok(lhs_result) + } + }), + ) } $crate::ffi::PyType_Slot { slot: $crate::ffi::$slot, @@ -560,17 +570,22 @@ macro_rules! generate_pyclass_pow_slot { _other: *mut $crate::ffi::PyObject, _mod: *mut $crate::ffi::PyObject, ) -> *mut $crate::ffi::PyObject { - $crate::callback::handle_panic(|py| { - use $crate::impl_::pyclass::*; - let collector = PyClassImplCollector::<$cls>::new(); - let lhs_result = collector.__pow__(py, _slf, _other, _mod)?; - if lhs_result == $crate::ffi::Py_NotImplemented() { - $crate::ffi::Py_DECREF(lhs_result); - collector.__rpow__(py, _other, _slf, _mod) - } else { - ::std::result::Result::Ok(lhs_result) - } - }) + let gil = $crate::GILPool::new(); + let py = gil.python(); + $crate::callback::panic_result_into_callback_output( + py, + ::std::panic::catch_unwind(move || -> $crate::PyResult<_> { + use $crate::impl_::pyclass::*; + let collector = PyClassImplCollector::<$cls>::new(); + let lhs_result = collector.__pow__(py, _slf, _other, _mod)?; + if lhs_result == $crate::ffi::Py_NotImplemented() { + $crate::ffi::Py_DECREF(lhs_result); + collector.__rpow__(py, _other, _slf, _mod) + } else { + ::std::result::Result::Ok(lhs_result) + } + }), + ) } $crate::ffi::PyType_Slot { slot: $crate::ffi::Py_nb_power, diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 11ab08e7..8d7a843b 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,9 +1,10 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -use std::{cell::UnsafeCell, panic::AssertUnwindSafe}; +use std::cell::UnsafeCell; use crate::{ - callback::handle_panic, ffi, types::PyModule, IntoPyPointer, Py, PyObject, PyResult, Python, + callback::panic_result_into_callback_output, ffi, types::PyModule, GILPool, IntoPyPointer, Py, + PyObject, PyResult, Python, }; /// `Sync` wrapper of `ffi::PyModuleDef`. @@ -64,8 +65,15 @@ impl ModuleDef { /// # Safety /// The Python GIL must be held. pub unsafe fn module_init(&'static self) -> *mut ffi::PyObject { - let unwind_safe_self = AssertUnwindSafe(self); - handle_panic(|py| Ok(unwind_safe_self.make_module(py)?.into_ptr())) + let pool = GILPool::new(); + let py = pool.python(); + let unwind_safe_self = std::panic::AssertUnwindSafe(self); + panic_result_into_callback_output( + py, + std::panic::catch_unwind(move || -> PyResult<_> { + Ok(unwind_safe_self.make_module(py)?.into_ptr()) + }), + ) } } diff --git a/tests/ui/static_ref.stderr b/tests/ui/static_ref.stderr index e948a2ff..f087904c 100644 --- a/tests/ui/static_ref.stderr +++ b/tests/ui/static_ref.stderr @@ -1,25 +1,11 @@ -error[E0495]: cannot infer an appropriate lifetime for lifetime parameter `'py` due to conflicting requirements +error[E0597]: `gil` does not live long enough --> tests/ui/static_ref.rs:4:1 | 4 | #[pyfunction] - | ^^^^^^^^^^^^^ + | ^^^^^^^^^^^^- + | | | + | | `gil` dropped here while still borrowed + | borrowed value does not live long enough + | cast requires that `gil` is borrowed for `'static` | -note: first, the lifetime cannot outlive the anonymous lifetime #1 defined here... - --> tests/ui/static_ref.rs:4:1 - | -4 | #[pyfunction] - | ^^^^^^^^^^^^^ -note: ...so that the expression is assignable - --> tests/ui/static_ref.rs:4:1 - | -4 | #[pyfunction] - | ^^^^^^^^^^^^^ - = note: expected `pyo3::Python<'_>` - found `pyo3::Python<'_>` - = note: but, the lifetime must be valid for the static lifetime... -note: ...so that reference does not outlive borrowed content - --> tests/ui/static_ref.rs:4:1 - | -4 | #[pyfunction] - | ^^^^^^^^^^^^^ = note: this error originates in the attribute macro `pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)