move PyDict::get_item_with_error to PyDict::get_item

guide/src/migration.md

@@ -9,6 +9,56 @@ For a detailed list of all changes, see the [CHANGELOG](changelog.md).
 
 PyO3 0.20 has increased minimum Rust version to 1.56. This enables use of newer language features and simplifies maintenance of the project.
 
+### `PyDict::get_item` now returns a `Result`
+
+`PyDict::get_item` in PyO3 0.19 and older was implemented using a Python API which would suppress all exceptions and return `None` in those cases. This included errors in `__hash__` and `__eq__` implementations of the key being looked up.
+
+Newer recommendations by the Python core developers advise against using these APIs which suppress exceptions, instead allowing exceptions to bubble upwards. `PyDict::get_item_with_error` already implemented this recommended behavior, so that API has been renamed to `PyDict::get_item`.
+
+Before:
+
+```rust,ignore
+use pyo3::prelude::*;
+use pyo3::exceptions::PyTypeError;
+use pyo3::types::{PyDict, IntoPyDict};
+
+# fn main() {
+# let _ =
+Python::with_gil(|py| {
+    let dict: &PyDict = [("a", 1)].into_py_dict(py);
+    // `a` is in the dictionary, with value 1
+    assert!(dict.get_item("a").map_or(Ok(false), |x| x.eq(1))?);
+    // `b` is not in the dictionary
+    assert!(dict.get_item("b").is_none());
+    // `dict` is not hashable, so this fails with a `TypeError`
+    assert!(dict.get_item_with_error(dict).unwrap_err().is_instance_of::<PyTypeError>(py));
+});
+# }
+```
+
+After:
+
+```rust
+use pyo3::prelude::*;
+use pyo3::exceptions::PyTypeError;
+use pyo3::types::{PyDict, IntoPyDict};
+
+# fn main() {
+# let _ =
+Python::with_gil(|py| -> PyResult<()> {
+    let dict: &PyDict = [("a", 1)].into_py_dict(py);
+    // `a` is in the dictionary, with value 1
+    assert!(dict.get_item("a")?.map_or(Ok(false), |x| x.eq(1))?);
+    // `b` is not in the dictionary
+    assert!(dict.get_item("b")?.is_none());
+    // `dict` is not hashable, so this fails with a `TypeError`
+    assert!(dict.get_item(dict).unwrap_err().is_instance_of::<PyTypeError>(py));
+
+    Ok(())
+});
+# }
+```
+
 ### Required arguments are no longer accepted after optional arguments
 
 [Trailing `Option<T>` arguments](./function/signature.md#trailing-optional-arguments) have an automatic default of `None`. To avoid unwanted changes when modifying function signatures, in PyO3 0.18 it was deprecated to have a required argument after an `Option<T>` argument without using `#[pyo3(signature = (...))]` to specify the intended defaults. In PyO3 0.20, this becomes a hard error.

newsfragments/3330.changed.md

@@ -0,0 +1 @@
+Change `PyDict::get_item` to no longer suppress arbitrary exceptions (the return type is now `PyResult<Option<&PyAny>>` instead of `Option<&PyAny>`), and deprecate `PyDict::get_item_with_error`.

pyo3-benches/benches/bench_decimal.rs

@@ -16,7 +16,7 @@ py_dec = decimal.Decimal("0.0")
             Some(locals),
         )
         .unwrap();
-        let py_dec = locals.get_item("py_dec").unwrap();
+        let py_dec = locals.get_item("py_dec").unwrap().unwrap();
 
         b.iter(|| {
             let _: Decimal = black_box(py_dec).extract().unwrap();

pyo3-benches/benches/bench_dict.rs

@@ -32,7 +32,12 @@ fn dict_get_item(b: &mut Bencher<'_>) {
         let mut sum = 0;
         b.iter(|| {
             for i in 0..LEN {
-                sum += dict.get_item(i).unwrap().extract::<usize>().unwrap();
+                sum += dict
+                    .get_item(i)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<usize>()
+                    .unwrap();
             }
         });
     });

src/conversions/chrono.rs

@@ -390,7 +390,7 @@ mod tests {
                 Some(locals),
             )
             .unwrap();
-            let result: PyResult<FixedOffset> = locals.get_item("zi").unwrap().extract();
+            let result: PyResult<FixedOffset> = locals.get_item("zi").unwrap().unwrap().extract();
             assert!(result.is_err());
             let res = result.err().unwrap();
             // Also check the error message is what we expect

src/conversions/hashbrown.rs

@@ -112,7 +112,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
             assert_eq!(map, py_map.extract().unwrap());
         });
     }
@@ -126,7 +134,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
         });
     }
 
@@ -139,7 +155,15 @@ mod tests {
             let py_map = map.into_py_dict(py);
 
             assert_eq!(py_map.len(), 1);
-            assert_eq!(py_map.get_item(1).unwrap().extract::<i32>().unwrap(), 1);
+            assert_eq!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap(),
+                1
+            );
         });
     }
 

src/conversions/indexmap.rs

@@ -148,7 +148,15 @@ mod test_indexmap {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
             assert_eq!(
                 map,
                 py_map.extract::<indexmap::IndexMap::<i32, i32>>().unwrap()
@@ -166,7 +174,15 @@ mod test_indexmap {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
         });
     }
 
@@ -179,7 +195,15 @@ mod test_indexmap {
             let py_map = map.into_py_dict(py);
 
             assert_eq!(py_map.len(), 1);
-            assert_eq!(py_map.get_item(1).unwrap().extract::<i32>().unwrap(), 1);
+            assert_eq!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap(),
+                1
+            );
         });
     }
 

src/conversions/rust_decimal.rs

@@ -130,7 +130,7 @@ mod test_rust_decimal {
                     )
                     .unwrap();
                     // Checks if Python Decimal -> Rust Decimal conversion is correct
-                    let py_dec = locals.get_item("py_dec").unwrap();
+                    let py_dec = locals.get_item("py_dec").unwrap().unwrap();
                     let py_result: Decimal = FromPyObject::extract(py_dec).unwrap();
                     assert_eq!(rs_orig, py_result);
                 })
@@ -192,7 +192,7 @@ mod test_rust_decimal {
                 Some(locals),
             )
             .unwrap();
-            let py_dec = locals.get_item("py_dec").unwrap();
+            let py_dec = locals.get_item("py_dec").unwrap().unwrap();
             let roundtripped: Result<Decimal, PyErr> = FromPyObject::extract(py_dec);
             assert!(roundtripped.is_err());
         })
@@ -208,7 +208,7 @@ mod test_rust_decimal {
                 Some(locals),
             )
             .unwrap();
-            let py_dec = locals.get_item("py_dec").unwrap();
+            let py_dec = locals.get_item("py_dec").unwrap().unwrap();
             let roundtripped: Result<Decimal, PyErr> = FromPyObject::extract(py_dec);
             assert!(roundtripped.is_err());
         })

src/conversions/std/map.rs

@@ -122,7 +122,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
             assert_eq!(map, py_map.extract().unwrap());
         });
     }
@@ -137,7 +145,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
             assert_eq!(map, py_map.extract().unwrap());
         });
     }
@@ -152,7 +168,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
         });
     }
 
@@ -166,7 +190,15 @@ mod tests {
             let py_map: &PyDict = m.downcast(py).unwrap();
 
             assert!(py_map.len() == 1);
-            assert!(py_map.get_item(1).unwrap().extract::<i32>().unwrap() == 1);
+            assert!(
+                py_map
+                    .get_item(1)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<i32>()
+                    .unwrap()
+                    == 1
+            );
         });
     }
 }

src/marker.rs

@@ -591,7 +591,7 @@ impl<'py> Python<'py> {
     ///         Some(locals),
     ///     )
     ///     .unwrap();
-    ///     let ret = locals.get_item("ret").unwrap();
+    ///     let ret = locals.get_item("ret").unwrap().unwrap();
     ///     let b64: &PyBytes = ret.downcast().unwrap();
     ///     assert_eq!(b64.as_bytes(), b"SGVsbG8gUnVzdCE=");
     /// });
@@ -1201,8 +1201,8 @@ mod tests {
             let namespace = PyDict::new(py);
             py.run("class Foo: pass", Some(namespace), Some(namespace))
                 .unwrap();
-            assert!(namespace.get_item("Foo").is_some());
-            assert!(namespace.get_item("__builtins__").is_some());
+            assert!(matches!(namespace.get_item("Foo"), Ok(Some(..))));
+            assert!(matches!(namespace.get_item("__builtins__"), Ok(Some(..))));
         })
     }
 }

src/sync.rs

@@ -264,7 +264,14 @@ mod tests {
             let dict = PyDict::new(py);
             dict.set_item(foo1, 42_usize).unwrap();
             assert!(dict.contains(foo2).unwrap());
-            assert_eq!(dict.get_item(foo3).unwrap().extract::<usize>().unwrap(), 42);
+            assert_eq!(
+                dict.get_item(foo3)
+                    .unwrap()
+                    .unwrap()
+                    .extract::<usize>()
+                    .unwrap(),
+                42
+            );
         });
     }