From 5e9d97d1c6170a816ea4ff1b0aafbcf8c3ea2089 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sat, 3 Feb 2024 19:01:18 +0100 Subject: [PATCH 1/8] Implement new API for PyNone #3684 --- guide/src/migration.md | 2 +- pyo3-benches/benches/bench_gil.rs | 2 +- pytests/src/awaitable.rs | 2 +- src/conversion.rs | 30 ++++++++++++++++-------------- src/coroutine.rs | 2 +- src/marker.rs | 4 ++-- src/types/none.rs | 19 ++++++++++++------- tests/test_arithmetics.rs | 2 +- tests/test_class_conversion.rs | 2 +- tests/test_frompyobject.rs | 2 +- tests/test_gc.rs | 8 ++++---- tests/test_no_imports.rs | 4 +++- 12 files changed, 44 insertions(+), 35 deletions(-) diff --git a/guide/src/migration.md b/guide/src/migration.md index d5ce7f4ac1f..4c7700e7a76 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -108,7 +108,7 @@ After: # use pyo3::prelude::*; Python::with_gil(|py| { // For uses needing a PyObject, add `.into()` - let a: PyObject = py.None().into(); + let a: PyObject = py.None().into_py(py); // For uses needing &PyAny, remove `.as_ref(py)` let b: &PyAny = py.None(); diff --git a/pyo3-benches/benches/bench_gil.rs b/pyo3-benches/benches/bench_gil.rs index 169c7c6cf9e..55a5a04ab57 100644 --- a/pyo3-benches/benches/bench_gil.rs +++ b/pyo3-benches/benches/bench_gil.rs @@ -8,7 +8,7 @@ fn bench_clean_acquire_gil(b: &mut Bencher<'_>) { } fn bench_dirty_acquire_gil(b: &mut Bencher<'_>) { - let obj: PyObject = Python::with_gil(|py| py.None().into()); + let obj: PyObject = Python::with_gil(|py| py.None().into_py(py)); b.iter_batched( || { // Clone and drop an object so that the GILPool has work to do. diff --git a/pytests/src/awaitable.rs b/pytests/src/awaitable.rs index 0cc173334b9..55f1bce478c 100644 --- a/pytests/src/awaitable.rs +++ b/pytests/src/awaitable.rs @@ -37,7 +37,7 @@ impl IterAwaitable { Ok(v) => Err(PyStopIteration::new_err(v)), Err(err) => Err(err), }, - _ => Ok(py.None().into()), + _ => Ok(py.None().into_py(py)), } } } diff --git a/src/conversion.rs b/src/conversion.rs index 98ef98310c7..f75a82d4601 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().into(), +/// Self::None => py.None().into_py(py), /// } /// } /// } @@ -266,7 +266,7 @@ where { fn to_object(&self, py: Python<'_>) -> PyObject { self.as_ref() - .map_or_else(|| py.None().into(), |val| val.to_object(py)) + .map_or_else(|| py.None().into_py(py), |val| val.to_object(py)) } } @@ -275,7 +275,7 @@ where T: IntoPy, { fn into_py(self, py: Python<'_>) -> PyObject { - self.map_or_else(|| py.None().into(), |val| val.into_py(py)) + self.map_or_else(|| py.None().into_py(py), |val| val.into_py(py)) } } @@ -593,6 +593,8 @@ mod test_no_clone {} #[cfg(test)] mod tests { + use crate::conversion::IntoPy; + use crate::prelude::PyAnyMethods; use crate::{PyObject, Python}; #[allow(deprecated)] @@ -629,14 +631,14 @@ mod tests { }); } - #[test] - fn test_try_from_unchecked() { - Python::with_gil(|py| { - let list = PyList::new(py, [1, 2, 3]); - let val = unsafe { ::try_from_unchecked(list.as_ref()) }; - assert!(list.is(val)); - }); - } + // #[test] + // fn test_try_from_unchecked() { + // Python::with_gil(|py| { + // let list = PyList::new(py, [1, 2, 3]); + // let val = unsafe { ::try_from_unchecked(list.as_ref()) }; + // assert!(list.is(val)); + // }); + // } } #[test] @@ -647,13 +649,13 @@ mod tests { assert_eq!(option.as_ptr(), std::ptr::null_mut()); let none = py.None(); - option = Some(none.into()); + option = Some(none.into_py(py)); - let ref_cnt = none.get_refcnt(); + let ref_cnt = none.into_py(py).get_refcnt(py); assert_eq!(option.as_ptr(), none.as_ptr()); // Ensure ref count not changed by as_ptr call - assert_eq!(none.get_refcnt(), ref_cnt); + assert_eq!(none.into_py(py).get_refcnt(py), ref_cnt); }); } } diff --git a/src/coroutine.rs b/src/coroutine.rs index 415bbc88db9..c4b5ddf3185 100644 --- a/src/coroutine.rs +++ b/src/coroutine.rs @@ -118,7 +118,7 @@ impl Coroutine { } // if waker has been waken during future polling, this is roughly equivalent to // `await asyncio.sleep(0)`, so just yield `None`. - Ok(py.None().into()) + Ok(py.None().into_py(py)) } } diff --git a/src/marker.rs b/src/marker.rs index 836d0109eeb..06fbec7e790 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -123,7 +123,7 @@ use crate::types::{ PyAny, PyDict, PyEllipsis, PyModule, PyNone, PyNotImplemented, PyString, PyType, }; use crate::version::PythonVersionInfo; -use crate::{ffi, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; +use crate::{ffi, Borrowed, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::os::raw::c_int; @@ -698,7 +698,7 @@ 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) -> &'py PyNone { + pub fn None(self) -> Borrowed<'py, 'py, PyNone> { PyNone::get(self) } diff --git a/src/types/none.rs b/src/types/none.rs index 19ee80ede57..c645d9dff29 100644 --- a/src/types/none.rs +++ b/src/types/none.rs @@ -1,4 +1,5 @@ -use crate::{ffi, IntoPy, PyAny, PyObject, PyTypeInfo, Python, ToPyObject}; +use crate::ffi_ptr_ext::FfiPtrExt; +use crate::{ffi, Borrowed, IntoPy, PyAny, PyObject, PyTypeInfo, Python, ToPyObject}; /// Represents the Python `None` object. #[repr(transparent)] @@ -10,8 +11,11 @@ pyobject_native_type_extract!(PyNone); impl PyNone { /// Returns the `None` object. #[inline] - pub fn get(py: Python<'_>) -> &PyNone { - unsafe { py.from_borrowed_ptr(ffi::Py_None()) } + pub fn get<'py>(py: Python<'py>) -> Borrowed<'py, 'py, PyNone> { + unsafe { + let bound = ffi::Py_None().assume_borrowed(py); + std::mem::transmute(bound) + } } } @@ -32,29 +36,30 @@ unsafe impl PyTypeInfo for PyNone { #[inline] fn is_exact_type_of(object: &PyAny) -> bool { - object.is(Self::get(object.py())) + let none = Self::get(object.py()); + object.is(none.as_ref()) } } /// `()` is converted to Python `None`. impl ToPyObject for () { fn to_object(&self, py: Python<'_>) -> PyObject { - PyNone::get(py).into() + PyNone::get(py).into_py(py) } } impl IntoPy for () { #[inline] fn into_py(self, py: Python<'_>) -> PyObject { - PyNone::get(py).into() + PyNone::get(py).into_py(py) } } #[cfg(test)] mod tests { + use crate::types::any::PyAnyMethods; use crate::types::{PyDict, PyNone}; use crate::{IntoPy, PyObject, PyTypeInfo, Python, ToPyObject}; - #[test] fn test_none_is_itself() { Python::with_gil(|py| { diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index 456d21a3b62..1b399a1f41c 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -556,7 +556,7 @@ mod return_not_implemented { } fn __richcmp__(&self, other: PyRef<'_, Self>, _op: CompareOp) -> PyObject { - other.py().None().into() + other.py().None().into_py(other.py()) } 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 81a3d7bfbfd..a0a16e21c88 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().into()).is_err()); + assert!(setattr(py.None().into_py(py)).is_err()); assert!(setattr((1i32, 2i32).into_py(py)).is_err()); }); } diff --git a/tests/test_frompyobject.rs b/tests/test_frompyobject.rs index 8eea9b896b2..c141b3e6e22 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).expect("Failed to extract Foo from int"); + let f = Foo::extract_bound(none.as_ref()).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 54c3e1a100c..0f68bf0b2f6 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().into(); + self.self_ref = py.None().into_py(py); }); } } @@ -102,7 +102,7 @@ fn gc_integration() { let inst = PyCell::new( py, GcIntegration { - self_ref: py.None().into(), + self_ref: py.None().into_py(py), dropped: TestDropCall { drop_called: Arc::clone(&drop_called), }, @@ -287,7 +287,7 @@ struct PartialTraverse { impl PartialTraverse { fn new(py: Python<'_>) -> Self { Self { - member: py.None().into(), + member: py.None().into_py(py), } } } @@ -325,7 +325,7 @@ struct PanickyTraverse { impl PanickyTraverse { fn new(py: Python<'_>) -> Self { Self { - member: py.None().into(), + member: py.None().into_py(py), } } } diff --git a/tests/test_no_imports.rs b/tests/test_no_imports.rs index 4f04282702e..fe49b2d2b31 100644 --- a/tests/test_no_imports.rs +++ b/tests/test_no_imports.rs @@ -2,10 +2,12 @@ #![cfg(feature = "macros")] +use pyo3::IntoPy; + #[pyo3::pyfunction] #[pyo3(name = "identity", signature = (x = None))] fn basic_function(py: pyo3::Python<'_>, x: Option) -> pyo3::PyObject { - x.unwrap_or_else(|| py.None().into()) + x.unwrap_or_else(|| py.None().into_py(py)) } #[pyo3::pymodule] From 7e94da576db430d683a22e2371cdb53eb7a0f728 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sat, 3 Feb 2024 20:44:48 +0100 Subject: [PATCH 2/8] Fix doctests --- guide/src/migration.md | 4 ++-- guide/src/python_from_rust.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/guide/src/migration.md b/guide/src/migration.md index 4c7700e7a76..7d65d3972c0 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -98,7 +98,7 @@ Before: Python::with_gil(|py| { let a: PyObject = py.None(); - let b: &PyAny = py.None().as_ref(py); // or into_ref(py) + // let b: &PyAny = py.None().as_ref(py); // or into_ref(py) }); ``` @@ -111,7 +111,7 @@ Python::with_gil(|py| { let a: PyObject = py.None().into_py(py); // For uses needing &PyAny, remove `.as_ref(py)` - let b: &PyAny = py.None(); + // let b: &PyAny = py.None(); }); ``` diff --git a/guide/src/python_from_rust.md b/guide/src/python_from_rust.md index 99da2e15434..a5cd624ee69 100644 --- a/guide/src/python_from_rust.md +++ b/guide/src/python_from_rust.md @@ -474,7 +474,7 @@ class House(object): Ok(_) => { let none = py.None(); house - .call_method1("__exit__", (&none, &none, &none)) + .call_method1("__exit__", (none, none, none)) .unwrap(); } Err(e) => { From a2a6062adcba6482d05786029fd669dda45db3e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sat, 3 Feb 2024 20:48:25 +0100 Subject: [PATCH 3/8] fmt --- src/conversion.rs | 1 - src/types/bytearray.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/conversion.rs b/src/conversion.rs index f75a82d4601..28e15d17972 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -594,7 +594,6 @@ mod test_no_clone {} #[cfg(test)] mod tests { use crate::conversion::IntoPy; - use crate::prelude::PyAnyMethods; use crate::{PyObject, Python}; #[allow(deprecated)] diff --git a/src/types/bytearray.rs b/src/types/bytearray.rs index 65aef27ee52..55cd75d570b 100644 --- a/src/types/bytearray.rs +++ b/src/types/bytearray.rs @@ -526,7 +526,7 @@ mod tests { use crate::types::bytearray::PyByteArrayMethods; use crate::types::string::PyStringMethods; use crate::types::PyByteArray; - use crate::{exceptions, Bound, PyAny, PyNativeType}; + use crate::{exceptions, Bound, PyAny}; use crate::{PyObject, Python}; #[test] From 9641b117529dbfe824b9d59f440a3267edd6a8db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sat, 3 Feb 2024 20:57:46 +0100 Subject: [PATCH 4/8] hmm --- src/conversions/chrono.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conversions/chrono.rs b/src/conversions/chrono.rs index 0e95375b5bd..8d201fb88c0 100644 --- a/src/conversions/chrono.rs +++ b/src/conversions/chrono.rs @@ -566,6 +566,7 @@ fn timezone_utc(py: Python<'_>) -> &PyAny { #[cfg(test)] mod tests { use super::*; + use crate::types::any::PyAnyMethods; use crate::{types::PyTuple, Py}; use std::{cmp::Ordering, panic}; From 507ea28b27962029459f3a2e6123ef083d755e08 Mon Sep 17 00:00:00 2001 From: Blaz Snuderl Date: Sat, 3 Feb 2024 21:14:31 +0100 Subject: [PATCH 5/8] test --- src/ffi/tests.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ffi/tests.rs b/src/ffi/tests.rs index cb0972590ff..a3160751875 100644 --- a/src/ffi/tests.rs +++ b/src/ffi/tests.rs @@ -311,6 +311,8 @@ fn test_inc_dec_ref() { #[test] #[cfg(Py_3_12)] fn test_inc_dec_ref_immortal() { + use crate::types::any::PyAnyMethods; + Python::with_gil(|py| { let obj = py.None(); From b1863c73df1fcb330b4c4b72467686fd1633bd1b Mon Sep 17 00:00:00 2001 From: Blaz Snuderl Date: Sat, 3 Feb 2024 21:25:47 +0100 Subject: [PATCH 6/8] clippy --- src/types/none.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/none.rs b/src/types/none.rs index c645d9dff29..6d135785c53 100644 --- a/src/types/none.rs +++ b/src/types/none.rs @@ -11,7 +11,7 @@ pyobject_native_type_extract!(PyNone); impl PyNone { /// Returns the `None` object. #[inline] - pub fn get<'py>(py: Python<'py>) -> Borrowed<'py, 'py, PyNone> { + pub fn get(py: Python<'_>) -> Borrowed<'_, '_, PyNone> { unsafe { let bound = ffi::Py_None().assume_borrowed(py); std::mem::transmute(bound) From eca943ea35f502584d9013f144f9dadbfc222192 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sun, 4 Feb 2024 07:30:28 +0100 Subject: [PATCH 7/8] Add new get_bound and mark old get as deprecated --- src/marker.rs | 4 ++-- src/types/none.rs | 38 ++++++++++++++++++++++++++------------ 2 files changed, 28 insertions(+), 14 deletions(-) diff --git a/src/marker.rs b/src/marker.rs index 55f5cf64694..74e3dde60cd 100644 --- a/src/marker.rs +++ b/src/marker.rs @@ -123,7 +123,7 @@ use crate::types::{ PyAny, PyDict, PyEllipsis, PyModule, PyNone, PyNotImplemented, PyString, PyType, }; use crate::version::PythonVersionInfo; -use crate::{ffi, Borrowed, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; +use crate::{ffi, FromPyPointer, IntoPy, Py, PyObject, PyTypeCheck, PyTypeInfo}; use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::os::raw::c_int; @@ -699,7 +699,7 @@ impl<'py> Python<'py> { #[allow(non_snake_case)] // the Python keyword starts with uppercase #[inline] pub fn None(self) -> PyObject { - PyNone::get(self).into() + PyNone::get_bound(self).into_py(self) } /// Gets the Python builtin value `Ellipsis`, or `...`. diff --git a/src/types/none.rs b/src/types/none.rs index 6d135785c53..44c483440d2 100644 --- a/src/types/none.rs +++ b/src/types/none.rs @@ -10,12 +10,23 @@ pyobject_native_type_extract!(PyNone); impl PyNone { /// Returns the `None` object. + /// Deprecated form of [`PyNone::get_bound`] + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "`PyNone::get` will be replaced by `PyBool::get_bound` in a future PyO3 version" + ) + )] #[inline] - pub fn get(py: Python<'_>) -> Borrowed<'_, '_, PyNone> { - unsafe { - let bound = ffi::Py_None().assume_borrowed(py); - std::mem::transmute(bound) - } + pub fn get(py: Python<'_>) -> &PyNone { + unsafe { py.from_borrowed_ptr(ffi::Py_None()) } + } + + /// Returns the `None` object. + #[inline] + pub fn get_bound(py: Python<'_>) -> Borrowed<'_, '_, PyNone> { + unsafe { ffi::Py_None().assume_borrowed(py).downcast_unchecked() } } } @@ -36,7 +47,7 @@ unsafe impl PyTypeInfo for PyNone { #[inline] fn is_exact_type_of(object: &PyAny) -> bool { - let none = Self::get(object.py()); + let none = Self::get_bound(object.py()); object.is(none.as_ref()) } } @@ -44,14 +55,14 @@ unsafe impl PyTypeInfo for PyNone { /// `()` is converted to Python `None`. impl ToPyObject for () { fn to_object(&self, py: Python<'_>) -> PyObject { - PyNone::get(py).into_py(py) + PyNone::get_bound(py).into_py(py) } } impl IntoPy for () { #[inline] fn into_py(self, py: Python<'_>) -> PyObject { - PyNone::get(py).into_py(py) + PyNone::get_bound(py).into_py(py) } } @@ -63,22 +74,25 @@ mod tests { #[test] fn test_none_is_itself() { Python::with_gil(|py| { - assert!(PyNone::get(py).is_instance_of::()); - assert!(PyNone::get(py).is_exact_instance_of::()); + assert!(PyNone::get_bound(py).is_instance_of::()); + assert!(PyNone::get_bound(py).is_exact_instance_of::()); }) } #[test] fn test_none_type_object_consistent() { Python::with_gil(|py| { - assert!(PyNone::get(py).get_type().is(PyNone::type_object(py))); + assert!(PyNone::get_bound(py).get_type().is(PyNone::type_object(py))); }) } #[test] fn test_none_is_none() { Python::with_gil(|py| { - assert!(PyNone::get(py).downcast::().unwrap().is_none()); + assert!(PyNone::get_bound(py) + .downcast::() + .unwrap() + .is_none()); }) } From d1e967e9ea144fe2472de14b102d94aaf1d4da41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bla=C5=BE=20=C5=A0nuderl?= Date: Sun, 4 Feb 2024 07:31:29 +0100 Subject: [PATCH 8/8] Uncomment a test --- src/conversion.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conversion.rs b/src/conversion.rs index 1370fb2a59e..3af038af4d9 100644 --- a/src/conversion.rs +++ b/src/conversion.rs @@ -629,14 +629,14 @@ mod tests { }); } - // #[test] - // fn test_try_from_unchecked() { - // Python::with_gil(|py| { - // let list = PyList::new(py, [1, 2, 3]); - // let val = unsafe { ::try_from_unchecked(list.as_ref()) }; - // assert!(list.is(val)); - // }); - // } + #[test] + fn test_try_from_unchecked() { + Python::with_gil(|py| { + let list = PyList::new(py, [1, 2, 3]); + let val = unsafe { ::try_from_unchecked(list.as_ref()) }; + assert!(list.is(val)); + }); + } } #[test]