From 233fdd555e01bc7d462a3682f8a6647396cffb7f Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Sun, 25 Jun 2023 20:01:58 +0100
Subject: [PATCH 1/6] add some style guide to Contributing.md

---
 Contributing.md | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Contributing.md b/Contributing.md
index c6d29e2d427..75b129edd1d 100644
--- a/Contributing.md
+++ b/Contributing.md
@@ -111,6 +111,36 @@ To include your changes in the release notes, you should create one (or more) ne
 - `removed` - for features which have been removed
 - `fixed` - for "changed" features which were classed as a bugfix
 
+### Style guide
+
+#### Generic code
+
+PyO3 has a lot of generic APIs to increase usability. These can come at the cost of generic code bloat. Where reasonable, try to implement a concrete sub-portion of generic functions. There are two forms of this:
+
+- If the concrete sub-portion doesn't benefit from re-use by other functions, name it `inner` and keep it as a local to the function.
+- If the concrete sub-portion is re-used by other functions, preferably name it `_foo` and place it directly below `foo` in the source code (where `foo` is the original generic function).
+
+#### FFI calls
+
+PyO3 makes a lot of FFI calls to Python's C API using raw pointers. Where possible try to avoid using pointers-to-temporaries in expressions:
+
+```rust
+// dangerous
+pyo3::ffi::Something(name.to_object(py).as_ptr());
+
+// because the following refactoring is a use-after-free error:
+let name = name.to_object(py).as_ptr();
+pyo3::ffi::Something(name)
+```
+
+Instead, prefer to bind the safe owned `PyObject` wrapper before passing to ffi functions:
+
+```rust
+let name: PyObject = name.to_object(py);
+pyo3::ffi::Something(name.as_ptr())
+// name will automatically be freed when it falls out of scope
+```
+
 ## Python and Rust version support policy
 
 PyO3 aims to keep sufficient compatibility to make packaging Python extensions built with PyO3 feasible on most common package managers.

From bf2f4415670ba38c63f1ae4492db92c740589678 Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 27 Jun 2023 08:33:16 +0100
Subject: [PATCH 2/6] prefer inner / _private naming

---
 pyo3-macros-backend/src/module.rs   |  2 +-
 pyo3-macros-backend/src/pymethod.rs |  2 +-
 src/buffer.rs                       | 18 ++++++------
 src/impl_/pyfunction.rs             |  2 +-
 src/impl_/pymethods.rs              |  2 +-
 src/impl_/trampoline.rs             | 14 +++++-----
 src/macros.rs                       |  4 +--
 src/pyclass/create_type_object.rs   | 12 ++++----
 src/types/dict.rs                   | 43 ++++++++++++++++-------------
 src/types/frozenset.rs              |  4 +--
 src/types/set.rs                    |  7 ++---
 tests/ui/traverse.stderr            |  4 +--
 12 files changed, 58 insertions(+), 56 deletions(-)

diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs
index 93f22bbe6cd..ccd84bb363a 100644
--- a/pyo3-macros-backend/src/module.rs
+++ b/pyo3-macros-backend/src/module.rs
@@ -119,7 +119,7 @@ pub fn process_functions_in_module(
                 let name = &func.sig.ident;
                 let statements: Vec<syn::Stmt> = syn::parse_quote! {
                     #wrapped_function
-                    #module_name.add_function(#krate::impl_::pyfunction::wrap_pyfunction_impl(&#name::DEF, #module_name)?)?;
+                    #module_name.add_function(#krate::impl_::pyfunction::_wrap_pyfunction(&#name::DEF, #module_name)?)?;
                 };
                 stmts.extend(statements);
             }
diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs
index 16f30d716ad..b6b6f3e81e5 100644
--- a/pyo3-macros-backend/src/pymethod.rs
+++ b/pyo3-macros-backend/src/pymethod.rs
@@ -420,7 +420,7 @@ fn impl_traverse_slot(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<MethodA
             visit: _pyo3::ffi::visitproc,
             arg: *mut ::std::os::raw::c_void,
         ) -> ::std::os::raw::c_int {
-            _pyo3::impl_::pymethods::call_traverse_impl::<#cls>(slf, #cls::#rust_fn_ident, visit, arg)
+            _pyo3::impl_::pymethods::_call_traverse::<#cls>(slf, #cls::#rust_fn_ident, visit, arg)
         }
     };
     let slot_def = quote! {
diff --git a/src/buffer.rs b/src/buffer.rs
index d3be9ee6187..79e416ba939 100644
--- a/src/buffer.rs
+++ b/src/buffer.rs
@@ -469,7 +469,7 @@ impl<T: Element> PyBuffer<T> {
     /// you can use `<T as buffer::Element>::is_compatible_format(buf.format())`.
     /// Alternatively, `match buffer::ElementType::from_format(buf.format())`.
     pub fn copy_to_slice(&self, py: Python<'_>, target: &mut [T]) -> PyResult<()> {
-        self.copy_to_slice_impl(py, target, b'C')
+        self._copy_to_slice(py, target, b'C')
     }
 
     /// Copies the buffer elements to the specified slice.
@@ -482,10 +482,10 @@ impl<T: Element> PyBuffer<T> {
     /// you can use `<T as buffer::Element>::is_compatible_format(buf.format())`.
     /// Alternatively, `match buffer::ElementType::from_format(buf.format())`.
     pub fn copy_to_fortran_slice(&self, py: Python<'_>, target: &mut [T]) -> PyResult<()> {
-        self.copy_to_slice_impl(py, target, b'F')
+        self._copy_to_slice(py, target, b'F')
     }
 
-    fn copy_to_slice_impl(&self, py: Python<'_>, target: &mut [T], fort: u8) -> PyResult<()> {
+    fn _copy_to_slice(&self, py: Python<'_>, target: &mut [T], fort: u8) -> PyResult<()> {
         if mem::size_of_val(target) != self.len_bytes() {
             return Err(PyBufferError::new_err(format!(
                 "slice to copy to (of length {}) does not match buffer length of {}",
@@ -516,7 +516,7 @@ impl<T: Element> PyBuffer<T> {
     ///
     /// Fails if the buffer format is not compatible with type `T`.
     pub fn to_vec(&self, py: Python<'_>) -> PyResult<Vec<T>> {
-        self.to_vec_impl(py, b'C')
+        self._to_vec(py, b'C')
     }
 
     /// Copies the buffer elements to a newly allocated vector.
@@ -524,10 +524,10 @@ impl<T: Element> PyBuffer<T> {
     ///
     /// Fails if the buffer format is not compatible with type `T`.
     pub fn to_fortran_vec(&self, py: Python<'_>) -> PyResult<Vec<T>> {
-        self.to_vec_impl(py, b'F')
+        self._to_vec(py, b'F')
     }
 
-    fn to_vec_impl(&self, py: Python<'_>, fort: u8) -> PyResult<Vec<T>> {
+    fn _to_vec(&self, py: Python<'_>, fort: u8) -> PyResult<Vec<T>> {
         let item_count = self.item_count();
         let mut vec: Vec<T> = Vec::with_capacity(item_count);
         unsafe {
@@ -564,7 +564,7 @@ impl<T: Element> PyBuffer<T> {
     /// use `<T as buffer::Element>::is_compatible_format(buf.format())`.
     /// Alternatively, `match buffer::ElementType::from_format(buf.format())`.
     pub fn copy_from_slice(&self, py: Python<'_>, source: &[T]) -> PyResult<()> {
-        self.copy_from_slice_impl(py, source, b'C')
+        self._copy_from_slice(py, source, b'C')
     }
 
     /// Copies the specified slice into the buffer.
@@ -578,10 +578,10 @@ impl<T: Element> PyBuffer<T> {
     /// use `<T as buffer::Element>::is_compatible_format(buf.format())`.
     /// Alternatively, `match buffer::ElementType::from_format(buf.format())`.
     pub fn copy_from_fortran_slice(&self, py: Python<'_>, source: &[T]) -> PyResult<()> {
-        self.copy_from_slice_impl(py, source, b'F')
+        self._copy_from_slice(py, source, b'F')
     }
 
-    fn copy_from_slice_impl(&self, py: Python<'_>, source: &[T], fort: u8) -> PyResult<()> {
+    fn _copy_from_slice(&self, py: Python<'_>, source: &[T], fort: u8) -> PyResult<()> {
         if self.readonly() {
             return Err(PyBufferError::new_err("cannot write to read-only buffer"));
         } else if mem::size_of_val(source) != self.len_bytes() {
diff --git a/src/impl_/pyfunction.rs b/src/impl_/pyfunction.rs
index 95d8350d270..14cdbd48f85 100644
--- a/src/impl_/pyfunction.rs
+++ b/src/impl_/pyfunction.rs
@@ -2,7 +2,7 @@ use crate::{derive_utils::PyFunctionArguments, types::PyCFunction, PyResult};
 
 pub use crate::impl_::pymethods::PyMethodDef;
 
-pub fn wrap_pyfunction_impl<'a>(
+pub fn _wrap_pyfunction<'a>(
     method_def: &PyMethodDef,
     py_or_module: impl Into<PyFunctionArguments<'a>>,
 ) -> PyResult<&'a PyCFunction> {
diff --git a/src/impl_/pymethods.rs b/src/impl_/pymethods.rs
index 60db3fbb1bf..98089d209d9 100644
--- a/src/impl_/pymethods.rs
+++ b/src/impl_/pymethods.rs
@@ -247,7 +247,7 @@ impl PySetterDef {
 
 /// Calls an implementation of __traverse__ for tp_traverse
 #[doc(hidden)]
-pub unsafe fn call_traverse_impl<T>(
+pub unsafe fn _call_traverse<T>(
     slf: *mut ffi::PyObject,
     impl_: fn(&T, PyVisit<'_>) -> Result<(), PyTraverseError>,
     visit: ffi::visitproc,
diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs
index efe50e20727..1fcdebc25e7 100644
--- a/src/impl_/trampoline.rs
+++ b/src/impl_/trampoline.rs
@@ -18,7 +18,7 @@ use crate::{
 pub unsafe fn module_init(
     f: for<'py> unsafe fn(Python<'py>) -> PyResult<Py<PyModule>>,
 ) -> *mut ffi::PyObject {
-    trampoline_inner(|py| f(py).map(|module| module.into_ptr()))
+    trampoline(|py| f(py).map(|module| module.into_ptr()))
 }
 
 #[inline]
@@ -28,7 +28,7 @@ pub unsafe fn noargs(
     f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject) -> PyResult<*mut ffi::PyObject>,
 ) -> *mut ffi::PyObject {
     debug_assert!(args.is_null());
-    trampoline_inner(|py| f(py, slf))
+    trampoline(|py| f(py, slf))
 }
 
 macro_rules! trampoline {
@@ -38,7 +38,7 @@ macro_rules! trampoline {
             $($arg_names: $arg_types,)*
             f: for<'py> unsafe fn (Python<'py>, $($arg_types),*) -> PyResult<$ret>,
         ) -> $ret {
-            trampoline_inner(|py| f(py, $($arg_names,)*))
+            trampoline(|py| f(py, $($arg_names,)*))
         }
     }
 }
@@ -131,7 +131,7 @@ pub unsafe fn releasebufferproc(
     buf: *mut ffi::Py_buffer,
     f: for<'py> unsafe fn(Python<'py>, *mut ffi::PyObject, *mut ffi::Py_buffer) -> PyResult<()>,
 ) {
-    trampoline_inner_unraisable(|py| f(py, slf, buf), slf)
+    trampoline_unraisable(|py| f(py, slf, buf), slf)
 }
 
 #[inline]
@@ -143,7 +143,7 @@ pub(crate) unsafe fn dealloc(
     // so pass null_mut() to the context.
     //
     // (Note that we don't allow the implementation `f` to fail.)
-    trampoline_inner_unraisable(
+    trampoline_unraisable(
         |py| {
             f(py, slf);
             Ok(())
@@ -168,7 +168,7 @@ trampoline!(
 /// Panics during execution are trapped so that they don't propagate through any
 /// outer FFI boundary.
 #[inline]
-pub(crate) fn trampoline_inner<F, R>(body: F) -> R
+pub(crate) fn trampoline<F, R>(body: F) -> R
 where
     F: for<'py> FnOnce(Python<'py>) -> PyResult<R> + UnwindSafe,
     R: PyCallbackOutput,
@@ -214,7 +214,7 @@ where
 ///
 /// ctx must be either a valid ffi::PyObject or NULL
 #[inline]
-unsafe fn trampoline_inner_unraisable<F>(body: F, ctx: *mut ffi::PyObject)
+unsafe fn trampoline_unraisable<F>(body: F, ctx: *mut ffi::PyObject)
 where
     F: for<'py> FnOnce(Python<'py>) -> PyResult<()> + UnwindSafe,
 {
diff --git a/src/macros.rs b/src/macros.rs
index d9237e2c959..560d43da1ca 100644
--- a/src/macros.rs
+++ b/src/macros.rs
@@ -125,12 +125,12 @@ macro_rules! wrap_pyfunction {
     ($function:path) => {
         &|py_or_module| {
             use $function as wrapped_pyfunction;
-            $crate::impl_::pyfunction::wrap_pyfunction_impl(&wrapped_pyfunction::DEF, py_or_module)
+            $crate::impl_::pyfunction::_wrap_pyfunction(&wrapped_pyfunction::DEF, py_or_module)
         }
     };
     ($function:path, $py_or_module:expr) => {{
         use $function as wrapped_pyfunction;
-        $crate::impl_::pyfunction::wrap_pyfunction_impl(&wrapped_pyfunction::DEF, $py_or_module)
+        $crate::impl_::pyfunction::_wrap_pyfunction(&wrapped_pyfunction::DEF, $py_or_module)
     }};
 }
 
diff --git a/src/pyclass/create_type_object.rs b/src/pyclass/create_type_object.rs
index bc2058f50f0..09aec7a4418 100644
--- a/src/pyclass/create_type_object.rs
+++ b/src/pyclass/create_type_object.rs
@@ -7,7 +7,7 @@ use crate::{
     },
     impl_::{
         pymethods::{get_doc, get_name, Getter, Setter},
-        trampoline::trampoline_inner,
+        trampoline::trampoline,
     },
     types::PyType,
     Py, PyClass, PyGetterDef, PyMethodDefType, PyResult, PySetterDef, PyTypeInfo, Python,
@@ -413,7 +413,7 @@ unsafe extern "C" fn no_constructor_defined(
     _args: *mut ffi::PyObject,
     _kwds: *mut ffi::PyObject,
 ) -> *mut ffi::PyObject {
-    trampoline_inner(|_| {
+    trampoline(|_| {
         Err(crate::exceptions::PyTypeError::new_err(
             "No constructor defined",
         ))
@@ -513,7 +513,7 @@ impl GetSetDefType {
                     ) -> *mut ffi::PyObject {
                         // Safety: PyO3 sets the closure when constructing the ffi getter so this cast should always be valid
                         let getter: Getter = std::mem::transmute(closure);
-                        trampoline_inner(|py| getter(py, slf))
+                        trampoline(|py| getter(py, slf))
                     }
                     (Some(getter), None, closure as Getter as _)
                 }
@@ -525,7 +525,7 @@ impl GetSetDefType {
                     ) -> c_int {
                         // Safety: PyO3 sets the closure when constructing the ffi setter so this cast should always be valid
                         let setter: Setter = std::mem::transmute(closure);
-                        trampoline_inner(|py| setter(py, slf, value))
+                        trampoline(|py| setter(py, slf, value))
                     }
                     (None, Some(setter), closure as Setter as _)
                 }
@@ -535,7 +535,7 @@ impl GetSetDefType {
                         closure: *mut c_void,
                     ) -> *mut ffi::PyObject {
                         let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter);
-                        trampoline_inner(|py| (getset.getter)(py, slf))
+                        trampoline(|py| (getset.getter)(py, slf))
                     }
 
                     unsafe extern "C" fn getset_setter(
@@ -544,7 +544,7 @@ impl GetSetDefType {
                         closure: *mut c_void,
                     ) -> c_int {
                         let getset: &GetterAndSetter = &*(closure as *const GetterAndSetter);
-                        trampoline_inner(|py| (getset.setter)(py, slf, value))
+                        trampoline(|py| (getset.setter)(py, slf, value))
                     }
                     (
                         Some(getset_getter),
diff --git a/src/types/dict.rs b/src/types/dict.rs
index 7a9eb0d3a69..2506c84e223 100644
--- a/src/types/dict.rs
+++ b/src/types/dict.rs
@@ -142,17 +142,20 @@ impl PyDict {
     where
         K: ToPyObject,
     {
-        self.get_item_impl(key.to_object(self.py()))
-    }
-
-    fn get_item_impl(&self, key: PyObject) -> Option<&PyAny> {
-        let py = self.py();
-        unsafe {
-            let ptr = ffi::PyDict_GetItem(self.as_ptr(), key.as_ptr());
+        fn inner(dict: &PyDict, key: PyObject) -> Option<&PyAny> {
+            let py = dict.py();
             // PyDict_GetItem returns a borrowed ptr, must make it owned for safety (see #890).
             // PyObject::from_borrowed_ptr_or_opt will take ownership in this way.
-            PyObject::from_borrowed_ptr_or_opt(py, ptr).map(|pyobject| pyobject.into_ref(py))
+            unsafe {
+                PyObject::from_borrowed_ptr_or_opt(
+                    py,
+                    ffi::PyDict_GetItem(dict.as_ptr(), key.as_ptr()),
+                )
+            }
+            .map(|pyobject| pyobject.into_ref(py))
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Gets an item from the dictionary,
@@ -164,20 +167,22 @@ impl PyDict {
     where
         K: ToPyObject,
     {
-        self.get_item_with_error_impl(key.to_object(self.py()))
-    }
-
-    fn get_item_with_error_impl(&self, key: PyObject) -> PyResult<Option<&PyAny>> {
-        let py = self.py();
-        unsafe {
-            let ptr = ffi::PyDict_GetItemWithError(self.as_ptr(), key.as_ptr());
+        fn inner(dict: &PyDict, key: PyObject) -> PyResult<Option<&PyAny>> {
+            let py = dict.py();
             // PyDict_GetItemWithError returns a borrowed ptr, must make it owned for safety (see #890).
             // PyObject::from_borrowed_ptr_or_opt will take ownership in this way.
-            PyObject::from_borrowed_ptr_or_opt(py, ptr)
-                .map(|pyobject| Ok(pyobject.into_ref(py)))
-                .or_else(|| PyErr::take(py).map(Err))
-                .transpose()
+            unsafe {
+                PyObject::from_borrowed_ptr_or_opt(
+                    py,
+                    ffi::PyDict_GetItemWithError(dict.as_ptr(), key.as_ptr()),
+                )
+            }
+            .map(|pyobject| Ok(pyobject.into_ref(py)))
+            .or_else(|| PyErr::take(py).map(Err))
+            .transpose()
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Sets an item value.
diff --git a/src/types/frozenset.rs b/src/types/frozenset.rs
index e759e97179b..776999e84b7 100644
--- a/src/types/frozenset.rs
+++ b/src/types/frozenset.rs
@@ -197,7 +197,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
     py: Python<'_>,
     elements: impl IntoIterator<Item = T>,
 ) -> PyResult<Py<PyFrozenSet>> {
-    fn new_from_iter_inner(
+    fn inner(
         py: Python<'_>,
         elements: &mut dyn Iterator<Item = PyObject>,
     ) -> PyResult<Py<PyFrozenSet>> {
@@ -217,7 +217,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
     }
 
     let mut iter = elements.into_iter().map(|e| e.to_object(py));
-    new_from_iter_inner(py, &mut iter)
+    inner(py, &mut iter)
 }
 
 #[cfg(test)]
diff --git a/src/types/set.rs b/src/types/set.rs
index b20e4ef1cb9..8dd73655175 100644
--- a/src/types/set.rs
+++ b/src/types/set.rs
@@ -248,10 +248,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
     py: Python<'_>,
     elements: impl IntoIterator<Item = T>,
 ) -> PyResult<Py<PySet>> {
-    fn new_from_iter_inner(
-        py: Python<'_>,
-        elements: &mut dyn Iterator<Item = PyObject>,
-    ) -> PyResult<Py<PySet>> {
+    fn inner(py: Python<'_>, elements: &mut dyn Iterator<Item = PyObject>) -> PyResult<Py<PySet>> {
         let set: Py<PySet> = unsafe {
             // We create the  `Py` pointer because its Drop cleans up the set if user code panics.
             Py::from_owned_ptr_or_err(py, ffi::PySet_New(std::ptr::null_mut()))?
@@ -268,7 +265,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
     }
 
     let mut iter = elements.into_iter().map(|e| e.to_object(py));
-    new_from_iter_inner(py, &mut iter)
+    inner(py, &mut iter)
 }
 
 #[cfg(test)]
diff --git a/tests/ui/traverse.stderr b/tests/ui/traverse.stderr
index 873ef864a23..e2718c76e56 100644
--- a/tests/ui/traverse.stderr
+++ b/tests/ui/traverse.stderr
@@ -19,5 +19,5 @@ error[E0308]: mismatched types
 note: function defined here
   --> src/impl_/pymethods.rs
    |
-   | pub unsafe fn call_traverse_impl<T>(
-   |               ^^^^^^^^^^^^^^^^^^
+   | pub unsafe fn _call_traverse<T>(
+   |               ^^^^^^^^^^^^^^

From 7613f65c893f2c5a30ce54a19aff0eefd9ac7a74 Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 4 Jul 2023 09:00:26 +0100
Subject: [PATCH 3/6] apply conventions for ffi calls

---
 src/conversions/std/num.rs    | 15 ++++-----
 src/err/mod.rs                | 18 +++++-----
 src/ffi/tests.rs              | 15 ++++-----
 src/impl_/extract_argument.rs | 19 ++++++-----
 src/types/any.rs              | 56 ++++++++++++++++---------------
 src/types/dict.rs             | 37 ++++++++++----------
 src/types/frozenset.rs        | 19 +++++++----
 src/types/list.rs             | 28 +++++++---------
 src/types/sequence.rs         | 63 +++++++++++++++++++----------------
 src/types/set.rs              | 29 ++++++++--------
 10 files changed, 154 insertions(+), 145 deletions(-)

diff --git a/src/conversions/std/num.rs b/src/conversions/std/num.rs
index 89e86b8a080..11c184f7373 100644
--- a/src/conversions/std/num.rs
+++ b/src/conversions/std/num.rs
@@ -248,19 +248,17 @@ mod slow_128bit_int_conversion {
 
             impl IntoPy<PyObject> for $rust_type {
                 fn into_py(self, py: Python<'_>) -> PyObject {
-                    let lower = self as u64;
-                    let upper = (self >> SHIFT) as $half_type;
+                    let lower = (self as u64).into_py(py);
+                    let upper = ((self >> SHIFT) as $half_type).into_py(py);
+                    let shift = SHIFT.into_py(py);
                     unsafe {
                         let shifted = PyObject::from_owned_ptr(
                             py,
-                            ffi::PyNumber_Lshift(
-                                upper.into_py(py).as_ptr(),
-                                SHIFT.into_py(py).as_ptr(),
-                            ),
+                            ffi::PyNumber_Lshift(upper.as_ptr(), shift.as_ptr()),
                         );
                         PyObject::from_owned_ptr(
                             py,
-                            ffi::PyNumber_Or(shifted.as_ptr(), lower.into_py(py).as_ptr()),
+                            ffi::PyNumber_Or(shifted.as_ptr(), lower.as_ptr()),
                         )
                     }
                 }
@@ -280,9 +278,10 @@ mod slow_128bit_int_conversion {
                             -1 as _,
                             ffi::PyLong_AsUnsignedLongLongMask(ob.as_ptr()),
                         )? as $rust_type;
+                        let shift = SHIFT.into_py(py);
                         let shifted = PyObject::from_owned_ptr_or_err(
                             py,
-                            ffi::PyNumber_Rshift(ob.as_ptr(), SHIFT.into_py(py).as_ptr()),
+                            ffi::PyNumber_Rshift(ob.as_ptr(), shift.as_ptr()),
                         )?;
                         let upper: $half_type = shifted.extract(py)?;
                         Ok((<$rust_type>::from(upper) << SHIFT) | lower)
diff --git a/src/err/mod.rs b/src/err/mod.rs
index 9c71b439352..05072601f78 100644
--- a/src/err/mod.rs
+++ b/src/err/mod.rs
@@ -314,7 +314,7 @@ impl PyErr {
             (ptype, pvalue, ptraceback)
         };
 
-        if ptype.as_ptr() == PanicException::type_object(py).as_ptr() {
+        if ptype.as_ptr() == PanicException::type_object_raw(py).cast() {
             let msg: String = pvalue
                 .as_ref()
                 .and_then(|obj| obj.extract(py).ok())
@@ -439,9 +439,8 @@ impl PyErr {
     where
         T: ToPyObject,
     {
-        unsafe {
-            ffi::PyErr_GivenExceptionMatches(self.type_ptr(py), exc.to_object(py).as_ptr()) != 0
-        }
+        let exc = exc.to_object(py);
+        unsafe { ffi::PyErr_GivenExceptionMatches(self.type_ptr(py), exc.as_ptr()) != 0 }
     }
 
     /// Returns true if the current exception is instance of `T`.
@@ -610,18 +609,21 @@ impl PyErr {
     /// Return the cause (either an exception instance, or None, set by `raise ... from ...`)
     /// associated with the exception, as accessible from Python through `__cause__`.
     pub fn cause(&self, py: Python<'_>) -> Option<PyErr> {
-        let ptr = unsafe { ffi::PyException_GetCause(self.value(py).as_ptr()) };
-        let obj = unsafe { py.from_owned_ptr_or_opt::<PyAny>(ptr) };
+        let value = self.value(py);
+        let obj =
+            unsafe { py.from_owned_ptr_or_opt::<PyAny>(ffi::PyException_GetCause(value.as_ptr())) };
         obj.map(Self::from_value)
     }
 
     /// Set the cause associated with the exception, pass `None` to clear it.
     pub fn set_cause(&self, py: Python<'_>, cause: Option<Self>) {
+        let value = self.value(py);
+        let cause = cause.map(|err| err.into_value(py));
         unsafe {
             // PyException_SetCause _steals_ a reference to cause, so must use .into_ptr()
             ffi::PyException_SetCause(
-                self.value(py).as_ptr(),
-                cause.map_or(std::ptr::null_mut(), |err| err.into_value(py).into_ptr()),
+                value.as_ptr(),
+                cause.map_or(std::ptr::null_mut(), IntoPyPointer::into_ptr),
             );
         }
     }
diff --git a/src/ffi/tests.rs b/src/ffi/tests.rs
index 68ddab76305..f9edd8ee3ac 100644
--- a/src/ffi/tests.rs
+++ b/src/ffi/tests.rs
@@ -71,12 +71,8 @@ fn test_timezone_from_offset() {
     use crate::types::PyDelta;
 
     Python::with_gil(|py| {
-        let tz: &PyAny = unsafe {
-            PyDateTime_IMPORT();
-            py.from_borrowed_ptr(PyTimeZone_FromOffset(
-                PyDelta::new(py, 0, 100, 0, false).unwrap().as_ptr(),
-            ))
-        };
+        let delta = PyDelta::new(py, 0, 100, 0, false).unwrap();
+        let tz: &PyAny = unsafe { py.from_borrowed_ptr(PyTimeZone_FromOffset(delta.as_ptr())) };
         crate::py_run!(
             py,
             tz,
@@ -92,11 +88,12 @@ fn test_timezone_from_offset_and_name() {
     use crate::types::PyDelta;
 
     Python::with_gil(|py| {
+        let delta = PyDelta::new(py, 0, 100, 0, false).unwrap();
+        let tzname = PyString::new(py, "testtz");
         let tz: &PyAny = unsafe {
-            PyDateTime_IMPORT();
             py.from_borrowed_ptr(PyTimeZone_FromOffsetAndName(
-                PyDelta::new(py, 0, 100, 0, false).unwrap().as_ptr(),
-                PyString::new(py, "testtz").as_ptr(),
+                delta.as_ptr(),
+                tzname.as_ptr(),
             ))
         };
         crate::py_run!(
diff --git a/src/impl_/extract_argument.rs b/src/impl_/extract_argument.rs
index 764a4e3c331..7da1b745f13 100644
--- a/src/impl_/extract_argument.rs
+++ b/src/impl_/extract_argument.rs
@@ -714,14 +714,14 @@ mod tests {
         };
 
         Python::with_gil(|py| {
+            let args = PyTuple::new(py, Vec::<&PyAny>::new());
+            let kwargs = [("foo".to_object(py).into_ref(py), 0u8)].into_py_dict(py);
             let err = unsafe {
                 function_description
                     .extract_arguments_tuple_dict::<NoVarargs, NoVarkeywords>(
                         py,
-                        PyTuple::new(py, Vec::<&PyAny>::new()).as_ptr(),
-                        [("foo".to_object(py).into_ref(py), 0u8)]
-                            .into_py_dict(py)
-                            .as_ptr(),
+                        args.as_ptr(),
+                        kwargs.as_ptr(),
                         &mut [],
                     )
                     .unwrap_err()
@@ -745,14 +745,14 @@ mod tests {
         };
 
         Python::with_gil(|py| {
+            let args = PyTuple::new(py, Vec::<&PyAny>::new());
+            let kwargs = [(1u8.to_object(py).into_ref(py), 1u8)].into_py_dict(py);
             let err = unsafe {
                 function_description
                     .extract_arguments_tuple_dict::<NoVarargs, NoVarkeywords>(
                         py,
-                        PyTuple::new(py, Vec::<&PyAny>::new()).as_ptr(),
-                        [(1u8.to_object(py).into_ref(py), 1u8)]
-                            .into_py_dict(py)
-                            .as_ptr(),
+                        args.as_ptr(),
+                        kwargs.as_ptr(),
                         &mut [],
                     )
                     .unwrap_err()
@@ -776,11 +776,12 @@ mod tests {
         };
 
         Python::with_gil(|py| {
+            let args = PyTuple::new(py, Vec::<&PyAny>::new());
             let mut output = [None, None];
             let err = unsafe {
                 function_description.extract_arguments_tuple_dict::<NoVarargs, NoVarkeywords>(
                     py,
-                    PyTuple::new(py, Vec::<&PyAny>::new()).as_ptr(),
+                    args.as_ptr(),
                     std::ptr::null_mut(),
                     &mut output,
                 )
diff --git a/src/types/any.rs b/src/types/any.rs
index 4a91e88dfe0..5b08d496604 100644
--- a/src/types/any.rs
+++ b/src/types/any.rs
@@ -369,13 +369,17 @@ impl PyAny {
     where
         O: ToPyObject,
     {
-        unsafe {
-            self.py().from_owned_ptr_or_err(ffi::PyObject_RichCompare(
-                self.as_ptr(),
-                other.to_object(self.py()).as_ptr(),
-                compare_op as c_int,
-            ))
+        fn inner(slf: &PyAny, other: PyObject, compare_op: CompareOp) -> PyResult<&PyAny> {
+            unsafe {
+                slf.py().from_owned_ptr_or_err(ffi::PyObject_RichCompare(
+                    slf.as_ptr(),
+                    other.as_ptr(),
+                    compare_op as c_int,
+                ))
+            }
         }
+
+        inner(self, other.to_object(self.py()), compare_op)
     }
 
     /// Tests whether this object is less than another.
@@ -767,12 +771,14 @@ impl PyAny {
     where
         K: ToPyObject,
     {
-        unsafe {
-            self.py().from_owned_ptr_or_err(ffi::PyObject_GetItem(
-                self.as_ptr(),
-                key.to_object(self.py()).as_ptr(),
-            ))
+        fn inner(slf: &PyAny, key: PyObject) -> PyResult<&PyAny> {
+            unsafe {
+                slf.py()
+                    .from_owned_ptr_or_err(ffi::PyObject_GetItem(slf.as_ptr(), key.as_ptr()))
+            }
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Sets a collection item value.
@@ -783,17 +789,14 @@ impl PyAny {
         K: ToPyObject,
         V: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyObject_SetItem(
-                    self.as_ptr(),
-                    key.to_object(py).as_ptr(),
-                    value.to_object(py).as_ptr(),
-                ),
-            )
+        fn inner(slf: &PyAny, key: PyObject, value: PyObject) -> PyResult<()> {
+            err::error_on_minusone(slf.py(), unsafe {
+                ffi::PyObject_SetItem(slf.as_ptr(), key.as_ptr(), value.as_ptr())
+            })
         }
+
+        let py = self.py();
+        inner(self, key.to_object(py), value.to_object(py))
     }
 
     /// Deletes an item from the collection.
@@ -803,12 +806,13 @@ impl PyAny {
     where
         K: ToPyObject,
     {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PyObject_DelItem(self.as_ptr(), key.to_object(self.py()).as_ptr()),
-            )
+        fn inner(slf: &PyAny, key: PyObject) -> PyResult<()> {
+            err::error_on_minusone(slf.py(), unsafe {
+                ffi::PyObject_DelItem(slf.as_ptr(), key.as_ptr())
+            })
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Takes an object and returns an iterator for it.
diff --git a/src/types/dict.rs b/src/types/dict.rs
index 2506c84e223..11f44b0cbe8 100644
--- a/src/types/dict.rs
+++ b/src/types/dict.rs
@@ -124,13 +124,15 @@ impl PyDict {
     where
         K: ToPyObject,
     {
-        unsafe {
-            match ffi::PyDict_Contains(self.as_ptr(), key.to_object(self.py()).as_ptr()) {
+        fn inner(dict: &PyDict, key: PyObject) -> PyResult<bool> {
+            match unsafe { ffi::PyDict_Contains(dict.as_ptr(), key.as_ptr()) } {
                 1 => Ok(true),
                 0 => Ok(false),
-                _ => Err(PyErr::fetch(self.py())),
+                _ => Err(PyErr::fetch(dict.py())),
             }
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Gets an item from the dictionary.
@@ -193,17 +195,14 @@ impl PyDict {
         K: ToPyObject,
         V: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyDict_SetItem(
-                    self.as_ptr(),
-                    key.to_object(py).as_ptr(),
-                    value.to_object(py).as_ptr(),
-                ),
-            )
+        fn inner(dict: &PyDict, key: PyObject, value: PyObject) -> PyResult<()> {
+            err::error_on_minusone(dict.py(), unsafe {
+                ffi::PyDict_SetItem(dict.as_ptr(), key.as_ptr(), value.as_ptr())
+            })
         }
+
+        let py = self.py();
+        inner(self, key.to_object(py), value.to_object(py))
     }
 
     /// Deletes an item.
@@ -213,13 +212,13 @@ impl PyDict {
     where
         K: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyDict_DelItem(self.as_ptr(), key.to_object(py).as_ptr()),
-            )
+        fn inner(dict: &PyDict, key: PyObject) -> PyResult<()> {
+            err::error_on_minusone(dict.py(), unsafe {
+                ffi::PyDict_DelItem(dict.as_ptr(), key.as_ptr())
+            })
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Returns a list of dict keys.
diff --git a/src/types/frozenset.rs b/src/types/frozenset.rs
index 776999e84b7..22e6d1ff5d2 100644
--- a/src/types/frozenset.rs
+++ b/src/types/frozenset.rs
@@ -28,10 +28,13 @@ impl<'py> PyFrozenSetBuilder<'py> {
     where
         K: ToPyObject,
     {
-        let py = self.py_frozen_set.py();
-        err::error_on_minusone(py, unsafe {
-            ffi::PySet_Add(self.py_frozen_set.as_ptr(), key.to_object(py).as_ptr())
-        })
+        fn inner(frozenset: &PyFrozenSet, key: PyObject) -> PyResult<()> {
+            err::error_on_minusone(frozenset.py(), unsafe {
+                ffi::PySet_Add(frozenset.as_ptr(), key.as_ptr())
+            })
+        }
+
+        inner(self.py_frozen_set, key.to_object(self.py_frozen_set.py()))
     }
 
     /// Finish building the set and take ownership of its current value
@@ -94,13 +97,15 @@ impl PyFrozenSet {
     where
         K: ToPyObject,
     {
-        unsafe {
-            match ffi::PySet_Contains(self.as_ptr(), key.to_object(self.py()).as_ptr()) {
+        fn inner(frozenset: &PyFrozenSet, key: PyObject) -> PyResult<bool> {
+            match unsafe { ffi::PySet_Contains(frozenset.as_ptr(), key.as_ptr()) } {
                 1 => Ok(true),
                 0 => Ok(false),
-                _ => Err(PyErr::fetch(self.py())),
+                _ => Err(PyErr::fetch(frozenset.py())),
             }
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Returns an iterator of values in this frozen set.
diff --git a/src/types/list.rs b/src/types/list.rs
index 1b52a614969..afcf2469c31 100644
--- a/src/types/list.rs
+++ b/src/types/list.rs
@@ -218,13 +218,13 @@ impl PyList {
     where
         I: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyList_Append(self.as_ptr(), item.to_object(py).as_ptr()),
-            )
+        fn inner(list: &PyList, item: PyObject) -> PyResult<()> {
+            err::error_on_minusone(list.py(), unsafe {
+                ffi::PyList_Append(list.as_ptr(), item.as_ptr())
+            })
         }
+
+        inner(self, item.to_object(self.py()))
     }
 
     /// Inserts an item at the specified index.
@@ -234,17 +234,13 @@ impl PyList {
     where
         I: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyList_Insert(
-                    self.as_ptr(),
-                    get_ssize_index(index),
-                    item.to_object(py).as_ptr(),
-                ),
-            )
+        fn inner(list: &PyList, index: usize, item: PyObject) -> PyResult<()> {
+            err::error_on_minusone(list.py(), unsafe {
+                ffi::PyList_Insert(list.as_ptr(), get_ssize_index(index), item.as_ptr())
+            })
         }
+
+        inner(self, index, item.to_object(self.py()))
     }
 
     /// Determines if self contains `value`.
diff --git a/src/types/sequence.rs b/src/types/sequence.rs
index e820c31f440..b43bc60d1a3 100644
--- a/src/types/sequence.rs
+++ b/src/types/sequence.rs
@@ -6,7 +6,7 @@ use crate::internal_tricks::get_ssize_index;
 use crate::sync::GILOnceCell;
 use crate::type_object::PyTypeInfo;
 use crate::types::{PyAny, PyList, PyString, PyTuple, PyType};
-use crate::{ffi, PyNativeType, ToPyObject};
+use crate::{ffi, PyNativeType, PyObject, ToPyObject};
 use crate::{AsPyPointer, IntoPyPointer, Py, Python};
 use crate::{FromPyObject, PyTryFrom};
 
@@ -128,17 +128,13 @@ impl PySequence {
     where
         I: ToPyObject,
     {
-        let py = self.py();
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PySequence_SetItem(
-                    self.as_ptr(),
-                    get_ssize_index(i),
-                    item.to_object(py).as_ptr(),
-                ),
-            )
+        fn inner(seq: &PySequence, i: usize, item: PyObject) -> PyResult<()> {
+            err::error_on_minusone(seq.py(), unsafe {
+                ffi::PySequence_SetItem(seq.as_ptr(), get_ssize_index(i), item.as_ptr())
+            })
         }
+
+        inner(self, i, item.to_object(self.py()))
     }
 
     /// Deletes the `i`th element of self.
@@ -193,13 +189,16 @@ impl PySequence {
     where
         V: ToPyObject,
     {
-        let r =
-            unsafe { ffi::PySequence_Count(self.as_ptr(), value.to_object(self.py()).as_ptr()) };
-        if r == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(r as usize)
+        fn inner(seq: &PySequence, value: PyObject) -> PyResult<usize> {
+            let r = unsafe { ffi::PySequence_Count(seq.as_ptr(), value.as_ptr()) };
+            if r == -1 {
+                Err(PyErr::fetch(seq.py()))
+            } else {
+                Ok(r as usize)
+            }
         }
+
+        inner(self, value.to_object(self.py()))
     }
 
     /// Determines if self contains `value`.
@@ -210,13 +209,16 @@ impl PySequence {
     where
         V: ToPyObject,
     {
-        let r =
-            unsafe { ffi::PySequence_Contains(self.as_ptr(), value.to_object(self.py()).as_ptr()) };
-        match r {
-            0 => Ok(false),
-            1 => Ok(true),
-            _ => Err(PyErr::fetch(self.py())),
+        fn inner(seq: &PySequence, value: PyObject) -> PyResult<bool> {
+            let r = unsafe { ffi::PySequence_Contains(seq.as_ptr(), value.as_ptr()) };
+            match r {
+                0 => Ok(false),
+                1 => Ok(true),
+                _ => Err(PyErr::fetch(seq.py())),
+            }
         }
+
+        inner(self, value.to_object(self.py()))
     }
 
     /// Returns the first index `i` for which `self[i] == value`.
@@ -227,13 +229,16 @@ impl PySequence {
     where
         V: ToPyObject,
     {
-        let r =
-            unsafe { ffi::PySequence_Index(self.as_ptr(), value.to_object(self.py()).as_ptr()) };
-        if r == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(r as usize)
+        fn inner(seq: &PySequence, value: PyObject) -> PyResult<usize> {
+            let r = unsafe { ffi::PySequence_Index(seq.as_ptr(), value.as_ptr()) };
+            if r == -1 {
+                Err(PyErr::fetch(seq.py()))
+            } else {
+                Ok(r as usize)
+            }
         }
+
+        inner(self, value.to_object(self.py()))
     }
 
     /// Returns a fresh list based on the Sequence.
diff --git a/src/types/set.rs b/src/types/set.rs
index 8dd73655175..fc26c834bc1 100644
--- a/src/types/set.rs
+++ b/src/types/set.rs
@@ -71,13 +71,15 @@ impl PySet {
     where
         K: ToPyObject,
     {
-        unsafe {
-            match ffi::PySet_Contains(self.as_ptr(), key.to_object(self.py()).as_ptr()) {
+        fn inner(set: &PySet, key: PyObject) -> PyResult<bool> {
+            match unsafe { ffi::PySet_Contains(set.as_ptr(), key.as_ptr()) } {
                 1 => Ok(true),
                 0 => Ok(false),
-                _ => Err(PyErr::fetch(self.py())),
+                _ => Err(PyErr::fetch(set.py())),
             }
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Removes the element from the set if it is present.
@@ -88,12 +90,10 @@ impl PySet {
         K: ToPyObject,
     {
         fn inner(set: &PySet, key: PyObject) -> PyResult<bool> {
-            unsafe {
-                match ffi::PySet_Discard(set.as_ptr(), key.as_ptr()) {
-                    1 => Ok(true),
-                    0 => Ok(false),
-                    _ => Err(PyErr::fetch(set.py())),
-                }
+            match unsafe { ffi::PySet_Discard(set.as_ptr(), key.as_ptr()) } {
+                1 => Ok(true),
+                0 => Ok(false),
+                _ => Err(PyErr::fetch(set.py())),
             }
         }
 
@@ -105,12 +105,13 @@ impl PySet {
     where
         K: ToPyObject,
     {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PySet_Add(self.as_ptr(), key.to_object(self.py()).as_ptr()),
-            )
+        fn inner(set: &PySet, key: PyObject) -> PyResult<()> {
+            err::error_on_minusone(set.py(), unsafe {
+                ffi::PySet_Add(set.as_ptr(), key.as_ptr())
+            })
         }
+
+        inner(self, key.to_object(self.py()))
     }
 
     /// Removes and returns an arbitrary element from the set.

From 43477a8e306aa408c6c796095fda45ed17f90cd5 Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 4 Jul 2023 20:34:23 +0100
Subject: [PATCH 4/6] use error_on_minusone in more cases

---
 src/conversions/std/osstr.rs |  6 +-----
 src/err/mod.rs               | 24 +++++++++++++++++++++---
 src/types/any.rs             | 14 ++++----------
 src/types/mapping.rs         |  7 ++-----
 src/types/sequence.rs        | 21 ++++++---------------
 5 files changed, 34 insertions(+), 38 deletions(-)

diff --git a/src/conversions/std/osstr.rs b/src/conversions/std/osstr.rs
index 12d00ffc431..f91822a874f 100644
--- a/src/conversions/std/osstr.rs
+++ b/src/conversions/std/osstr.rs
@@ -1,6 +1,4 @@
 use crate::types::PyString;
-#[cfg(windows)]
-use crate::PyErr;
 use crate::{
     ffi, AsPyPointer, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, ToPyObject,
 };
@@ -92,9 +90,7 @@ impl FromPyObject<'_> for OsString {
             // ourselves
             let size =
                 unsafe { ffi::PyUnicode_AsWideChar(pystring.as_ptr(), std::ptr::null_mut(), 0) };
-            if size == -1 {
-                return Err(PyErr::fetch(ob.py()));
-            }
+            crate::err::error_on_minusone(ob.py(), size)?;
 
             let mut buffer = vec![0; size as usize];
             let bytes_read =
diff --git a/src/err/mod.rs b/src/err/mod.rs
index 05072601f78..9eee0a67faa 100644
--- a/src/err/mod.rs
+++ b/src/err/mod.rs
@@ -9,7 +9,6 @@ use crate::{AsPyPointer, IntoPy, IntoPyPointer, Py, PyAny, PyObject, Python, ToP
 use std::borrow::Cow;
 use std::cell::UnsafeCell;
 use std::ffi::CString;
-use std::os::raw::c_int;
 
 mod err_state;
 mod impls;
@@ -792,14 +791,33 @@ pub fn panic_after_error(_py: Python<'_>) -> ! {
 
 /// Returns Ok if the error code is not -1.
 #[inline]
-pub fn error_on_minusone(py: Python<'_>, result: c_int) -> PyResult<()> {
-    if result != -1 {
+pub(crate) fn error_on_minusone<T: SignedInteger>(py: Python<'_>, result: T) -> PyResult<()> {
+    if result != T::MINUS_ONE {
         Ok(())
     } else {
         Err(PyErr::fetch(py))
     }
 }
 
+pub(crate) trait SignedInteger: Eq {
+    const MINUS_ONE: Self;
+}
+
+macro_rules! impl_signed_integer {
+    ($t:ty) => {
+        impl SignedInteger for $t {
+            const MINUS_ONE: Self = -1;
+        }
+    };
+}
+
+impl_signed_integer!(i8);
+impl_signed_integer!(i16);
+impl_signed_integer!(i32);
+impl_signed_integer!(i64);
+impl_signed_integer!(i128);
+impl_signed_integer!(isize);
+
 #[inline]
 fn exceptions_must_derive_from_base_exception(py: Python<'_>) -> PyErr {
     PyErr::from_state(PyErrState::exceptions_must_derive_from_base_exception(py))
diff --git a/src/types/any.rs b/src/types/any.rs
index 5b08d496604..69e2c3f59a7 100644
--- a/src/types/any.rs
+++ b/src/types/any.rs
@@ -947,11 +947,8 @@ impl PyAny {
     /// This is equivalent to the Python expression `hash(self)`.
     pub fn hash(&self) -> PyResult<isize> {
         let v = unsafe { ffi::PyObject_Hash(self.as_ptr()) };
-        if v == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(v)
-        }
+        crate::err::error_on_minusone(self.py(), v)?;
+        Ok(v)
     }
 
     /// Returns the length of the sequence or mapping.
@@ -959,11 +956,8 @@ impl PyAny {
     /// This is equivalent to the Python expression `len(self)`.
     pub fn len(&self) -> PyResult<usize> {
         let v = unsafe { ffi::PyObject_Size(self.as_ptr()) };
-        if v == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(v as usize)
-        }
+        crate::err::error_on_minusone(self.py(), v)?;
+        Ok(v as usize)
     }
 
     /// Returns the list of attributes of this object.
diff --git a/src/types/mapping.rs b/src/types/mapping.rs
index fa73176d142..f580e7ffe55 100644
--- a/src/types/mapping.rs
+++ b/src/types/mapping.rs
@@ -17,11 +17,8 @@ impl PyMapping {
     #[inline]
     pub fn len(&self) -> PyResult<usize> {
         let v = unsafe { ffi::PyMapping_Size(self.as_ptr()) };
-        if v == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(v as usize)
-        }
+        crate::err::error_on_minusone(self.py(), v)?;
+        Ok(v as usize)
     }
 
     /// Returns whether the mapping is empty.
diff --git a/src/types/sequence.rs b/src/types/sequence.rs
index b43bc60d1a3..91ca703987d 100644
--- a/src/types/sequence.rs
+++ b/src/types/sequence.rs
@@ -23,11 +23,8 @@ impl PySequence {
     #[inline]
     pub fn len(&self) -> PyResult<usize> {
         let v = unsafe { ffi::PySequence_Size(self.as_ptr()) };
-        if v == -1 {
-            Err(PyErr::fetch(self.py()))
-        } else {
-            Ok(v as usize)
-        }
+        crate::err::error_on_minusone(self.py(), v)?;
+        Ok(v as usize)
     }
 
     /// Returns whether the sequence is empty.
@@ -191,11 +188,8 @@ impl PySequence {
     {
         fn inner(seq: &PySequence, value: PyObject) -> PyResult<usize> {
             let r = unsafe { ffi::PySequence_Count(seq.as_ptr(), value.as_ptr()) };
-            if r == -1 {
-                Err(PyErr::fetch(seq.py()))
-            } else {
-                Ok(r as usize)
-            }
+            crate::err::error_on_minusone(seq.py(), r)?;
+            Ok(r as usize)
         }
 
         inner(self, value.to_object(self.py()))
@@ -231,11 +225,8 @@ impl PySequence {
     {
         fn inner(seq: &PySequence, value: PyObject) -> PyResult<usize> {
             let r = unsafe { ffi::PySequence_Index(seq.as_ptr(), value.as_ptr()) };
-            if r == -1 {
-                Err(PyErr::fetch(seq.py()))
-            } else {
-                Ok(r as usize)
-            }
+            crate::err::error_on_minusone(seq.py(), r)?;
+            Ok(r as usize)
         }
 
         inner(self, value.to_object(self.py()))

From 76f3a395ef1beabc4c4378f84683b48d5737633b Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 4 Jul 2023 20:50:54 +0100
Subject: [PATCH 5/6] move `unsafe` block inside `error_on_minusone` calls

---
 src/buffer.rs                         | 119 ++++++++++++--------------
 src/conversions/std/num.rs            |  43 +++++-----
 src/err/mod.rs                        |  36 ++++----
 src/impl_/pyclass/lazy_type_object.rs |   5 +-
 src/instance.rs                       |   9 +-
 src/marker.rs                         |   3 +-
 src/types/any.rs                      |  26 +++---
 src/types/dict.rs                     |  21 ++---
 src/types/frozenset.rs                |   4 +-
 src/types/list.rs                     |  36 ++++----
 src/types/mapping.rs                  |   2 +-
 src/types/sequence.rs                 |  35 +++-----
 src/types/set.rs                      |   4 +-
 13 files changed, 154 insertions(+), 189 deletions(-)

diff --git a/src/buffer.rs b/src/buffer.rs
index 79e416ba939..eb48a7564c6 100644
--- a/src/buffer.rs
+++ b/src/buffer.rs
@@ -193,14 +193,13 @@ impl<T: Element> PyBuffer<T> {
     pub fn get(obj: &PyAny) -> PyResult<PyBuffer<T>> {
         // TODO: use nightly API Box::new_uninit() once stable
         let mut buf = Box::new(mem::MaybeUninit::uninit());
-        let buf: Box<ffi::Py_buffer> = unsafe {
-            err::error_on_minusone(
-                obj.py(),
-                ffi::PyObject_GetBuffer(obj.as_ptr(), buf.as_mut_ptr(), ffi::PyBUF_FULL_RO),
-            )?;
+        let buf: Box<ffi::Py_buffer> = {
+            err::error_on_minusone(obj.py(), unsafe {
+                ffi::PyObject_GetBuffer(obj.as_ptr(), buf.as_mut_ptr(), ffi::PyBUF_FULL_RO)
+            })?;
             // Safety: buf is initialized by PyObject_GetBuffer.
             // TODO: use nightly API Box::assume_init() once stable
-            mem::transmute(buf)
+            unsafe { mem::transmute(buf) }
         };
         // Create PyBuffer immediately so that if validation checks fail, the PyBuffer::drop code
         // will call PyBuffer_Release (thus avoiding any leaks).
@@ -493,22 +492,20 @@ impl<T: Element> PyBuffer<T> {
                 self.item_count()
             )));
         }
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyBuffer_ToContiguous(
-                    target.as_ptr() as *mut raw::c_void,
-                    #[cfg(Py_3_11)]
-                    &*self.0,
-                    #[cfg(not(Py_3_11))]
-                    {
-                        &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
-                    },
-                    self.0.len,
-                    fort as std::os::raw::c_char,
-                ),
+
+        err::error_on_minusone(py, unsafe {
+            ffi::PyBuffer_ToContiguous(
+                target.as_ptr() as *mut raw::c_void,
+                #[cfg(Py_3_11)]
+                &*self.0,
+                #[cfg(not(Py_3_11))]
+                {
+                    &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
+                },
+                self.0.len,
+                fort as std::os::raw::c_char,
             )
-        }
+        })
     }
 
     /// Copies the buffer elements to a newly allocated vector.
@@ -530,26 +527,24 @@ impl<T: Element> PyBuffer<T> {
     fn _to_vec(&self, py: Python<'_>, fort: u8) -> PyResult<Vec<T>> {
         let item_count = self.item_count();
         let mut vec: Vec<T> = Vec::with_capacity(item_count);
-        unsafe {
-            // Copy the buffer into the uninitialized space in the vector.
-            // Due to T:Copy, we don't need to be concerned with Drop impls.
-            err::error_on_minusone(
-                py,
-                ffi::PyBuffer_ToContiguous(
-                    vec.as_ptr() as *mut raw::c_void,
-                    #[cfg(Py_3_11)]
-                    &*self.0,
-                    #[cfg(not(Py_3_11))]
-                    {
-                        &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
-                    },
-                    self.0.len,
-                    fort as std::os::raw::c_char,
-                ),
-            )?;
-            // set vector length to mark the now-initialized space as usable
-            vec.set_len(item_count);
-        }
+
+        // Copy the buffer into the uninitialized space in the vector.
+        // Due to T:Copy, we don't need to be concerned with Drop impls.
+        err::error_on_minusone(py, unsafe {
+            ffi::PyBuffer_ToContiguous(
+                vec.as_ptr() as *mut raw::c_void,
+                #[cfg(Py_3_11)]
+                &*self.0,
+                #[cfg(not(Py_3_11))]
+                {
+                    &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
+                },
+                self.0.len,
+                fort as std::os::raw::c_char,
+            )
+        })?;
+        // set vector length to mark the now-initialized space as usable
+        unsafe { vec.set_len(item_count) };
         Ok(vec)
     }
 
@@ -591,29 +586,27 @@ impl<T: Element> PyBuffer<T> {
                 self.item_count()
             )));
         }
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyBuffer_FromContiguous(
-                    #[cfg(Py_3_11)]
-                    &*self.0,
-                    #[cfg(not(Py_3_11))]
-                    {
-                        &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
-                    },
-                    #[cfg(Py_3_11)]
-                    {
-                        source.as_ptr() as *const raw::c_void
-                    },
-                    #[cfg(not(Py_3_11))]
-                    {
-                        source.as_ptr() as *mut raw::c_void
-                    },
-                    self.0.len,
-                    fort as std::os::raw::c_char,
-                ),
+
+        err::error_on_minusone(py, unsafe {
+            ffi::PyBuffer_FromContiguous(
+                #[cfg(Py_3_11)]
+                &*self.0,
+                #[cfg(not(Py_3_11))]
+                {
+                    &*self.0 as *const ffi::Py_buffer as *mut ffi::Py_buffer
+                },
+                #[cfg(Py_3_11)]
+                {
+                    source.as_ptr() as *const raw::c_void
+                },
+                #[cfg(not(Py_3_11))]
+                {
+                    source.as_ptr() as *mut raw::c_void
+                },
+                self.0.len,
+                fort as std::os::raw::c_char,
             )
-        }
+        })
     }
 
     /// Releases the buffer object, freeing the reference to the Python object
diff --git a/src/conversions/std/num.rs b/src/conversions/std/num.rs
index 11c184f7373..3427942ee11 100644
--- a/src/conversions/std/num.rs
+++ b/src/conversions/std/num.rs
@@ -178,16 +178,18 @@ mod fast_128bit_int_conversion {
             }
             impl IntoPy<PyObject> for $rust_type {
                 fn into_py(self, py: Python<'_>) -> PyObject {
+                    // Always use little endian
+                    let bytes = self.to_le_bytes();
                     unsafe {
-                        // Always use little endian
-                        let bytes = self.to_le_bytes();
-                        let obj = ffi::_PyLong_FromByteArray(
-                            bytes.as_ptr() as *const std::os::raw::c_uchar,
-                            bytes.len(),
-                            1,
-                            $is_signed,
-                        );
-                        PyObject::from_owned_ptr(py, obj)
+                        PyObject::from_owned_ptr(
+                            py,
+                            ffi::_PyLong_FromByteArray(
+                                bytes.as_ptr() as *const std::os::raw::c_uchar,
+                                bytes.len(),
+                                1,
+                                $is_signed,
+                            ),
+                        )
                     }
                 }
 
@@ -199,23 +201,20 @@ mod fast_128bit_int_conversion {
 
             impl<'source> FromPyObject<'source> for $rust_type {
                 fn extract(ob: &'source PyAny) -> PyResult<$rust_type> {
-                    unsafe {
-                        let num = ffi::PyNumber_Index(ob.as_ptr());
-                        if num.is_null() {
-                            return Err(PyErr::fetch(ob.py()));
-                        }
-                        let mut buffer = [0; std::mem::size_of::<$rust_type>()];
-                        let ok = ffi::_PyLong_AsByteArray(
-                            num as *mut ffi::PyLongObject,
+                    let num = unsafe {
+                        PyObject::from_owned_ptr_or_err(ob.py(), ffi::PyNumber_Index(ob.as_ptr()))?
+                    };
+                    let mut buffer = [0; std::mem::size_of::<$rust_type>()];
+                    crate::err::error_on_minusone(ob.py(), unsafe {
+                        ffi::_PyLong_AsByteArray(
+                            num.as_ptr() as *mut ffi::PyLongObject,
                             buffer.as_mut_ptr(),
                             buffer.len(),
                             1,
                             $is_signed,
-                        );
-                        ffi::Py_DECREF(num);
-                        crate::err::error_on_minusone(ob.py(), ok)?;
-                        Ok(<$rust_type>::from_le_bytes(buffer))
-                    }
+                        )
+                    })?;
+                    Ok(<$rust_type>::from_le_bytes(buffer))
                 }
 
                 #[cfg(feature = "experimental-inspect")]
diff --git a/src/err/mod.rs b/src/err/mod.rs
index 9eee0a67faa..28f2b3295cb 100644
--- a/src/err/mod.rs
+++ b/src/err/mod.rs
@@ -529,16 +529,13 @@ impl PyErr {
     /// ```
     pub fn warn(py: Python<'_>, category: &PyAny, message: &str, stacklevel: i32) -> PyResult<()> {
         let message = CString::new(message)?;
-        unsafe {
-            error_on_minusone(
-                py,
-                ffi::PyErr_WarnEx(
-                    category.as_ptr(),
-                    message.as_ptr(),
-                    stacklevel as ffi::Py_ssize_t,
-                ),
+        error_on_minusone(py, unsafe {
+            ffi::PyErr_WarnEx(
+                category.as_ptr(),
+                message.as_ptr(),
+                stacklevel as ffi::Py_ssize_t,
             )
-        }
+        })
     }
 
     /// Issues a warning message, with more control over the warning attributes.
@@ -569,19 +566,16 @@ impl PyErr {
             None => std::ptr::null_mut(),
             Some(obj) => obj.as_ptr(),
         };
-        unsafe {
-            error_on_minusone(
-                py,
-                ffi::PyErr_WarnExplicit(
-                    category.as_ptr(),
-                    message.as_ptr(),
-                    filename.as_ptr(),
-                    lineno,
-                    module_ptr,
-                    registry,
-                ),
+        error_on_minusone(py, unsafe {
+            ffi::PyErr_WarnExplicit(
+                category.as_ptr(),
+                message.as_ptr(),
+                filename.as_ptr(),
+                lineno,
+                module_ptr,
+                registry,
             )
-        }
+        })
     }
 
     /// Clone the PyErr. This requires the GIL, which is why PyErr does not implement Clone.
diff --git a/src/impl_/pyclass/lazy_type_object.rs b/src/impl_/pyclass/lazy_type_object.rs
index 551b052ae71..0e606b588d0 100644
--- a/src/impl_/pyclass/lazy_type_object.rs
+++ b/src/impl_/pyclass/lazy_type_object.rs
@@ -202,8 +202,9 @@ fn initialize_tp_dict(
     // We hold the GIL: the dictionary update can be considered atomic from
     // the POV of other threads.
     for (key, val) in items {
-        let ret = unsafe { ffi::PyObject_SetAttrString(type_object, key.as_ptr(), val.into_ptr()) };
-        crate::err::error_on_minusone(py, ret)?;
+        crate::err::error_on_minusone(py, unsafe {
+            ffi::PyObject_SetAttrString(type_object, key.as_ptr(), val.into_ptr())
+        })?;
     }
     Ok(())
 }
diff --git a/src/instance.rs b/src/instance.rs
index fe9d9682d0e..c75574ba8fe 100644
--- a/src/instance.rs
+++ b/src/instance.rs
@@ -650,12 +650,9 @@ impl<T> Py<T> {
         let attr_name = attr_name.into_py(py);
         let value = value.into_py(py);
 
-        unsafe {
-            err::error_on_minusone(
-                py,
-                ffi::PyObject_SetAttr(self.as_ptr(), attr_name.as_ptr(), value.as_ptr()),
-            )
-        }
+        err::error_on_minusone(py, unsafe {
+            ffi::PyObject_SetAttr(self.as_ptr(), attr_name.as_ptr(), value.as_ptr())
+        })
     }
 
     /// Calls the object.
diff --git a/src/marker.rs b/src/marker.rs
index 33bc987dafe..fb1d747284a 100644
--- a/src/marker.rs
+++ b/src/marker.rs
@@ -876,8 +876,7 @@ impl<'py> Python<'py> {
     /// [1]: https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_checksignals#c.PyErr_CheckSignals
     /// [2]: https://docs.python.org/3/library/signal.html
     pub fn check_signals(self) -> PyResult<()> {
-        let v = unsafe { ffi::PyErr_CheckSignals() };
-        err::error_on_minusone(self, v)
+        err::error_on_minusone(self, unsafe { ffi::PyErr_CheckSignals() })
     }
 
     /// Create a new pool for managing PyO3's owned references.
diff --git a/src/types/any.rs b/src/types/any.rs
index 69e2c3f59a7..2c0b17892ce 100644
--- a/src/types/any.rs
+++ b/src/types/any.rs
@@ -227,14 +227,14 @@ impl PyAny {
         N: IntoPy<Py<PyString>>,
         V: ToPyObject,
     {
-        let py = self.py();
-        let attr_name = attr_name.into_py(py);
-        let value = value.to_object(py);
-
-        unsafe {
-            let ret = ffi::PyObject_SetAttr(self.as_ptr(), attr_name.as_ptr(), value.as_ptr());
-            err::error_on_minusone(py, ret)
+        fn inner(any: &PyAny, attr_name: Py<PyString>, value: PyObject) -> PyResult<()> {
+            err::error_on_minusone(any.py(), unsafe {
+                ffi::PyObject_SetAttr(any.as_ptr(), attr_name.as_ptr(), value.as_ptr())
+            })
         }
+
+        let py = self.py();
+        inner(self, attr_name.into_py(py), value.to_object(py))
     }
 
     /// Deletes an attribute.
@@ -247,13 +247,13 @@ impl PyAny {
     where
         N: IntoPy<Py<PyString>>,
     {
-        let py = self.py();
-        let attr_name = attr_name.into_py(py);
-
-        unsafe {
-            let ret = ffi::PyObject_DelAttr(self.as_ptr(), attr_name.as_ptr());
-            err::error_on_minusone(py, ret)
+        fn inner(any: &PyAny, attr_name: Py<PyString>) -> PyResult<()> {
+            err::error_on_minusone(any.py(), unsafe {
+                ffi::PyObject_DelAttr(any.as_ptr(), attr_name.as_ptr())
+            })
         }
+
+        inner(self, attr_name.into_py(self.py()))
     }
 
     /// Returns an [`Ordering`] between `self` and `other`.
diff --git a/src/types/dict.rs b/src/types/dict.rs
index 11f44b0cbe8..4670f52229f 100644
--- a/src/types/dict.rs
+++ b/src/types/dict.rs
@@ -68,14 +68,11 @@ impl PyDict {
     /// this keeps the last entry seen.
     #[cfg(not(PyPy))]
     pub fn from_sequence(py: Python<'_>, seq: PyObject) -> PyResult<&PyDict> {
-        unsafe {
-            let dict = py.from_owned_ptr::<PyDict>(ffi::PyDict_New());
-            err::error_on_minusone(
-                py,
-                ffi::PyDict_MergeFromSeq2(dict.into_ptr(), seq.into_ptr(), 1),
-            )?;
-            Ok(dict)
-        }
+        let dict = Self::new(py);
+        err::error_on_minusone(py, unsafe {
+            ffi::PyDict_MergeFromSeq2(dict.into_ptr(), seq.into_ptr(), 1)
+        })?;
+        Ok(dict)
     }
 
     /// Returns a new dictionary that contains the same key-value pairs as self.
@@ -273,7 +270,9 @@ impl PyDict {
     /// to use `self.update(other.as_mapping())`, note: `PyDict::as_mapping` is a zero-cost conversion.
     pub fn update(&self, other: &PyMapping) -> PyResult<()> {
         let py = self.py();
-        unsafe { err::error_on_minusone(py, ffi::PyDict_Update(self.as_ptr(), other.as_ptr())) }
+        err::error_on_minusone(py, unsafe {
+            ffi::PyDict_Update(self.as_ptr(), other.as_ptr())
+        })
     }
 
     /// Add key/value pairs from another dictionary to this one only when they do not exist in this.
@@ -286,7 +285,9 @@ impl PyDict {
     /// so should have the same performance as `update`.
     pub fn update_if_missing(&self, other: &PyMapping) -> PyResult<()> {
         let py = self.py();
-        unsafe { err::error_on_minusone(py, ffi::PyDict_Merge(self.as_ptr(), other.as_ptr(), 0)) }
+        err::error_on_minusone(py, unsafe {
+            ffi::PyDict_Merge(self.as_ptr(), other.as_ptr(), 0)
+        })
     }
 }
 
diff --git a/src/types/frozenset.rs b/src/types/frozenset.rs
index 22e6d1ff5d2..12bdf1d85e5 100644
--- a/src/types/frozenset.rs
+++ b/src/types/frozenset.rs
@@ -213,9 +213,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
         let ptr = set.as_ptr();
 
         for obj in elements {
-            unsafe {
-                err::error_on_minusone(py, ffi::PySet_Add(ptr, obj.as_ptr()))?;
-            }
+            err::error_on_minusone(py, unsafe { ffi::PySet_Add(ptr, obj.as_ptr()) })?;
         }
 
         Ok(set)
diff --git a/src/types/list.rs b/src/types/list.rs
index afcf2469c31..727e60696ef 100644
--- a/src/types/list.rs
+++ b/src/types/list.rs
@@ -167,16 +167,13 @@ impl PyList {
     where
         I: ToPyObject,
     {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PyList_SetItem(
-                    self.as_ptr(),
-                    get_ssize_index(index),
-                    item.to_object(self.py()).into_ptr(),
-                ),
-            )
+        fn inner(list: &PyList, index: usize, item: PyObject) -> PyResult<()> {
+            err::error_on_minusone(list.py(), unsafe {
+                ffi::PyList_SetItem(list.as_ptr(), get_ssize_index(index), item.into_ptr())
+            })
         }
+
+        inner(self, index, item.to_object(self.py()))
     }
 
     /// Deletes the `index`th element of self.
@@ -192,17 +189,14 @@ impl PyList {
     /// This is equivalent to the Python statement `self[low:high] = v`.
     #[inline]
     pub fn set_slice(&self, low: usize, high: usize, seq: &PyAny) -> PyResult<()> {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PyList_SetSlice(
-                    self.as_ptr(),
-                    get_ssize_index(low),
-                    get_ssize_index(high),
-                    seq.as_ptr(),
-                ),
+        err::error_on_minusone(self.py(), unsafe {
+            ffi::PyList_SetSlice(
+                self.as_ptr(),
+                get_ssize_index(low),
+                get_ssize_index(high),
+                seq.as_ptr(),
             )
-        }
+        })
     }
 
     /// Deletes the slice from `low` to `high` from `self`.
@@ -275,12 +269,12 @@ impl PyList {
 
     /// Sorts the list in-place. Equivalent to the Python expression `l.sort()`.
     pub fn sort(&self) -> PyResult<()> {
-        unsafe { err::error_on_minusone(self.py(), ffi::PyList_Sort(self.as_ptr())) }
+        err::error_on_minusone(self.py(), unsafe { ffi::PyList_Sort(self.as_ptr()) })
     }
 
     /// Reverses the list in-place. Equivalent to the Python expression `l.reverse()`.
     pub fn reverse(&self) -> PyResult<()> {
-        unsafe { err::error_on_minusone(self.py(), ffi::PyList_Reverse(self.as_ptr())) }
+        err::error_on_minusone(self.py(), unsafe { ffi::PyList_Reverse(self.as_ptr()) })
     }
 
     /// Return a new tuple containing the contents of the list; equivalent to the Python expression `tuple(list)`.
diff --git a/src/types/mapping.rs b/src/types/mapping.rs
index f580e7ffe55..b947f619a22 100644
--- a/src/types/mapping.rs
+++ b/src/types/mapping.rs
@@ -1,4 +1,4 @@
-use crate::err::{PyDowncastError, PyErr, PyResult};
+use crate::err::{PyDowncastError, PyResult};
 use crate::sync::GILOnceCell;
 use crate::type_object::PyTypeInfo;
 use crate::types::{PyAny, PyDict, PySequence, PyType};
diff --git a/src/types/sequence.rs b/src/types/sequence.rs
index 91ca703987d..ec4d72f0692 100644
--- a/src/types/sequence.rs
+++ b/src/types/sequence.rs
@@ -139,12 +139,9 @@ impl PySequence {
     /// This is equivalent to the Python statement `del self[i]`.
     #[inline]
     pub fn del_item(&self, i: usize) -> PyResult<()> {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PySequence_DelItem(self.as_ptr(), get_ssize_index(i)),
-            )
-        }
+        err::error_on_minusone(self.py(), unsafe {
+            ffi::PySequence_DelItem(self.as_ptr(), get_ssize_index(i))
+        })
     }
 
     /// Assigns the sequence `v` to the slice of `self` from `i1` to `i2`.
@@ -152,17 +149,14 @@ impl PySequence {
     /// This is equivalent to the Python statement `self[i1:i2] = v`.
     #[inline]
     pub fn set_slice(&self, i1: usize, i2: usize, v: &PyAny) -> PyResult<()> {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PySequence_SetSlice(
-                    self.as_ptr(),
-                    get_ssize_index(i1),
-                    get_ssize_index(i2),
-                    v.as_ptr(),
-                ),
+        err::error_on_minusone(self.py(), unsafe {
+            ffi::PySequence_SetSlice(
+                self.as_ptr(),
+                get_ssize_index(i1),
+                get_ssize_index(i2),
+                v.as_ptr(),
             )
-        }
+        })
     }
 
     /// Deletes the slice from `i1` to `i2` from `self`.
@@ -170,12 +164,9 @@ impl PySequence {
     /// This is equivalent to the Python statement `del self[i1:i2]`.
     #[inline]
     pub fn del_slice(&self, i1: usize, i2: usize) -> PyResult<()> {
-        unsafe {
-            err::error_on_minusone(
-                self.py(),
-                ffi::PySequence_DelSlice(self.as_ptr(), get_ssize_index(i1), get_ssize_index(i2)),
-            )
-        }
+        err::error_on_minusone(self.py(), unsafe {
+            ffi::PySequence_DelSlice(self.as_ptr(), get_ssize_index(i1), get_ssize_index(i2))
+        })
     }
 
     /// Returns the number of occurrences of `value` in self, that is, return the
diff --git a/src/types/set.rs b/src/types/set.rs
index fc26c834bc1..b84ae440900 100644
--- a/src/types/set.rs
+++ b/src/types/set.rs
@@ -257,9 +257,7 @@ pub(crate) fn new_from_iter<T: ToPyObject>(
         let ptr = set.as_ptr();
 
         for obj in elements {
-            unsafe {
-                err::error_on_minusone(py, ffi::PySet_Add(ptr, obj.as_ptr()))?;
-            }
+            err::error_on_minusone(py, unsafe { ffi::PySet_Add(ptr, obj.as_ptr()) })?;
         }
 
         Ok(set)

From 81c0328d9123bf5da73441e795708d5c7b57d6ef Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Tue, 4 Jul 2023 20:54:42 +0100
Subject: [PATCH 6/6] use concrete inner for `PyErr:matches`

---
 src/err/mod.rs | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/err/mod.rs b/src/err/mod.rs
index 28f2b3295cb..3af7b92bbc8 100644
--- a/src/err/mod.rs
+++ b/src/err/mod.rs
@@ -438,14 +438,16 @@ impl PyErr {
     where
         T: ToPyObject,
     {
-        let exc = exc.to_object(py);
-        unsafe { ffi::PyErr_GivenExceptionMatches(self.type_ptr(py), exc.as_ptr()) != 0 }
+        fn inner(err: &PyErr, py: Python<'_>, exc: PyObject) -> bool {
+            (unsafe { ffi::PyErr_GivenExceptionMatches(err.type_ptr(py), exc.as_ptr()) }) != 0
+        }
+        inner(self, py, exc.to_object(py))
     }
 
     /// Returns true if the current exception is instance of `T`.
     #[inline]
     pub fn is_instance(&self, py: Python<'_>, ty: &PyAny) -> bool {
-        unsafe { ffi::PyErr_GivenExceptionMatches(self.type_ptr(py), ty.as_ptr()) != 0 }
+        (unsafe { ffi::PyErr_GivenExceptionMatches(self.type_ptr(py), ty.as_ptr()) }) != 0
     }
 
     /// Returns true if the current exception is instance of `T`.