Merge pull request #3578 from davidhewitt/typed-helpers

Change return types of `py.None()`, `py.NotImplemented()` and `py.Ellipsis()` to typed singletons

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(),
             }
         }
     }

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<PyObject>`, 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);
 # })

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).

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.

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()));
             }
         });
     });

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())),
                 }
             }
         };

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())),
         }
     }
 }

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<PyObject>,
 {
     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);
         });
     }
 }

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::<Duration>().unwrap_err().to_string(),
                 "TypeError: 'NoneType' object cannot be converted to 'PyDelta'"

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::<PyObject>::None)
         };
 
         PyErr::from_state(state)

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);
     })
 }

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.

src/pyclass.rs

@@ -161,7 +161,7 @@ where
     fn convert(self, py: Python<'_>) -> PyResult<PyIterNextOutput> {
         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<PyIterANextOutput> {
         match self {
             Some(o) => Ok(PyIterANextOutput::Yield(o.into_py(py))),
-            None => Ok(PyIterANextOutput::Return(py.None())),
+            None => Ok(PyIterANextOutput::Return(py.None().into())),
         }
     }
 }

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::<exceptions::PyTypeError>(py));
             } else {
                 panic!("error");

src/types/notimplemented.rs

@@ -47,7 +47,7 @@ mod tests {
             assert!(PyNotImplemented::get(py)
                 .downcast::<PyNotImplemented>()
                 .unwrap()
-                .is(&py.NotImplemented()));
+                .is(py.NotImplemented()));
         })
     }
 

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> {

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());
     });
 }

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");

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),

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(),
+        }
     }
 }
 

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>) -> pyo3::PyObject {
-    x.unwrap_or_else(|| py.None())
+    x.unwrap_or_else(|| py.None().into())
 }
 
 #[pyo3::pymodule]