fix(allocator): statically prevent memory leaks in allocator (#8570)

Prevent memory leaks by statically preventing `Drop` types from being allocated in the arena.

Attempting to allocate any `Drop` type in the arena now produces a compilation failure.

The stabilization of `const {}` blocks in Rust 1.79.0 gave the mechanism required to enforce this at compile time without a mess of generics and traits, and in a way which should not hurt compile times (and zero runtime cost).

This PR is what discovered `CompactString`s being stored in arena in the mangler (fixed in #8557).

Note: The compilation failure occurs in `cargo build` not `cargo check`. So unfortunately errors don't appear in Rust Analyser, only when you run `cargo build`. From what I've read, stable Rust does not offer any solution to this at present. But the errors are reasonably clear what the problem is, and point to the line where it occurs.

Author: overlookmotel

crates/oxc_allocator/src/boxed.rs

@@ -7,6 +7,7 @@ use std::{
     fmt::{self, Debug, Formatter},
     hash::{Hash, Hasher},
     marker::PhantomData,
+    mem::needs_drop,
     ops::{self, Deref},
     ptr::{self, NonNull},
 };
@@ -18,14 +19,16 @@ use crate::Allocator;
 
 /// A `Box` without [`Drop`], which stores its data in the arena allocator.
 ///
-/// Should only be used for storing AST types.
+/// ## No `Drop`s
 ///
-/// Must NOT be used to store types which have a [`Drop`] implementation.
-/// `T::drop` will NOT be called on the `Box`'s contents when the `Box` is dropped.
-/// If `T` owns memory outside of the arena, this will be a memory leak.
+/// Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). Memory is released in bulk
+/// when the allocator is dropped, without dropping the individual objects in the arena.
 ///
-/// Note: This is not a soundness issue, as Rust does not support relying on `drop`
-/// being called to guarantee soundness.
+/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena
+/// which own memory allocations outside the arena.
+///
+/// Static checks make this impossible to do. [`Box::new_in`] will refuse to compile if called
+/// with a [`Drop`] type.
 pub struct Box<'alloc, T: ?Sized>(NonNull<T>, PhantomData<(&'alloc (), T)>);
 
 impl<T> Box<'_, T> {
@@ -68,6 +71,10 @@ impl<T> Box<'_, T> {
     /// let in_arena: Box<i32> = Box::new_in(5, &arena);
     /// ```
     pub fn new_in(value: T, allocator: &Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Box<T> where T is a Drop type");
+        }
+
         Self(NonNull::from(allocator.alloc(value)), PhantomData)
     }
 
@@ -78,6 +85,10 @@ impl<T> Box<'_, T> {
     /// Only purpose is for mocking types without allocating for const assertions.
     #[allow(unsafe_code, clippy::missing_safety_doc)]
     pub const unsafe fn dangling() -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Box<T> where T is a Drop type");
+        }
+
         Self(NonNull::dangling(), PhantomData)
     }
 }
@@ -97,6 +108,10 @@ impl<T: ?Sized> Box<'_, T> {
     /// `ptr` must have been created from a `*mut T` or `&mut T` (not a `*const T` / `&T`).
     #[inline]
     pub(crate) const unsafe fn from_non_null(ptr: NonNull<T>) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Box<T> where T is a Drop type");
+        }
+
         Self(ptr, PhantomData)
     }
 

crates/oxc_allocator/src/hash_map.rs

@@ -9,7 +9,7 @@
 
 use std::{
     hash::Hash,
-    mem::ManuallyDrop,
+    mem::{needs_drop, ManuallyDrop},
     ops::{Deref, DerefMut},
 };
 
@@ -36,12 +36,16 @@ type FxHashMap<'alloc, K, V> = hashbrown::HashMap<K, V, FxBuildHasher, &'alloc B
 /// All APIs are the same, except create a [`HashMap`] with
 /// either [`new_in`](HashMap::new_in) or [`with_capacity_in`](HashMap::with_capacity_in).
 ///
-/// Must NOT be used to store types which have a [`Drop`] implementation.
-/// `K::drop` and `V::drop` will NOT be called on the `HashMap`'s contents when the `HashMap` is dropped.
-/// If `K` or `V` own memory outside of the arena, this will be a memory leak.
+/// ## No `Drop`s
 ///
-/// Note: This is not a soundness issue, as Rust does not support relying on `drop`
-/// being called to guarantee soundness.
+/// Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). Memory is released in bulk
+/// when the allocator is dropped, without dropping the individual objects in the arena.
+///
+/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena
+/// which own memory allocations outside the arena.
+///
+/// Static checks make this impossible to do. [`HashMap::new_in`] and all other methods which create
+/// a [`HashMap`] will refuse to compile if called with a [`Drop`] type.
 ///
 /// [`FxHasher`]: rustc_hash::FxHasher
 pub struct HashMap<'alloc, K, V>(ManuallyDrop<FxHashMap<'alloc, K, V>>);
@@ -56,6 +60,11 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {
     /// until it is first inserted into.
     #[inline(always)]
     pub fn new_in(allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<K>(), "Cannot create a HashMap<K, V> where K is a Drop type");
+            assert!(!needs_drop::<V>(), "Cannot create a HashMap<K, V> where V is a Drop type");
+        }
+
         let inner = FxHashMap::with_hasher_in(FxBuildHasher, allocator.bump());
         Self(ManuallyDrop::new(inner))
     }
@@ -66,6 +75,11 @@ impl<'alloc, K, V> HashMap<'alloc, K, V> {
     /// If capacity is 0, the hash map will not allocate.
     #[inline(always)]
     pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<K>(), "Cannot create a HashMap<K, V> where K is a Drop type");
+            assert!(!needs_drop::<V>(), "Cannot create a HashMap<K, V> where V is a Drop type");
+        }
+
         let inner =
             FxHashMap::with_capacity_and_hasher_in(capacity, FxBuildHasher, allocator.bump());
         Self(ManuallyDrop::new(inner))

crates/oxc_allocator/src/lib.rs

@@ -5,27 +5,41 @@
 //! memory management data types from `std` adapted to use this arena.
 //!
 //! ## No `Drop`s
-//! Objects allocated into oxc memory arenas are never [`Dropped`](Drop), making
-//! it relatively easy to leak memory if you're not careful. Memory is released
-//! in bulk when the allocator is dropped.
+//!
+//! Objects allocated into Oxc memory arenas are never [`Dropped`](Drop).
+//! Memory is released in bulk when the allocator is dropped, without dropping the individual
+//! objects in the arena.
+//!
+//! Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena
+//! which own memory allocations outside the arena.
+//!
+//! Static checks make this impossible to do. [`Allocator::alloc`], [`Box::new_in`], [`Vec::new_in`],
+//! and all other methods which store data in the arena will refuse to compile if called with
+//! a [`Drop`] type.
 //!
 //! ## Examples
-//! ```
+//!
+//! ```ignore
 //! use oxc_allocator::{Allocator, Box};
 //!
 //! struct Foo {
 //!     pub a: i32
 //! }
+//!
 //! impl std::ops::Drop for Foo {
-//!     fn drop(&mut self) {
-//!         // Arena boxes are never dropped.
-//!         unreachable!();
-//!     }
+//!     fn drop(&mut self) {}
+//! }
+//!
+//! struct Bar {
+//!     v: std::vec::Vec<u8>,
 //! }
 //!
 //! let allocator = Allocator::default();
+//!
+//! // This will fail to compile because `Foo` implements `Drop`
 //! let foo = Box::new_in(Foo { a: 0 }, &allocator);
-//! drop(foo);
+//! // This will fail to compile because `Bar` contains a `std::vec::Vec`, and it implements `Drop`
+//! let bar = Box::new_in(Bar { v: vec![1, 2, 3] }, &allocator);
 //! ```
 //!
 //! Consumers of the [`oxc` umbrella crate](https://crates.io/crates/oxc) pass
@@ -41,6 +55,8 @@
 
 #![warn(missing_docs)]
 
+use std::mem::needs_drop;
+
 use bumpalo::Bump;
 
 mod address;
@@ -92,6 +108,10 @@ impl Allocator {
     #[expect(clippy::inline_always)]
     #[inline(always)]
     pub fn alloc<T>(&self, val: T) -> &mut T {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot allocate Drop type in arena");
+        }
+
         self.bump.alloc(val)
     }
 

crates/oxc_allocator/src/vec.rs

@@ -9,7 +9,7 @@ use std::{
     self,
     fmt::{self, Debug},
     hash::{Hash, Hasher},
-    mem::ManuallyDrop,
+    mem::{needs_drop, ManuallyDrop},
     ops,
     ptr::NonNull,
     slice::SliceIndex,
@@ -25,12 +25,16 @@ use crate::{Allocator, Box, String};
 
 /// A `Vec` without [`Drop`], which stores its data in the arena allocator.
 ///
-/// Must NOT be used to store types which have a [`Drop`] implementation.
-/// `T::drop` will NOT be called on the `Vec`'s contents when the `Vec` is dropped.
-/// If `T` owns memory outside of the arena, this will be a memory leak.
+/// ## No `Drop`s
 ///
-/// Note: This is not a soundness issue, as Rust does not support relying on `drop`
-/// being called to guarantee soundness.
+/// Objects allocated into Oxc memory arenas are never [`Dropped`](Drop). Memory is released in bulk
+/// when the allocator is dropped, without dropping the individual objects in the arena.
+///
+/// Therefore, it would produce a memory leak if you allocated [`Drop`] types into the arena
+/// which own memory allocations outside the arena.
+///
+/// Static checks make this impossible to do. [`Vec::new_in`] and all other methods which create
+/// a [`Vec`] will refuse to compile if called with a [`Drop`] type.
 #[derive(PartialEq, Eq)]
 pub struct Vec<'alloc, T>(pub(crate) ManuallyDrop<InnerVec<T, &'alloc Bump>>);
 
@@ -56,6 +60,10 @@ impl<'alloc, T> Vec<'alloc, T> {
     /// ```
     #[inline(always)]
     pub fn new_in(allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Vec<T> where T is a Drop type");
+        }
+
         Self(ManuallyDrop::new(InnerVec::new_in(allocator.bump())))
     }
 
@@ -108,6 +116,10 @@ impl<'alloc, T> Vec<'alloc, T> {
     /// ```
     #[inline(always)]
     pub fn with_capacity_in(capacity: usize, allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Vec<T> where T is a Drop type");
+        }
+
         Self(ManuallyDrop::new(InnerVec::with_capacity_in(capacity, allocator.bump())))
     }
 
@@ -117,6 +129,10 @@ impl<'alloc, T> Vec<'alloc, T> {
     /// This is behaviorially identical to [`FromIterator::from_iter`].
     #[inline]
     pub fn from_iter_in<I: IntoIterator<Item = T>>(iter: I, allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Vec<T> where T is a Drop type");
+        }
+
         let iter = iter.into_iter();
         let hint = iter.size_hint();
         let capacity = hint.1.unwrap_or(hint.0);
@@ -143,6 +159,10 @@ impl<'alloc, T> Vec<'alloc, T> {
     /// ```
     #[inline]
     pub fn from_array_in<const N: usize>(array: [T; N], allocator: &'alloc Allocator) -> Self {
+        const {
+            assert!(!needs_drop::<T>(), "Cannot create a Vec<T> where T is a Drop type");
+        }
+
         let boxed = Box::new_in(array, allocator);
         let ptr = Box::into_non_null(boxed).as_ptr().cast::<T>();
         // SAFETY: `ptr` has correct alignment - it was just allocated as `[T; N]`.