refactor PyDict::get_item[_with_error] implementations

src/types/dict.rs

@@ -5,7 +5,6 @@ use crate::types::{PyAny, PyList};
 use crate::{ffi, AsPyPointer, Python, ToPyObject};
 #[cfg(not(PyPy))]
 use crate::{IntoPyPointer, PyObject};
-use std::ptr::NonNull;
 
 /// Represents a Python `dict`.
 #[repr(transparent)]
@@ -143,12 +142,17 @@ impl PyDict {
     where
         K: ToPyObject,
     {
+        let key = key.to_object(self.py());
+        self.get_item_impl(key)
+    }
+
+    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 +165,20 @@ impl PyDict {
     where
         K: ToPyObject,
     {
+        let key = key.to_object(self.py());
+        self.get_item_with_error_impl(key)
+    }
+
+    fn get_item_with_error_impl(&self, key: PyObject) -> PyResult<Option<&PyAny>> {
+        let py = self.py();
         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()));
+            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.
+            match PyObject::from_borrowed_ptr_or_opt(py, ptr) {
+                Some(pyobject) => Ok(Some(pyobject.into_ref(py))),
+                None => PyErr::take(py).map(Err).transpose(),
             }
-
-            Ok(NonNull::new(ptr).map(|p| self.py().from_owned_ptr(ffi::_Py_NewRef(p.as_ptr()))))
         }
     }