Add bounds in macro to fix NSValue<f32> being Eq

Author: madsmtm

objc2-foundation/src/array.rs

@@ -359,7 +359,7 @@ mod tests {
     use objc2::rc::{autoreleasepool, Id, Owned, Shared};
 
     use super::{INSArray, INSMutableArray, NSArray, NSMutableArray};
-    use crate::{INSObject, INSString, NSObject, NSString};
+    use crate::{INSObject, INSString, INSValue, NSObject, NSString, NSValue};
 
     fn sample_array(len: usize) -> Id<NSArray<NSObject, Owned>, Owned> {
         let mut vec = Vec::with_capacity(len);
@@ -369,6 +369,14 @@ mod tests {
         NSArray::from_vec(vec)
     }
 
+    fn sample_number_array(len: u8) -> Id<NSArray<NSValue<u8>, Shared>, Shared> {
+        let mut vec = Vec::with_capacity(len as usize);
+        for i in 0..len {
+            vec.push(NSValue::new(i));
+        }
+        NSArray::from_vec(vec)
+    }
+
     fn retain_count<T: INSObject>(obj: &T) -> usize {
         unsafe { msg_send![obj, retainCount] }
     }
@@ -382,6 +390,21 @@ mod tests {
         assert_eq!(array.len(), 4);
     }
 
+    #[test]
+    fn test_equality() {
+        let array1 = sample_array(3);
+        let array2 = sample_array(3);
+        assert_ne!(array1, array2);
+
+        let array1 = sample_number_array(3);
+        let array2 = sample_number_array(3);
+        assert_eq!(array1, array2);
+
+        let array1 = sample_number_array(3);
+        let array2 = sample_number_array(4);
+        assert_ne!(array1, array2);
+    }
+
     #[test]
     fn test_get() {
         let array = sample_array(4);

objc2-foundation/src/macros.rs

@@ -39,22 +39,45 @@ macro_rules! object {
             }
         }
 
-        impl<$($t $(: $b)?),*> ::core::cmp::PartialEq for $name<$($t),*> {
+        // Objective-C equality has approximately the same semantics as Rust
+        // equality (although less aptly specified).
+        //
+        // At the very least, equality is _expected_ to be symmetric and
+        // transitive, and that's about the best we can do.
+        //
+        // `T: PartialEq` bound added because e.g. `NSArray` does deep
+        // (instead of shallow) equality comparisons.
+        //
+        // See also https://nshipster.com/equality/
+        impl<$($t: ::core::cmp::PartialEq $(+ $b)?),*> ::core::cmp::PartialEq for $name<$($t),*> {
+            #[inline]
             fn eq(&self, other: &Self) -> bool {
                 use $crate::INSObject;
                 self.is_equal(other)
             }
         }
 
-        impl<$($t $(: $b)?),*> ::core::cmp::Eq for $name<$($t),*> {}
+        // Most types' equality is reflexive.
+        //
+        // `T: Eq` bound added to prevent e.g. `NSValue<f32>` from being `Eq`
+        // (even though `[NAN isEqual: NAN]` is true in Objective-C).
+        impl<$($t: ::core::cmp::Eq $(+ $b)?),*> ::core::cmp::Eq for $name<$($t),*> {}
 
-        impl<$($t $(: $b)?),*> ::core::hash::Hash for $name<$($t),*> {
+        // Hashing in Objective-C has the exact same requirement as in Rust:
+        //
+        // > If two objects are equal (as determined by the isEqual: method),
+        // > they must have the same hash value.
+        //
+        // See https://developer.apple.com/documentation/objectivec/1418956-nsobject/1418859-hash
+        impl<$($t: ::core::hash::Hash $(+ $b)?),*> ::core::hash::Hash for $name<$($t),*> {
+            #[inline]
             fn hash<H: ::core::hash::Hasher>(&self, state: &mut H) {
                 use $crate::INSObject;
                 self.hash_code().hash(state);
             }
         }
 
+        // TODO: Consider T: Debug bound
         impl<$($t $(: $b)?),*> ::core::fmt::Debug for $name<$($t),*> {
             fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
                 use $crate::{INSObject, INSString};

objc2-foundation/src/object.rs

@@ -51,13 +51,11 @@ mod tests {
     use objc2::rc::autoreleasepool;
 
     #[test]
-    fn test_is_equal() {
+    fn test_equality() {
         let obj1 = NSObject::new();
-        assert!(obj1.is_equal(&*obj1));
-        assert_eq!(obj1, obj1); // Using forwarding impl on Id
+        assert_eq!(obj1, obj1);
 
         let obj2 = NSObject::new();
-        assert!(!obj1.is_equal(&*obj2));
         assert_ne!(obj1, obj2);
     }
 

objc2-foundation/src/string.rs

@@ -129,6 +129,17 @@ mod tests {
         unsafe { objc2::__gnustep_hack::get_class_to_force_linkage() };
     }
 
+    #[test]
+    fn test_equality() {
+        let s1 = NSString::from_str("abc");
+        let s2 = NSString::from_str("abc");
+        assert_eq!(s1, s1);
+        assert_eq!(s1, s2);
+
+        let s3 = NSString::from_str("def");
+        assert_ne!(s1, s3);
+    }
+
     #[test]
     fn test_empty() {
         let s1 = NSString::from_str("");

objc2-foundation/src/value.rs

@@ -84,9 +84,18 @@ unsafe impl<T: 'static> INSCopying for NSValue<T> {
     type Output = NSValue<T>;
 }
 
+/// ```compile_fail
+/// use objc2_foundation::NSValue;
+/// fn needs_eq<T: Eq>() {}
+/// needs_eq::<NSValue<f32>>();
+/// ```
+#[cfg(doctest)]
+pub struct NSValueFloatNotEq;
+
 #[cfg(test)]
 mod tests {
     use crate::{INSValue, NSRange, NSValue};
+    use objc2::rc::{Id, Shared};
     use objc2::Encode;
 
     #[test]
@@ -96,6 +105,27 @@ mod tests {
         assert!(&u32::ENCODING == val.encoding().unwrap());
     }
 
+    #[test]
+    fn test_equality() {
+        let val1 = NSValue::new(123u32);
+        let val2 = NSValue::new(123u32);
+        assert_eq!(val1, val1);
+        assert_eq!(val1, val2);
+
+        let val3 = NSValue::new(456u32);
+        assert_ne!(val1, val3);
+    }
+
+    #[test]
+    fn test_equality_across_types() {
+        let val1 = NSValue::new(123);
+        let val2: Id<NSValue<u32>, Shared> = NSValue::new(123);
+        let val2: Id<NSValue<u8>, Shared> = unsafe { core::mem::transmute(val2) };
+
+        // Test that `objCType` is checked when comparing equality
+        assert_ne!(val1, val2);
+    }
+
     #[test]
     fn test_value_nsrange() {
         let val = NSValue::new(NSRange::from(1..2));

objc2/CHANGELOG.md

@@ -8,6 +8,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ### Added
 * Export `objc-sys` as `ffi` module.
+* Added common trait impls on `rc::Owned` and `rc::Shared` (useful in generic
+  contexts).
 
 ### Changed
 * Deprecated `runtime::BOOL`, `runtime::YES` and `runtime::NO`. Use the

objc2/src/rc/ownership.rs

@@ -1,9 +1,11 @@
 /// A type used to mark that a struct owns the object(s) it contains,
 /// so it has the sole references to them.
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 pub enum Owned {}
 
 /// A type used to mark that the object(s) a struct contains are shared,
 /// so there may be other references to them.
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
 pub enum Shared {}
 
 mod private {