From 96b8c9facffbd6c4ad90353310a2ad88c561f28b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Mon, 19 Feb 2024 22:14:26 +0000 Subject: [PATCH] migrate some final `FromPyObject` implementations to the `Bound` API (#3869) * update `Py::extract` to use `extract_bound` * update docstring of `FromPyObject` * move `Option` conversions to new module & update * move `Cell` conversions to new module & update --- src/conversion.rs | 121 ++++++---------------------------- src/conversions/std/cell.rs | 24 +++++++ src/conversions/std/mod.rs | 2 + src/conversions/std/option.rs | 73 ++++++++++++++++++++ src/instance.rs | 4 +- 5 files changed, 120 insertions(+), 104 deletions(-) create mode 100644 src/conversions/std/cell.rs create mode 100644 src/conversions/std/option.rs diff --git a/src/conversion.rs b/src/conversion.rs index d9536aa9..d9f79ffa 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -8,7 +8,6 @@ use crate::types::PyTuple; use crate::{ ffi, gil, Bound, Py, PyAny, PyCell, PyClass, PyNativeType, PyObject, PyRef, PyRefMut, Python, }; -use std::cell::Cell; use std::ptr::NonNull; /// Returns a borrowed pointer to a Python object. @@ -63,18 +62,6 @@ pub unsafe trait AsPyPointer { fn as_ptr(&self) -> *mut ffi::PyObject; } -/// Convert `None` into a null pointer. -unsafe impl AsPyPointer for Option -where - T: AsPyPointer, -{ - #[inline] - fn as_ptr(&self) -> *mut ffi::PyObject { - self.as_ref() - .map_or_else(std::ptr::null_mut, |t| t.as_ptr()) - } -} - /// Conversion trait that allows various objects to be converted into `PyObject`. pub trait ToPyObject { /// Converts self into a Python object. @@ -180,7 +167,7 @@ pub trait IntoPy: Sized { /// Extract a type from a Python object. /// /// -/// Normal usage is through the `extract` methods on [`Py`] and [`PyAny`], which forward to this trait. +/// Normal usage is through the `extract` methods on [`Bound`] and [`Py`], which forward to this trait. /// /// # Examples /// @@ -190,30 +177,32 @@ pub trait IntoPy: Sized { /// /// # fn main() -> PyResult<()> { /// Python::with_gil(|py| { -/// let obj: Py = PyString::new_bound(py, "blah").into(); -/// -/// // Straight from an owned reference -/// let s: &str = obj.extract(py)?; +/// // Calling `.extract()` on a `Bound` smart pointer +/// let obj: Bound<'_, PyString> = PyString::new_bound(py, "blah"); +/// let s: &str = obj.extract()?; /// # assert_eq!(s, "blah"); /// -/// // Or from a borrowed reference -/// let obj: &PyString = obj.as_ref(py); -/// let s: &str = obj.extract()?; +/// // Calling `.extract(py)` on a `Py` smart pointer +/// let obj: Py = obj.unbind(); +/// let s: &str = obj.extract(py)?; /// # assert_eq!(s, "blah"); /// # Ok(()) /// }) /// # } /// ``` /// -/// Note: depending on the implementation, the lifetime of the extracted result may -/// depend on the lifetime of the `obj` or the `prepared` variable. -/// -/// For example, when extracting `&str` from a Python byte string, the resulting string slice will -/// point to the existing string data (lifetime: `'py`). -/// On the other hand, when extracting `&str` from a Python Unicode string, the preparation step -/// will convert the string to UTF-8, and the resulting string slice will have lifetime `'prepared`. -/// Since which case applies depends on the runtime type of the Python object, -/// both the `obj` and `prepared` variables must outlive the resulting string slice. +// /// FIXME: until `FromPyObject` can pick up a second lifetime, the below commentary is no longer +// /// true. Update and restore this documentation at that time. +// /// +// /// Note: depending on the implementation, the lifetime of the extracted result may +// /// depend on the lifetime of the `obj` or the `prepared` variable. +// /// +// /// For example, when extracting `&str` from a Python byte string, the resulting string slice will +// /// point to the existing string data (lifetime: `'py`). +// /// On the other hand, when extracting `&str` from a Python Unicode string, the preparation step +// /// will convert the string to UTF-8, and the resulting string slice will have lifetime `'prepared`. +// /// Since which case applies depends on the runtime type of the Python object, +// /// both the `obj` and `prepared` variables must outlive the resulting string slice. /// /// During the migration of PyO3 from the "GIL Refs" API to the `Bound` smart pointer, this trait /// has two methods `extract` and `extract_bound` which are defaulted to call each other. To avoid @@ -258,27 +247,6 @@ impl ToPyObject for &'_ T { } } -/// `Option::Some` is converted like `T`. -/// `Option::None` is converted to Python `None`. -impl ToPyObject for Option -where - T: ToPyObject, -{ - fn to_object(&self, py: Python<'_>) -> PyObject { - self.as_ref() - .map_or_else(|| py.None(), |val| val.to_object(py)) - } -} - -impl IntoPy for Option -where - T: IntoPy, -{ - fn into_py(self, py: Python<'_>) -> PyObject { - self.map_or_else(|| py.None(), |val| val.into_py(py)) - } -} - impl IntoPy for &'_ PyAny { #[inline] fn into_py(self, py: Python<'_>) -> PyObject { @@ -296,24 +264,6 @@ where } } -impl ToPyObject for Cell { - fn to_object(&self, py: Python<'_>) -> PyObject { - self.get().to_object(py) - } -} - -impl> IntoPy for Cell { - fn into_py(self, py: Python<'_>) -> PyObject { - self.get().into_py(py) - } -} - -impl<'py, T: FromPyObject<'py>> FromPyObject<'py> for Cell { - fn extract(ob: &'py PyAny) -> PyResult { - T::extract(ob).map(Cell::new) - } -} - impl<'py, T> FromPyObject<'py> for &'py PyCell where T: PyClass, @@ -353,19 +303,6 @@ where } } -impl<'py, T> FromPyObject<'py> for Option -where - T: FromPyObject<'py>, -{ - fn extract(obj: &'py PyAny) -> PyResult { - if obj.as_ptr() == unsafe { ffi::Py_None() } { - Ok(None) - } else { - T::extract(obj).map(Some) - } - } -} - /// Trait implemented by Python object types that allow a checked downcast. /// If `T` implements `PyTryFrom`, we can convert `&PyAny` to `&T`. /// @@ -593,8 +530,6 @@ mod test_no_clone {} #[cfg(test)] mod tests { - use crate::{PyObject, Python}; - #[allow(deprecated)] mod deprecated { use super::super::PyTryFrom; @@ -638,22 +573,4 @@ mod tests { }); } } - - #[test] - fn test_option_as_ptr() { - Python::with_gil(|py| { - use crate::AsPyPointer; - let mut option: Option = None; - assert_eq!(option.as_ptr(), std::ptr::null_mut()); - - let none = py.None(); - option = Some(none.clone()); - - let ref_cnt = none.get_refcnt(py); - assert_eq!(option.as_ptr(), none.as_ptr()); - - // Ensure ref count not changed by as_ptr call - assert_eq!(none.get_refcnt(py), ref_cnt); - }); - } } diff --git a/src/conversions/std/cell.rs b/src/conversions/std/cell.rs new file mode 100644 index 00000000..69d99091 --- /dev/null +++ b/src/conversions/std/cell.rs @@ -0,0 +1,24 @@ +use std::cell::Cell; + +use crate::{ + types::any::PyAnyMethods, Bound, FromPyObject, IntoPy, PyAny, PyObject, PyResult, Python, + ToPyObject, +}; + +impl ToPyObject for Cell { + fn to_object(&self, py: Python<'_>) -> PyObject { + self.get().to_object(py) + } +} + +impl> IntoPy for Cell { + fn into_py(self, py: Python<'_>) -> PyObject { + self.get().into_py(py) + } +} + +impl<'py, T: FromPyObject<'py>> FromPyObject<'py> for Cell { + fn extract_bound(ob: &Bound<'py, PyAny>) -> PyResult { + ob.extract().map(Cell::new) + } +} diff --git a/src/conversions/std/mod.rs b/src/conversions/std/mod.rs index 9b10b59f..305344b1 100644 --- a/src/conversions/std/mod.rs +++ b/src/conversions/std/mod.rs @@ -1,7 +1,9 @@ mod array; +mod cell; mod ipaddr; mod map; mod num; +mod option; mod osstr; mod path; mod set; diff --git a/src/conversions/std/option.rs b/src/conversions/std/option.rs new file mode 100644 index 00000000..2fa082ba --- /dev/null +++ b/src/conversions/std/option.rs @@ -0,0 +1,73 @@ +use crate::{ + ffi, types::any::PyAnyMethods, AsPyPointer, Bound, FromPyObject, IntoPy, PyAny, PyObject, + PyResult, Python, ToPyObject, +}; + +/// `Option::Some` is converted like `T`. +/// `Option::None` is converted to Python `None`. +impl ToPyObject for Option +where + T: ToPyObject, +{ + fn to_object(&self, py: Python<'_>) -> PyObject { + self.as_ref() + .map_or_else(|| py.None(), |val| val.to_object(py)) + } +} + +impl IntoPy for Option +where + T: IntoPy, +{ + fn into_py(self, py: Python<'_>) -> PyObject { + self.map_or_else(|| py.None(), |val| val.into_py(py)) + } +} + +impl<'py, T> FromPyObject<'py> for Option +where + T: FromPyObject<'py>, +{ + fn extract_bound(obj: &Bound<'py, PyAny>) -> PyResult { + if obj.is_none() { + Ok(None) + } else { + obj.extract().map(Some) + } + } +} + +/// Convert `None` into a null pointer. +unsafe impl AsPyPointer for Option +where + T: AsPyPointer, +{ + #[inline] + fn as_ptr(&self) -> *mut ffi::PyObject { + self.as_ref() + .map_or_else(std::ptr::null_mut, |t| t.as_ptr()) + } +} + +#[cfg(test)] +mod tests { + use crate::{PyObject, Python}; + + #[test] + fn test_option_as_ptr() { + Python::with_gil(|py| { + use crate::AsPyPointer; + let mut option: Option = None; + assert_eq!(option.as_ptr(), std::ptr::null_mut()); + + let none = py.None(); + option = Some(none.clone()); + + let ref_cnt = none.get_refcnt(py); + assert_eq!(option.as_ptr(), none.as_ptr()); + + // Ensure ref count not changed by as_ptr call + assert_eq!(none.get_refcnt(py), ref_cnt); + }); + } +} diff --git a/src/instance.rs b/src/instance.rs index b83b69a2..a280f179 100644 --- a/src/instance.rs +++ b/src/instance.rs @@ -1250,11 +1250,11 @@ impl Py { /// Extracts some type from the Python object. /// /// This is a wrapper function around `FromPyObject::extract()`. - pub fn extract<'py, D>(&'py self, py: Python<'py>) -> PyResult + pub fn extract<'py, D>(&self, py: Python<'py>) -> PyResult where D: FromPyObject<'py>, { - FromPyObject::extract(unsafe { py.from_borrowed_ptr(self.as_ptr()) }) + self.bind(py).as_any().extract() } /// Retrieves an attribute value.