From bd0174aa5d361751bab65c14b89b90f4b2415355 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 16 Nov 2023 15:41:58 +0000 Subject: [PATCH] Change return types of `py.None()`, `py.NotImplemented()` and `py.Ellipsis()` to typed singletons --- guide/src/class/protocols.md | 2 +- guide/src/migration.md | 42 ++++++++++++++++++++++++-- newsfragments/3578.changed.md | 1 + pyo3-benches/benches/bench_gil.rs | 2 +- pyo3-benches/benches/bench_pyobject.rs | 2 +- pyo3-macros-backend/src/pyclass.rs | 6 ++-- pytests/src/awaitable.rs | 2 +- src/conversion.rs | 12 ++++---- src/conversions/chrono.rs | 2 +- src/err/mod.rs | 2 +- src/ffi/tests.rs | 6 ++-- src/marker.rs | 12 ++++---- src/pyclass.rs | 4 +-- src/types/bytearray.rs | 2 +- src/types/notimplemented.rs | 2 +- tests/test_arithmetics.rs | 4 +-- tests/test_class_conversion.rs | 2 +- tests/test_exceptions.rs | 2 +- tests/test_frompyobject.rs | 2 +- tests/test_gc.rs | 12 +++++--- tests/test_no_imports.rs | 2 +- 21 files changed, 82 insertions(+), 41 deletions(-) create mode 100644 newsfragments/3578.changed.md diff --git a/guide/src/class/protocols.md b/guide/src/class/protocols.md index 411978f0..4577a510 100644 --- a/guide/src/class/protocols.md +++ b/guide/src/class/protocols.md @@ -103,7 +103,7 @@ given signatures should be interpreted as follows: match op { CompareOp::Eq => (self.0 == other.0).into_py(py), CompareOp::Ne => (self.0 != other.0).into_py(py), - _ => py.NotImplemented(), + _ => py.NotImplemented().into(), } } } diff --git a/guide/src/migration.md b/guide/src/migration.md index ec6c6c84..75fe7453 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -3,6 +3,42 @@ This guide can help you upgrade code through breaking changes from one PyO3 version to the next. For a detailed list of all changes, see the [CHANGELOG](changelog.md). +## from 0.20.* to 0.21 + +### `py.None()`, `py.NotImplemented()` and `py.Ellipsis()` now return typed singletons + +Previously `py.None()`, `py.NotImplemented()` and `py.Ellipsis()` would return `PyObject`. This had a few downsides: + - `PyObject` does not carry static type information + - `PyObject` takes ownership of a reference to the singletons, adding refcounting performance overhead + - `PyObject` is not gil-bound, meaning follow up method calls might again need `py`, causing repetition + +To avoid these downsides, these methods now return typed gil-bound references to the singletons, e.g. `py.None()` returns `&PyNone`. These typed singletons all implement `Into`, so migration is straightforward. + +Before: + +```rust,compile_fail +# use pyo3::prelude::*; +Python::with_gil(|py| { + let a: PyObject = py.None(); + + let b: &PyAny = py.None().as_ref(py); // or into_ref(py) +}); +``` + +After: + +```rust +# use pyo3::prelude::*; +Python::with_gil(|py| { + // For uses needing a PyObject, add `.into()` + let a: PyObject = py.None().into(); + + // For uses needing &PyAny, remove `.as_ref(py)` + let b: &PyAny = py.None(); +}); +``` + + ## from 0.19.* to 0.20 ### Drop support for older technologies @@ -158,7 +194,7 @@ fn raise_err() -> anyhow::Result<()> { Err(PyValueError::new_err("original error message").into()) } -fn main() { +# fn main() { Python::with_gil(|py| { let rs_func = wrap_pyfunction!(raise_err, py).unwrap(); pyo3::py_run!( @@ -936,14 +972,14 @@ ensure that the Python GIL was held by the current thread). Technically, this wa To migrate, just pass a `py` argument to any calls to these methods. Before: -```rust,compile_fail +```rust,ignore # pyo3::Python::with_gil(|py| { py.None().get_refcnt(); # }) ``` After: -```rust +```rust,compile_fail # pyo3::Python::with_gil(|py| { py.None().get_refcnt(py); # }) diff --git a/newsfragments/3578.changed.md b/newsfragments/3578.changed.md new file mode 100644 index 00000000..53fc9943 --- /dev/null +++ b/newsfragments/3578.changed.md @@ -0,0 +1 @@ +Change return type of `py.None()`, `py.NotImplemented()`, and `py.Ellipsis()` from `PyObject` to typed singletons (`&PyNone`, `&PyNotImplemented` and `PyEllipsis` respectively). diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index 283e9149..e25345e1 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -16,7 +16,7 @@ fn bench_clean_acquire_gil(b: &mut Bencher<'_>) { } fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { - let obj = Python::with_gil(|py| py.None()); + let obj: PyObject = Python::with_gil(|py| py.None().into()); b.iter_batched( || { // Clone and drop an object so that the GILPool has work to do. diff --git a/pyo3-benches/benches/bench_pyobject.rs b/pyo3-benches/benches/bench_pyobject.rs index af25d61c..169cf1f0 100644 --- a/pyo3-benches/benches/bench_pyobject.rs +++ b/pyo3-benches/benches/bench_pyobject.rs @@ -6,7 +6,7 @@ fn drop_many_objects(b: &mut Bencher<'_>) { Python::with_gil(|py| { b.iter(|| { for _ in 0..1000 { - std::mem::drop(py.None()); + std::mem::drop(PyObject::from(py.None())); } }); }); diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index ab19a091..e3988ec1 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -587,7 +587,7 @@ fn impl_enum( return Ok((self_val == other.__pyo3__int__()).to_object(py)); } - return Ok(py.NotImplemented()); + return Ok(::std::convert::Into::into(py.NotImplemented())); } _pyo3::basic::CompareOp::Ne => { let self_val = self.__pyo3__int__(); @@ -598,9 +598,9 @@ fn impl_enum( return Ok((self_val != other.__pyo3__int__()).to_object(py)); } - return Ok(py.NotImplemented()); + return Ok(::std::convert::Into::into(py.NotImplemented())); } - _ => Ok(py.NotImplemented()), + _ => Ok(::std::convert::Into::into(py.NotImplemented())), } } }; diff --git a/pytests/src/awaitable.rs b/pytests/src/awaitable.rs index acbb8e21..1f798aa4 100644 --- a/pytests/src/awaitable.rs +++ b/pytests/src/awaitable.rs @@ -36,7 +36,7 @@ impl IterAwaitable { Ok(v) => Ok(IterNextOutput::Return(v)), Err(err) => Err(err), }, - _ => Ok(IterNextOutput::Yield(py.None())), + _ => Ok(IterNextOutput::Yield(py.None().into())), } } } diff --git a/src/conversion.rs b/src/conversion.rs index 5263442e..4ae166eb 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -141,7 +141,7 @@ pub trait ToPyObject { /// match self { /// Self::Integer(val) => val.into_py(py), /// Self::String(val) => val.into_py(py), -/// Self::None => py.None(), +/// Self::None => py.None().into(), /// } /// } /// } @@ -251,7 +251,7 @@ where { fn to_object(&self, py: Python<'_>) -> PyObject { self.as_ref() - .map_or_else(|| py.None(), |val| val.to_object(py)) + .map_or_else(|| py.None().into(), |val| val.to_object(py)) } } @@ -260,7 +260,7 @@ where T: IntoPy, { fn into_py(self, py: Python<'_>) -> PyObject { - self.map_or_else(|| py.None(), |val| val.into_py(py)) + self.map_or_else(|| py.None().into(), |val| val.into_py(py)) } } @@ -622,13 +622,13 @@ mod tests { assert_eq!(option.as_ptr(), std::ptr::null_mut()); let none = py.None(); - option = Some(none.clone()); + option = Some(none.into()); - let ref_cnt = none.get_refcnt(py); + let ref_cnt = none.get_refcnt(); assert_eq!(option.as_ptr(), none.as_ptr()); // Ensure ref count not changed by as_ptr call - assert_eq!(none.get_refcnt(py), ref_cnt); + assert_eq!(none.get_refcnt(), ref_cnt); }); } } diff --git a/src/conversions/chrono.rs b/src/conversions/chrono.rs index e29348fc..9ecbce37 100644 --- a/src/conversions/chrono.rs +++ b/src/conversions/chrono.rs @@ -481,7 +481,7 @@ mod tests { // Test that if a user tries to convert a python's timezone aware datetime into a naive // one, the conversion fails. Python::with_gil(|py| { - let none = py.None().into_ref(py); + let none = py.None(); assert_eq!( none.extract::().unwrap_err().to_string(), "TypeError: 'NoneType' object cannot be converted to 'PyDelta'" diff --git a/src/err/mod.rs b/src/err/mod.rs index f3aee16b..cd2139b6 100644 --- a/src/err/mod.rs +++ b/src/err/mod.rs @@ -181,7 +181,7 @@ impl PyErr { } else { // Assume obj is Type[Exception]; let later normalization handle if this // is not the case - PyErrState::lazy(obj, obj.py().None()) + PyErrState::lazy(obj, Option::::None) }; PyErr::from_state(state) diff --git a/src/ffi/tests.rs b/src/ffi/tests.rs index b4f423ca..2cbd84dc 100644 --- a/src/ffi/tests.rs +++ b/src/ffi/tests.rs @@ -314,15 +314,15 @@ fn test_inc_dec_ref_immortal() { Python::with_gil(|py| { let obj = py.None(); - let ref_count = obj.get_refcnt(py); + let ref_count = obj.get_refcnt(); let ptr = obj.as_ptr(); unsafe { Py_INCREF(ptr) }; - assert_eq!(obj.get_refcnt(py), ref_count); + assert_eq!(obj.get_refcnt(), ref_count); unsafe { Py_DECREF(ptr) }; - assert_eq!(obj.get_refcnt(py), ref_count); + assert_eq!(obj.get_refcnt(), ref_count); }) } diff --git a/src/marker.rs b/src/marker.rs index dfe876c8..a789e565 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -697,22 +697,22 @@ impl<'py> Python<'py> { /// Gets the Python builtin value `None`. #[allow(non_snake_case)] // the Python keyword starts with uppercase #[inline] - pub fn None(self) -> PyObject { - PyNone::get(self).into() + pub fn None(self) -> &'py PyNone { + PyNone::get(self) } /// Gets the Python builtin value `Ellipsis`, or `...`. #[allow(non_snake_case)] // the Python keyword starts with uppercase #[inline] - pub fn Ellipsis(self) -> PyObject { - PyEllipsis::get(self).into() + pub fn Ellipsis(self) -> &'py PyEllipsis { + PyEllipsis::get(self) } /// Gets the Python builtin value `NotImplemented`. #[allow(non_snake_case)] // the Python keyword starts with uppercase #[inline] - pub fn NotImplemented(self) -> PyObject { - PyNotImplemented::get(self).into() + pub fn NotImplemented(self) -> &'py PyNotImplemented { + PyNotImplemented::get(self) } /// Gets the running Python interpreter version as a string. diff --git a/src/pyclass.rs b/src/pyclass.rs index ed0c4f62..23affb4b 100644 --- a/src/pyclass.rs +++ b/src/pyclass.rs @@ -161,7 +161,7 @@ where fn convert(self, py: Python<'_>) -> PyResult { match self { Some(o) => Ok(PyIterNextOutput::Yield(o.into_py(py))), - None => Ok(PyIterNextOutput::Return(py.None())), + None => Ok(PyIterNextOutput::Return(py.None().into())), } } } @@ -210,7 +210,7 @@ where fn convert(self, py: Python<'_>) -> PyResult { match self { Some(o) => Ok(PyIterANextOutput::Yield(o.into_py(py))), - None => Ok(PyIterANextOutput::Return(py.None())), + None => Ok(PyIterANextOutput::Return(py.None().into())), } } } diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index f7306d2d..e8873ccb 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -324,7 +324,7 @@ mod tests { #[test] fn test_from_err() { Python::with_gil(|py| { - if let Err(err) = PyByteArray::from(py.None().as_ref(py)) { + if let Err(err) = PyByteArray::from(py.None()) { assert!(err.is_instance_of::(py)); } else { panic!("error"); diff --git a/src/types/notimplemented.rs b/src/types/notimplemented.rs index 2f08f945..258e584c 100644 --- a/src/types/notimplemented.rs +++ b/src/types/notimplemented.rs @@ -47,7 +47,7 @@ mod tests { assert!(PyNotImplemented::get(py) .downcast::() .unwrap() - .is(&py.NotImplemented())); + .is(py.NotImplemented())); }) } diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index 25b6fe35..86078080 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -470,7 +470,7 @@ impl RichComparisons2 { match op { CompareOp::Eq => true.into_py(other.py()), CompareOp::Ne => false.into_py(other.py()), - _ => other.py().NotImplemented(), + _ => other.py().NotImplemented().into(), } } } @@ -540,7 +540,7 @@ mod return_not_implemented { } fn __richcmp__(&self, other: PyRef<'_, Self>, _op: CompareOp) -> PyObject { - other.py().None() + other.py().None().into() } fn __add__<'p>(slf: PyRef<'p, Self>, _other: PyRef<'p, Self>) -> PyRef<'p, Self> { diff --git a/tests/test_class_conversion.rs b/tests/test_class_conversion.rs index 27a8f604..81a3d7bf 100644 --- a/tests/test_class_conversion.rs +++ b/tests/test_class_conversion.rs @@ -119,7 +119,7 @@ fn test_polymorphic_container_does_not_accept_other_types() { let setattr = |value: PyObject| p.as_ref(py).setattr("inner", value); assert!(setattr(1i32.into_py(py)).is_err()); - assert!(setattr(py.None()).is_err()); + assert!(setattr(py.None().into()).is_err()); assert!(setattr((1i32, 2i32).into_py(py)).is_err()); }); } diff --git a/tests/test_exceptions.rs b/tests/test_exceptions.rs index 18de62ae..700a5c54 100644 --- a/tests/test_exceptions.rs +++ b/tests/test_exceptions.rs @@ -117,7 +117,7 @@ fn test_write_unraisable() { assert!(object.is_none(py)); let err = PyRuntimeError::new_err("bar"); - err.write_unraisable(py, Some(py.NotImplemented().as_ref(py))); + err.write_unraisable(py, Some(py.NotImplemented())); let (err, object) = capture.borrow_mut(py).capture.take().unwrap(); assert_eq!(err.to_string(), "RuntimeError: bar"); diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index 3b6ba288..47e5ec53 100644 --- a/tests/test_frompyobject.rs +++ b/tests/test_frompyobject.rs @@ -352,7 +352,7 @@ fn test_enum() { _ => panic!("Expected extracting Foo::TransparentTuple, got {:?}", f), } let none = py.None(); - let f = Foo::extract(none.as_ref(py)).expect("Failed to extract Foo from int"); + let f = Foo::extract(none).expect("Failed to extract Foo from int"); match f { Foo::TransparentStructVar { a } => assert!(a.is_none()), _ => panic!("Expected extracting Foo::TransparentStructVar, got {:?}", f), diff --git a/tests/test_gc.rs b/tests/test_gc.rs index 032c7de8..bcb5c543 100644 --- a/tests/test_gc.rs +++ b/tests/test_gc.rs @@ -89,7 +89,7 @@ impl GcIntegration { fn __clear__(&mut self) { Python::with_gil(|py| { - self.self_ref = py.None(); + self.self_ref = py.None().into(); }); } } @@ -102,7 +102,7 @@ fn gc_integration() { let inst = PyCell::new( py, GcIntegration { - self_ref: py.None(), + self_ref: py.None().into(), dropped: TestDropCall { drop_called: Arc::clone(&drop_called), }, @@ -286,7 +286,9 @@ struct PartialTraverse { impl PartialTraverse { fn new(py: Python<'_>) -> Self { - Self { member: py.None() } + Self { + member: py.None().into(), + } } } @@ -322,7 +324,9 @@ struct PanickyTraverse { impl PanickyTraverse { fn new(py: Python<'_>) -> Self { - Self { member: py.None() } + Self { + member: py.None().into(), + } } } diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index df73b9a2..4f042827 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -5,7 +5,7 @@ #[pyo3::pyfunction] #[pyo3(name = "identity", signature = (x = None))] fn basic_function(py: pyo3::Python<'_>, x: Option) -> pyo3::PyObject { - x.unwrap_or_else(|| py.None()) + x.unwrap_or_else(|| py.None().into()) } #[pyo3::pymodule]