From 518a05cac01fc8ce11d5beff42042d2c85fcd49e Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sat, 30 Oct 2021 21:07:52 +0200 Subject: [PATCH] Make INSString::as_str take the current AutoreleasePool as parameter See the code comments and my explanation here: https://github.com/SSheldon/rust-objc/pull/103#issuecomment-894464665 This also has the nice "side-effect" of fixing the memory leaks that as_str was otherwise exhibiting when using non-ascii strings, see https://github.com/SSheldon/rust-objc-foundation/issues/15. --- objc2/src/rc/autorelease.rs | 45 ++++---- objc2_foundation/examples/basic_usage.rs | 10 +- objc2_foundation/src/array.rs | 10 +- objc2_foundation/src/dictionary.rs | 18 ++- objc2_foundation/src/macros.rs | 4 +- objc2_foundation/src/object.rs | 7 +- objc2_foundation/src/string.rs | 135 ++++++++++++++++++++--- 7 files changed, 173 insertions(+), 56 deletions(-) diff --git a/objc2/src/rc/autorelease.rs b/objc2/src/rc/autorelease.rs index b23bba35b..66153591d 100644 --- a/objc2/src/rc/autorelease.rs +++ b/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 } diff --git a/objc2_foundation/examples/basic_usage.rs b/objc2_foundation/examples/basic_usage.rs index 2d4510772..18eb7bda6 100644 --- a/objc2_foundation/examples/basic_usage.rs +++ b/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]; diff --git a/objc2_foundation/src/array.rs b/objc2_foundation/src/array.rs index 33f9e0c4d..5042c2c3f 100644 --- a/objc2_foundation/src/array.rs +++ b/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, 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"); + }); } } diff --git a/objc2_foundation/src/dictionary.rs b/objc2_foundation/src/dictionary.rs index df510b474..5140b2d57 100644 --- a/objc2_foundation/src/dictionary.rs +++ b/objc2_foundation/src/dictionary.rs @@ -166,7 +166,7 @@ impl<'a, K: INSObject, V: INSObject> Index<&'a K> for NSDictionary { #[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,7 +220,9 @@ 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()); } @@ -226,7 +230,9 @@ mod tests { 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); diff --git a/objc2_foundation/src/macros.rs b/objc2_foundation/src/macros.rs index b9256da8c..6468cad00 100644 --- a/objc2_foundation/src/macros.rs +++ b/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) + }) } } }; diff --git a/objc2_foundation/src/object.rs b/objc2_foundation/src/object.rs index ead384cd3..e78c74cbe 100644 --- a/objc2_foundation/src/object.rs +++ b/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,7 +78,7 @@ 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] @@ -85,7 +86,9 @@ mod tests { let obj = NSObject::new(); let description = obj.description(); let expected = format!("", &*obj); - assert_eq!(description.as_str(), &*expected); + autoreleasepool(|pool| { + assert_eq!(description.as_str(pool), &*expected); + }); let expected = format!("\"\"", &*obj); assert_eq!(format!("{:?}", obj), expected); diff --git a/objc2_foundation/src/string.rs b/objc2_foundation/src/string.rs index 6fe89d788..b7ef1d23d 100644 --- a/objc2_foundation/src/string.rs +++ b/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 { @@ -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()); } }