Skip to content

Commit 518a05c

Browse files
committed
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.
1 parent cb3cc16 commit 518a05c

File tree

7 files changed

+173
-56
lines changed

7 files changed

+173
-56
lines changed

objc2/src/rc/autorelease.rs

+21-24
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,23 @@ impl AutoreleasePool {
5858
Self { context }
5959
}
6060

61+
/// This will be removed in a future version.
62+
#[cfg_attr(
63+
all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
64+
inline
65+
)]
66+
#[doc(hidden)]
67+
pub fn __verify_is_inner(&self) {
68+
#[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
69+
POOLS.with(|c| {
70+
assert_eq!(
71+
c.borrow().last(),
72+
Some(&self.context),
73+
"Tried to use lifetime from pool that was not innermost"
74+
)
75+
});
76+
}
77+
6178
/// Returns a shared reference to the given autoreleased pointer object.
6279
///
6380
/// This is the preferred way to make references from autoreleased
@@ -70,20 +87,10 @@ impl AutoreleasePool {
7087
///
7188
/// This is equivalent to `&*ptr`, and shares the unsafety of that, except
7289
/// the lifetime is bound to the pool instead of being unbounded.
73-
#[cfg_attr(
74-
all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
75-
inline
76-
)]
90+
#[inline]
7791
#[allow(clippy::needless_lifetimes)]
7892
pub unsafe fn ptr_as_ref<'p, T>(&'p self, ptr: *const T) -> &'p T {
79-
#[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
80-
POOLS.with(|c| {
81-
assert_eq!(
82-
c.borrow().last(),
83-
Some(&self.context),
84-
"Tried to create shared reference with a lifetime from a pool that was not the innermost pool"
85-
)
86-
});
93+
self.__verify_is_inner();
8794
// SAFETY: Checked by the caller
8895
&*ptr
8996
}
@@ -100,21 +107,11 @@ impl AutoreleasePool {
100107
///
101108
/// This is equivalent to `&mut *ptr`, and shares the unsafety of that,
102109
/// except the lifetime is bound to the pool instead of being unbounded.
103-
#[cfg_attr(
104-
all(debug_assertions, not(feature = "unstable_autoreleasesafe")),
105-
inline
106-
)]
110+
#[inline]
107111
#[allow(clippy::needless_lifetimes)]
108112
#[allow(clippy::mut_from_ref)]
109113
pub unsafe fn ptr_as_mut<'p, T>(&'p self, ptr: *mut T) -> &'p mut T {
110-
#[cfg(all(debug_assertions, not(feature = "unstable_autoreleasesafe")))]
111-
POOLS.with(|c| {
112-
assert_eq!(
113-
c.borrow().last(),
114-
Some(&self.context),
115-
"Tried to create unique reference with a lifetime from a pool that was not the innermost pool")
116-
}
117-
);
114+
self.__verify_is_inner();
118115
// SAFETY: Checked by the caller
119116
&mut *ptr
120117
}

objc2_foundation/examples/basic_usage.rs

+7-3
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
use objc2::rc::autoreleasepool;
12
use objc2_foundation::{
23
INSArray, INSCopying, INSDictionary, INSObject, INSString, NSArray, NSDictionary, NSObject,
34
NSString,
@@ -25,9 +26,12 @@ fn main() {
2526

2627
// Create an NSString from a str slice
2728
let string = NSString::from_str("Hello, world!");
28-
println!("{}", string.as_str());
29-
let string2 = string.copy();
30-
println!("{}", string2.as_str());
29+
// Use an autoreleasepool to get the `str` contents of the NSString
30+
autoreleasepool(|pool| {
31+
println!("{}", string.as_str(pool));
32+
let string2 = string.copy();
33+
println!("{}", string2.as_str(pool));
34+
});
3135

3236
// Create a dictionary mapping strings to objects
3337
let keys = &[&*string];

objc2_foundation/src/array.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ mod tests {
412412

413413
use super::{INSArray, INSMutableArray, NSArray, NSMutableArray};
414414
use crate::{INSObject, INSString, NSObject, NSString};
415-
use objc2::rc::{Id, Owned};
415+
use objc2::rc::{autoreleasepool, Id, Owned};
416416

417417
fn sample_array(len: usize) -> Id<NSArray<NSObject, Owned>, Owned> {
418418
let mut vec = Vec::with_capacity(len);
@@ -525,8 +525,10 @@ mod tests {
525525
let strings = vec![NSString::from_str("hello"), NSString::from_str("hi")];
526526
let mut strings = NSMutableArray::from_vec(strings);
527527

528-
strings.sort_by(|s1, s2| s1.as_str().len().cmp(&s2.as_str().len()));
529-
assert_eq!(strings[0].as_str(), "hi");
530-
assert_eq!(strings[1].as_str(), "hello");
528+
autoreleasepool(|pool| {
529+
strings.sort_by(|s1, s2| s1.as_str(pool).len().cmp(&s2.as_str(pool).len()));
530+
assert_eq!(strings[0].as_str(pool), "hi");
531+
assert_eq!(strings[1].as_str(pool), "hello");
532+
});
531533
}
532534
}

objc2_foundation/src/dictionary.rs

+13-5
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ impl<'a, K: INSObject, V: INSObject> Index<&'a K> for NSDictionary<K, V> {
166166
#[cfg(test)]
167167
mod tests {
168168
use alloc::vec;
169-
use objc2::rc::{Id, Shared};
169+
use objc2::rc::{autoreleasepool, Id, Shared};
170170

171171
use super::{INSDictionary, NSDictionary};
172172
use crate::{INSArray, INSObject, INSString, NSObject, NSString};
@@ -200,7 +200,9 @@ mod tests {
200200
let keys = dict.keys();
201201

202202
assert_eq!(keys.len(), 1);
203-
assert_eq!(keys[0].as_str(), "abcd");
203+
autoreleasepool(|pool| {
204+
assert_eq!(keys[0].as_str(pool), "abcd");
205+
});
204206
}
205207

206208
#[test]
@@ -218,15 +220,19 @@ mod tests {
218220

219221
assert_eq!(keys.len(), 1);
220222
assert_eq!(objs.len(), 1);
221-
assert_eq!(keys[0].as_str(), "abcd");
223+
autoreleasepool(|pool| {
224+
assert_eq!(keys[0].as_str(pool), "abcd");
225+
});
222226
assert_eq!(objs[0], dict.get(keys[0]).unwrap());
223227
}
224228

225229
#[test]
226230
fn test_key_enumerator() {
227231
let dict = sample_dict("abcd");
228232
assert_eq!(dict.key_enumerator().count(), 1);
229-
assert_eq!(dict.key_enumerator().next().unwrap().as_str(), "abcd");
233+
autoreleasepool(|pool| {
234+
assert_eq!(dict.key_enumerator().next().unwrap().as_str(pool), "abcd");
235+
});
230236
}
231237

232238
#[test]
@@ -241,7 +247,9 @@ mod tests {
241247

242248
let keys = dict.keys_array();
243249
assert_eq!(keys.len(), 1);
244-
assert_eq!(keys[0].as_str(), "abcd");
250+
autoreleasepool(|pool| {
251+
assert_eq!(keys[0].as_str(pool), "abcd");
252+
});
245253

246254
// let objs = INSDictionary::into_values_array(dict);
247255
// assert_eq!(objs.len(), 1);

objc2_foundation/src/macros.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ macro_rules! object_struct {
3939
impl ::core::fmt::Debug for $name {
4040
fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
4141
use $crate::{INSObject, INSString};
42-
::core::fmt::Debug::fmt(self.description().as_str(), f)
42+
::objc2::rc::autoreleasepool(|pool| {
43+
::core::fmt::Debug::fmt(self.description().as_str(pool), f)
44+
})
4345
}
4446
}
4547
};

objc2_foundation/src/object.rs

+5-2
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ mod tests {
6262
use super::{INSObject, NSObject};
6363
use crate::{INSString, NSString};
6464
use alloc::format;
65+
use objc2::rc::autoreleasepool;
6566

6667
#[test]
6768
fn test_is_equal() {
@@ -77,15 +78,17 @@ mod tests {
7778
#[test]
7879
fn test_hash_code() {
7980
let obj = NSObject::new();
80-
assert!(obj.hash_code() == obj.hash_code());
81+
assert_eq!(obj.hash_code(), obj.hash_code());
8182
}
8283

8384
#[test]
8485
fn test_description() {
8586
let obj = NSObject::new();
8687
let description = obj.description();
8788
let expected = format!("<NSObject: {:p}>", &*obj);
88-
assert_eq!(description.as_str(), &*expected);
89+
autoreleasepool(|pool| {
90+
assert_eq!(description.as_str(pool), &*expected);
91+
});
8992

9093
let expected = format!("\"<NSObject: {:p}>\"", &*obj);
9194
assert_eq!(format!("{:?}", obj), expected);

objc2_foundation/src/string.rs

+118-17
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use core::str;
66
use std::os::raw::c_char;
77

88
use objc2::msg_send;
9+
use objc2::rc::{autoreleasepool, AutoreleasePool};
910
use objc2::rc::{Id, Owned, Shared};
1011

1112
use super::INSObject;
@@ -57,16 +58,58 @@ pub trait INSString: INSObject {
5758
self.len() == 0
5859
}
5960

60-
fn as_str(&self) -> &str {
61-
let bytes = unsafe {
62-
let bytes: *const c_char = msg_send![self, UTF8String];
63-
bytes as *const u8
64-
};
61+
/// TODO
62+
///
63+
/// ```compile_fail
64+
/// # use objc2::rc::autoreleasepool;
65+
/// # use objc2_foundation::{INSObject, INSString, NSString};
66+
/// autoreleasepool(|pool| {
67+
/// let ns_string = NSString::new();
68+
/// let s = ns_string.as_str(pool);
69+
/// drop(ns_string);
70+
/// println!("{}", s);
71+
/// });
72+
/// ```
73+
///
74+
/// ```compile_fail
75+
/// # use objc2::rc::autoreleasepool;
76+
/// # use objc2_foundation::{INSObject, INSString, NSString};
77+
/// let ns_string = NSString::new();
78+
/// let s = autoreleasepool(|pool| ns_string.as_str(pool));
79+
/// ```
80+
fn as_str<'r, 's: 'r, 'p: 'r>(&'s self, pool: &'p AutoreleasePool) -> &'r str {
81+
// This is necessary until `auto` types stabilizes.
82+
pool.__verify_is_inner();
83+
84+
// The documentation on `UTF8String` is a bit sparse, but with
85+
// educated guesses and testing I've determined that NSString stores
86+
// a pointer to the string data, sometimes with an UTF-8 encoding,
87+
// (usual for ascii data), sometimes in other encodings (UTF-16?).
88+
//
89+
// `UTF8String` then checks the internal encoding:
90+
// - If the data is UTF-8 encoded, it returns the internal pointer.
91+
// - If the data is in another encoding, it creates a new allocation,
92+
// writes the UTF-8 representation of the string into it,
93+
// autoreleases the allocation and returns a pointer to it.
94+
//
95+
// So the lifetime of the returned pointer is either the same as the
96+
// NSString OR the lifetime of the innermost @autoreleasepool.
97+
let bytes: *const c_char = unsafe { msg_send![self, UTF8String] };
98+
let bytes = bytes as *const u8;
6599
let len = self.len();
66-
unsafe {
67-
let bytes = slice::from_raw_parts(bytes, len);
68-
str::from_utf8(bytes).unwrap()
69-
}
100+
101+
// SAFETY:
102+
// The held AutoreleasePool is the innermost, and the reference is
103+
// constrained both by the pool and the NSString.
104+
//
105+
// `len` is the length of the string in the UTF-8 encoding.
106+
//
107+
// `bytes` is a null-terminated C string (with length = len + 1), so
108+
// it is never a NULL pointer.
109+
let bytes: &'r [u8] = unsafe { slice::from_raw_parts(bytes, len) };
110+
111+
// TODO: Always UTF-8, so should we use `from_utf8_unchecked`?
112+
str::from_utf8(bytes).unwrap()
70113
}
71114

72115
fn from_str(string: &str) -> Id<Self, Self::Ownership> {
@@ -95,40 +138,98 @@ impl INSCopying for NSString {
95138

96139
impl fmt::Display for NSString {
97140
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
98-
fmt::Display::fmt(self.as_str(), f)
141+
autoreleasepool(|pool| fmt::Display::fmt(self.as_str(pool), f))
99142
}
100143
}
101144

102145
#[cfg(test)]
103146
mod tests {
104-
use super::{INSCopying, INSString, NSString};
147+
use super::*;
105148

106-
#[cfg(not(target_vendor = "apple"))]
149+
#[cfg(gnustep)]
107150
#[test]
108151
fn ensure_linkage() {
109152
unsafe { objc2::__gnustep_hack::get_class_to_force_linkage() };
110153
}
111154

155+
#[test]
156+
fn test_empty() {
157+
let s1 = NSString::from_str("");
158+
let s2 = NSString::new();
159+
160+
assert_eq!(s1.len(), 0);
161+
assert_eq!(s2.len(), 0);
162+
163+
assert_eq!(s1, s2);
164+
165+
autoreleasepool(|pool| {
166+
assert_eq!(s1.as_str(pool), "");
167+
assert_eq!(s2.as_str(pool), "");
168+
});
169+
}
170+
112171
#[test]
113172
fn test_utf8() {
114173
let expected = "ประเทศไทย中华Việt Nam";
115174
let s = NSString::from_str(expected);
116-
assert!(s.len() == expected.len());
117-
assert!(s.as_str() == expected);
175+
assert_eq!(s.len(), expected.len());
176+
autoreleasepool(|pool| {
177+
assert_eq!(s.as_str(pool), expected);
178+
});
118179
}
119180

120181
#[test]
121182
fn test_interior_nul() {
122183
let expected = "Hello\0World";
123184
let s = NSString::from_str(expected);
124-
assert!(s.len() == expected.len());
125-
assert!(s.as_str() == expected);
185+
assert_eq!(s.len(), expected.len());
186+
autoreleasepool(|pool| {
187+
assert_eq!(s.as_str(pool), expected);
188+
});
126189
}
127190

128191
#[test]
129192
fn test_copy() {
130193
let s = NSString::from_str("Hello!");
131194
let copied = s.copy();
132-
assert!(copied.as_str() == s.as_str());
195+
autoreleasepool(|pool| {
196+
assert_eq!(copied.as_str(pool), s.as_str(pool));
197+
});
198+
}
199+
200+
#[test]
201+
fn test_copy_nsstring_is_same() {
202+
let string1 = NSString::from_str("Hello, world!");
203+
let string2 = string1.copy();
204+
205+
let s1: *const NSString = &*string1;
206+
let s2: *const NSString = &*string2;
207+
208+
assert_eq!(s1, s2, "Cloned NSString didn't have the same address");
209+
}
210+
211+
#[test]
212+
/// Apparently NSString does this for some reason?
213+
fn test_strips_first_leading_zero_width_no_break_space() {
214+
let ns_string = NSString::from_str("\u{feff}");
215+
let expected = "";
216+
autoreleasepool(|pool| {
217+
assert_eq!(ns_string.as_str(pool), expected);
218+
});
219+
assert_eq!(ns_string.len(), 0);
220+
221+
let s = "\u{feff}\u{feff}a\u{feff}";
222+
223+
// Huh, this difference might be a GNUStep bug?
224+
#[cfg(not(gnustep))]
225+
let expected = "\u{feff}a\u{feff}";
226+
#[cfg(gnustep)]
227+
let expected = "a\u{feff}";
228+
229+
let ns_string = NSString::from_str(s);
230+
autoreleasepool(|pool| {
231+
assert_eq!(ns_string.as_str(pool), expected);
232+
});
233+
assert_eq!(ns_string.len(), expected.len());
133234
}
134235
}

0 commit comments

Comments
 (0)