Skip to content

Commit

Permalink
Add PyList_GetItemRef and use it in list.get_item (#4410)
Browse files Browse the repository at this point in the history
* Add PyList_GetItemRef bindings and compat shim

* Use PyList_GetItemRef in list.get_item()

* add release notes

* apply code review comments

* Update newsfragments/4410.added.md

Co-authored-by: David Hewitt <mail@davidhewitt.dev>

* apply `all()` doc cfg hints

---------

Co-authored-by: David Hewitt <mail@davidhewitt.dev>
  • Loading branch information
ngoldbaum and davidhewitt committed Sep 3, 2024
1 parent d103c76 commit 5e6e432
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 7 deletions.
1 change: 1 addition & 0 deletions newsfragments/4410.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add ffi binding `PyList_GetItemRef` and `pyo3_ffi::compat::PyList_GetItemRef`
3 changes: 3 additions & 0 deletions newsfragments/4410.fixed.md
Original file line number Diff line number Diff line change
@@ -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.
20 changes: 18 additions & 2 deletions pyo3-ffi/src/compat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
#[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)))]
#[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,
Expand All @@ -42,3 +44,17 @@ 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))]
#[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};

let item: *mut PyObject = PyList_GetItem(arg1, arg2);
Py_XINCREF(item);
item
}
2 changes: 2 additions & 0 deletions pyo3-ffi/src/listobject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
7 changes: 2 additions & 5 deletions src/types/list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::iter::FusedIterator;
use crate::err::{self, 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};
#[cfg(feature = "gil-refs")]
Expand Down Expand Up @@ -434,10 +433,8 @@ impl<'py> PyListMethods<'py> for Bound<'py, PyList> {
/// ```
fn get_item(&self, index: usize) -> PyResult<Bound<'py, PyAny>> {
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)
ffi::compat::PyList_GetItemRef(self.as_ptr(), index as Py_ssize_t)
.assume_owned_or_err(self.py())
}
}

Expand Down

0 comments on commit 5e6e432

Please sign in to comment.