diff --git a/src/ffi_ptr_ext.rs b/src/ffi_ptr_ext.rs index 108f088ea18..0287649eacc 100644 --- a/src/ffi_ptr_ext.rs +++ b/src/ffi_ptr_ext.rs @@ -1,3 +1,4 @@ +use crate::internal::err::ErrorAlreadySet; use crate::sealed::Sealed; use crate::{ ffi, @@ -11,6 +12,12 @@ pub(crate) trait FfiPtrExt: Sealed { /// If the pointer is NULL, this function will fetch an error. unsafe fn assume_owned_or_err(self, py: Python<'_>) -> PyResult>; + /// Similar to the above, but doesn't fetch the error. + unsafe fn assume_owned_or_err_set( + self, + py: Python<'_>, + ) -> Result, ErrorAlreadySet<'_>>; + /// Same as `assume_owned_or_err`, but doesn't fetch an error on NULL. unsafe fn assume_owned_or_opt(self, py: Python<'_>) -> Option>; @@ -43,6 +50,14 @@ impl FfiPtrExt for *mut ffi::PyObject { unsafe { Bound::from_owned_ptr_or_err(py, self) } } + #[inline] + unsafe fn assume_owned_or_err_set( + self, + py: Python<'_>, + ) -> Result, ErrorAlreadySet<'_>> { + unsafe { Bound::from_owned_ptr_or_opt(py, self).ok_or_else(|| ErrorAlreadySet::new(py)) } + } + #[inline] unsafe fn assume_owned_or_opt(self, py: Python<'_>) -> Option> { unsafe { Bound::from_owned_ptr_or_opt(py, self) } diff --git a/src/internal.rs b/src/internal.rs index b8f841ac0ae..135c7e126c2 100644 --- a/src/internal.rs +++ b/src/internal.rs @@ -1,5 +1,6 @@ //! Holding place for code which is not intended to be reachable from outside of PyO3. +pub(crate) mod err; pub(crate) mod get_slot; pub(crate) mod pyclass_init; pub(crate) mod state; diff --git a/src/internal/err.rs b/src/internal/err.rs new file mode 100644 index 00000000000..b833d97262b --- /dev/null +++ b/src/internal/err.rs @@ -0,0 +1,39 @@ +use std::mem::ManuallyDrop; + +use crate::{ffi, PyErr, Python}; + +/// Denotes that an error occurred and the Python interpreter has an error set +/// in the thread state. +/// +/// While this type is alive, it is unsafe to do most Python C API calls. +/// +/// Dropping this type will clear the error in the Python interpreter. This +/// type also has a conversion to `PyErr` to fetch the error. +pub struct ErrorAlreadySet<'py>(Python<'py>); + +impl<'py> ErrorAlreadySet<'py> { + /// # Safety + /// - Caller is responsible to ensure that no Python C APIs are called while + /// this type is alive (except for those specifically for error handling). + #[inline] + pub unsafe fn new(py: Python<'py>) -> Self { + Self(py) + } +} + +impl Drop for ErrorAlreadySet<'_> { + #[inline] + fn drop(&mut self) { + // SAFETY: `self.0` proves the thread is attached to the interpreter. + unsafe { ffi::PyErr_Clear() } + } +} + +impl From> for PyErr { + #[inline] + fn from(err: ErrorAlreadySet<'_>) -> Self { + // Avoid calling `PyErr_Clear` after fetching + let err = ManuallyDrop::new(err); + PyErr::fetch(err.0) + } +} diff --git a/src/py_result_ext.rs b/src/py_result_ext.rs index c5370b8c8b8..bf175c828ad 100644 --- a/src/py_result_ext.rs +++ b/src/py_result_ext.rs @@ -1,13 +1,16 @@ -use crate::{Bound, PyAny, PyResult, PyTypeCheck}; +use crate::{internal::err::ErrorAlreadySet, Bound, PyAny, PyErr, PyResult, PyTypeCheck}; pub(crate) trait PyResultExt<'py>: crate::sealed::Sealed { - fn cast_into(self) -> PyResult>; - unsafe fn cast_into_unchecked(self) -> PyResult>; + type Error; + fn cast_into(self) -> Result, PyErr>; + unsafe fn cast_into_unchecked(self) -> Result, Self::Error>; } impl<'py> PyResultExt<'py> for PyResult> { + type Error = PyErr; + #[inline] - fn cast_into(self) -> PyResult> where { + fn cast_into(self) -> PyResult> { self.and_then(|instance| instance.cast_into().map_err(Into::into)) } @@ -16,3 +19,17 @@ impl<'py> PyResultExt<'py> for PyResult> { self.map(|instance| unsafe { instance.cast_into_unchecked() }) } } + +impl<'py> PyResultExt<'py> for Result, ErrorAlreadySet<'py>> { + type Error = ErrorAlreadySet<'py>; + + #[inline] + fn cast_into(self) -> Result, PyErr> { + self?.cast_into().map_err(Into::into) + } + + #[inline] + unsafe fn cast_into_unchecked(self) -> Result, ErrorAlreadySet<'py>> { + self.map(|instance| unsafe { instance.cast_into_unchecked() }) + } +} diff --git a/src/sealed.rs b/src/sealed.rs index 0155aa93349..2cac7cfeaf3 100644 --- a/src/sealed.rs +++ b/src/sealed.rs @@ -1,4 +1,5 @@ use crate::impl_::pyfunction::PyFunctionDef; +use crate::internal::err::ErrorAlreadySet; #[cfg(all(not(Py_LIMITED_API), not(PyPy), not(GraalPy)))] use crate::types::PyFrame; use crate::types::{ @@ -23,6 +24,7 @@ impl Sealed for *mut ffi::PyObject {} // for PyResultExt impl Sealed for PyResult> {} +impl<'py> Sealed for Result, ErrorAlreadySet<'py>> {} // for Py(...)Methods impl Sealed for Bound<'_, PyAny> {} diff --git a/src/types/set.rs b/src/types/set.rs index 279d96a2baa..bfbff91ebfb 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -207,7 +207,7 @@ impl<'py> PySetMethods<'py> for Bound<'py, PySet> { } fn pop(&self) -> Option> { - let element = unsafe { ffi::PySet_Pop(self.as_ptr()).assume_owned_or_err(self.py()) }; + let element = unsafe { ffi::PySet_Pop(self.as_ptr()).assume_owned_or_err_set(self.py()) }; element.ok() } diff --git a/src/types/tuple.rs b/src/types/tuple.rs index bfbbe9a048f..d0f8f0135c0 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -3,7 +3,9 @@ use crate::ffi_ptr_ext::FfiPtrExt; #[cfg(feature = "experimental-inspect")] use crate::inspect::{type_hint_subscript, PyStaticExpr}; use crate::instance::Borrowed; +use crate::internal::err::ErrorAlreadySet; use crate::internal_tricks::get_ssize_index; +use crate::py_result_ext::PyResultExt; #[cfg(feature = "experimental-inspect")] use crate::type_object::PyTypeInfo; use crate::types::{sequence::PySequenceMethods, PyList, PySequence}; @@ -40,26 +42,22 @@ fn try_new_from_iter<'py>( mut elements: impl ExactSizeIterator>>, ) -> PyResult> { #[cfg(not(RustPython))] - unsafe { + { // PyTuple_New checks for overflow but has a bad error message, so we check ourselves let len: Py_ssize_t = elements .len() .try_into() .expect("out of range integral type conversion attempted on `elements.len()`"); - let ptr = ffi::PyTuple_New(len); - - // - Panics if the ptr is null - // - Cleans up the tuple if `convert` or the asserts panic - let tup = ptr.assume_owned(py).cast_into_unchecked(); + // SAFETY: this function will fully initialize the tuple, or drop it on error / panic paths + let tup = unsafe { PyTuple::new_uninit(py, len)? }; let mut counter: Py_ssize_t = 0; - for obj in (&mut elements).take(len as usize) { - #[cfg(not(any(Py_LIMITED_API, PyPy, GraalPy)))] - ffi::PyTuple_SET_ITEM(ptr, counter, obj?.into_ptr()); - #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))] - ffi::PyTuple_SetItem(ptr, counter, obj?.into_ptr()); + for result in (&mut elements).take(len as usize) { + let obj = result?; + // SAFETY: `tup` needs `len` elements initialized + unsafe { tup.as_borrowed().set_item_unchecked(counter, obj) }; counter += 1; } @@ -70,12 +68,14 @@ fn try_new_from_iter<'py>( } #[cfg(RustPython)] - unsafe { + { let elements = elements.collect::>>()?; // SAFETY: list is layout compatible with *const *mut crate::PyObject - ffi::PyTuple_FromArray(elements.as_ptr().cast(), elements.len() as _) - .assume_owned_or_err(py) - .cast_into_unchecked() + unsafe { + ffi::PyTuple_FromArray(elements.as_ptr().cast(), elements.len() as _) + .assume_owned_or_err(py) + .cast_into_unchecked() + } } } @@ -154,7 +154,40 @@ impl PyTuple { /// Constructs an empty tuple (on the Python side, a singleton object). pub fn empty(py: Python<'_>) -> Bound<'_, PyTuple> { - unsafe { ffi::PyTuple_New(0).assume_owned(py).cast_into_unchecked() } + // SAFETY: can never fail to construct empty tuple, and it's immediately fully init + #[cfg(not(Py_3_13))] + unsafe { + Self::new_uninit(py, 0).unwrap_unchecked() + } + + #[cfg(Py_3_13)] + // SAFETY: + // - thread is attached to the interpreter + // - constant is known to be owned and of the correct type + unsafe { + ffi::Py_GetConstant(ffi::Py_CONSTANT_EMPTY_TUPLE) + .assume_owned_unchecked(py) + .cast_into_unchecked() + } + } + + /// # Safety + /// - Caller must set all elements with `set_item_unchecked` before exposing the + /// returned tuple to user code. + /// - `ErrorAlreadySet` must be handled by caller + #[inline] + unsafe fn new_uninit<'py>( + py: Python<'py>, + len: Py_ssize_t, + ) -> Result, ErrorAlreadySet<'py>> { + // SAFETY: + // - thread is attached to the interpreter + // - success return is guaranteed to be a tuple + unsafe { + ffi::PyTuple_New(len) + .assume_owned_or_err_set(py) + .cast_into_unchecked() + } } } @@ -365,6 +398,32 @@ impl<'a, 'py> Borrowed<'a, 'py, PyTuple> { } } + /// # Safety + /// - `self` must be a tuple under initialization with `PyTuple::new_uninit` that is not yet + /// shared with external code. + /// - `self` mut not already have a value set at `index`. + /// - `index` must be within the bounds of the tuple. + #[inline] + unsafe fn set_item_unchecked(self, index: Py_ssize_t, obj: Bound<'_, PyAny>) { + #[cfg(not(any(Py_LIMITED_API, PyPy, GraalPy)))] + // SAFETY: + // - Caller-provided invariants and smart pointers guarantee correctness + // - `PyTuple_SET_ITEM` steals a reference to `obj` + unsafe { + ffi::PyTuple_SET_ITEM(self.as_ptr(), index, obj.into_ptr()) + } + + // SAFETY: + // - Caller-provided invariants and smart pointers guarantee correctness + // - `PyTuple_SetItem` steals a reference to `obj` + // - `PyTuple_SetItem` returns -1 if `index` is out of bounds, but we are + // not checking for errors here. + #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))] + unsafe { + ffi::PyTuple_SetItem(self.as_ptr(), index, obj.into_ptr()); + } + } + pub(crate) fn iter_borrowed(self) -> BorrowedTupleIterator<'a, 'py> { BorrowedTupleIterator::new(self) } @@ -662,7 +721,7 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ ); fn into_pyobject(self, py: Python<'py>) -> Result { - Ok(array_into_tuple(py, [$(self.$n.into_bound_py_any(py)?),+])) + Ok(array_into_tuple(py, [$(self.$n.into_bound_py_any(py)?),+])?) } } @@ -681,7 +740,7 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ ); fn into_pyobject(self, py: Python<'py>) -> Result { - Ok(array_into_tuple(py, [$(self.$n.into_bound_py_any(py)?),+])) + Ok(array_into_tuple(py, [$(self.$n.into_bound_py_any(py)?),+])?) } } @@ -965,26 +1024,26 @@ macro_rules! tuple_conversion ({$length:expr,$(($refN:ident, $n:tt, $T:ident)),+ fn array_into_tuple<'py, const N: usize>( py: Python<'py>, array: [Bound<'py, PyAny>; N], -) -> Bound<'py, PyTuple> { +) -> Result, ErrorAlreadySet<'py>> { + let len: Py_ssize_t = N.try_into().expect("0 < N <= 12"); + #[cfg(not(RustPython))] - unsafe { - let ptr = ffi::PyTuple_New(N.try_into().expect("0 < N <= 12")); - let tup = ptr.assume_owned(py).cast_into_unchecked(); + { + // SAFETY: this function will fully initialize the tuple + let tup = unsafe { PyTuple::new_uninit(py, len)? }; for (index, obj) in array.into_iter().enumerate() { - #[cfg(not(any(Py_LIMITED_API, PyPy, GraalPy)))] - ffi::PyTuple_SET_ITEM(ptr, index as ffi::Py_ssize_t, obj.into_ptr()); - #[cfg(any(Py_LIMITED_API, PyPy, GraalPy))] - ffi::PyTuple_SetItem(ptr, index as ffi::Py_ssize_t, obj.into_ptr()); + // SAFETY: index is guaranteed to be < N + unsafe { tup.as_borrowed().set_item_unchecked(index as _, obj) }; } - tup + Ok(tup) } // SAFETY: array is layout compatible with *const *mut crate::PyObject // and does not steal the bound reference. #[cfg(RustPython)] unsafe { - ffi::PyTuple_FromArray(array.as_ptr().cast(), N.try_into().expect("0 < N <= 12")) - .assume_owned(py) + ffi::PyTuple_FromArray(array.as_ptr().cast(), len) + .assume_owned_or_err_set(py) .cast_into_unchecked() } }