Make INSString::as_str take the current AutoreleasePool as parameter

See the code comments and my explanation here: SSheldon/rust-objc#103 (comment)

This also has the nice "side-effect" of fixing the memory leaks that as_str was otherwise exhibiting when using non-ascii strings, see SSheldon/rust-objc-foundation#15.

Author: madsmtm

objc2/src/rc/autorelease.rs

@@ -58,6 +58,23 @@ impl AutoreleasePool {
         Self { context }
     }
 
+    /// This will be removed in a future version.
+    #[cfg_attr(
+        all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
+        inline
+    )]
+    #[doc(hidden)]
+    pub fn __verify_is_inner(&self) {
+        #[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
+        POOLS.with(|c| {
+            assert_eq!(
+                c.borrow().last(),
+                Some(&self.context),
+                "Tried to use lifetime from pool that was not innermost"
+            )
+        });
+    }
+
     /// Returns a shared reference to the given autoreleased pointer object.
     ///
     /// This is the preferred way to make references from autoreleased
@@ -70,20 +87,10 @@ impl AutoreleasePool {
     ///
     /// This is equivalent to `&*ptr`, and shares the unsafety of that, except
     /// the lifetime is bound to the pool instead of being unbounded.
-    #[cfg_attr(
-        all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
-        inline
-    )]
+    #[inline]
     #[allow(clippy::needless_lifetimes)]
     pub unsafe fn ptr_as_ref<'p, T>(&'p self, ptr: *const T) -> &'p T {
-        #[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
-        POOLS.with(|c| {
-            assert_eq!(
-                c.borrow().last(),
-                Some(&self.context),
-                "Tried to create shared reference with a lifetime from a pool that was not the innermost pool"
-            )
-        });
+        self.__verify_is_inner();
         // SAFETY: Checked by the caller
         &*ptr
     }
@@ -100,21 +107,11 @@ impl AutoreleasePool {
     ///
     /// This is equivalent to `&mut *ptr`, and shares the unsafety of that,
     /// except the lifetime is bound to the pool instead of being unbounded.
-    #[cfg_attr(
-        all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
-        inline
-    )]
+    #[inline]
     #[allow(clippy::needless_lifetimes)]
     #[allow(clippy::mut_from_ref)]
     pub unsafe fn ptr_as_mut<'p, T>(&'p self, ptr: *mut T) -> &'p mut T {
-        #[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
-        POOLS.with(|c| {
-            assert_eq!(
-                c.borrow().last(),
-                Some(&self.context),
-                "Tried to create unique reference with a lifetime from a pool that was not the innermost pool")
-            }
-        );
+        self.__verify_is_inner();
         // SAFETY: Checked by the caller
         &mut *ptr
     }

objc2_foundation/examples/basic_usage.rs

@@ -1,3 +1,4 @@
+use objc2::rc::autoreleasepool;
 use objc2_foundation::{
     INSArray, INSCopying, INSDictionary, INSObject, INSString, NSArray, NSDictionary, NSObject,
     NSString,
@@ -25,9 +26,12 @@ fn main() {
 
     // Create an NSString from a str slice
     let string = NSString::from_str("Hello, world!");
-    println!("{}", string.as_str());
-    let string2 = string.copy();
-    println!("{}", string2.as_str());
+    // Use an autoreleasepool to get the `str` contents of the NSString
+    autoreleasepool(|pool| {
+        println!("{}", string.as_str(pool));
+        let string2 = string.copy();
+        println!("{}", string2.as_str(pool));
+    });
 
     // Create a dictionary mapping strings to objects
     let keys = &[&*string];

objc2_foundation/src/array.rs

@@ -412,7 +412,7 @@ mod tests {
 
     use super::{INSArray, INSMutableArray, NSArray, NSMutableArray};
     use crate::{INSObject, INSString, NSObject, NSString};
-    use objc2::rc::{Id, Owned};
+    use objc2::rc::{autoreleasepool, Id, Owned};
 
     fn sample_array(len: usize) -> Id<NSArray<NSObject, Owned>, Owned> {
         let mut vec = Vec::with_capacity(len);
@@ -525,8 +525,10 @@ mod tests {
         let strings = vec![NSString::from_str("hello"), NSString::from_str("hi")];
         let mut strings = NSMutableArray::from_vec(strings);
 
-        strings.sort_by(|s1, s2| s1.as_str().len().cmp(&s2.as_str().len()));
-        assert_eq!(strings[0].as_str(), "hi");
-        assert_eq!(strings[1].as_str(), "hello");
+        autoreleasepool(|pool| {
+            strings.sort_by(|s1, s2| s1.as_str(pool).len().cmp(&s2.as_str(pool).len()));
+            assert_eq!(strings[0].as_str(pool), "hi");
+            assert_eq!(strings[1].as_str(pool), "hello");
+        });
     }
 }

objc2_foundation/src/dictionary.rs

@@ -166,7 +166,7 @@ impl<'a, K: INSObject, V: INSObject> Index<&'a K> for NSDictionary<K, V> {
 #[cfg(test)]
 mod tests {
     use alloc::vec;
-    use objc2::rc::{Id, Shared};
+    use objc2::rc::{autoreleasepool, Id, Shared};
 
     use super::{INSDictionary, NSDictionary};
     use crate::{INSArray, INSObject, INSString, NSObject, NSString};
@@ -200,7 +200,9 @@ mod tests {
         let keys = dict.keys();
 
         assert_eq!(keys.len(), 1);
-        assert_eq!(keys[0].as_str(), "abcd");
+        autoreleasepool(|pool| {
+            assert_eq!(keys[0].as_str(pool), "abcd");
+        });
     }
 
     #[test]
@@ -218,15 +220,19 @@ mod tests {
 
         assert_eq!(keys.len(), 1);
         assert_eq!(objs.len(), 1);
-        assert_eq!(keys[0].as_str(), "abcd");
+        autoreleasepool(|pool| {
+            assert_eq!(keys[0].as_str(pool), "abcd");
+        });
         assert_eq!(objs[0], dict.get(keys[0]).unwrap());
     }
 
     #[test]
     fn test_key_enumerator() {
         let dict = sample_dict("abcd");
         assert_eq!(dict.key_enumerator().count(), 1);
-        assert_eq!(dict.key_enumerator().next().unwrap().as_str(), "abcd");
+        autoreleasepool(|pool| {
+            assert_eq!(dict.key_enumerator().next().unwrap().as_str(pool), "abcd");
+        });
     }
 
     #[test]
@@ -241,7 +247,9 @@ mod tests {
 
         let keys = dict.keys_array();
         assert_eq!(keys.len(), 1);
-        assert_eq!(keys[0].as_str(), "abcd");
+        autoreleasepool(|pool| {
+            assert_eq!(keys[0].as_str(pool), "abcd");
+        });
 
         // let objs = INSDictionary::into_values_array(dict);
         // assert_eq!(objs.len(), 1);

objc2_foundation/src/macros.rs

@@ -39,7 +39,9 @@ macro_rules! object_struct {
         impl ::core::fmt::Debug for $name {
             fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
                 use $crate::{INSObject, INSString};
-                ::core::fmt::Debug::fmt(self.description().as_str(), f)
+                ::objc2::rc::autoreleasepool(|pool| {
+                    ::core::fmt::Debug::fmt(self.description().as_str(pool), f)
+                })
             }
         }
     };

objc2_foundation/src/object.rs

@@ -62,6 +62,7 @@ mod tests {
     use super::{INSObject, NSObject};
     use crate::{INSString, NSString};
     use alloc::format;
+    use objc2::rc::autoreleasepool;
 
     #[test]
     fn test_is_equal() {
@@ -77,15 +78,17 @@ mod tests {
     #[test]
     fn test_hash_code() {
         let obj = NSObject::new();
-        assert!(obj.hash_code() == obj.hash_code());
+        assert_eq!(obj.hash_code(), obj.hash_code());
     }
 
     #[test]
     fn test_description() {
         let obj = NSObject::new();
         let description = obj.description();
         let expected = format!("<NSObject: {:p}>", &*obj);
-        assert_eq!(description.as_str(), &*expected);
+        autoreleasepool(|pool| {
+            assert_eq!(description.as_str(pool), &*expected);
+        });
 
         let expected = format!("\"<NSObject: {:p}>\"", &*obj);
         assert_eq!(format!("{:?}", obj), expected);

objc2_foundation/src/string.rs

@@ -6,6 +6,7 @@ use core::str;
 use std::os::raw::c_char;
 
 use objc2::msg_send;
+use objc2::rc::{autoreleasepool, AutoreleasePool};
 use objc2::rc::{Id, Owned, Shared};
 
 use super::INSObject;
@@ -57,16 +58,58 @@ pub trait INSString: INSObject {
         self.len() == 0
     }
 
-    fn as_str(&self) -> &str {
-        let bytes = unsafe {
-            let bytes: *const c_char = msg_send![self, UTF8String];
-            bytes as *const u8
-        };
+    /// TODO
+    ///
+    /// ```compile_fail
+    /// # use objc2::rc::autoreleasepool;
+    /// # use objc2_foundation::{INSObject, INSString, NSString};
+    /// autoreleasepool(|pool| {
+    ///     let ns_string = NSString::new();
+    ///     let s = ns_string.as_str(pool);
+    ///     drop(ns_string);
+    ///     println!("{}", s);
+    /// });
+    /// ```
+    ///
+    /// ```compile_fail
+    /// # use objc2::rc::autoreleasepool;
+    /// # use objc2_foundation::{INSObject, INSString, NSString};
+    /// let ns_string = NSString::new();
+    /// let s = autoreleasepool(|pool| ns_string.as_str(pool));
+    /// ```
+    fn as_str<'r, 's: 'r, 'p: 'r>(&'s self, pool: &'p AutoreleasePool) -> &'r str {
+        // This is necessary until `auto` types stabilizes.
+        pool.__verify_is_inner();
+
+        // The documentation on `UTF8String` is a bit sparse, but with
+        // educated guesses and testing I've determined that NSString stores
+        // a pointer to the string data, sometimes with an UTF-8 encoding,
+        // (usual for ascii data), sometimes in other encodings (UTF-16?).
+        //
+        // `UTF8String` then checks the internal encoding:
+        // - If the data is UTF-8 encoded, it returns the internal pointer.
+        // - If the data is in another encoding, it creates a new allocation,
+        //   writes the UTF-8 representation of the string into it,
+        //   autoreleases the allocation and returns a pointer to it.
+        //
+        // So the lifetime of the returned pointer is either the same as the
+        // NSString OR the lifetime of the innermost @autoreleasepool.
+        let bytes: *const c_char = unsafe { msg_send![self, UTF8String] };
+        let bytes = bytes as *const u8;
         let len = self.len();
-        unsafe {
-            let bytes = slice::from_raw_parts(bytes, len);
-            str::from_utf8(bytes).unwrap()
-        }
+
+        // SAFETY:
+        // The held AutoreleasePool is the innermost, and the reference is
+        // constrained both by the pool and the NSString.
+        //
+        // `len` is the length of the string in the UTF-8 encoding.
+        //
+        // `bytes` is a null-terminated C string (with length = len + 1), so
+        // it is never a NULL pointer.
+        let bytes: &'r [u8] = unsafe { slice::from_raw_parts(bytes, len) };
+
+        // TODO: Always UTF-8, so should we use `from_utf8_unchecked`?
+        str::from_utf8(bytes).unwrap()
     }
 
     fn from_str(string: &str) -> Id<Self, Self::Ownership> {
@@ -95,40 +138,98 @@ impl INSCopying for NSString {
 
 impl fmt::Display for NSString {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        fmt::Display::fmt(self.as_str(), f)
+        autoreleasepool(|pool| fmt::Display::fmt(self.as_str(pool), f))
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use super::{INSCopying, INSString, NSString};
+    use super::*;
 
-    #[cfg(not(target_vendor = "apple"))]
+    #[cfg(gnustep)]
     #[test]
     fn ensure_linkage() {
         unsafe { objc2::__gnustep_hack::get_class_to_force_linkage() };
     }
 
+    #[test]
+    fn test_empty() {
+        let s1 = NSString::from_str("");
+        let s2 = NSString::new();
+
+        assert_eq!(s1.len(), 0);
+        assert_eq!(s2.len(), 0);
+
+        assert_eq!(s1, s2);
+
+        autoreleasepool(|pool| {
+            assert_eq!(s1.as_str(pool), "");
+            assert_eq!(s2.as_str(pool), "");
+        });
+    }
+
     #[test]
     fn test_utf8() {
         let expected = "ประเทศไทย中华Việt Nam";
         let s = NSString::from_str(expected);
-        assert!(s.len() == expected.len());
-        assert!(s.as_str() == expected);
+        assert_eq!(s.len(), expected.len());
+        autoreleasepool(|pool| {
+            assert_eq!(s.as_str(pool), expected);
+        });
     }
 
     #[test]
     fn test_interior_nul() {
         let expected = "Hello\0World";
         let s = NSString::from_str(expected);
-        assert!(s.len() == expected.len());
-        assert!(s.as_str() == expected);
+        assert_eq!(s.len(), expected.len());
+        autoreleasepool(|pool| {
+            assert_eq!(s.as_str(pool), expected);
+        });
     }
 
     #[test]
     fn test_copy() {
         let s = NSString::from_str("Hello!");
         let copied = s.copy();
-        assert!(copied.as_str() == s.as_str());
+        autoreleasepool(|pool| {
+            assert_eq!(copied.as_str(pool), s.as_str(pool));
+        });
+    }
+
+    #[test]
+    fn test_copy_nsstring_is_same() {
+        let string1 = NSString::from_str("Hello, world!");
+        let string2 = string1.copy();
+
+        let s1: *const NSString = &*string1;
+        let s2: *const NSString = &*string2;
+
+        assert_eq!(s1, s2, "Cloned NSString didn't have the same address");
+    }
+
+    #[test]
+    /// Apparently NSString does this for some reason?
+    fn test_strips_first_leading_zero_width_no_break_space() {
+        let ns_string = NSString::from_str("\u{feff}");
+        let expected = "";
+        autoreleasepool(|pool| {
+            assert_eq!(ns_string.as_str(pool), expected);
+        });
+        assert_eq!(ns_string.len(), 0);
+
+        let s = "\u{feff}\u{feff}a\u{feff}";
+
+        // Huh, this difference might be a GNUStep bug?
+        #[cfg(not(gnustep))]
+        let expected = "\u{feff}a\u{feff}";
+        #[cfg(gnustep)]
+        let expected = "a\u{feff}";
+
+        let ns_string = NSString::from_str(s);
+        autoreleasepool(|pool| {
+            assert_eq!(ns_string.as_str(pool), expected);
+        });
+        assert_eq!(ns_string.len(), expected.len());
     }
 }