Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add bound method variants for PyTypeInfo #3782

Merged
merged 1 commit into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion guide/src/class/numeric.md
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ fn my_module(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
# fn main() -> PyResult<()> {
# Python::with_gil(|py| -> PyResult<()> {
# let globals = PyModule::import(py, "__main__")?.dict();
# globals.set_item("Number", Number::type_object(py))?;
# globals.set_item("Number", Number::type_object_bound(py))?;
#
# py.run(SCRIPT, Some(globals), None)?;
# Ok(())
Expand Down
11 changes: 7 additions & 4 deletions guide/src/migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ To minimise breakage of code using the GIL-Refs API, the `Bound<T>` smart pointe
For example, the following APIs have gained updated variants:
- `PyList::new`, `PyTyple::new` and similar constructors have replacements `PyList::new_bound`, `PyTuple::new_bound` etc.
- `FromPyObject::extract` has a new `FromPyObject::extract_bound` (see the section below)
- The `PyTypeInfo` trait has had new `_bound` methods added to accept / return `Bound<T>`.

Because the new `Bound<T>` API brings ownership out of the PyO3 framework and into user code, there are a few places where user code is expected to need to adjust while switching to the new API:
- Code will need to add the occasional `&` to borrow the new smart pointer as `&Bound<T>` to pass these types around (or use `.clone()` at the very small cost of increasing the Python reference count)
Expand Down Expand Up @@ -245,6 +246,8 @@ impl<'py> FromPyObject<'py> for MyType {

The expectation is that in 0.22 `extract_bound` will have the default implementation removed and in 0.23 `extract` will be removed.



## from 0.19.* to 0.20

### Drop support for older technologies
Expand Down Expand Up @@ -656,7 +659,7 @@ To migrate, update trait bounds and imports from `PyTypeObject` to `PyTypeInfo`.

Before:

```rust,compile_fail
```rust,ignore
use pyo3::Python;
use pyo3::type_object::PyTypeObject;
use pyo3::types::PyType;
Expand All @@ -668,7 +671,7 @@ fn get_type_object<T: PyTypeObject>(py: Python<'_>) -> &PyType {

After

```rust
```rust,ignore
use pyo3::{Python, PyTypeInfo};
use pyo3::types::PyType;

Expand Down Expand Up @@ -995,13 +998,13 @@ makes it possible to interact with Python exception objects.

The new types also have names starting with the "Py" prefix. For example, before:

```rust,compile_fail
```rust,ignore
let err: PyErr = TypeError::py_err("error message");
```

After:

```rust,compile_fail
```rust,ignore
# use pyo3::{PyErr, PyResult, Python, type_object::PyTypeObject};
# use pyo3::exceptions::{PyBaseException, PyTypeError};
# Python::with_gil(|py| -> PyResult<()> {
Expand Down
5 changes: 2 additions & 3 deletions src/conversions/chrono_tz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@
//! ```
use crate::exceptions::PyValueError;
use crate::sync::GILOnceCell;
use crate::types::any::PyAnyMethods;
use crate::types::PyType;
use crate::types::{any::PyAnyMethods, PyType};
use crate::{
intern, Bound, FromPyObject, IntoPy, Py, PyAny, PyObject, PyResult, Python, ToPyObject,
};
Expand All @@ -51,7 +50,7 @@ impl ToPyObject for Tz {
.unwrap()
.call1((self.name(),))
.unwrap()
.into()
.unbind()
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/conversions/std/ipaddr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ impl ToPyObject for Ipv4Addr {
.expect("failed to load ipaddress.IPv4Address")
.call1((u32::from_be_bytes(self.octets()),))
.expect("failed to construct ipaddress.IPv4Address")
.to_object(py)
.unbind()
}
}

Expand All @@ -48,7 +48,7 @@ impl ToPyObject for Ipv6Addr {
.expect("failed to load ipaddress.IPv6Address")
.call1((u128::from_be_bytes(self.octets()),))
.expect("failed to construct ipaddress.IPv6Address")
.to_object(py)
.unbind()
}
}

Expand Down
20 changes: 13 additions & 7 deletions src/err/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ impl PyErr {
{
PyErr::from_state(PyErrState::Lazy(Box::new(move |py| {
PyErrStateLazyFnOutput {
ptype: T::type_object(py).into(),
ptype: T::type_object_bound(py).into(),
pvalue: args.arguments(py),
}
})))
Expand Down Expand Up @@ -540,7 +540,7 @@ impl PyErr {
where
T: PyTypeInfo,
{
self.is_instance(py, T::type_object(py))
self.is_instance(py, T::type_object_bound(py).as_gil_ref())
}

/// Writes the error back to the Python interpreter's global state.
Expand Down Expand Up @@ -1077,18 +1077,24 @@ mod tests {
fn test_pyerr_matches() {
Python::with_gil(|py| {
let err = PyErr::new::<PyValueError, _>("foo");
assert!(err.matches(py, PyValueError::type_object(py)));
assert!(err.matches(py, PyValueError::type_object_bound(py)));

assert!(err.matches(
py,
(PyValueError::type_object(py), PyTypeError::type_object(py))
(
PyValueError::type_object_bound(py),
PyTypeError::type_object_bound(py)
)
));

assert!(!err.matches(py, PyTypeError::type_object(py)));
assert!(!err.matches(py, PyTypeError::type_object_bound(py)));

// String is not a valid exception class, so we should get a TypeError
let err: PyErr = PyErr::from_type(crate::types::PyString::type_object(py), "foo");
assert!(err.matches(py, PyTypeError::type_object(py)));
let err: PyErr = PyErr::from_type(
crate::types::PyString::type_object_bound(py).as_gil_ref(),
"foo",
);
assert!(err.matches(py, PyTypeError::type_object_bound(py)));
})
}

Expand Down
2 changes: 1 addition & 1 deletion src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ impl<'py> Python<'py> {
where
T: PyTypeInfo,
{
T::type_object(self)
T::type_object_bound(self).into_gil_ref()
}

/// Imports the Python module with the specified name.
Expand Down
4 changes: 2 additions & 2 deletions src/pycell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ use crate::{
type_object::get_tp_free,
PyTypeInfo,
};
use crate::{ffi, IntoPy, PyErr, PyNativeType, PyObject, PyResult, PyTypeCheck, Python};
use crate::{ffi, Bound, IntoPy, PyErr, PyNativeType, PyObject, PyResult, PyTypeCheck, Python};
use std::cell::UnsafeCell;
use std::fmt;
use std::mem::ManuallyDrop;
Expand Down Expand Up @@ -553,7 +553,7 @@ where
{
const NAME: &'static str = <T as PyTypeCheck>::NAME;

fn type_check(object: &PyAny) -> bool {
fn type_check(object: &Bound<'_, PyAny>) -> bool {
<T as PyTypeCheck>::type_check(object)
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/sync.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! Synchronization mechanisms based on the Python GIL.
use crate::{instance::Bound, types::PyString, types::PyType, Py, PyResult, PyVisit, Python};
use crate::{types::PyString, types::PyType, Bound, Py, PyResult, PyVisit, Python};
use std::cell::UnsafeCell;

/// Value with concurrent access protected by the GIL.
Expand Down Expand Up @@ -196,9 +196,9 @@ impl GILOnceCell<Py<PyType>> {
py: Python<'py>,
module_name: &str,
attr_name: &str,
) -> PyResult<&'py PyType> {
) -> PyResult<&Bound<'py, PyType>> {
self.get_or_try_init(py, || py.import(module_name)?.getattr(attr_name)?.extract())
.map(|ty| ty.as_ref(py))
.map(|ty| ty.bind(py))
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/tests/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,11 +138,11 @@ mod inner {
($py:expr, $body:expr, [$(($category:ty, $message:literal)),+] $(,)? ) => {{
$crate::tests::common::CatchWarnings::enter($py, |w| {
$body;
let expected_warnings = [$((<$category as $crate::type_object::PyTypeInfo>::type_object($py), $message)),+];
let expected_warnings = [$((<$category as $crate::type_object::PyTypeInfo>::type_object_bound($py), $message)),+];
assert_eq!(w.len(), expected_warnings.len());
for (warning, (category, message)) in w.iter().zip(expected_warnings) {

assert!(warning.getattr("category").unwrap().is(category));
assert!(warning.getattr("category").unwrap().is(&category));
assert_eq!(
warning.getattr("message").unwrap().str().unwrap().to_string_lossy(),
message
Expand Down
62 changes: 58 additions & 4 deletions src/type_object.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
//! Python type object information
use crate::ffi_ptr_ext::FfiPtrExt;
use crate::types::any::PyAnyMethods;
use crate::types::{PyAny, PyType};
use crate::{ffi, PyNativeType, Python};
use crate::{ffi, Bound, PyNativeType, Python};

/// `T: PyLayout<U>` represents that `T` is a concrete representation of `U` in the Python heap.
/// E.g., `PyCell` is a concrete representation of all `pyclass`es, and `ffi::PyObject`
Expand Down Expand Up @@ -64,19 +66,71 @@ pub unsafe trait PyTypeInfo: Sized + HasPyGilRef {

/// Returns the safe abstraction over the type object.
#[inline]
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyTypeInfo::type_object` will be replaced by `PyTypeInfo::type_object_bound` in a future PyO3 version"
)
)]
fn type_object(py: Python<'_>) -> &PyType {
// This isn't implemented in terms of `type_object_bound` because this just borrowed the
// object, for legacy reasons.
unsafe { py.from_borrowed_ptr(Self::type_object_raw(py) as _) }
}

/// Returns the safe abstraction over the type object.
#[inline]
fn type_object_bound(py: Python<'_>) -> Bound<'_, PyType> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered whether to make type_object_bound, is_type_of_bound and is_exact_type_of_bound have defaults implemented in terms of their original methods (or vice versa).

Given that we don't expect users to be implementing PyTypeInfo, I decided it was probably simpler to just give them all defaults which match their final expected forms.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the new methods should be implemented in their final form. Whats the reason not reimplementing the old API in terms of the new one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very good point, I will go ahead and do that 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, there is one reason: type_object_bound now returns an owned Bound instead of a borrowed gil-ref, which I think is correct for the safety reasoning I express in the note below.

But is_type_of and is_exact_type_of can be implemented in terms of their new replacements 👍

// Making the borrowed object `Bound` is necessary for soundness reasons. It's an extreme
// edge case, but arbitrary Python code _could_ change the __class__ of an object and cause
// the type object to be freed.
//
// By making `Bound` we assume ownership which is then safe against races.
unsafe {
Self::type_object_raw(py)
.cast::<ffi::PyObject>()
.assume_borrowed_unchecked(py)
.to_owned()
.downcast_into_unchecked()
}
}

/// Checks if `object` is an instance of this type or a subclass of this type.
#[inline]
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyTypeInfo::is_type_of` will be replaced by `PyTypeInfo::is_type_of_bound` in a future PyO3 version"
)
)]
fn is_type_of(object: &PyAny) -> bool {
Self::is_type_of_bound(&object.as_borrowed())
}

/// Checks if `object` is an instance of this type or a subclass of this type.
#[inline]
fn is_type_of_bound(object: &Bound<'_, PyAny>) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_bound_type_of might read a little nicer (maybe, not sure 🙃, maybe someone else has an opinion too ). Similarly for the others

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm a bit stuck on that too. I don't really like either option, so my thinking was to stick with _bound as a suffix because that matches the majority of the other APIs...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree, they both read a bit odd, so its probably better to stick with the system we've introduced

unsafe { ffi::PyObject_TypeCheck(object.as_ptr(), Self::type_object_raw(object.py())) != 0 }
}

/// Checks if `object` is an instance of this type.
#[inline]
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyTypeInfo::is_exact_type_of` will be replaced by `PyTypeInfo::is_exact_type_of_bound` in a future PyO3 version"
)
)]
fn is_exact_type_of(object: &PyAny) -> bool {
Self::is_exact_type_of_bound(&object.as_borrowed())
}

/// Checks if `object` is an instance of this type.
#[inline]
fn is_exact_type_of_bound(object: &Bound<'_, PyAny>) -> bool {
unsafe { ffi::Py_TYPE(object.as_ptr()) == Self::type_object_raw(object.py()) }
}
}
Expand All @@ -89,7 +143,7 @@ pub trait PyTypeCheck: HasPyGilRef {
/// Checks if `object` is an instance of `Self`, which may include a subtype.
///
/// This should be equivalent to the Python expression `isinstance(object, Self)`.
fn type_check(object: &PyAny) -> bool;
fn type_check(object: &Bound<'_, PyAny>) -> bool;
}

impl<T> PyTypeCheck for T
Expand All @@ -99,8 +153,8 @@ where
const NAME: &'static str = <T as PyTypeInfo>::NAME;

#[inline]
fn type_check(object: &PyAny) -> bool {
<T as PyTypeInfo>::is_type_of(object)
fn type_check(object: &Bound<'_, PyAny>) -> bool {
T::is_type_of_bound(object)
}
}

Expand Down
12 changes: 6 additions & 6 deletions src/types/any.rs
Original file line number Diff line number Diff line change
Expand Up @@ -733,7 +733,7 @@ impl PyAny {
where
T: PyTypeCheck<AsRefTarget = T>,
{
if T::type_check(self) {
if T::type_check(&self.as_borrowed()) {
// Safety: type_check is responsible for ensuring that the type is correct
Ok(unsafe { self.downcast_unchecked() })
} else {
Expand Down Expand Up @@ -776,7 +776,7 @@ impl PyAny {
where
T: PyTypeInfo<AsRefTarget = T>,
{
if T::is_exact_type_of(self) {
if T::is_exact_type_of_bound(&self.as_borrowed()) {
// Safety: type_check is responsible for ensuring that the type is correct
Ok(unsafe { self.downcast_unchecked() })
} else {
Expand Down Expand Up @@ -2100,7 +2100,7 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> {
where
T: PyTypeCheck,
{
if T::type_check(self.as_gil_ref()) {
if T::type_check(self) {
// Safety: type_check is responsible for ensuring that the type is correct
Ok(unsafe { self.downcast_unchecked() })
} else {
Expand All @@ -2113,7 +2113,7 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> {
where
T: PyTypeCheck,
{
if T::type_check(self.as_gil_ref()) {
if T::type_check(&self) {
// Safety: type_check is responsible for ensuring that the type is correct
Ok(unsafe { self.downcast_into_unchecked() })
} else {
Expand Down Expand Up @@ -2218,12 +2218,12 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> {

#[inline]
fn is_instance_of<T: PyTypeInfo>(&self) -> bool {
T::is_type_of(self.as_gil_ref())
T::is_type_of_bound(self)
}

#[inline]
fn is_exact_instance_of<T: PyTypeInfo>(&self) -> bool {
T::is_exact_type_of(self.as_gil_ref())
T::is_exact_type_of_bound(self)
}

fn contains<V>(&self, value: V) -> PyResult<bool>
Expand Down
15 changes: 9 additions & 6 deletions src/types/ellipsis.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
use crate::{ffi, ffi_ptr_ext::FfiPtrExt, Borrowed, PyAny, PyTypeInfo, Python};
use crate::{
ffi, ffi_ptr_ext::FfiPtrExt, types::any::PyAnyMethods, Borrowed, Bound, PyAny, PyTypeInfo,
Python,
};

/// Represents the Python `Ellipsis` object.
#[repr(transparent)]
Expand Down Expand Up @@ -38,14 +41,14 @@ unsafe impl PyTypeInfo for PyEllipsis {
}

#[inline]
fn is_type_of(object: &PyAny) -> bool {
fn is_type_of_bound(object: &Bound<'_, PyAny>) -> bool {
// ellipsis is not usable as a base type
Self::is_exact_type_of(object)
Self::is_exact_type_of_bound(object)
}

#[inline]
fn is_exact_type_of(object: &PyAny) -> bool {
object.is(Self::get_bound(object.py()).as_ref())
fn is_exact_type_of_bound(object: &Bound<'_, PyAny>) -> bool {
object.is(&**Self::get_bound(object.py()))
}
}

Expand All @@ -68,7 +71,7 @@ mod tests {
Python::with_gil(|py| {
assert!(PyEllipsis::get_bound(py)
.get_type()
.is(PyEllipsis::type_object(py)));
.is(&PyEllipsis::type_object_bound(py)));
})
}

Expand Down
Loading
Loading