Merge pull request #3273 from davidhewitt/conventions

add some style guide to Contributing.md

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.

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);
             }

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! {

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).
@@ -469,7 +468,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,74 +481,70 @@ 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 {}",
                 target.len(),
                 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.
     /// If the buffer is multi-dimensional, the elements are written in C-style order.
     ///
     /// 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.
     /// If the buffer is multi-dimensional, the elements are written in Fortran-style order.
     ///
     /// 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 {
-            // 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)
     }
 
@@ -564,7 +559,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 +573,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() {
@@ -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

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")]
@@ -248,19 +247,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 +277,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)

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 =