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

Impl partialeq for pylong #4317

Merged
merged 10 commits into from
Jul 27, 2024
1 change: 1 addition & 0 deletions newsfragments/4317.added.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Implement `PartialEq` for `Bound<'py, PyInt>` with `u8`, `u16`, `u32`, `u64`, `u128`, `usize`, `i8`, `i16`, `i32`, `i64`, `i128` and `isize`.
114 changes: 113 additions & 1 deletion src/types/num.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
use crate::{ffi, PyAny};
use super::any::PyAnyMethods;

use crate::{ffi, instance::Bound, PyAny};

/// Represents a Python `int` object.
///
Expand All @@ -17,3 +19,113 @@ pyobject_native_type_core!(PyInt, pyobject_native_static_type_object!(ffi::PyLon
/// Deprecated alias for [`PyInt`].
#[deprecated(since = "0.23.0", note = "use `PyInt` instead")]
pub type PyLong = PyInt;

macro_rules! int_compare {
($rust_type: ty) => {
impl PartialEq<$rust_type> for Bound<'_, PyInt> {
#[inline]
fn eq(&self, other: &$rust_type) -> bool {
if let Ok(value) = self.extract::<$rust_type>() {
value == *other
} else {
false
}
}
}
impl PartialEq<Bound<'_, PyInt>> for $rust_type {
#[inline]
fn eq(&self, other: &Bound<'_, PyInt>) -> bool {
if let Ok(value) = other.extract::<$rust_type>() {
value == *self
} else {
false
}
}
}
};
}

int_compare!(i8);
int_compare!(u8);
int_compare!(i16);
int_compare!(u16);
int_compare!(i32);
int_compare!(u32);
int_compare!(i64);
int_compare!(u64);
int_compare!(i128);
int_compare!(u128);
int_compare!(isize);
int_compare!(usize);

#[cfg(test)]
mod tests {
use super::PyInt;
use crate::{types::PyAnyMethods, IntoPy, Python};

#[test]
fn test_partial_eq() {
Python::with_gil(|py| {
let v_i8 = 123i8;
let v_u8 = 123i8;
let v_i16 = 123i16;
let v_u16 = 123u16;
let v_i32 = 123i32;
let v_u32 = 123u32;
let v_i64 = 123i64;
let v_u64 = 123u64;
let v_i128 = 123i128;
let v_u128 = 123u128;
let v_isize = 123isize;
let v_usize = 123usize;
let obj = 123_i64.into_py(py).downcast_bound(py).unwrap().clone();
assert_eq!(v_i8, obj);
assert_eq!(obj, v_i8);

assert_eq!(v_u8, obj);
assert_eq!(obj, v_u8);

assert_eq!(v_i16, obj);
assert_eq!(obj, v_i16);

assert_eq!(v_u16, obj);
assert_eq!(obj, v_u16);

assert_eq!(v_i32, obj);
assert_eq!(obj, v_i32);

assert_eq!(v_u32, obj);
assert_eq!(obj, v_u32);

assert_eq!(v_i64, obj);
assert_eq!(obj, v_i64);

assert_eq!(v_u64, obj);
assert_eq!(obj, v_u64);

assert_eq!(v_i128, obj);
assert_eq!(obj, v_i128);

assert_eq!(v_u128, obj);
assert_eq!(obj, v_u128);

assert_eq!(v_isize, obj);
assert_eq!(obj, v_isize);

assert_eq!(v_usize, obj);
assert_eq!(obj, v_usize);

WilliamTakeshi marked this conversation as resolved.
Show resolved Hide resolved
let big_num = (u8::MAX as u16) + 1;
let big_obj = big_num
.into_py(py)
.into_bound(py)
.downcast_into::<PyInt>()
.unwrap();

for x in 0u8..=u8::MAX {
assert_ne!(x, big_obj);
assert_ne!(big_obj, x);
}
});
}
}
2 changes: 1 addition & 1 deletion tests/test_class_attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ macro_rules! test_case {
let struct_obj = struct_class.call0().unwrap();
assert!(struct_obj.setattr($renamed_field_name, 2).is_ok());
let attr = struct_obj.getattr($renamed_field_name).unwrap();
assert_eq!(2, attr.extract().unwrap());
assert_eq!(2, attr.extract::<u8>().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

So just completing my read of the full diff, this is interesting.

What I see here is that the introduction of these implementations has changed type inference here. It's a potentially breaking change.

Is it a problem? I don't think so; user code could already define other types implementing FromPyObject and PartialEq<u8> and thus have the same inference failure.

And I believe that this new PartialEq implementation here still holds potential convenience and usability improvements.

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 see how this can be a breaking change. However, I don't think it's a major issue.

Ultimately, I'll leave the final decision to the maintainers haha 😃 .

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think we move ahead with this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't consider inference failures like this as breaking changes, so I'm also fine with this.

});
}
};
Expand Down
Loading