From 351c6a0a49d67b21837487e12e0375ed1ab75144 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Thu, 21 Mar 2024 07:24:40 +0000 Subject: [PATCH] deprecate optional GIL Ref in function argument (#3975) --- guide/src/migration.md | 2 +- src/conversions/chrono.rs | 2 +- src/impl_/deprecations.rs | 23 +++++++++++++++++++++-- tests/test_arithmetics.rs | 4 ++-- tests/test_mapping.rs | 4 ++-- tests/test_methods.rs | 3 ++- tests/test_sequence.rs | 4 ++-- tests/ui/deprecations.rs | 3 +++ tests/ui/deprecations.stderr | 26 ++++++++++++++++---------- 9 files changed, 50 insertions(+), 21 deletions(-) diff --git a/guide/src/migration.md b/guide/src/migration.md index 5a362ec4..ac20c775 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -301,7 +301,7 @@ The expectation is that in 0.22 `extract_bound` will have the default implementa Despite a large amount of deprecations warnings produced by PyO3 to aid with the transition from GIL Refs to the Bound API, there are a few cases where PyO3 cannot automatically warn on uses of GIL Refs. It is worth checking for these cases manually after the deprecation warnings have all been addressed: - Individual implementations of the `FromPyObject` trait cannot be deprecated, so PyO3 cannot warn about uses of code patterns like `.extract<&PyAny>()` which produce a GIL Ref. -- GIL Refs in `#[pyfunction]` arguments emit a warning, but if the GIL Ref is wrapped inside another container such as `Option<&PyAny>` or `Vec<&PyAny>` then PyO3 cannot warn against this. +- GIL Refs in `#[pyfunction]` arguments emit a warning, but if the GIL Ref is wrapped inside another container such as `Vec<&PyAny>` then PyO3 cannot warn against this. - The `wrap_pyfunction!(function)(py)` deferred argument form of the `wrap_pyfunction` macro taking `py: Python<'py>` produces a GIL Ref, and due to limitations in type inference PyO3 cannot warn against this specific case. diff --git a/src/conversions/chrono.rs b/src/conversions/chrono.rs index 2e46a9e5..544d1cf2 100644 --- a/src/conversions/chrono.rs +++ b/src/conversions/chrono.rs @@ -292,7 +292,7 @@ impl FromPyObject<'py>> FromPyObject<'_> for DateTime = dt.getattr(intern!(dt.py(), "tzinfo"))?.extract()?; + let tzinfo: Option> = dt.getattr(intern!(dt.py(), "tzinfo"))?.extract()?; let tz = if let Some(tzinfo) = tzinfo { tzinfo.extract()? diff --git a/src/impl_/deprecations.rs b/src/impl_/deprecations.rs index 459eba91..9eb1da05 100644 --- a/src/impl_/deprecations.rs +++ b/src/impl_/deprecations.rs @@ -13,7 +13,8 @@ pub fn inspect_fn(f: fn(A) -> PyResult, _: &GilRefs) -> fn(A) -> PyR f } -pub struct GilRefs(NotAGilRef); +pub struct GilRefs(OptionGilRefs); +pub struct OptionGilRefs(NotAGilRef); pub struct NotAGilRef(std::marker::PhantomData); pub trait IsGilRef {} @@ -23,7 +24,7 @@ impl IsGilRef for &'_ T {} impl GilRefs { #[allow(clippy::new_without_default)] pub fn new() -> Self { - GilRefs(NotAGilRef(std::marker::PhantomData)) + GilRefs(OptionGilRefs(NotAGilRef(std::marker::PhantomData))) } } @@ -54,6 +55,17 @@ impl GilRefs { pub fn from_py_with_arg(&self) {} } +impl OptionGilRefs> { + #[cfg_attr( + not(feature = "gil-refs"), + deprecated( + since = "0.21.0", + note = "use `Option<&Bound<'_, T>>` instead for this function argument" + ) + )] + pub fn function_arg(&self) {} +} + impl NotAGilRef { pub fn function_arg(&self) {} pub fn from_py_with_arg(&self) {} @@ -61,6 +73,13 @@ impl NotAGilRef { } impl std::ops::Deref for GilRefs { + type Target = OptionGilRefs; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl std::ops::Deref for OptionGilRefs { type Target = NotAGilRef; fn deref(&self) -> &Self::Target { &self.0 diff --git a/tests/test_arithmetics.rs b/tests/test_arithmetics.rs index da8d72ea..b1efcd0a 100644 --- a/tests/test_arithmetics.rs +++ b/tests/test_arithmetics.rs @@ -289,7 +289,7 @@ impl RhsArithmetic { format!("{:?} | RA", other) } - fn __rpow__(&self, other: &Bound<'_, PyAny>, _mod: Option<&PyAny>) -> String { + fn __rpow__(&self, other: &Bound<'_, PyAny>, _mod: Option<&Bound<'_, PyAny>>) -> String { format!("{:?} ** RA", other) } } @@ -406,7 +406,7 @@ impl LhsAndRhs { format!("{:?} | RA", other) } - fn __rpow__(&self, other: &Bound<'_, PyAny>, _mod: Option<&PyAny>) -> String { + fn __rpow__(&self, other: &Bound<'_, PyAny>, _mod: Option<&Bound<'_, PyAny>>) -> String { format!("{:?} ** RA", other) } diff --git a/tests/test_mapping.rs b/tests/test_mapping.rs index 4e24db97..675dd14d 100644 --- a/tests/test_mapping.rs +++ b/tests/test_mapping.rs @@ -21,11 +21,11 @@ struct Mapping { #[pymethods] impl Mapping { #[new] - fn new(elements: Option<&PyList>) -> PyResult { + fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult { if let Some(pylist) = elements { let mut elems = HashMap::with_capacity(pylist.len()); for (i, pyelem) in pylist.into_iter().enumerate() { - let elem = String::extract(pyelem)?; + let elem = pyelem.extract()?; elems.insert(elem, i); } Ok(Self { index: elems }) diff --git a/tests/test_methods.rs b/tests/test_methods.rs index fa681337..2b5396e9 100644 --- a/tests/test_methods.rs +++ b/tests/test_methods.rs @@ -2,6 +2,7 @@ use pyo3::prelude::*; use pyo3::py_run; +use pyo3::types::PySequence; use pyo3::types::{IntoPyDict, PyDict, PyList, PySet, PyString, PyTuple, PyType}; #[path = "../src/tests/common.rs"] @@ -857,7 +858,7 @@ struct FromSequence { #[pymethods] impl FromSequence { #[new] - fn new(seq: Option<&pyo3::types::PySequence>) -> PyResult { + fn new(seq: Option<&Bound<'_, PySequence>>) -> PyResult { if let Some(seq) = seq { Ok(FromSequence { numbers: seq.as_ref().extract::>()?, diff --git a/tests/test_sequence.rs b/tests/test_sequence.rs index 4e128a0b..3715affc 100644 --- a/tests/test_sequence.rs +++ b/tests/test_sequence.rs @@ -17,11 +17,11 @@ struct ByteSequence { #[pymethods] impl ByteSequence { #[new] - fn new(elements: Option<&PyList>) -> PyResult { + fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult { if let Some(pylist) = elements { let mut elems = Vec::with_capacity(pylist.len()); for pyelem in pylist { - let elem = u8::extract(pyelem)?; + let elem = pyelem.extract()?; elems.push(elem); } Ok(Self { elements: elems }) diff --git a/tests/ui/deprecations.rs b/tests/ui/deprecations.rs index f623215f..6f3a1fed 100644 --- a/tests/ui/deprecations.rs +++ b/tests/ui/deprecations.rs @@ -95,6 +95,9 @@ fn pyfunction_from_py_with( #[pyfunction] fn pyfunction_gil_ref(_any: &PyAny) {} +#[pyfunction] +fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} + #[derive(Debug, FromPyObject)] pub struct Zap { #[pyo3(item)] diff --git a/tests/ui/deprecations.stderr b/tests/ui/deprecations.stderr index b133b214..1d87f31c 100644 --- a/tests/ui/deprecations.stderr +++ b/tests/ui/deprecations.stderr @@ -64,34 +64,40 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::::function_arg` 96 | fn pyfunction_gil_ref(_any: &PyAny) {} | ^ +error: use of deprecated method `pyo3::deprecations::OptionGilRefs::>::function_arg`: use `Option<&Bound<'_, T>>` instead for this function argument + --> tests/ui/deprecations.rs:99:36 + | +99 | fn pyfunction_option_gil_ref(_any: Option<&PyAny>) {} + | ^^^^^^ + error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:103:27 + --> tests/ui/deprecations.rs:106:27 | -103 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] +106 | #[pyo3(from_py_with = "PyAny::len", item("my_object"))] | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:113:27 + --> tests/ui/deprecations.rs:116:27 | -113 | #[pyo3(from_py_with = "PyAny::len")] usize, +116 | #[pyo3(from_py_with = "PyAny::len")] usize, | ^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:119:31 + --> tests/ui/deprecations.rs:122:31 | -119 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), +122 | Zip(#[pyo3(from_py_with = "extract_gil_ref")] i32), | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::::from_py_with_arg`: use `&Bound<'_, PyAny>` as the argument for this `from_py_with` extractor - --> tests/ui/deprecations.rs:126:27 + --> tests/ui/deprecations.rs:129:27 | -126 | #[pyo3(from_py_with = "extract_gil_ref")] +129 | #[pyo3(from_py_with = "extract_gil_ref")] | ^^^^^^^^^^^^^^^^^ error: use of deprecated method `pyo3::deprecations::GilRefs::>::is_python`: use `wrap_pyfunction_bound!` instead - --> tests/ui/deprecations.rs:139:13 + --> tests/ui/deprecations.rs:142:13 | -139 | let _ = wrap_pyfunction!(double, py); +142 | let _ = wrap_pyfunction!(double, py); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in the macro `wrap_pyfunction` (in Nightly builds, run with -Z macro-backtrace for more info)