Remove HasStableHash requirement from NSSet and NSDictionary

This was added in an abundance of caution in #505 to fix #337, but
prevents real-world usage of these types, and isn't actually needed for
soundness (the documentation mentions the collection being "corrupt" if
the hash is changed, but that's not the same as it being UB).

crates/objc2/src/topics/about_generated/CHANGELOG.md

@@ -22,6 +22,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ### Changed
 * `objc2-foundation`: Allow using `MainThreadBound` without the `NSThread`
   feature flag.
+* `objc2-foundation`: Removed `HasStableHash` requirement on `NSDictionary` and
+  `NSSet` creation methods. This was added in an abundance of caution, but
+  prevents real-world usage of these types, and isn't actually needed for
+  soundness (the documentation mentions the collection being "corrupt" if the
+  hash is changed, but that's not the same as it being unsound).
 
 ### Deprecated
 * `objc2-foundation`: Moved `MainThreadMarker` to `objc2`.

crates/test-fuzz/fuzz_targets/collection_interior_mut.rs

@@ -1,7 +1,10 @@
 //! Fuzz hashing collection operations with interior mutability.
 //!
 //! This is explicitly not done with any form of oracle, since while this is
-//! not language-level undefined behaviour, the behaviour is not specified.
+//! not language-level undefined behaviour, the behaviour is not specified,
+//! and will "corrupt the collection", and may behave differently on different
+//! versions of Foundation:
+//! <https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69>
 #![cfg_attr(not(feature = "afl"), no_main)]
 use std::cell::Cell;
 use std::hint::black_box;
@@ -48,8 +51,7 @@ declare_class!(
 
     unsafe impl ClassType for Key {
         type Super = NSObject;
-        // Intentionally `Immutable` to see what breaks if we allow mutation.
-        type Mutability = mutability::Immutable;
+        type Mutability = mutability::InteriorMutable;
         const NAME: &'static str = "Key";
     }
 

framework-crates/objc2-foundation/src/dictionary.rs

@@ -10,7 +10,7 @@ use core::ptr::{self, NonNull};
 
 #[cfg(feature = "NSObject")]
 use objc2::mutability::IsRetainable;
-use objc2::mutability::{CounterpartOrSelf, HasStableHash, IsIdCloneable, IsMutable};
+use objc2::mutability::{CounterpartOrSelf, IsIdCloneable, IsMutable};
 use objc2::rc::Retained;
 #[cfg(feature = "NSObject")]
 use objc2::runtime::ProtocolObject;
@@ -38,7 +38,11 @@ where
 }
 
 #[cfg(feature = "NSObject")]
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSDictionary<K, V> {
+    // The dictionary copies its keys, which is why we require `NSCopying` and
+    // use `CounterpartOrSelf` on all input data - we want to ensure that the
+    // type-system knows that it's not actually `NSMutableString` that is
+    // being stored, but instead `NSString`.
     pub fn from_vec<Q>(keys: &[&Q], mut objects: Vec<Retained<V>>) -> Retained<Self>
     where
         Q: Message + NSCopying + CounterpartOrSelf<Immutable = K>,
@@ -56,20 +60,15 @@ impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
         // SAFETY:
         // - The objects are valid, similar reasoning as `NSArray::from_vec`.
         //
-        // - The key must not be mutated, as that may cause the hash value to
-        //   change, which is unsound as stated in:
-        //   https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69
+        // - The length is lower than or equal to the length of the two arrays.
         //
-        //   The dictionary always copies its keys, which is why we require
-        //   `NSCopying` and use `CounterpartOrSelf` on all input data - we
-        //   want to ensure that it is very clear that it's not actually
-        //   `NSMutableString` that is being stored, but `NSString`.
+        // - While recommended against in the below link, the key _can_ be
+        //   mutated, it'll "just" corrupt the collection's invariants (but
+        //   won't cause undefined behaviour).
         //
-        //   But that is not by itself enough to verify that the key does not
-        //   still contain interior mutable objects (e.g. if the copy was only
-        //   a shallow copy), which is why we also require `HasStableHash`.
+        //   This is tested via. fuzzing in `collection_interior_mut.rs`.
         //
-        // - The length is lower than or equal to the length of the two arrays.
+        //   <https://developer.apple.com/library/archive/documentation/General/Conceptual/CocoaEncyclopedia/ObjectMutability/ObjectMutability.html#//apple_ref/doc/uid/TP40010810-CH5-SW69>
         unsafe { Self::initWithObjects_forKeys_count(Self::alloc(), objects, keys, count) }
     }
 
@@ -103,7 +102,7 @@ impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSDictionary<K, V> {
 }
 
 #[cfg(feature = "NSObject")]
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSMutableDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSMutableDictionary<K, V> {
     pub fn from_vec<Q>(keys: &[&Q], mut objects: Vec<Retained<V>>) -> Retained<Self>
     where
         Q: Message + NSCopying + CounterpartOrSelf<Immutable = K>,
@@ -311,7 +310,7 @@ impl<K: Message, V: Message> NSDictionary<K, V> {
     }
 }
 
-impl<K: Message + Eq + Hash + HasStableHash, V: Message> NSMutableDictionary<K, V> {
+impl<K: Message + Eq + Hash, V: Message> NSMutableDictionary<K, V> {
     /// Inserts a key-value pair into the dictionary.
     ///
     /// If the dictionary did not have this key present, None is returned.

framework-crates/objc2-foundation/src/set.rs

@@ -4,7 +4,7 @@ use alloc::vec::Vec;
 use core::fmt;
 use core::hash::Hash;
 
-use objc2::mutability::{HasStableHash, IsIdCloneable, IsRetainable};
+use objc2::mutability::{IsIdCloneable, IsRetainable};
 use objc2::rc::{Retained, RetainedFromIterator};
 use objc2::{extern_methods, ClassType, Message};
 
@@ -56,10 +56,7 @@ impl<T: Message + Eq + Hash> NSSet<T> {
     /// let strs = ["one", "two", "three"].map(NSString::from_str).to_vec();
     /// let set = NSSet::from_vec(strs);
     /// ```
-    pub fn from_vec(mut vec: Vec<Retained<T>>) -> Retained<Self>
-    where
-        T: HasStableHash,
-    {
+    pub fn from_vec(mut vec: Vec<Retained<T>>) -> Retained<Self> {
         let len = vec.len();
         let ptr = util::retained_ptr_cast(vec.as_mut_ptr());
         // SAFETY: Same as `NSArray::from_vec`.
@@ -78,7 +75,7 @@ impl<T: Message + Eq + Hash> NSSet<T> {
     /// ```
     pub fn from_id_slice(slice: &[Retained<T>]) -> Retained<Self>
     where
-        T: HasStableHash + IsIdCloneable,
+        T: IsIdCloneable,
     {
         let len = slice.len();
         let ptr = util::retained_ptr_cast_const(slice.as_ptr());
@@ -88,7 +85,7 @@ impl<T: Message + Eq + Hash> NSSet<T> {
 
     pub fn from_slice(slice: &[&T]) -> Retained<Self>
     where
-        T: HasStableHash + IsRetainable,
+        T: IsRetainable,
     {
         let len = slice.len();
         let ptr = util::ref_ptr_cast_const(slice.as_ptr());
@@ -171,10 +168,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
     /// let strs = ["one", "two", "three"].map(NSString::from_str).to_vec();
     /// let set = NSMutableSet::from_vec(strs);
     /// ```
-    pub fn from_vec(mut vec: Vec<Retained<T>>) -> Retained<Self>
-    where
-        T: HasStableHash,
-    {
+    pub fn from_vec(mut vec: Vec<Retained<T>>) -> Retained<Self> {
         let len = vec.len();
         let ptr = util::retained_ptr_cast(vec.as_mut_ptr());
         // SAFETY: Same as `NSArray::from_vec`.
@@ -193,7 +187,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
     /// ```
     pub fn from_id_slice(slice: &[Retained<T>]) -> Retained<Self>
     where
-        T: HasStableHash + IsIdCloneable,
+        T: IsIdCloneable,
     {
         let len = slice.len();
         let ptr = util::retained_ptr_cast_const(slice.as_ptr());
@@ -203,7 +197,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
 
     pub fn from_slice(slice: &[&T]) -> Retained<Self>
     where
-        T: HasStableHash + IsRetainable,
+        T: IsRetainable,
     {
         let len = slice.len();
         let ptr = util::ref_ptr_cast_const(slice.as_ptr());
@@ -376,7 +370,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
     #[doc(alias = "addObject:")]
     pub fn insert(&mut self, value: &T) -> bool
     where
-        T: HasStableHash + IsRetainable,
+        T: IsRetainable,
     {
         let contains_value = self.contains(value);
         // SAFETY: Because of the `T: IsRetainable` bound, it is safe for the
@@ -400,10 +394,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
     /// assert_eq!(set.len(), 1);
     /// ```
     #[doc(alias = "addObject:")]
-    pub fn insert_id(&mut self, value: Retained<T>) -> bool
-    where
-        T: HasStableHash,
-    {
+    pub fn insert_id(&mut self, value: Retained<T>) -> bool {
         let contains_value = self.contains(&value);
         // SAFETY: We've consumed ownership of the object.
         unsafe { self.addObject(&value) };
@@ -425,10 +416,7 @@ impl<T: Message + Eq + Hash> NSMutableSet<T> {
     /// assert_eq!(set.remove(ns_string!("one")), false);
     /// ```
     #[doc(alias = "removeObject:")]
-    pub fn remove(&mut self, value: &T) -> bool
-    where
-        T: HasStableHash,
-    {
+    pub fn remove(&mut self, value: &T) -> bool {
         let contains_value = self.contains(value);
         unsafe { self.removeObject(value) };
         contains_value
@@ -563,39 +551,37 @@ impl<T: fmt::Debug + Message> fmt::Debug for crate::Foundation::NSCountedSet<T>
     }
 }
 
-impl<T: Message + Eq + Hash + HasStableHash> Extend<Retained<T>> for NSMutableSet<T> {
+impl<T: Message + Eq + Hash> Extend<Retained<T>> for NSMutableSet<T> {
     fn extend<I: IntoIterator<Item = Retained<T>>>(&mut self, iter: I) {
         iter.into_iter().for_each(move |item| {
             self.insert_id(item);
         });
     }
 }
 
-impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable> Extend<&'a T> for NSMutableSet<T> {
+impl<'a, T: Message + Eq + Hash + IsRetainable> Extend<&'a T> for NSMutableSet<T> {
     fn extend<I: IntoIterator<Item = &'a T>>(&mut self, iter: I) {
         iter.into_iter().for_each(move |item| {
             self.insert(item);
         });
     }
 }
 
-impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFromIterator<&'a T>
-    for NSSet<T>
-{
+impl<'a, T: Message + Eq + Hash + IsRetainable + 'a> RetainedFromIterator<&'a T> for NSSet<T> {
     fn id_from_iter<I: IntoIterator<Item = &'a T>>(iter: I) -> Retained<Self> {
         let vec = Vec::from_iter(iter);
         Self::from_slice(&vec)
     }
 }
 
-impl<T: Message + Eq + Hash + HasStableHash> RetainedFromIterator<Retained<T>> for NSSet<T> {
+impl<T: Message + Eq + Hash> RetainedFromIterator<Retained<T>> for NSSet<T> {
     fn id_from_iter<I: IntoIterator<Item = Retained<T>>>(iter: I) -> Retained<Self> {
         let vec = Vec::from_iter(iter);
         Self::from_vec(vec)
     }
 }
 
-impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFromIterator<&'a T>
+impl<'a, T: Message + Eq + Hash + IsRetainable + 'a> RetainedFromIterator<&'a T>
     for NSMutableSet<T>
 {
     fn id_from_iter<I: IntoIterator<Item = &'a T>>(iter: I) -> Retained<Self> {
@@ -604,7 +590,7 @@ impl<'a, T: Message + Eq + Hash + HasStableHash + IsRetainable + 'a> RetainedFro
     }
 }
 
-impl<T: Message + Eq + Hash + HasStableHash> RetainedFromIterator<Retained<T>> for NSMutableSet<T> {
+impl<T: Message + Eq + Hash> RetainedFromIterator<Retained<T>> for NSMutableSet<T> {
     fn id_from_iter<I: IntoIterator<Item = Retained<T>>>(iter: I) -> Retained<Self> {
         let vec = Vec::from_iter(iter);
         Self::from_vec(vec)

framework-crates/objc2-foundation/src/tests/set.rs

@@ -4,7 +4,7 @@
 use alloc::vec::Vec;
 use alloc::{format, vec};
 
-use crate::Foundation::{self, ns_string, NSNumber, NSSet, NSString};
+use crate::Foundation::{self, ns_string, NSNumber, NSObject, NSSet, NSString};
 
 #[test]
 fn test_new() {
@@ -210,3 +210,8 @@ fn invalid_generic() {
     let _ = set.get_any().unwrap().get(0).unwrap();
     // something_interior_mutable.setAbc(...)
 }
+
+#[test]
+fn new_from_nsobject() {
+    let _ = NSSet::from_vec(vec![NSObject::new()]);
+}