From 6b249e91f02263d91f4495bcd871e0b2c97d862a Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 1 Aug 2024 14:35:06 -0600 Subject: [PATCH 1/6] Add PyList_GetItemRef bindings and compat shim --- pyo3-ffi/src/compat.rs | 15 +++++++++++++++ pyo3-ffi/src/listobject.rs | 2 ++ 2 files changed, 17 insertions(+) diff --git a/pyo3-ffi/src/compat.rs b/pyo3-ffi/src/compat.rs index ef9a3fbe1c9..2a91dc6ca9e 100644 --- a/pyo3-ffi/src/compat.rs +++ b/pyo3-ffi/src/compat.rs @@ -12,6 +12,8 @@ #[cfg(not(Py_3_13))] use crate::object::PyObject; #[cfg(not(Py_3_13))] +use crate::pyport::Py_ssize_t; +#[cfg(not(Py_3_13))] use std::os::raw::c_int; #[cfg_attr(docsrs, doc(cfg(all)))] @@ -42,3 +44,16 @@ pub unsafe fn PyDict_GetItemRef( -1 } } + +#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg(Py_3_13)] +pub use crate::PyList_GetItemRef; + +#[cfg(not(Py_3_13))] +pub unsafe fn PyList_GetItemRef(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject { + use crate::{PyList_GetItem, Py_XINCREF}; + + let item: *mut PyObject = PyList_GetItem(arg1, arg2); + Py_XINCREF(item); + item +} diff --git a/pyo3-ffi/src/listobject.rs b/pyo3-ffi/src/listobject.rs index a6f47eadf83..1096f2fe0c8 100644 --- a/pyo3-ffi/src/listobject.rs +++ b/pyo3-ffi/src/listobject.rs @@ -28,6 +28,8 @@ extern "C" { pub fn PyList_Size(arg1: *mut PyObject) -> Py_ssize_t; #[cfg_attr(PyPy, link_name = "PyPyList_GetItem")] pub fn PyList_GetItem(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject; + #[cfg(Py_3_13)] + pub fn PyList_GetItemRef(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject; #[cfg_attr(PyPy, link_name = "PyPyList_SetItem")] pub fn PyList_SetItem(arg1: *mut PyObject, arg2: Py_ssize_t, arg3: *mut PyObject) -> c_int; #[cfg_attr(PyPy, link_name = "PyPyList_Insert")] From 4b00a10b8bd47024707f6be0e147370150669a6d Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 1 Aug 2024 14:35:26 -0600 Subject: [PATCH 2/6] Use PyList_GetItemRef in list.get_item() --- src/types/list.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/types/list.rs b/src/types/list.rs index 8f8bd2eb1a4..b10ad6825f3 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -1,9 +1,8 @@ use std::iter::FusedIterator; -use crate::err::{self, PyResult}; +use crate::err::{self, PyErr, PyResult}; use crate::ffi::{self, Py_ssize_t}; use crate::ffi_ptr_ext::FfiPtrExt; -use crate::instance::Borrowed; use crate::internal_tricks::get_ssize_index; use crate::types::{PySequence, PyTuple}; use crate::{Bound, PyAny, PyObject, Python, ToPyObject}; @@ -288,12 +287,12 @@ impl<'py> PyListMethods<'py> for Bound<'py, PyList> { /// }); /// ``` fn get_item(&self, index: usize) -> PyResult> { - unsafe { - // PyList_GetItem return borrowed ptr; must make owned for safety (see #890). - ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t) - .assume_borrowed_or_err(self.py()) - .map(Borrowed::to_owned) + let py = self.py(); + let result = unsafe { ffi::compat::PyList_GetItemRef(self.as_ptr(), index as Py_ssize_t) }; + if !result.is_null() { + return Ok(unsafe { result.assume_owned(py) }); } + Err(PyErr::fetch(py)) } /// Gets the list item at the specified index. Undefined behavior on bad index. Use with caution. From b2015e05b31545145d3bba751c9d03c95e70e034 Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Thu, 1 Aug 2024 15:07:06 -0600 Subject: [PATCH 3/6] add release notes --- newsfragments/4410.added.md | 5 +++++ newsfragments/4410.fixed.md | 3 +++ 2 files changed, 8 insertions(+) create mode 100644 newsfragments/4410.added.md create mode 100644 newsfragments/4410.fixed.md diff --git a/newsfragments/4410.added.md b/newsfragments/4410.added.md new file mode 100644 index 00000000000..6e3acbe9fd7 --- /dev/null +++ b/newsfragments/4410.added.md @@ -0,0 +1,5 @@ +* Added bindings for `PyList_GetItemRef` on Python 3.13 and newer. Also added + `ffi::compat::PyList_GetItemRef` which re-exports the FFI binding on Python + 3.13 and newer and defines a compatibility shim on older versions. This + function returns an owned reference to the item in the list and should be + preferred if the list is not known *a priori* to be shared between threads. diff --git a/newsfragments/4410.fixed.md b/newsfragments/4410.fixed.md new file mode 100644 index 00000000000..f4403409aea --- /dev/null +++ b/newsfragments/4410.fixed.md @@ -0,0 +1,3 @@ +* Avoid creating temporary borrowed reference in list.get_item + bindings. Temporary borrowed references are unsafe in the free-threaded python + build if the list is shared between threads. From a0b87d5ae7f4a58f15999443f74062f880204a4e Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 7 Aug 2024 14:04:52 -0600 Subject: [PATCH 4/6] apply code review comments --- pyo3-ffi/src/compat.rs | 4 ++-- src/types/list.rs | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/pyo3-ffi/src/compat.rs b/pyo3-ffi/src/compat.rs index 2a91dc6ca9e..1b67c34faf4 100644 --- a/pyo3-ffi/src/compat.rs +++ b/pyo3-ffi/src/compat.rs @@ -16,11 +16,11 @@ use crate::pyport::Py_ssize_t; #[cfg(not(Py_3_13))] use std::os::raw::c_int; -#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg_attr(docsrs, doc(cfg(all())))] #[cfg(Py_3_13)] pub use crate::dictobject::PyDict_GetItemRef; -#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg_attr(docsrs, doc(cfg(all())))] #[cfg(not(Py_3_13))] pub unsafe fn PyDict_GetItemRef( dp: *mut PyObject, diff --git a/src/types/list.rs b/src/types/list.rs index b10ad6825f3..66e5f4661a3 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -1,6 +1,6 @@ use std::iter::FusedIterator; -use crate::err::{self, PyErr, PyResult}; +use crate::err::{self, PyResult}; use crate::ffi::{self, Py_ssize_t}; use crate::ffi_ptr_ext::FfiPtrExt; use crate::internal_tricks::get_ssize_index; @@ -287,12 +287,10 @@ impl<'py> PyListMethods<'py> for Bound<'py, PyList> { /// }); /// ``` fn get_item(&self, index: usize) -> PyResult> { - let py = self.py(); - let result = unsafe { ffi::compat::PyList_GetItemRef(self.as_ptr(), index as Py_ssize_t) }; - if !result.is_null() { - return Ok(unsafe { result.assume_owned(py) }); + unsafe { + ffi::compat::PyList_GetItemRef(self.as_ptr(), index as Py_ssize_t) + .assume_owned_or_err(self.py()) } - Err(PyErr::fetch(py)) } /// Gets the list item at the specified index. Undefined behavior on bad index. Use with caution. From 4fdc1410fbf0fadd76f216f94a46d5059ff216da Mon Sep 17 00:00:00 2001 From: Nathan Goldbaum Date: Wed, 7 Aug 2024 14:07:11 -0600 Subject: [PATCH 5/6] Update newsfragments/4410.added.md Co-authored-by: David Hewitt --- newsfragments/4410.added.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/newsfragments/4410.added.md b/newsfragments/4410.added.md index 6e3acbe9fd7..350a54531bd 100644 --- a/newsfragments/4410.added.md +++ b/newsfragments/4410.added.md @@ -1,5 +1 @@ -* Added bindings for `PyList_GetItemRef` on Python 3.13 and newer. Also added - `ffi::compat::PyList_GetItemRef` which re-exports the FFI binding on Python - 3.13 and newer and defines a compatibility shim on older versions. This - function returns an owned reference to the item in the list and should be - preferred if the list is not known *a priori* to be shared between threads. +Add ffi binding `PyList_GetItemRef` and `pyo3_ffi::compat::PyList_GetItemRef` From 86116c3c236f519ee98951554827e891a5c04e26 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 7 Aug 2024 23:46:30 +0100 Subject: [PATCH 6/6] apply `all()` doc cfg hints --- pyo3-ffi/src/compat.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pyo3-ffi/src/compat.rs b/pyo3-ffi/src/compat.rs index 1b67c34faf4..85986817d93 100644 --- a/pyo3-ffi/src/compat.rs +++ b/pyo3-ffi/src/compat.rs @@ -45,11 +45,12 @@ pub unsafe fn PyDict_GetItemRef( } } -#[cfg_attr(docsrs, doc(cfg(all)))] +#[cfg_attr(docsrs, doc(cfg(all())))] #[cfg(Py_3_13)] pub use crate::PyList_GetItemRef; #[cfg(not(Py_3_13))] +#[cfg_attr(docsrs, doc(cfg(all())))] pub unsafe fn PyList_GetItemRef(arg1: *mut PyObject, arg2: Py_ssize_t) -> *mut PyObject { use crate::{PyList_GetItem, Py_XINCREF};