migrate PyDictMethods trait bounds (#4493)

* migrate `PyDictMethods` trait bounds * migrate `IntoPyDict` * make `IntoPyDict::into_py_dict` fallible * update migration guide * remove unnecessary `clone` Co-authored-by: David Hewitt <mail@davidhewitt.dev> --------- Co-authored-by: David Hewitt <mail@davidhewitt.dev>
PyO3 · Oct 1, 2024 · faed5e2 · faed5e2
1 parent ce18f79
commit faed5e2
Show file tree

Hide file tree

Showing 36 changed files with 278 additions and 205 deletions.
diff --git a/README.md b/README.md
@@ -156,7 +156,7 @@ fn main() -> PyResult<()> {
         let sys = py.import("sys")?;
         let version: String = sys.getattr("version")?.extract()?;
 
-        let locals = [("os", py.import("os")?)].into_py_dict(py);
+        let locals = [("os", py.import("os")?)].into_py_dict(py)?;
         let code = c_str!("os.getenv('USER') or os.getenv('USERNAME') or 'Unknown'");
         let user: String = py.eval(code, None, Some(&locals))?.extract()?;
 

diff --git a/guide/src/exception.md b/guide/src/exception.md
@@ -23,15 +23,18 @@ use pyo3::exceptions::PyException;
 
 create_exception!(mymodule, CustomError, PyException);
 
+# fn main() -> PyResult<()> {
 Python::with_gil(|py| {
-    let ctx = [("CustomError", py.get_type::<CustomError>())].into_py_dict(py);
+    let ctx = [("CustomError", py.get_type::<CustomError>())].into_py_dict(py)?;
     pyo3::py_run!(
         py,
         *ctx,
         "assert str(CustomError) == \"<class 'mymodule.CustomError'>\""
     );
     pyo3::py_run!(py, *ctx, "assert CustomError('oops').args == ('oops',)");
-});
+#   Ok(())
+})
+# }
 ```
 
 When using PyO3 to create an extension module, you can add the new exception to

diff --git a/guide/src/migration.md b/guide/src/migration.md
@@ -45,23 +45,26 @@ let tup = PyTuple::new(py, [1, 2, 3]);
 <details open>
 <summary><small>Click to expand</small></summary>
 
-The `IntoPyDict::into_py_dict_bound` method has been renamed to `IntoPyDict::into_py_dict`. If you implemented `IntoPyDict` for your type, you should implement `into_py_dict` instead of `into_py_dict_bound`. The old name is still available but deprecated.
+The `IntoPyDict::into_py_dict_bound` method has been renamed to `IntoPyDict::into_py_dict` and is now fallible. If you implemented `IntoPyDict` for your type, you should implement `into_py_dict` instead of `into_py_dict_bound`. The old name is still available but deprecated.
 
 Before:
 
 ```rust,ignore
 # use pyo3::prelude::*;
 # use pyo3::types::{PyDict, IntoPyDict};
-# use pyo3::types::dict::PyDictItem;
-impl<T, I> IntoPyDict for I
+# use std::collections::HashMap;
+
+struct MyMap<K, V>(HashMap<K, V>);
+
+impl<K, V> IntoPyDict for MyMap<K, V>
 where
-    T: PyDictItem,
-    I: IntoIterator<Item = T>,
+    K: ToPyObject,
+    V: ToPyObject,
 {
     fn into_py_dict_bound(self, py: Python<'_>) -> Bound<'_, PyDict> {
-        let dict = PyDict::new(py);
-        for item in self {
-            dict.set_item(item.key(), item.value())
+        let dict = PyDict::new_bound(py);
+        for (key, value) in self.0 {
+            dict.set_item(key, value)
                 .expect("Failed to set_item on dict");
         }
         dict
@@ -71,22 +74,25 @@ where
 
 After:
 
-```rust,ignore
+```rust
 # use pyo3::prelude::*;
 # use pyo3::types::{PyDict, IntoPyDict};
-# use pyo3::types::dict::PyDictItem;
-impl<T, I> IntoPyDict for I
+# use std::collections::HashMap;
+
+# #[allow(dead_code)]
+# struct MyMap<K, V>(HashMap<K, V>);
+
+impl<'py, K, V> IntoPyDict<'py> for MyMap<K, V>
 where
-    T: PyDictItem,
-    I: IntoIterator<Item = T>,
+    K: IntoPyObject<'py>,
+    V: IntoPyObject<'py>,
 {
-    fn into_py_dict(self, py: Python<'_>) -> Bound<'_, PyDict> {
+    fn into_py_dict(self, py: Python<'py>) -> PyResult<Bound<'py, PyDict>> {
         let dict = PyDict::new(py);
-        for item in self {
-            dict.set_item(item.key(), item.value())
-                .expect("Failed to set_item on dict");
+        for (key, value) in self.0 {
+            dict.set_item(key, value)?;
         }
-        dict
+        Ok(dict)
     }
 }
 ```

diff --git a/guide/src/module.md b/guide/src/module.md
@@ -90,7 +90,7 @@ fn func() -> String {
 #    use pyo3::types::IntoPyDict;
 #    use pyo3::ffi::c_str;
 #    let parent_module = wrap_pymodule!(parent_module)(py);
-#    let ctx = [("parent_module", parent_module)].into_py_dict(py);
+#    let ctx = [("parent_module", parent_module)].into_py_dict(py).unwrap();
 #
 #    py.run(c_str!("assert parent_module.child_module.func() == 'func'"), None, Some(&ctx)).unwrap();
 # })

diff --git a/guide/src/python-from-rust/calling-existing-code.md b/guide/src/python-from-rust/calling-existing-code.md
@@ -131,7 +131,7 @@ def leaky_relu(x, slope=0.01):
     let relu_result: f64 = activators.getattr("relu")?.call1((-1.0,))?.extract()?;
     assert_eq!(relu_result, 0.0);
 
-    let kwargs = [("slope", 0.2)].into_py_dict(py);
+    let kwargs = [("slope", 0.2)].into_py_dict(py)?;
     let lrelu_result: f64 = activators
         .getattr("leaky_relu")?
         .call((-1.0,), Some(&kwargs))?

diff --git a/guide/src/python-from-rust/function-calls.md b/guide/src/python-from-rust/function-calls.md
@@ -90,17 +90,17 @@ fn main() -> PyResult<()> {
         .into();
 
         // call object with PyDict
-        let kwargs = [(key1, val1)].into_py_dict(py);
+        let kwargs = [(key1, val1)].into_py_dict(py)?;
         fun.call(py, (), Some(&kwargs))?;
 
         // pass arguments as Vec
         let kwargs = vec![(key1, val1), (key2, val2)];
-        fun.call(py, (), Some(&kwargs.into_py_dict(py)))?;
+        fun.call(py, (), Some(&kwargs.into_py_dict(py)?))?;
 
         // pass arguments as HashMap
         let mut kwargs = HashMap::<&str, i32>::new();
         kwargs.insert(key1, 1);
-        fun.call(py, (), Some(&kwargs.into_py_dict(py)))?;
+        fun.call(py, (), Some(&kwargs.into_py_dict(py)?))?;
 
         Ok(())
     })

diff --git a/newsfragments/4493.changed.md b/newsfragments/4493.changed.md
@@ -0,0 +1 @@
+`IntoPyDict::into_py_dict` is now fallible due to `IntoPyObject` migration.
diff --git a/src/conversions/anyhow.rs b/src/conversions/anyhow.rs
@@ -145,7 +145,7 @@ mod test_anyhow {
         let pyerr = PyErr::from(err);
 
         Python::with_gil(|py| {
-            let locals = [("err", pyerr)].into_py_dict(py);
+            let locals = [("err", pyerr)].into_py_dict(py).unwrap();
             let pyerr = py
                 .run(ffi::c_str!("raise err"), None, Some(&locals))
                 .unwrap_err();
@@ -164,7 +164,7 @@ mod test_anyhow {
         let pyerr = PyErr::from(err);
 
         Python::with_gil(|py| {
-            let locals = [("err", pyerr)].into_py_dict(py);
+            let locals = [("err", pyerr)].into_py_dict(py).unwrap();
             let pyerr = py
                 .run(ffi::c_str!("raise err"), None, Some(&locals))
                 .unwrap_err();

diff --git a/src/conversions/chrono.rs b/src/conversions/chrono.rs
@@ -1365,7 +1365,7 @@ mod tests {
             fn test_pyo3_offset_fixed_frompyobject_created_in_python(timestamp in 0..(i32::MAX as i64), timedelta in -86399i32..=86399i32) {
                 Python::with_gil(|py| {
 
-                    let globals = [("datetime", py.import("datetime").unwrap())].into_py_dict(py);
+                    let globals = [("datetime", py.import("datetime").unwrap())].into_py_dict(py).unwrap();
                     let code = format!("datetime.datetime.fromtimestamp({}).replace(tzinfo=datetime.timezone(datetime.timedelta(seconds={})))", timestamp, timedelta);
                     let t = py.eval(&CString::new(code).unwrap(), Some(&globals), None).unwrap();
 

diff --git a/src/conversions/eyre.rs b/src/conversions/eyre.rs
@@ -151,7 +151,7 @@ mod tests {
         let pyerr = PyErr::from(err);
 
         Python::with_gil(|py| {
-            let locals = [("err", pyerr)].into_py_dict(py);
+            let locals = [("err", pyerr)].into_py_dict(py).unwrap();
             let pyerr = py
                 .run(ffi::c_str!("raise err"), None, Some(&locals))
                 .unwrap_err();
@@ -170,7 +170,7 @@ mod tests {
         let pyerr = PyErr::from(err);
 
         Python::with_gil(|py| {
-            let locals = [("err", pyerr)].into_py_dict(py);
+            let locals = [("err", pyerr)].into_py_dict(py).unwrap();
             let pyerr = py
                 .run(ffi::c_str!("raise err"), None, Some(&locals))
                 .unwrap_err();

diff --git a/src/conversions/hashbrown.rs b/src/conversions/hashbrown.rs
@@ -23,9 +23,9 @@ use crate::{
         dict::PyDictMethods,
         frozenset::PyFrozenSetMethods,
         set::{new_from_iter, try_new_from_iter, PySetMethods},
-        IntoPyDict, PyDict, PyFrozenSet, PySet,
+        PyDict, PyFrozenSet, PySet,
     },
-    Bound, BoundObject, FromPyObject, IntoPy, PyAny, PyErr, PyObject, PyResult, Python, ToPyObject,
+    Bound, FromPyObject, IntoPy, PyAny, PyErr, PyObject, PyResult, Python, ToPyObject,
 };
 use std::{cmp, hash};
 
@@ -36,7 +36,11 @@ where
     H: hash::BuildHasher,
 {
     fn to_object(&self, py: Python<'_>) -> PyObject {
-        IntoPyDict::into_py_dict(self, py).into()
+        let dict = PyDict::new(py);
+        for (k, v) in self {
+            dict.set_item(k.to_object(py), v.to_object(py)).unwrap();
+        }
+        dict.into_any().unbind()
     }
 }
 
@@ -47,10 +51,11 @@ where
     H: hash::BuildHasher,
 {
     fn into_py(self, py: Python<'_>) -> PyObject {
-        let iter = self
-            .into_iter()
-            .map(|(k, v)| (k.into_py(py), v.into_py(py)));
-        IntoPyDict::into_py_dict(iter, py).into()
+        let dict = PyDict::new(py);
+        for (k, v) in self {
+            dict.set_item(k.into_py(py), v.into_py(py)).unwrap();
+        }
+        dict.into_any().unbind()
     }
 }
 
@@ -67,10 +72,7 @@ where
     fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
         let dict = PyDict::new(py);
         for (k, v) in self {
-            dict.set_item(
-                k.into_pyobject(py).map_err(Into::into)?.into_bound(),
-                v.into_pyobject(py).map_err(Into::into)?.into_bound(),
-            )?;
+            dict.set_item(k, v)?;
         }
         Ok(dict)
     }
@@ -89,10 +91,7 @@ where
     fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
         let dict = PyDict::new(py);
         for (k, v) in self {
-            dict.set_item(
-                k.into_pyobject(py).map_err(Into::into)?.into_bound(),
-                v.into_pyobject(py).map_err(Into::into)?.into_bound(),
-            )?;
+            dict.set_item(k, v)?;
         }
         Ok(dict)
     }
@@ -187,6 +186,7 @@ where
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::types::IntoPyDict;
 
     #[test]
     fn test_hashbrown_hashmap_to_python() {
@@ -238,7 +238,7 @@ mod tests {
             let mut map = hashbrown::HashMap::<i32, i32>::new();
             map.insert(1, 1);
 
-            let py_map = map.into_py_dict(py);
+            let py_map = map.into_py_dict(py).unwrap();
 
             assert_eq!(py_map.len(), 1);
             assert_eq!(

diff --git a/src/conversions/indexmap.rs b/src/conversions/indexmap.rs
@@ -89,7 +89,7 @@
 
 use crate::conversion::IntoPyObject;
 use crate::types::*;
-use crate::{Bound, BoundObject, FromPyObject, IntoPy, PyErr, PyObject, Python, ToPyObject};
+use crate::{Bound, FromPyObject, IntoPy, PyErr, PyObject, Python, ToPyObject};
 use std::{cmp, hash};
 
 impl<K, V, H> ToPyObject for indexmap::IndexMap<K, V, H>
@@ -99,7 +99,11 @@ where
     H: hash::BuildHasher,
 {
     fn to_object(&self, py: Python<'_>) -> PyObject {
-        IntoPyDict::into_py_dict(self, py).into()
+        let dict = PyDict::new(py);
+        for (k, v) in self {
+            dict.set_item(k.to_object(py), v.to_object(py)).unwrap();
+        }
+        dict.into_any().unbind()
     }
 }
 
@@ -110,10 +114,11 @@ where
     H: hash::BuildHasher,
 {
     fn into_py(self, py: Python<'_>) -> PyObject {
-        let iter = self
-            .into_iter()
-            .map(|(k, v)| (k.into_py(py), v.into_py(py)));
-        IntoPyDict::into_py_dict(iter, py).into()
+        let dict = PyDict::new(py);
+        for (k, v) in self {
+            dict.set_item(k.into_py(py), v.into_py(py)).unwrap();
+        }
+        dict.into_any().unbind()
     }
 }
 
@@ -130,10 +135,7 @@ where
     fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
         let dict = PyDict::new(py);
         for (k, v) in self {
-            dict.set_item(
-                k.into_pyobject(py).map_err(Into::into)?.into_bound(),
-                v.into_pyobject(py).map_err(Into::into)?.into_bound(),
-            )?;
+            dict.set_item(k, v)?;
         }
         Ok(dict)
     }
@@ -152,10 +154,7 @@ where
     fn into_pyobject(self, py: Python<'py>) -> Result<Self::Output, Self::Error> {
         let dict = PyDict::new(py);
         for (k, v) in self {
-            dict.set_item(
-                k.into_pyobject(py).map_err(Into::into)?.into_bound(),
-                v.into_pyobject(py).map_err(Into::into)?.into_bound(),
-            )?;
+            dict.set_item(k, v)?;
         }
         Ok(dict)
     }
@@ -237,7 +236,7 @@ mod test_indexmap {
             let mut map = indexmap::IndexMap::<i32, i32>::new();
             map.insert(1, 1);
 
-            let py_map = map.into_py_dict(py);
+            let py_map = map.into_py_dict(py).unwrap();
 
             assert_eq!(py_map.len(), 1);
             assert_eq!(
@@ -266,7 +265,7 @@ mod test_indexmap {
                 }
             }
 
-            let py_map = map.clone().into_py_dict(py);
+            let py_map = (&map).into_py_dict(py).unwrap();
 
             let trip_map = py_map.extract::<indexmap::IndexMap<i32, i32>>().unwrap();