fix(allocator): remove Vec::bump method

Author: overlookmotel

crates/oxc_allocator/src/vec2/mod.rs

@@ -720,26 +720,6 @@ impl<'a, T: 'a, A: Alloc> Vec<'a, T, A> {
         self.buf.set_len(new_len);
     }
 
-    /// Returns a shared reference to the allocator backing this `Vec`.
-    ///
-    /// # Examples
-    ///
-    /// ```
-    /// use bumpalo::{Bump, collections::Vec};
-    ///
-    /// // uses the same allocator as the provided `Vec`
-    /// fn add_strings<'a>(vec: &mut Vec<'a, &'a str>) {
-    ///     for string in ["foo", "bar", "baz"] {
-    ///         vec.push(vec.bump().alloc_str(string));
-    ///     }
-    /// }
-    /// ```
-    #[inline]
-    #[must_use]
-    pub fn bump(&self) -> &'a A {
-        self.buf.bump()
-    }
-
     /// Reserves capacity for at least `additional` more elements to be inserted
     /// in the given `Vec<'a, T>`. The collection may reserve more space to avoid
     /// frequent reallocations. After calling `reserve`, capacity will be
@@ -1735,7 +1715,10 @@ impl<'a, T: 'a, A: Alloc> Vec<'a, T, A> {
         assert!(at <= self.len_usize(), "`at` out of bounds");
 
         let other_len = self.len_usize() - at;
-        let mut other = Vec::with_capacity_in(other_len, self.buf.bump());
+        // SAFETY: This method takes a `&mut self`. It lives for the duration of this method
+        // - longer than we use `bump` for.
+        let bump = unsafe { self.buf.bump() };
+        let mut other = Vec::with_capacity_in(other_len, bump);
 
         // Unsafely `set_len` and copy items to `other`.
         unsafe {
@@ -2683,7 +2666,15 @@ impl<I: Iterator, A: Alloc> Drop for Splice<'_, '_, I, A> {
 
             // Collect any remaining elements.
             // This is a zero-length vector which does not allocate if `lower_bound` was exact.
-            let mut collected = Vec::new_in(self.drain.vec.as_ref().buf.bump());
+
+            // SAFETY: `Splice` iterator is created in `Vec::splice`, which takes a `&mut self`.
+            // `Splice` inherits the lifetime of `&mut self` from that method, so the mut borrow
+            // of the `Vec` is held for the life of the `Splice`.
+            // Therefore we have exclusive access to the `Vec` until end of this method.
+            // That is longer than we use `bump` for.
+            let bump = self.drain.vec.as_ref().buf.bump();
+
+            let mut collected = Vec::new_in(bump);
             collected.extend(self.replace_with.by_ref());
             let mut collected = collected.into_iter();
             // Now we have an exact count.

crates/oxc_allocator/src/vec2/raw_vec.rs

@@ -72,6 +72,8 @@ pub struct RawVec<'a, T, A: Alloc> {
     ptr: NonNull<T>,
     len: u32,
     cap: u32,
+    // SAFETY: Methods must not mutate the allocator (e.g. allocate into it), unless they can guarantee
+    // they have exclusive access to it, by taking `&mut self`
     alloc: &'a A,
 }
 
@@ -192,8 +194,47 @@ impl<'a, T, A: Alloc> RawVec<'a, T, A> {
         if mem::size_of::<T>() == 0 { !0 } else { self.cap as usize }
     }
 
-    /// Returns a shared reference to the allocator backing this RawVec.
-    pub fn bump(&self) -> &'a A {
+    /// Get a shared reference to the allocator backing this `RawVec`.
+    ///
+    /// This method is hazardous.
+    ///
+    /// `Vec` is `Sync`, but `Bump` is not, because it utilizes interior mutability.
+    /// It is possible to make allocations into the arena while holding only a `&Bump`.
+    /// Because `Vec` is `Sync`, it's possible for multiple `&Vec` references to the same `Vec`,
+    /// or references to multiple `Vec`s attached to the same `Bump`, to exist simultaneously
+    /// on different threads.
+    ///
+    /// So this method could be used to obtain 2 `&Bump` references simultaneously on different threads.
+    /// Utilizing those references to allocate into the arena simultaneously from different threads
+    /// would be UB.
+    ///
+    /// We cannot rely on the type system or borrow checker to ensure correct synchronization.
+    ///
+    /// Therefore callers must ensure by other means that they have exclusive access, by:
+    ///
+    /// 1. Taking a `&mut self`.
+    ///    No methods of `Vec` or `RawVec` which do not hold a `&mut Vec` / `&mut RawVec` can use this method.
+    ///
+    /// 2. That `&mut self` must be held for at least as long as the `&'a A` reference returned by
+    ///    this method is held.
+    ///
+    /// Note: It's tempting to think we could make this a safe method by making it take `&mut self`,
+    /// but that's insufficient. That would enforce that the caller holds a `&mut self`, but the `&'a A`
+    /// returned by this method outlives the lifetime of `&self` that the method takes, so it would
+    /// NOT guarantee anything about *how long* they hold it for.
+    /// Taking a `&'a mut self` *would* be safe, but it'd be impractical.
+    ///
+    /// For further information, see comments on the `impl Sync` implementation of `Vec`.
+    ///
+    /// # IMPORTANT
+    /// The ability to obtain a reference to the allocator MUST NOT be exposed to user,
+    /// outside of `Vec`'s internals.
+    ///
+    /// # SAFETY
+    /// Caller must ensure they have exclusive access, but holding a `&mut Vec` or `&mut RawVec`
+    /// for the duration that the reference returned by this method is held.
+    /// See text above for further detail.
+    pub unsafe fn bump(&self) -> &'a A {
         self.alloc
     }