Skip to content

Commit

Permalink
deprecate optional GIL Ref in function argument (#3975)
Browse files Browse the repository at this point in the history
  • Loading branch information
davidhewitt authored Mar 21, 2024
1 parent 870a4bb commit 351c6a0
Show file tree
Hide file tree
Showing 9 changed files with 50 additions and 21 deletions.
2 changes: 1 addition & 1 deletion guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

</details>
Expand Down
2 changes: 1 addition & 1 deletion src/conversions/chrono.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ impl<Tz: TimeZone + for<'py> FromPyObject<'py>> FromPyObject<'_> for DateTime<Tz
#[cfg(not(Py_LIMITED_API))]
let tzinfo = dt.get_tzinfo_bound();
#[cfg(Py_LIMITED_API)]
let tzinfo: Option<&PyAny> = dt.getattr(intern!(dt.py(), "tzinfo"))?.extract()?;
let tzinfo: Option<Bound<'_, PyAny>> = dt.getattr(intern!(dt.py(), "tzinfo"))?.extract()?;

let tz = if let Some(tzinfo) = tzinfo {
tzinfo.extract()?
Expand Down
23 changes: 21 additions & 2 deletions src/impl_/deprecations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ pub fn inspect_fn<A, T>(f: fn(A) -> PyResult<T>, _: &GilRefs<A>) -> fn(A) -> PyR
f
}

pub struct GilRefs<T>(NotAGilRef<T>);
pub struct GilRefs<T>(OptionGilRefs<T>);
pub struct OptionGilRefs<T>(NotAGilRef<T>);
pub struct NotAGilRef<T>(std::marker::PhantomData<T>);

pub trait IsGilRef {}
Expand All @@ -23,7 +24,7 @@ impl<T: crate::PyNativeType> IsGilRef for &'_ T {}
impl<T> GilRefs<T> {
#[allow(clippy::new_without_default)]
pub fn new() -> Self {
GilRefs(NotAGilRef(std::marker::PhantomData))
GilRefs(OptionGilRefs(NotAGilRef(std::marker::PhantomData)))
}
}

Expand Down Expand Up @@ -54,13 +55,31 @@ impl<T: IsGilRef> GilRefs<T> {
pub fn from_py_with_arg(&self) {}
}

impl<T: IsGilRef> OptionGilRefs<Option<T>> {
#[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<T> NotAGilRef<T> {
pub fn function_arg(&self) {}
pub fn from_py_with_arg(&self) {}
pub fn is_python(&self) {}
}

impl<T> std::ops::Deref for GilRefs<T> {
type Target = OptionGilRefs<T>;
fn deref(&self) -> &Self::Target {
&self.0
}
}

impl<T> std::ops::Deref for OptionGilRefs<T> {
type Target = NotAGilRef<T>;
fn deref(&self) -> &Self::Target {
&self.0
Expand Down
4 changes: 2 additions & 2 deletions tests/test_arithmetics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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)
}

Expand Down
4 changes: 2 additions & 2 deletions tests/test_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ struct Mapping {
#[pymethods]
impl Mapping {
#[new]
fn new(elements: Option<&PyList>) -> PyResult<Self> {
fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult<Self> {
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 })
Expand Down
3 changes: 2 additions & 1 deletion tests/test_methods.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Expand Down Expand Up @@ -857,7 +858,7 @@ struct FromSequence {
#[pymethods]
impl FromSequence {
#[new]
fn new(seq: Option<&pyo3::types::PySequence>) -> PyResult<Self> {
fn new(seq: Option<&Bound<'_, PySequence>>) -> PyResult<Self> {
if let Some(seq) = seq {
Ok(FromSequence {
numbers: seq.as_ref().extract::<Vec<_>>()?,
Expand Down
4 changes: 2 additions & 2 deletions tests/test_sequence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ struct ByteSequence {
#[pymethods]
impl ByteSequence {
#[new]
fn new(elements: Option<&PyList>) -> PyResult<Self> {
fn new(elements: Option<&Bound<'_, PyList>>) -> PyResult<Self> {
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 })
Expand Down
3 changes: 3 additions & 0 deletions tests/ui/deprecations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down
26 changes: 16 additions & 10 deletions tests/ui/deprecations.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -64,34 +64,40 @@ error: use of deprecated method `pyo3::deprecations::GilRefs::<T>::function_arg`
96 | fn pyfunction_gil_ref(_any: &PyAny) {}
| ^

error: use of deprecated method `pyo3::deprecations::OptionGilRefs::<std::option::Option<T>>::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::<T>::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::<T>::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::<T>::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::<T>::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::<pyo3::Python<'_>>::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)

0 comments on commit 351c6a0

Please sign in to comment.