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.

Sign up for GitHub

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 constructor for PyBool #3790

Merged
merged 1 commit into from
Feb 3, 2024
Merged

add Bound constructor for PyBool #3790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/exception.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ use pyo3::prelude::*;
use pyo3::types::{PyBool, PyList};

Python::with_gil(|py| {
assert!(PyBool::new(py, true).is_instance_of::<PyBool>());
assert!(PyBool::new_bound(py, true).is_instance_of::<PyBool>());
let list = PyList::new_bound(py, &[1, 2, 3, 4]);
assert!(!list.is_instance_of::<PyBool>());
assert!(list.is_instance_of::<PyList>());
Expand Down
2 changes: 1 addition & 1 deletion src/impl_/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,7 +813,7 @@ slot_fragment_trait! {
// By default `__ne__` will try `__eq__` and invert the result
let slf: &PyAny = py.from_borrowed_ptr(slf);
let other: &PyAny = py.from_borrowed_ptr(other);
slf.eq(other).map(|is_eq| PyBool::new(py, !is_eq).into_ptr())
slf.eq(other).map(|is_eq| PyBool::new_bound(py, !is_eq).to_owned().into_ptr())
}
}

Expand Down
8 changes: 8 additions & 0 deletions src/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,14 @@ impl<'a, 'py> Borrowed<'a, 'py, PyAny> {
pub(crate) unsafe fn from_ptr_unchecked(py: Python<'py>, ptr: *mut ffi::PyObject) -> Self {
Self(NonNull::new_unchecked(ptr), PhantomData, py)
}

/// Converts this `PyAny` to a concrete Python type without checking validity.
///
/// # Safety
/// Callers must ensure that the type is valid or risk type confusion.
pub(crate) unsafe fn downcast_unchecked<T>(self) -> Borrowed<'a, 'py, T> {
Borrowed(self.0, PhantomData, self.2)
}
}

impl<'a, 'py, T> From<&'a Bound<'py, T>> for Borrowed<'a, 'py, T> {
Expand Down
20 changes: 11 additions & 9 deletions src/types/any.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -759,9 +759,9 @@ impl PyAny {
/// use pyo3::types::{PyBool, PyLong};
///
/// Python::with_gil(|py| {
/// let b = PyBool::new(py, true);
/// let b = PyBool::new_bound(py, true);
/// assert!(b.is_instance_of::<PyBool>());
/// let any: &PyAny = b.as_ref();
/// let any: &Bound<'_, PyAny> = b.as_any();
///
/// // `bool` is a subtype of `int`, so `downcast` will accept a `bool` as an `int`
/// // but `downcast_exact` will not.
Expand Down Expand Up @@ -1583,9 +1583,9 @@ pub trait PyAnyMethods<'py> {
/// use pyo3::types::{PyBool, PyLong};
///
/// Python::with_gil(|py| {
/// let b = PyBool::new(py, true);
/// let b = PyBool::new_bound(py, true);
/// assert!(b.is_instance_of::<PyBool>());
/// let any: &PyAny = b.as_ref();
/// let any: &Bound<'_, PyAny> = b.as_any();
///
/// // `bool` is a subtype of `int`, so `downcast` will accept a `bool` as an `int`
/// // but `downcast_exact` will not.
Expand Down Expand Up @@ -2530,7 +2530,7 @@ class SimpleClass:
let x = 5.to_object(py).into_ref(py);
assert!(x.is_exact_instance_of::<PyLong>());

let t = PyBool::new(py, true);
let t = PyBool::new_bound(py, true);
assert!(t.is_instance_of::<PyLong>());
assert!(!t.is_exact_instance_of::<PyLong>());
assert!(t.is_exact_instance_of::<PyBool>());
Expand All @@ -2543,10 +2543,12 @@ class SimpleClass:
#[test]
fn test_any_is_exact_instance() {
Python::with_gil(|py| {
let t = PyBool::new(py, true);
assert!(t.is_instance(py.get_type::<PyLong>()).unwrap());
assert!(!t.is_exact_instance(py.get_type::<PyLong>()));
assert!(t.is_exact_instance(py.get_type::<PyBool>()));
let t = PyBool::new_bound(py, true);
assert!(t
.is_instance(&py.get_type::<PyLong>().as_borrowed())
.unwrap());
assert!(!t.is_exact_instance(&py.get_type::<PyLong>().as_borrowed()));
assert!(t.is_exact_instance(&py.get_type::<PyBool>().as_borrowed()));
});
}

Expand Down
49 changes: 36 additions & 13 deletions src/types/boolobject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#[cfg(feature = "experimental-inspect")]
use crate::inspect::types::TypeInfo;
use crate::{
exceptions::PyTypeError, ffi, instance::Bound, FromPyObject, IntoPy, PyAny, PyNativeType,
PyObject, PyResult, Python, ToPyObject,
exceptions::PyTypeError, ffi, ffi_ptr_ext::FfiPtrExt, instance::Bound, Borrowed, FromPyObject,
IntoPy, PyAny, PyNativeType, PyObject, PyResult, Python, ToPyObject,
};

use super::any::PyAnyMethods;
Expand All @@ -14,12 +14,33 @@ pub struct PyBool(PyAny);
pyobject_native_type!(PyBool, ffi::PyObject, pyobject_native_static_type_object!(ffi::PyBool_Type), #checkfunction=ffi::PyBool_Check);

impl PyBool {
/// Depending on `val`, returns `true` or `false`.
/// Deprecated form of [`PyBool::new_bound`]
#[cfg_attr(
not(feature = "gil-refs"),
deprecated(
since = "0.21.0",
note = "`PyBool::new` will be replaced by `PyBool::new_bound` in a future PyO3 version"
)
)]
#[inline]
pub fn new(py: Python<'_>, val: bool) -> &PyBool {
unsafe { py.from_borrowed_ptr(if val { ffi::Py_True() } else { ffi::Py_False() }) }
}

/// Depending on `val`, returns `true` or `false`.
///
/// # Note
/// This returns a [`Borrowed`] reference to one of Pythons `True` or
/// `False` singletons
#[inline]
pub fn new_bound(py: Python<'_>, val: bool) -> Borrowed<'_, '_, Self> {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being late to the party: If these are singletons, shouldn't the first lifetime even be 'static, i.e.

pub fn new_bound(py: Python<'py>, val: bool) -> Borrowed<'static, 'py, Self>;

or are they still bound to the interpreter?

Copy link
Member

Choose a reason for hiding this comment

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

I wondered about this, I think while static is possibly fine I preferred having this limited to the interpreter. Just in case this becomes relevant for things like subinterpreters in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can always expand this later if users have a need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about this too, but it seemed safer to go with this option now, and relax it in the future if possible.

unsafe {
if val { ffi::Py_True() } else { ffi::Py_False() }
.assume_borrowed(py)
.downcast_unchecked()
}
}

/// Gets whether this boolean is `true`.
#[inline]
pub fn is_true(&self) -> bool {
Expand Down Expand Up @@ -65,7 +86,7 @@ impl ToPyObject for bool {
impl IntoPy<PyObject> for bool {
#[inline]
fn into_py(self, py: Python<'_>) -> PyObject {
PyBool::new(py, self).into()
PyBool::new_bound(py, self).into_py(py)
}

#[cfg(feature = "experimental-inspect")]
Expand Down Expand Up @@ -135,27 +156,29 @@ impl FromPyObject<'_> for bool {

#[cfg(test)]
mod tests {
use crate::types::{PyAny, PyBool};
use crate::types::any::PyAnyMethods;
use crate::types::boolobject::PyBoolMethods;
use crate::types::PyBool;
use crate::Python;
use crate::ToPyObject;

#[test]
fn test_true() {
Python::with_gil(|py| {
assert!(PyBool::new(py, true).is_true());
let t: &PyAny = PyBool::new(py, true).into();
assert!(t.extract::<bool>().unwrap());
assert!(true.to_object(py).is(PyBool::new(py, true)));
assert!(PyBool::new_bound(py, true).is_true());
let t = PyBool::new_bound(py, true);
assert!(t.as_any().extract::<bool>().unwrap());
assert!(true.to_object(py).is(&*PyBool::new_bound(py, true)));
});
}

#[test]
fn test_false() {
Python::with_gil(|py| {
assert!(!PyBool::new(py, false).is_true());
let t: &PyAny = PyBool::new(py, false).into();
assert!(!t.extract::<bool>().unwrap());
assert!(false.to_object(py).is(PyBool::new(py, false)));
assert!(!PyBool::new_bound(py, false).is_true());
let t = PyBool::new_bound(py, false);
assert!(!t.as_any().extract::<bool>().unwrap());
assert!(false.to_object(py).is(&*PyBool::new_bound(py, false)));
});
}
}
4 changes: 2 additions & 2 deletions tests/test_class_attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,12 @@ fn test_renaming_all_struct_fields() {
let struct_class = py.get_type::<StructWithRenamedFields>();
let struct_obj = struct_class.call0().unwrap();
assert!(struct_obj
.setattr("firstField", PyBool::new(py, false))
.setattr("firstField", PyBool::new_bound(py, false))
.is_ok());
py_assert!(py, struct_obj, "struct_obj.firstField == False");
py_assert!(py, struct_obj, "struct_obj.secondField == 5");
assert!(struct_obj
.setattr("third_field", PyBool::new(py, true))
.setattr("third_field", PyBool::new_bound(py, true))
.is_ok());
py_assert!(py, struct_obj, "struct_obj.third_field == True");
});
Expand Down
Loading