fix(allocator): remove unsound Send impl and tighten Sync requirements for Vec (#13041)

overlookmotel · overlookmotel · commit 0815a894bfe4 · 2025-08-19T21:35:38.000Z
It's unsound for `Vec` to be `Send` because if you can move a `Vec` to another thread, you can call `push` on it and it may allocate in the arena. This is unsound because another thread may be simultaneously be doing the same with another `Vec`, and `Bump` is not thread-safe. Remove the `Send` impl on `Vec`. However, we do want `Vec` to be `Sync`. There's no reason why you shouldn't have 2 `&Vec` references on different threads, as `&Vec` doesn't allow mutating the `Vec` in any way. The only hole we had previously which made this unsound was the `Vec::bump` method, which was removed in #13039. Just tighten up the trait bounds so that `Vec<T>` is `Sync` only if `T` is. This mirrors the standard library in relation to `T`. We're being more liberal than standard library in not requiring the allocator to be `Sync` too (`Bump` isn't), but that's OK because we don't have an equivalent of `std::vec::Vec`'s [`allocator` method](https://doc.rust-lang.org/std/vec/struct.Vec.html#method.allocator) (that was `Vec::bump`, and we've removed it). The reason these 2 impls were added originally was as a quick fix to make `oxc_semantic::Scoping` `Send` and `Sync`. That is addressed in a later PR in this stack.
diff --git a/crates/oxc_allocator/src/vec.rs b/crates/oxc_allocator/src/vec.rs
@@ -39,10 +39,20 @@ type InnerVec<'a, T> = InnerVecGeneric<'a, T, Bump>;
 #[derive(PartialEq, Eq)]
 pub struct Vec<'alloc, T>(InnerVec<'alloc, T>);
 
-/// SAFETY: Not actually safe, but for enabling `Send` for downstream crates.
-unsafe impl<T> Send for Vec<'_, T> {}
-/// SAFETY: Not actually safe, but for enabling `Sync` for downstream crates.
-unsafe impl<T> Sync for Vec<'_, T> {}
+/// SAFETY: Even though `Bump` is not `Sync`, we can make `Vec<T>` `Sync` if `T` is `Sync` because:
+///
+/// 1. No public methods allow access to the `&Bump` that `Vec` contains (in `RawVec`),
+///    so user cannot illegally obtain 2 `&Bump`s on different threads via `Vec`.
+///
+/// 2. All internal methods which access the `&Bump` take a `&mut self`.
+///    `&mut Vec` cannot be transferred across threads, and nor can an owned `Vec` (`Vec` is not `Send`).
+///    Therefore these methods taking `&mut self` can be sure they're not operating on a `Vec`
+///    which has been moved across threads.
+///
+/// Note: `Vec` CANNOT be `Send`, even if `T` is `Send`, because that would allow 2 `Vec`s on different
+/// threads to both allocate into same arena simultaneously. `Bump` is not thread-safe, and this would
+/// be undefined behavior.
+unsafe impl<T: Sync> Sync for Vec<'_, T> {}
 
 impl<'alloc, T> Vec<'alloc, T> {
     /// Const assertion that `T` is not `Drop`.
