diff --git a/crates/oxc_allocator/src/hash_map.rs b/crates/oxc_allocator/src/hash_map.rs index 0ead3447116ec..d8fac045dca64 100644 --- a/crates/oxc_allocator/src/hash_map.rs +++ b/crates/oxc_allocator/src/hash_map.rs @@ -50,10 +50,41 @@ type FxHashMap<'alloc, K, V> = hashbrown::HashMap(ManuallyDrop>); -/// SAFETY: Not actually safe, but for enabling `Send` for downstream crates. -unsafe impl Send for HashMap<'_, K, V> {} -/// SAFETY: Not actually safe, but for enabling `Sync` for downstream crates. -unsafe impl Sync for HashMap<'_, K, V> {} +/// SAFETY: Even though `Bump` is not `Sync`, we can make `HashMap` `Sync` if both `K` and `V` +/// are `Sync` because: +/// +/// 1. No public methods allow access to the `&Bump` that `HashMap` contains (in `hashbrown::HashMap`), +/// so user cannot illegally obtain 2 `&Bump`s on different threads via `HashMap`. +/// +/// 2. All internal methods which access the `&Bump` take a `&mut self`. +/// `&mut HashMap` cannot be transferred across threads, and nor can an owned `HashMap` +/// (`HashMap` is not `Send`). +/// Therefore these methods taking `&mut self` can be sure they're not operating on a `HashMap` +/// which has been moved across threads. +/// +/// Note: `HashMap` CANNOT be `Send`, even if `K` and `V` are `Send`, because that would allow 2 `HashMap`s +/// on different threads to both allocate into same arena simultaneously. `Bump` is not thread-safe, +/// and this would be undefined behavior. +/// +/// ### Soundness holes +/// +/// This is not actually fully sound. There are 2 holes I (@overlookmotel) am aware of: +/// +/// 1. `allocator` method, which does allow access to the `&Bump` that `HashMap` contains. +/// 2. `Clone` impl on `hashbrown::HashMap`, which may perform allocations in the arena, given only a +/// `&self` reference. +/// +/// [`HashMap::allocator`] prevents accidental access to the underlying method of `hashbrown::HashMap`, +/// and `clone` called on a `&HashMap` clones the `&HashMap` reference, not the `HashMap` itself (harmless). +/// But both can be accessed via explicit `Deref` (`hash_map.deref().allocator()` or `hash_map.deref().clone()`), +/// so we don't have complete soundness. +/// +/// To close these holes we need to remove `Deref` and `DerefMut` impls on `HashMap`, and instead add +/// methods to `HashMap` itself which pass on calls to the inner `hashbrown::HashMap`. +/// +/// TODO: Fix these holes. +/// TODO: Remove any other methods that currently allow performing allocations with only a `&self` reference. +unsafe impl Sync for HashMap<'_, K, V> {} // TODO: `IntoIter`, `Drain`, and other consuming iterators provided by `hashbrown` are `Drop`. // Wrap them in `ManuallyDrop` to prevent that. @@ -114,6 +145,23 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> { let inner = ManuallyDrop::into_inner(self.0); inner.into_values() } + + /// Calling this method produces a compile-time panic. + /// + /// This method would be unsound, because [`HashMap`] is `Sync`, and the underlying allocator + /// (`bumpalo::Bump`) is not `Sync`. + /// + /// This method exists only to block access as much as possible to the underlying + /// `hashbrown::HashMap::allocator` method. That method can still be accessed via explicit `Deref` + /// (`hash_map.deref().allocator()`), but that's unsound. + /// + /// We'll prevent access to it completely and remove this method as soon as we can. + // TODO: Do that! + #[expect(clippy::unused_self)] + pub fn allocator(&self) -> &'alloc Bump { + const { panic!("This method cannot be called") }; + unreachable!(); + } } // Provide access to all `hashbrown::HashMap`'s methods via deref