Repository: ParkMyCar/compact_str
Pull request #262 (2023-02-20 19:06:58)

refactor: Change CompactString::as_ptr to take &self
I realized this recently that `CompactString::as_ptr` took a mutable reference to self (aka `&mut self`), when all it really needs is a normal reference (aka `&self`). This PR updates the API to make that change


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 32763f5a..0ee7b46c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,4 +1,6 @@
 # Upcoming
+* Change `as_ptr()` to require only `&self` and not `&mut self`
+  * Implemented in [`refactor: Change CompactString::as_ptr to take &self`](https://github.com/ParkMyCar/compact_str/pull/262)
 * Add `into_bytes()` method behind the `smallvec` feature which converts a `CompactString` into a byte vector using a [`SmallVec`](https://docs.rs/smallvec/latest/smallvec/)
   * Implemented in [`api: Add CompactString::into_bytes`](https://github.com/ParkMyCar/compact_str/pull/258)
 * Add `from_string_buffer()` method which __always__ re-uses the underlying buffer from `String`
diff --git a/compact_str/src/features/smallvec.rs b/compact_str/src/features/smallvec.rs
index 2ba6f648..118ba04d 100644
--- a/compact_str/src/features/smallvec.rs
+++ b/compact_str/src/features/smallvec.rs
@@ -44,7 +44,7 @@ mod tests {
 
     #[test]
     fn test_buffer_reuse() {
-        let mut c = CompactString::from("I am a longer string that will be on the heap");
+        let c = CompactString::from("I am a longer string that will be on the heap");
         let c_ptr = c.as_ptr();
 
         let bytes = c.into_bytes();
@@ -61,7 +61,7 @@ mod tests {
     #[proptest]
     #[cfg_attr(miri, ignore)]
     fn proptest_buffer_reuse(#[strategy(rand_long_unicode())] s: String) {
-        let mut c = CompactString::from(s);
+        let c = CompactString::from(s);
         let c_ptr = c.as_ptr();
 
         let bytes = c.into_bytes();
diff --git a/compact_str/src/lib.rs b/compact_str/src/lib.rs
index 6c941241..d8272469 100644
--- a/compact_str/src/lib.rs
+++ b/compact_str/src/lib.rs
@@ -880,7 +880,7 @@ impl CompactString {
 
     /// Converts a [`CompactString`] to a raw pointer.
     #[inline]
-    pub fn as_ptr(&mut self) -> *const u8 {
+    pub fn as_ptr(&self) -> *const u8 {
         self.0.as_slice().as_ptr()
     }
 
diff --git a/compact_str/src/repr/iter.rs b/compact_str/src/repr/iter.rs
index e25efd3c..38ca6bcd 100644
--- a/compact_str/src/repr/iter.rs
+++ b/compact_str/src/repr/iter.rs
@@ -49,8 +49,10 @@ impl FromIterator<char> for Repr {
             curr_len += char_len;
         }
 
+        // SAFETY: Everything we just pushed onto the buffer is a `str` which is valid UTF-8
         unsafe { inline_buf.set_len(curr_len) }
-        unsafe { core::mem::transmute(inline_buf) }
+
+        Repr::from_inline(inline_buf)
     }
 }
 
@@ -99,8 +101,10 @@ where
         curr_len += bytes_len;
     }
 
+    // SAFETY: Everything we just pushed onto the buffer is a `str` which is valid UTF-8
     unsafe { inline_buf.set_len(curr_len) }
-    unsafe { core::mem::transmute(inline_buf) }
+
+    Repr::from_inline(inline_buf)
 }
 
 impl<'a> FromIterator<&'a str> for Repr {
diff --git a/compact_str/src/repr/mod.rs b/compact_str/src/repr/mod.rs
index 2c2c43d9..e6a0e811 100644
--- a/compact_str/src/repr/mod.rs
+++ b/compact_str/src/repr/mod.rs
@@ -61,12 +61,10 @@ impl Repr {
         } else if len <= MAX_SIZE {
             // SAFETY: We checked that the length of text is less than or equal to MAX_SIZE
             let inline = unsafe { InlineBuffer::new(text) };
-            // SAFETY: `InlineString` and `Repr` are the same size
-            unsafe { mem::transmute(inline) }
+            Repr::from_inline(inline)
         } else {
             let heap = HeapBuffer::new(text);
-            // SAFETY: `BoxString` and `Repr` are the same size
-            unsafe { mem::transmute(heap) }
+            Repr::from_heap(heap)
         }
     }
 
@@ -76,8 +74,7 @@ impl Repr {
 
         if len <= MAX_SIZE {
             let inline = InlineBuffer::new_const(text);
-            // SAFETY: `InlineString` and `Repr` are the same size
-            unsafe { mem::transmute(inline) }
+            Repr::from_inline(inline)
         } else {
             panic!("Inline string was too long, max length is `std::mem::size_of::<CompactString>()` bytes");
         }
@@ -90,7 +87,7 @@ impl Repr {
             EMPTY
         } else {
             let heap = HeapBuffer::with_capacity(capacity);
-            unsafe { mem::transmute(heap) }
+            Repr::from_heap(heap)
         }
     }
 
@@ -155,8 +152,7 @@ impl Repr {
         #[cold]
         fn capacity_on_heap(s: String) -> Repr {
             let heap = HeapBuffer::new(s.as_str());
-            // SAFETY: `HeapBuffer` and `Repr` are the same size
-            unsafe { mem::transmute(heap) }
+            Repr::from_heap(heap)
         }
 
         #[cold]
@@ -174,8 +170,7 @@ impl Repr {
         } else if should_inline && s.len() <= MAX_SIZE {
             // SAFETY: Checked to make sure the string would fit inline
             let inline = unsafe { InlineBuffer::new(s.as_str()) };
-            // SAFETY: `InlineBuffer` and `Repr` are the same size
-            unsafe { mem::transmute(inline) }
+            Repr::from_inline(inline)
         } else {
             let mut s = mem::ManuallyDrop::new(s.into_bytes());
             let len = s.len();
@@ -184,8 +179,7 @@ impl Repr {
             let ptr = ptr::NonNull::new(raw_ptr).expect("string with capacity has null ptr?");
             let heap = HeapBuffer { ptr, len, cap };
 
-            // SAFETY: `HeapBuffer` and `Repr` are the same size
-            unsafe { mem::transmute(heap) }
+            Repr::from_heap(heap)
         }
     }
 
@@ -205,8 +199,8 @@ impl Repr {
         }
 
         if last_byte == HEAP_MASK {
-            // SAFTEY: this is only ever called if we're heap allocated
-            let heap_buffer: HeapBuffer = unsafe { mem::transmute(self) };
+            // SAFTEY: we just checked that the discriminant indicates we're a HeapBuffer
+            let heap_buffer = unsafe { self.into_heap() };
 
             if heap_buffer.cap.is_heap() {
                 // We don't expect capacity to be on the heap often, so we mark it as cold
@@ -259,16 +253,16 @@ impl Repr {
             //
             // SAFTEY: Our needed_capacity is >= our length, which is <= than MAX_SIZE
             let inline = unsafe { InlineBuffer::new(self.as_str()) };
-            // SAFETY: `InlineBuffer` and `Repr` are the same size
-            *self = unsafe { mem::transmute(inline) };
+            *self = Repr::from_inline(inline);
         } else if !self.is_heap_allocated() {
             // We're not heap allocated, but need to be, create a HeapBuffer
             let heap = HeapBuffer::with_additional(self.as_str(), additional);
-            // SAFETY: `HeapBuffer` and `Repr` are the same size
-            *self = unsafe { mem::transmute(heap) };
+            *self = Repr::from_heap(heap);
         } else {
             // We're already heap allocated, but we need more capacity
-            let heap_buffer = unsafe { &mut *(self as *mut _ as *mut HeapBuffer) };
+            //
+            // SAFETY: We checked above to see if we're heap allocated
+            let heap_buffer = unsafe { self.as_mut_heap() };
 
             // To reduce allocations, we amortize our growth
             let amortized_capacity = heap::amortized_growth(len, additional);
@@ -276,8 +270,7 @@ impl Repr {
             if heap_buffer.realloc(amortized_capacity).is_err() {
                 // Create a new HeapBuffer
                 let heap = HeapBuffer::with_additional(self.as_str(), additional);
-                // SAFETY: `HeapBuffer` and `Repr` are the same size
-                *self = unsafe { mem::transmute(heap) };
+                *self = Repr::from_heap(heap);
             }
         }
     }
@@ -288,22 +281,26 @@ impl Repr {
         // Note: We can't shrink the inline variant since it's buffer is a fixed size, so we only
         // take action here if our string is heap allocated
         if last_byte == HEAP_MASK {
-            let heap = unsafe { &mut *(self as *mut _ as *mut HeapBuffer) };
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap = unsafe { self.as_mut_heap() };
 
             let old_capacity = heap.capacity();
             let new_capacity = heap.len.max(min_capacity);
 
             if new_capacity <= MAX_SIZE {
-                // // String can be inlined.
+                // String can be inlined.
                 let mut inline = InlineBuffer::empty();
+                // SAFETY: Our src is on the heap, so it does not overlap with our new inline
+                // buffer, and the src is a `Repr` so we can assume it's valid UTF-8
                 unsafe {
                     inline
                         .0
                         .as_mut_ptr()
                         .copy_from_nonoverlapping(heap.ptr.as_ptr(), heap.len)
                 };
+                // SAFETY: The src we wrote from was a `Repr` which we can assume is valid UTF-8
                 unsafe { inline.set_len(heap.len) }
-                *self = unsafe { mem::transmute(inline) };
+                *self = Repr::from_inline(inline);
             } else if new_capacity < old_capacity {
                 // String can be shrunk.
                 // We can ignore the result. The string keeps its old capacity, but that's okay.
@@ -372,6 +369,8 @@ impl Repr {
             *length_ref = heap_length;
         }
 
+        // SAFETY: We know the data is valid, aligned, and part of the same contiguous allocated
+        // chunk. It's also valid for the lifetime of self
         unsafe { core::slice::from_raw_parts(pointer, length) }
     }
 
@@ -412,8 +411,8 @@ impl Repr {
 
         #[cold]
         fn heap_capacity(this: &Repr) -> usize {
-            // SAFETY: A `HeapBuffer` and `Repr` have the same size
-            let heap_buffer = unsafe { &*(this as *const _ as *const HeapBuffer) };
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap_buffer = unsafe { this.as_heap() };
             heap_buffer.capacity()
         }
 
@@ -439,8 +438,8 @@ impl Repr {
         let last_byte = self.last_byte();
 
         let (ptr, cap) = if last_byte == HEAP_MASK {
-            // SAFETY: A `HeapBuffer` and `Repr` have the same size
-            let heap_buffer = &*(self as *const _ as *const HeapBuffer);
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap_buffer = self.as_heap();
             let ptr = heap_buffer.ptr.as_ptr();
             let cap = heap_buffer.capacity();
 
@@ -463,13 +462,14 @@ impl Repr {
         let last_byte = self.last_byte();
 
         if last_byte == HEAP_MASK {
-            // SAFETY: A `HeapBuffer` and `Repr` have the same size
-            let heap_buffer = &mut *(self as *mut _ as *mut HeapBuffer);
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap_buffer = self.as_mut_heap();
+            // SAFETY: The caller guarantees that `len` bytes is valid UTF-8
             heap_buffer.set_len(len);
         } else {
-            // SAFETY: A `InlineBuffer` and `Repr` have the same size
-            let inline_buffer = &mut *(self as *mut _ as *mut InlineBuffer);
-            // SAFETY: The caller guarantees that len <= MAX_SIZE
+            // SAFETY: We just checked the discriminant to make sure we're an InlineBuffer
+            let inline_buffer = self.as_mut_inline();
+            // SAFETY: The caller guarantees that len <= MAX_SIZE, and `len` bytes is valid UTF-8
             inline_buffer.set_len(len);
         }
     }
@@ -491,6 +491,119 @@ impl Repr {
         };
         last_byte as u8
     }
+
+    /// Reinterprets an [`InlineBuffer`] into a [`Repr`]
+    ///
+    /// Note: This is safe because [`InlineBuffer`] and [`Repr`] are the same size. We used to
+    /// define [`Repr`] as a `union` which implicitly transmuted between the two types, but that
+    /// prevented us from defining a "niche" value to make `Option<CompactString>` the same size as
+    /// just `CompactString`
+    #[inline(always)]
+    const fn from_inline(inline: InlineBuffer) -> Self {
+        // SAFETY: An `InlineBuffer` and `Repr` have the same size
+        unsafe { core::mem::transmute(inline) }
+    }
+
+    /// Reinterprets a [`HeapBuffer`] into a [`Repr`]
+    ///
+    /// Note: This is safe because [`HeapBuffer`] and [`Repr`] are the same size. We used to define
+    /// [`Repr`] as a `union` which implicitly transmuted between the two types, but that prevented
+    /// us from defining a "niche" value to make `Option<CompactString>` the same size as just
+    /// `CompactString`
+    #[inline(always)]
+    const fn from_heap(heap: HeapBuffer) -> Self {
+        // SAFETY: A `HeapBuffer` and `Repr` have the same size
+        unsafe { core::mem::transmute(heap) }
+    }
+
+    /// Reinterprets a [`Repr`] as a [`HeapBuffer`]
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually a [`HeapBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    const unsafe fn into_heap(self) -> HeapBuffer {
+        core::mem::transmute(self)
+    }
+
+    /// Reinterprets a `&mut Repr` as a `&mut HeapBuffer`
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually a [`HeapBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    unsafe fn as_mut_heap(&mut self) -> &mut HeapBuffer {
+        // SAFETY: A `HeapBuffer` and `Repr` have the same size
+        &mut *(self as *mut _ as *mut HeapBuffer)
+    }
+
+    /// Reinterprets a `&Repr` as a `&HeapBuffer`
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually a [`HeapBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    unsafe fn as_heap(&self) -> &HeapBuffer {
+        // SAFETY: A `HeapBuffer` and `Repr` have the same size
+        &*(self as *const _ as *const HeapBuffer)
+    }
+
+    /// Reinterprets a [`Repr`] as an [`InlineBuffer`]
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually an [`InlineBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    #[cfg(feature = "smallvec")]
+    const unsafe fn into_inline(self) -> InlineBuffer {
+        core::mem::transmute(self)
+    }
+
+    /// Reinterprets a `&mut Repr` as an `&mut InlineBuffer`
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually an [`InlineBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    unsafe fn as_mut_inline(&mut self) -> &mut InlineBuffer {
+        // SAFETY: An `InlineBuffer` and `Repr` have the same size
+        &mut *(self as *mut _ as *mut InlineBuffer)
+    }
+
+    /// Reinterprets a `&Repr` as an `&InlineBuffer`
+    ///
+    /// # SAFETY
+    /// * The caller must guarantee that the provided [`Repr`] is actually an [`InlineBuffer`] by
+    /// checking the discriminant
+    ///
+    /// Note: We used to define [`Repr`] as a `union` which implicitly transmuted between the two
+    /// types, but that prevented us from defining a "niche" value to make `Option<CompactString>`
+    /// the same size as just `CompactString`
+    #[inline(always)]
+    unsafe fn as_inline(&self) -> &InlineBuffer {
+        // SAFETY: An `InlineBuffer` and `Repr` have the same size
+        &*(self as *const _ as *const InlineBuffer)
+    }
 }
 
 impl Clone for Repr {
@@ -500,27 +613,26 @@ impl Clone for Repr {
 
         #[cold]
         fn clone_heap(this: &Repr) -> Repr {
-            let heap = unsafe { &*(this as *const _ as *const HeapBuffer) };
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap = unsafe { this.as_heap() };
 
             // If the contained string is small enough, we will inline it instead of allocating
             if heap.len <= MAX_SIZE {
                 // SAFETY: Checked to make sure the length is <= MAX_SIZE
                 let inline = unsafe { InlineBuffer::new(this.as_str()) };
-                // SAFETY: InlineBuffer and Repr have the same layout
-                unsafe { mem::transmute(inline) }
+                Repr::from_inline(inline)
             } else {
                 let new = heap.clone();
-                // SAFETY: HeapBuffer and Repr have the same layout
-                unsafe { mem::transmute(new) }
+                Repr::from_heap(new)
             }
         }
 
         if last_byte == HEAP_MASK {
             clone_heap(self)
         } else {
-            let inline = unsafe { &*(self as *const _ as *const InlineBuffer) };
-            let new = inline.copy();
-            unsafe { mem::transmute(new) }
+            // SAFETY: We checked above that the discriminant indicates we're inline
+            let inline = unsafe { self.as_inline() };
+            Repr::from_inline(inline.copy())
         }
     }
 }
@@ -536,8 +648,8 @@ impl Drop for Repr {
 
         #[cold]
         fn outlined_drop(this: &mut Repr) {
-            // SAFTEY: this is only ever called if we're heap allocated
-            let heap_buffer: &mut HeapBuffer = unsafe { &mut *(this as *mut _ as *mut _) };
+            // SAFETY: We just checked the discriminant to make sure we're heap allocated
+            let heap_buffer = unsafe { this.as_mut_heap() };
             heap_buffer.dealloc();
         }
     }
diff --git a/compact_str/src/repr/smallvec.rs b/compact_str/src/repr/smallvec.rs
index e0ba8bec..f1538866 100644
--- a/compact_str/src/repr/smallvec.rs
+++ b/compact_str/src/repr/smallvec.rs
@@ -1,6 +1,5 @@
 use smallvec::SmallVec;
 
-use super::inline::InlineBuffer;
 use super::{
     Repr,
     HEAP_MASK,
@@ -20,7 +19,8 @@ impl Repr {
             let bytes = string.into_bytes();
             SmallVec::from_vec(bytes)
         } else {
-            let inline: InlineBuffer = unsafe { std::mem::transmute(self) };
+            // SAFETY: We just checked the discriminant to make sure we're an InlineBuffer
+            let inline = unsafe { self.into_inline() };
             let (array, length) = inline.into_array();
             SmallVec::from_buf_and_len(array, length)
         }
diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml
index 1c4b41cb..47128690 100644
--- a/fuzz/Cargo.toml
+++ b/fuzz/Cargo.toml
@@ -11,7 +11,7 @@ cargo-fuzz = true
 [dependencies]
 arbitrary = { version = "1.1.1", features = ["derive"] }
 bytes = "1"
-compact_str = { path = "../compact_str", features = ["bytes"] }
+compact_str = { path = "../compact_str", features = ["bytes", "smallvec"] }
 rand = { version = "0.8", features = ["small_rng"] }
 rand_distr = "0.4"
 
diff --git a/fuzz/src/actions.rs b/fuzz/src/actions.rs
index bc51387d..918d1cbd 100644
--- a/fuzz/src/actions.rs
+++ b/fuzz/src/actions.rs
@@ -44,6 +44,8 @@ pub enum Action<'a> {
     Retain(u8, char),
     /// Clones each string, and drops the originals
     CloneAndDrop,
+    /// Calls into_bytes, validates equality, and converts back into strings
+    RoundTripIntoBytes,
 }
 
 impl Action<'_> {
@@ -343,6 +345,33 @@ impl Action<'_> {
                 let og = std::mem::replace(compact, compact_clone);
                 drop(og);
             }
+            RoundTripIntoBytes => {
+                // cloning truncates capacity, so we only use the clones for checking equality
+                let control_clone = control.clone();
+                let compact_clone = compact.clone();
+
+                let og_control = std::mem::take(control);
+                let og_compact = std::mem::take(compact);
+
+                let control_bytes = og_control.into_bytes();
+                let comapct_bytes = og_compact.into_bytes();
+
+                // check to make sure the bytes are the same!
+                assert_eq!(control_bytes.len(), comapct_bytes.len());
+                assert_eq!(&control_bytes[..], &comapct_bytes[..]);
+
+                // roundtrip back into strings
+                let new_control = String::from_utf8(control_bytes).expect("valid UTF-8");
+                let new_compact = CompactString::from_utf8(comapct_bytes).expect("valid UTF-8");
+
+                // make sure we roundtripped successfully
+                assert_eq!(new_control, control_clone);
+                assert_eq!(new_compact, compact_clone);
+
+                // re-assign
+                *control = new_control;
+                *compact = new_compact;
+            }
         }
     }
 }


Test patch:
diff --git a/compact_str/src/tests.rs b/compact_str/src/tests.rs
index d7f3d4e8..4514e70a 100644
--- a/compact_str/src/tests.rs
+++ b/compact_str/src/tests.rs
@@ -930,7 +930,7 @@ fn test_from_string_buffer_small_string_with_excess_capacity() {
     let str_cap = string.capacity();
 
     // using from_string_buffer should always re-use the underlying buffer
-    let mut compact = CompactString::from_string_buffer(string);
+    let compact = CompactString::from_string_buffer(string);
     assert!(compact.is_heap_allocated());
 
     let cpt_ptr = compact.as_ptr();
@@ -964,7 +964,7 @@ fn test_from_string_buffer_small_string_with_no_excess_capacity() {
     let str_cap = string.capacity();
 
     // using from_string_buffer should always re-use the underlying buffer
-    let mut compact = CompactString::from_string_buffer(string);
+    let compact = CompactString::from_string_buffer(string);
     assert!(compact.is_heap_allocated());
 
     let cpt_ptr = compact.as_ptr();