From 6b18a9333534c14e33b7804fc34d9e0d56a91092 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 13 Nov 2023 22:15:00 +0000 Subject: [PATCH] introduce traits to make ffi ptr handling cleaner --- src/ffi_ptr_ext.rs | 28 +++++++++++++++++++ src/lib.rs | 2 ++ src/prelude.rs | 4 +-- src/py_result_ext.rs | 22 +++++++++++++++ src/types/any.rs | 54 ++++++++++++++++++------------------ src/types/iterator.rs | 9 +++--- src/types/sequence.rs | 64 ++++++++++++++++++------------------------- 7 files changed, 111 insertions(+), 72 deletions(-) create mode 100644 src/ffi_ptr_ext.rs create mode 100644 src/py_result_ext.rs diff --git a/src/ffi_ptr_ext.rs b/src/ffi_ptr_ext.rs new file mode 100644 index 00000000000..6be8e28bac1 --- /dev/null +++ b/src/ffi_ptr_ext.rs @@ -0,0 +1,28 @@ +use crate::{ffi, instance::Py2, PyAny, PyResult, Python}; + +mod ffi_ptr_ext_sealed { + use super::*; + + pub trait Sealed {} + + impl Sealed for *mut ffi::PyObject {} +} + +use ffi_ptr_ext_sealed::Sealed; + +pub(crate) trait FfiPtrExt: Sealed { + unsafe fn assume_owned_or_ffi_error<'py>(self, py: Python<'py>) -> PyResult>; + unsafe fn assume_owned_and_infallible<'py>(self, py: Python<'py>) -> Py2<'py, PyAny>; +} + +impl FfiPtrExt for *mut ffi::PyObject { + #[inline] + unsafe fn assume_owned_or_ffi_error<'py>(self, py: Python<'py>) -> PyResult> { + Py2::from_owned_ptr_or_err(py, self) + } + + #[inline] + unsafe fn assume_owned_and_infallible<'py>(self, py: Python<'py>) -> Py2<'py, PyAny> { + Py2::from_owned_ptr(py, self) + } +} diff --git a/src/lib.rs b/src/lib.rs index a2e5d9601c7..110963d8942 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -317,6 +317,8 @@ pub use crate::version::PythonVersionInfo; // Expected to become public API in 0.21 under a different name pub(crate) use crate::instance::Py2; +pub(crate) mod ffi_ptr_ext; +pub(crate) mod py_result_ext; /// Old module which contained some implementation details of the `#[pyproto]` module. /// diff --git a/src/prelude.rs b/src/prelude.rs index 7909413fe1a..2daf36fb70d 100644 --- a/src/prelude.rs +++ b/src/prelude.rs @@ -23,6 +23,6 @@ pub use pyo3_macros::{pyclass, pyfunction, pymethods, pymodule, FromPyObject}; pub use crate::wrap_pyfunction; // Expected to become public API in 0.21 -pub(crate) use crate::instance::Py2; // Will be stabilized with a different name -pub(crate) use crate::types::any::PyAnyMethods; +// pub(crate) use crate::instance::Py2; // Will be stabilized with a different name +// pub(crate) use crate::types::any::PyAnyMethods; // pub(crate) use crate::types::sequence::PySequenceMethods; diff --git a/src/py_result_ext.rs b/src/py_result_ext.rs new file mode 100644 index 00000000000..630d039084f --- /dev/null +++ b/src/py_result_ext.rs @@ -0,0 +1,22 @@ +use crate::{types::any::PyAnyMethods, Py2, PyAny, PyResult}; + +mod py_result_ext_sealed { + use super::*; + + pub trait Sealed {} + + impl Sealed for PyResult> {} +} + +use py_result_ext_sealed::Sealed; + +pub(crate) trait PyResultExt<'py>: Sealed { + unsafe fn downcast_into_unchecked(self) -> PyResult>; +} + +impl<'py> PyResultExt<'py> for PyResult> { + #[inline] + unsafe fn downcast_into_unchecked(self) -> PyResult> { + self.map(|instance| instance.downcast_into_unchecked()) + } +} diff --git a/src/types/any.rs b/src/types/any.rs index 8139c06563c..4dc7822c4d9 100644 --- a/src/types/any.rs +++ b/src/types/any.rs @@ -2,6 +2,7 @@ use crate::class::basic::CompareOp; use crate::conversion::{AsPyPointer, FromPyObject, IntoPy, PyTryFrom, ToPyObject}; use crate::err::{PyDowncastError, PyErr, PyResult}; use crate::exceptions::{PyAttributeError, PyTypeError}; +use crate::ffi_ptr_ext::FfiPtrExt; use crate::instance::Py2; use crate::type_object::PyTypeInfo; #[cfg(not(PyPy))] @@ -12,6 +13,8 @@ use std::cell::UnsafeCell; use std::cmp::Ordering; use std::os::raw::c_int; +use crate::py_result_ext::PyResultExt; + /// Represents any Python object. /// /// It currently only appears as a *reference*, `&PyAny`, @@ -1707,10 +1710,8 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { attr_name: Py2<'_, PyString>, ) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - any.py(), - ffi::PyObject_GetAttr(any.as_ptr(), attr_name.as_ptr()), - ) + ffi::PyObject_GetAttr(any.as_ptr(), attr_name.as_ptr()) + .assume_owned_or_ffi_error(any.py()) } } @@ -1760,12 +1761,12 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { O: ToPyObject, { fn inner(any: &Py2<'_, PyAny>, other: Py2<'_, PyAny>) -> PyResult { - let py = any.py(); let other = other.as_ptr(); // Almost the same as ffi::PyObject_RichCompareBool, but this one doesn't try self == other. // See https://github.com/PyO3/pyo3/issues/985 for more. let do_compare = |other, op| unsafe { - Py2::from_owned_ptr_or_err(py, ffi::PyObject_RichCompare(any.as_ptr(), other, op)) + ffi::PyObject_RichCompare(any.as_ptr(), other, op) + .assume_owned_or_ffi_error(any.py()) .and_then(|obj| obj.is_true()) }; if do_compare(other, ffi::Py_EQ)? { @@ -1795,10 +1796,8 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { compare_op: CompareOp, ) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - any.py(), - ffi::PyObject_RichCompare(any.as_ptr(), other.as_ptr(), compare_op as c_int), - ) + ffi::PyObject_RichCompare(any.as_ptr(), other.as_ptr(), compare_op as c_int) + .assume_owned_or_ffi_error(any.py()) } } @@ -1869,14 +1868,12 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { kwargs: Option<&PyDict>, ) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - any.py(), - ffi::PyObject_Call( - any.as_ptr(), - args.as_ptr(), - kwargs.map_or(std::ptr::null_mut(), |dict| dict.as_ptr()), - ), + ffi::PyObject_Call( + any.as_ptr(), + args.as_ptr(), + kwargs.map_or(std::ptr::null_mut(), |dict| dict.as_ptr()), ) + .assume_owned_or_ffi_error(any.py()) } } @@ -1892,7 +1889,7 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { ))] { // Optimized path on python 3.9+ unsafe { - Py2::from_owned_ptr_or_err(self.py(), ffi::PyObject_CallNoArgs(self.as_ptr())) + ffi::PyObject_CallNoArgs(self.as_ptr()).assume_owned_or_ffi_error(self.py()) } } else { self.call((), None) @@ -1929,7 +1926,7 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { // Optimized path on python 3.9+ unsafe { let name = name.into_py(py).attach_into(py); - Py2::from_owned_ptr_or_err(py, ffi::PyObject_CallMethodNoArgs(self.as_ptr(), name.as_ptr())) + ffi::PyObject_CallMethodNoArgs(self.as_ptr(), name.as_ptr()).assume_owned_or_ffi_error(py) } } else { self.call_method(name, (), None) @@ -1970,10 +1967,8 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { { fn inner<'py>(any: &Py2<'py, PyAny>, key: Py2<'_, PyAny>) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - any.py(), - ffi::PyObject_GetItem(any.as_ptr(), key.as_ptr()), - ) + ffi::PyObject_GetItem(any.as_ptr(), key.as_ptr()) + .assume_owned_or_ffi_error(any.py()) } } @@ -2102,15 +2097,17 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { fn repr(&self) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err(self.py(), ffi::PyObject_Repr(self.as_ptr())) - .map(|any| any.downcast_into_unchecked()) + ffi::PyObject_Repr(self.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } fn str(&self) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err(self.py(), ffi::PyObject_Str(self.as_ptr())) - .map(|any| any.downcast_into_unchecked()) + ffi::PyObject_Str(self.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } @@ -2128,7 +2125,8 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> { fn dir(&self) -> Py2<'py, PyList> { unsafe { - Py2::from_owned_ptr(self.py(), ffi::PyObject_Dir(self.as_ptr())) + ffi::PyObject_Dir(self.as_ptr()) + .assume_owned_and_infallible(self.py()) .downcast_into_unchecked() } } diff --git a/src/types/iterator.rs b/src/types/iterator.rs index d88240f3880..89a1900fc78 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -1,8 +1,8 @@ +use crate::ffi_ptr_ext::FfiPtrExt; +use crate::py_result_ext::PyResultExt; use crate::{ffi, AsPyPointer, Py, Py2, PyAny, PyErr, PyNativeType, PyResult, Python}; use crate::{PyDowncastError, PyTryFrom}; -use super::any::PyAnyMethods; - /// A Python iterator object. /// /// # Examples @@ -42,8 +42,9 @@ impl PyIterator { pub(crate) fn from_object2<'py>(obj: &Py2<'py, PyAny>) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err(obj.py(), ffi::PyObject_GetIter(obj.as_ptr())) - .map(|any| any.downcast_into_unchecked()) + ffi::PyObject_GetIter(obj.as_ptr()) + .assume_owned_or_ffi_error(obj.py()) + .downcast_into_unchecked() } } } diff --git a/src/types/sequence.rs b/src/types/sequence.rs index bdd8b9cf422..07143c95df2 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -1,9 +1,11 @@ use crate::err::{self, PyDowncastError, PyErr, PyResult}; use crate::exceptions::PyTypeError; +use crate::ffi_ptr_ext::FfiPtrExt; #[cfg(feature = "experimental-inspect")] use crate::inspect::types::TypeInfo; +use crate::instance::Py2; use crate::internal_tricks::get_ssize_index; -use crate::prelude::*; +use crate::py_result_ext::PyResultExt; use crate::sync::GILOnceCell; use crate::type_object::PyTypeInfo; use crate::types::{PyAny, PyList, PyString, PyTuple, PyType}; @@ -341,69 +343,53 @@ impl<'py> PySequenceMethods<'py> for Py2<'py, PySequence> { #[inline] fn concat(&self, other: &Py2<'_, PySequence>) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_Concat(self.as_ptr(), other.as_ptr()), - ) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_Concat(self.as_ptr(), other.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } #[inline] fn repeat(&self, count: usize) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_Repeat(self.as_ptr(), get_ssize_index(count)), - ) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_Repeat(self.as_ptr(), get_ssize_index(count)) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } #[inline] fn in_place_concat(&self, other: &Py2<'_, PySequence>) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_InPlaceConcat(self.as_ptr(), other.as_ptr()), - ) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_InPlaceConcat(self.as_ptr(), other.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } #[inline] fn in_place_repeat(&self, count: usize) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_InPlaceRepeat(self.as_ptr(), get_ssize_index(count)), - ) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_InPlaceRepeat(self.as_ptr(), get_ssize_index(count)) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } #[inline] fn get_item(&self, index: usize) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_GetItem(self.as_ptr(), get_ssize_index(index)), - ) + ffi::PySequence_GetItem(self.as_ptr(), get_ssize_index(index)) + .assume_owned_or_ffi_error(self.py()) } } #[inline] fn get_slice(&self, begin: usize, end: usize) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err( - self.py(), - ffi::PySequence_GetSlice( - self.as_ptr(), - get_ssize_index(begin), - get_ssize_index(end), - ), - ) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_GetSlice(self.as_ptr(), get_ssize_index(begin), get_ssize_index(end)) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } @@ -500,16 +486,18 @@ impl<'py> PySequenceMethods<'py> for Py2<'py, PySequence> { #[inline] fn to_list(&self) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err(self.py(), ffi::PySequence_List(self.as_ptr())) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_List(self.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } #[inline] fn to_tuple(&self) -> PyResult> { unsafe { - Py2::from_owned_ptr_or_err(self.py(), ffi::PySequence_Tuple(self.as_ptr())) - .map(|py2| py2.downcast_into_unchecked()) + ffi::PySequence_Tuple(self.as_ptr()) + .assume_owned_or_ffi_error(self.py()) + .downcast_into_unchecked() } } }