From db91642bae3ee9628a17a946e5e549830d0fa0e0 Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 25 Jun 2023 19:26:08 +0100 Subject: [PATCH 1/2] add `PyDict::get_item_with_error` for PyPy --- newsfragments/3270.added.md | 1 + pyo3-ffi/src/dictobject.rs | 1 + src/types/dict.rs | 1 - 3 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 newsfragments/3270.added.md diff --git a/newsfragments/3270.added.md b/newsfragments/3270.added.md new file mode 100644 index 00000000000..66b2db9d83d --- /dev/null +++ b/newsfragments/3270.added.md @@ -0,0 +1 @@ +Add `PyDict::get_item_with_error` on PyPy. diff --git a/pyo3-ffi/src/dictobject.rs b/pyo3-ffi/src/dictobject.rs index 3215d882c5c..8d522df97e2 100644 --- a/pyo3-ffi/src/dictobject.rs +++ b/pyo3-ffi/src/dictobject.rs @@ -24,6 +24,7 @@ extern "C" { pub fn PyDict_New() -> *mut PyObject; #[cfg_attr(PyPy, link_name = "PyPyDict_GetItem")] pub fn PyDict_GetItem(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject; + #[cfg_attr(PyPy, link_name = "PyPyDict_GetItemWithError")] pub fn PyDict_GetItemWithError(mp: *mut PyObject, key: *mut PyObject) -> *mut PyObject; #[cfg_attr(PyPy, link_name = "PyPyDict_SetItem")] pub fn PyDict_SetItem(mp: *mut PyObject, key: *mut PyObject, item: *mut PyObject) -> c_int; diff --git a/src/types/dict.rs b/src/types/dict.rs index cb1036cf946..51619a1a4af 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -157,7 +157,6 @@ impl PyDict { /// returns `Ok(None)` if item is not present, or `Err(PyErr)` if an error occurs. /// /// To get a `KeyError` for non-existing keys, use `PyAny::get_item_with_error`. - #[cfg(not(PyPy))] pub fn get_item_with_error(&self, key: K) -> PyResult> where K: ToPyObject, From 20f909c97f6097dcc0a0337ec3dc434a1cb9947f Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sun, 25 Jun 2023 19:26:51 +0100 Subject: [PATCH 2/2] refactor `PyDict::get_item[_with_error]` implementations --- src/types/dict.rs | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 51619a1a4af..7a9eb0d3a69 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -2,10 +2,9 @@ use super::PyMapping; use crate::err::{self, PyErr, PyResult}; use crate::ffi::Py_ssize_t; use crate::types::{PyAny, PyList}; -use crate::{ffi, AsPyPointer, Python, ToPyObject}; #[cfg(not(PyPy))] -use crate::{IntoPyPointer, PyObject}; -use std::ptr::NonNull; +use crate::IntoPyPointer; +use crate::{ffi, AsPyPointer, PyObject, Python, ToPyObject}; /// Represents a Python `dict`. #[repr(transparent)] @@ -143,12 +142,16 @@ impl PyDict { where K: ToPyObject, { + self.get_item_impl(key.to_object(self.py())) + } + + fn get_item_impl(&self, key: PyObject) -> Option<&PyAny> { + let py = self.py(); unsafe { - let ptr = ffi::PyDict_GetItem(self.as_ptr(), key.to_object(self.py()).as_ptr()); - NonNull::new(ptr).map(|p| { - // PyDict_GetItem return s borrowed ptr, must make it owned for safety (see #890). - self.py().from_owned_ptr(ffi::_Py_NewRef(p.as_ptr())) - }) + let ptr = ffi::PyDict_GetItem(self.as_ptr(), key.as_ptr()); + // PyDict_GetItem returns a borrowed ptr, must make it owned for safety (see #890). + // PyObject::from_borrowed_ptr_or_opt will take ownership in this way. + PyObject::from_borrowed_ptr_or_opt(py, ptr).map(|pyobject| pyobject.into_ref(py)) } } @@ -161,14 +164,19 @@ impl PyDict { where K: ToPyObject, { - unsafe { - let ptr = - ffi::PyDict_GetItemWithError(self.as_ptr(), key.to_object(self.py()).as_ptr()); - if !ffi::PyErr_Occurred().is_null() { - return Err(PyErr::fetch(self.py())); - } + self.get_item_with_error_impl(key.to_object(self.py())) + } - Ok(NonNull::new(ptr).map(|p| self.py().from_owned_ptr(ffi::_Py_NewRef(p.as_ptr())))) + fn get_item_with_error_impl(&self, key: PyObject) -> PyResult> { + let py = self.py(); + unsafe { + let ptr = ffi::PyDict_GetItemWithError(self.as_ptr(), key.as_ptr()); + // PyDict_GetItemWithError returns a borrowed ptr, must make it owned for safety (see #890). + // PyObject::from_borrowed_ptr_or_opt will take ownership in this way. + PyObject::from_borrowed_ptr_or_opt(py, ptr) + .map(|pyobject| Ok(pyobject.into_ref(py))) + .or_else(|| PyErr::take(py).map(Err)) + .transpose() } }