Merge pull request #92 from madsmtm/refactor-foundation-macros

Refactor `objc2-foundation` macros

Author: madsmtm

objc2-foundation/CHANGELOG.md

@@ -10,15 +10,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 * **BREAKING**: Added associated `Ownership` type to `NSCopying`.
 * **BREAKING**: Added associated `Ownership` type to `INSData`.
 * **BREAKING**: Added associated `Ownership` type to `INSArray`.
+* Added common trait impls (`PartialEq`, `Eq`, `Hash` and `Debug`) to
+  `NSValue`, `NSDictionary`, `NSArray` and `NSMutableArray`.
 
 ### Changed
 * **BREAKING**: Made some creation methods a bit less generic (e.g.
   `INSDictionary::from_keys_and_objects` now always returns `Id<_, Shared>`).
+* Relax bounds on generic `INSObject` impls.
 
 ### Removed
 * **BREAKING**: Removed associated `Ownership` type from `INSObject`; instead,
   it is present on the types that actually need it (for example `NSCopying`).
 
+### Fixed
+* Soundness issue with `NSValue`, `NSDictionary`, `NSArray` and
+  `NSMutableArray` not being `#[repr(C)]`.
+
 ## 0.2.0-alpha.2 - 2021-11-22
 
 ### Added

objc2-foundation/src/array.rs

@@ -5,9 +5,9 @@ use core::marker::PhantomData;
 use core::ops::{Index, Range};
 use core::ptr::NonNull;
 
+use objc2::msg_send;
 use objc2::rc::{Id, Owned, Ownership, Shared, SliceId};
-use objc2::runtime::{Class, Object};
-use objc2::{class, msg_send};
+use objc2::runtime::Object;
 
 use super::{
     INSCopying, INSFastEnumeration, INSMutableCopying, INSObject, NSComparisonResult, NSEnumerator,
@@ -158,27 +158,21 @@ pub unsafe trait INSArray: INSObject {
     }
 }
 
-/// TODO
-///
-/// You can have a `Id<NSArray<T, Owned>, Owned>`, which allows mutable access
-/// to the elements (without modifying the array itself), and
-/// `Id<NSArray<T, Shared>, Shared>` which allows sharing the array.
-///
-/// `Id<NSArray<T, Owned>, Shared>` is possible, but pretty useless.
-/// TODO: Can we make it impossible? Should we?
-///
-/// What about `Id<NSArray<T, Shared>, Owned>`?
-pub struct NSArray<T, O: Ownership> {
-    item: PhantomData<Id<T, O>>,
-}
-
-object_impl!(unsafe NSArray<T, O: Ownership>);
-
-unsafe impl<T: INSObject, O: Ownership> INSObject for NSArray<T, O> {
-    fn class() -> &'static Class {
-        class!(NSArray)
+object!(
+    /// TODO
+    ///
+    /// You can have a `Id<NSArray<T, Owned>, Owned>`, which allows mutable access
+    /// to the elements (without modifying the array itself), and
+    /// `Id<NSArray<T, Shared>, Shared>` which allows sharing the array.
+    ///
+    /// `Id<NSArray<T, Owned>, Shared>` is possible, but pretty useless.
+    /// TODO: Can we make it impossible? Should we?
+    ///
+    /// What about `Id<NSArray<T, Shared>, Owned>`?
+    unsafe pub struct NSArray<T, O: Ownership> {
+        item: PhantomData<Id<T, O>>,
     }
-}
+);
 
 unsafe impl<T: INSObject, O: Ownership> INSArray for NSArray<T, O> {
     /// The `NSArray` itself (length and number of items) is always immutable,
@@ -319,17 +313,11 @@ pub unsafe trait INSMutableArray: INSArray {
     }
 }
 
-pub struct NSMutableArray<T, O: Ownership> {
-    item: PhantomData<Id<T, O>>,
-}
-
-object_impl!(unsafe NSMutableArray<T, O: Ownership>);
-
-unsafe impl<T: INSObject, O: Ownership> INSObject for NSMutableArray<T, O> {
-    fn class() -> &'static Class {
-        class!(NSMutableArray)
+object!(
+    unsafe pub struct NSMutableArray<T, O: Ownership> {
+        item: PhantomData<Id<T, O>>,
     }
-}
+);
 
 unsafe impl<T: INSObject, O: Ownership> INSArray for NSMutableArray<T, O> {
     type Ownership = Owned;
@@ -364,14 +352,15 @@ impl<T: INSObject, O: Ownership> Index<usize> for NSMutableArray<T, O> {
 
 #[cfg(test)]
 mod tests {
+    use alloc::format;
     use alloc::vec;
     use alloc::vec::Vec;
 
     use objc2::msg_send;
     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);
@@ -381,6 +370,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] }
     }
@@ -394,6 +391,33 @@ 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]
+    #[ignore = "Output is different depending on OS version and runtime"]
+    fn test_debug() {
+        let obj = sample_number_array(3);
+        let expected = r#"(
+    "<00>",
+    "<01>",
+    "<02>"
+)"#;
+        assert_eq!(format!("{:?}", obj), format!("{:?}", expected));
+    }
+
     #[test]
     fn test_get() {
         let array = sample_array(4);

objc2-foundation/src/data.rs

@@ -92,7 +92,7 @@ pub unsafe trait INSData: INSObject {
     }
 }
 
-object_struct!(unsafe NSData);
+object!(unsafe pub struct NSData);
 
 unsafe impl INSData for NSData {
     type Ownership = Shared;
@@ -155,7 +155,7 @@ pub unsafe trait INSMutableData: INSData {
     }
 }
 
-object_struct!(unsafe NSMutableData);
+object!(unsafe pub struct NSMutableData);
 
 unsafe impl INSData for NSMutableData {
     type Ownership = Owned;

objc2-foundation/src/dictionary.rs

@@ -4,9 +4,8 @@ use core::marker::PhantomData;
 use core::ops::Index;
 use core::ptr::{self, NonNull};
 
+use objc2::msg_send;
 use objc2::rc::{Id, Owned, Ownership, Shared, SliceId};
-use objc2::runtime::Class;
-use objc2::{class, msg_send};
 
 use super::{INSCopying, INSFastEnumeration, INSObject, NSArray, NSEnumerator};
 
@@ -139,23 +138,17 @@ pub unsafe trait INSDictionary: INSObject {
     }
 }
 
-pub struct NSDictionary<K, V> {
-    key: PhantomData<Id<K, Shared>>,
-    obj: PhantomData<Id<V, Owned>>,
-}
-
-object_impl!(unsafe NSDictionary<K, V>);
+object!(
+    unsafe pub struct NSDictionary<K, V> {
+        key: PhantomData<Id<K, Shared>>,
+        obj: PhantomData<Id<V, Owned>>,
+    }
+);
 
 impl<K: INSObject, V: INSObject> NSDictionary<K, V> {
     unsafe_def_fn!(pub fn new -> Shared);
 }
 
-unsafe impl<K: INSObject, V: INSObject> INSObject for NSDictionary<K, V> {
-    fn class() -> &'static Class {
-        class!(NSDictionary)
-    }
-}
-
 unsafe impl<K: INSObject, V: INSObject> INSDictionary for NSDictionary<K, V> {
     type Key = K;
     type Value = V;

objc2-foundation/src/macros.rs

@@ -6,43 +6,79 @@
 /// and it's instances must have the raw encoding `Encoding::Object` (an
 /// example: `NSAutoreleasePool` does not have this). Finally the ownership
 /// must be correct for this class.
-macro_rules! object_struct {
-    (unsafe $name:ident) => {
+macro_rules! object {
+    (
+        $(#[$m:meta])*
+        unsafe $v:vis struct $name:ident
+    ) => {
+        object!($(#[$m])* unsafe $v struct $name<> {});
+    };
+    (
+        $(#[$m:meta])*
+        unsafe $v:vis struct $name:ident<$($t:ident $(: $b:ident)?),*> {
+            $($p:ident: $pty:ty,)*
+        }
+    ) => {
         // TODO: `extern type`
+        $(#[$m])*
         #[repr(C)]
-        pub struct $name {
+        $v struct $name<$($t $(: $b)?),*> {
             _private: [u8; 0],
+            $($p: $pty),*
         }
 
-        unsafe impl ::objc2::Message for $name {}
+        unsafe impl<$($t $(: $b)?),*> ::objc2::Message for $name<$($t),*> { }
 
-        unsafe impl ::objc2::RefEncode for $name {
+        unsafe impl<$($t $(: $b)?),*> ::objc2::RefEncode for $name<$($t),*> {
             const ENCODING_REF: ::objc2::Encoding<'static> = ::objc2::Encoding::Object;
         }
 
-        unsafe impl $crate::INSObject for $name {
+        unsafe impl<$($t $(: $b)?),*> $crate::INSObject for $name<$($t),*> {
             fn class() -> &'static ::objc2::runtime::Class {
                 ::objc2::class!($name)
             }
         }
 
-        impl ::core::cmp::PartialEq for $name {
+        // 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 ::core::cmp::Eq for $name {}
+        // 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 ::core::hash::Hash for $name {
+        // 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);
             }
         }
 
-        impl ::core::fmt::Debug for $name {
+        // 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::rc::autoreleasepool(|pool| {
@@ -53,27 +89,6 @@ macro_rules! object_struct {
     };
 }
 
-/// TODO
-///
-/// # Safety
-///
-/// The given type must be valid as an Objective-C object. TODO: More.
-macro_rules! object_impl {
-    (unsafe $name:ident) => (
-        object_impl!(unsafe $name,);
-    );
-    (unsafe $name:ident<$($t:ident$(: $b:ident)?),+>) => (
-        object_impl!(unsafe $name, $($t$(: $b)?),+);
-    );
-    (unsafe $name:ident, $($t:ident$(: $b:ident)?),*) => (
-        unsafe impl<$($t$(:($b))?),*> ::objc2::Message for $name<$($t),*> { }
-
-        unsafe impl<$($t$(: $b)?),*> ::objc2::RefEncode for $name<$($t),*> {
-            const ENCODING_REF: ::objc2::Encoding<'static> = ::objc2::Encoding::Object;
-        }
-    );
-}
-
 macro_rules! unsafe_def_fn {
     ($v:vis fn new -> $o:ty) => {
         $v fn new() -> Id<Self, $o> {

objc2-foundation/src/object.rs

@@ -37,7 +37,7 @@ pub unsafe trait INSObject: Sized + Message {
     }
 }
 
-object_struct!(unsafe NSObject);
+object!(unsafe pub struct NSObject);
 
 impl NSObject {
     unsafe_def_fn!(pub fn new -> Owned);
@@ -46,18 +46,15 @@ impl NSObject {
 #[cfg(test)]
 mod tests {
     use super::{INSObject, NSObject};
-    use crate::{INSString, NSString};
+    use crate::NSString;
     use alloc::format;
-    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);
     }
 
@@ -68,16 +65,10 @@ mod tests {
     }
 
     #[test]
-    fn test_description() {
+    fn test_debug() {
         let obj = NSObject::new();
-        let description = obj.description();
         let expected = format!("<NSObject: {:p}>", &*obj);
-        autoreleasepool(|pool| {
-            assert_eq!(description.as_str(pool), &*expected);
-        });
-
-        let expected = format!("\"<NSObject: {:p}>\"", &*obj);
-        assert_eq!(format!("{:?}", obj), expected);
+        assert_eq!(format!("{:?}", obj), format!("{:?}", expected));
     }
 
     #[test]