Require an explicit drop call on a gc allocation

gc/src/allocator.rs

@@ -2,6 +2,7 @@ use std::ptr::NonNull;
 
 use super::gc_ref::GcRef;
 use super::trace;
+use crate::trace::GCTrace;
 
 #[derive(Default)]
 pub struct GCAllocator {
@@ -10,6 +11,7 @@ pub struct GCAllocator {
 
 impl GCAllocator {
 	#[inline(always)]
+	/// Allocate a value on the heap
 	pub fn alloc<T: trace::GCTrace>(&mut self, x: T) -> GcRef<T> {
 		let alloc = Allocation::new(x);
 		let ptr = alloc.ptr as *mut T;
@@ -28,8 +30,14 @@ impl GCAllocator {
 		root.trace(&mut tracer);
 
 		// And sweep
-		self.allocations
+		let inaccessible = self
+			.allocations
 			.drain_filter(|a| !tracer.is_accessible(a.ptr));
+
+		// And sweep
+		for mut to_free in inaccessible {
+			to_free.drop();
+		}
 	}
 
 	// Specialization when ;-;
@@ -44,9 +52,14 @@ impl GCAllocator {
 		tracer.mark_reachable_rec(root);
 		root.trace(&mut tracer);
 
-		// And sweep
-		self.allocations
+		let inaccessible = self
+			.allocations
 			.drain_filter(|a| !tracer.is_accessible(a.ptr));
+
+		// And sweep
+		for mut to_free in inaccessible {
+			to_free.drop();
+		}
 	}
 }
 
@@ -61,14 +74,12 @@ impl Allocation {
 		let ptr = Box::into_raw(alloc) as *mut ();
 
 		let drop = |ptr| unsafe {
-			Box::from_raw(ptr as *mut T);
+			std::mem::drop(Box::from_raw(ptr as *mut T));
 		};
 
 		Self { ptr, drop }
 	}
-}
 
-impl Drop for Allocation {
 	fn drop(&mut self) {
 		unsafe { (self.drop)(self.ptr) };
 	}

gc/src/gc_ref.rs

@@ -1,4 +1,4 @@
-use std::{marker::PhantomData, ops::Deref, ptr::NonNull};
+use std::{marker::PhantomData, ptr::NonNull};
 
 use crate::trace::GCTrace;
 
@@ -10,23 +10,34 @@ impl<T: GCTrace> Clone for GcRef<T> {
 	}
 }
 
-impl<T: GCTrace> Deref for GcRef<T> {
-	type Target = T;
-
-	fn deref(&self) -> &Self::Target {
-		unsafe { self.0.as_ref() }
-	}
-}
-
 impl<T: GCTrace> GcRef<T> {
 	pub(crate) unsafe fn new(ptr: NonNull<T>) -> Self {
 		Self(ptr, PhantomData)
 	}
 
 	/// # Safety
-	/// Ensure that this is the only instance of a pointer to the underlying value.
-	/// You might want to instead use one of various [cell][`std::cell`] types as the allocated
-	/// type
+	/// The caller needs to ensure that the underlying pointer hasn't been garbage collected.
+	/// Since the drop order for garbage collected structs is undefined that means it is never
+	/// safe to call this function in a [Drop::drop].
+	///
+	/// Do note that this doesn't mean that any particular instance of GcRef has to be marked
+	/// as reachable during a call to [GCAllocator::gc][`crate::allocator::GCAllocator::gc`]
+	/// but instead any GcRef instance referring to the same underlying pointer has to be reachable
+	pub unsafe fn get(&self) -> &T {
+		unsafe { self.0.as_ref() }
+	}
+
+	/// # Safety
+	/// The caller needs to ensure that the underlying pointer hasn't been garbage collected.
+	/// See [GcRef::get] for more details
+	///
+	/// The caller needs to ensure that this is the only instance of a pointer to the underlying value
+	/// (in other words that [Clone] hasn't been called, or that all other clones of the pointer have
+	/// already been dropped).
+	///
+	/// This function is hard(but not impossible) to use without causing UB. Unless you have a
+	/// really special use case you might want to instead use one of various [cell][`std::cell`]
+	/// types as the allocated type.
 	pub unsafe fn get_mut(this: &mut Self) -> &mut T {
 		this.0.as_mut()
 	}

gc/src/lib.rs

@@ -11,11 +11,12 @@ pub mod test_utils;
 
 #[cfg(test)]
 pub(crate) mod tests {
+
 	use super::allocator::GCAllocator;
 	use super::test_utils::GotDropped;
 
 	#[test]
-	fn it_works() {
+	fn gc_allocates_and_frees_structs() {
 		let got_dropped = GotDropped::default();
 
 		let mut gc = GCAllocator::default();

gc/src/trace_impl.rs

@@ -7,6 +7,7 @@ unsafe impl GCTrace for i64 {}
 unsafe impl GCTrace for u64 {}
 unsafe impl GCTrace for f32 {}
 unsafe impl GCTrace for f64 {}
+unsafe impl GCTrace for bool {}
 unsafe impl GCTrace for isize {}
 unsafe impl GCTrace for usize {}
 unsafe impl GCTrace for String {}

src/interpreter/world.rs

@@ -20,10 +20,10 @@ impl Environment {
 	pub fn update_var(&mut self, name: &str, v: Primitive) -> Option<Primitive> {
 		if let Some(cur) = self.variables.get_mut(name) {
 			Some(std::mem::replace(cur, v))
+		} else if let Some(ref parent) = self.parent {
+			unsafe { parent.get().borrow_mut().update_var(name, v) }
 		} else {
-			self.parent
-				.as_ref()
-				.and_then(|parent| parent.borrow_mut().update_var(name, v))
+			None
 		}
 	}
 
@@ -34,9 +34,11 @@ impl Environment {
 
 	pub fn get_var(&self, name: &str) -> Option<Primitive> {
 		self.variables.get(name).cloned().or_else(|| {
-			self.parent
-				.as_ref()
-				.and_then(|v| (**v).borrow().get_var(name))
+			if let Some(ref parent) = self.parent {
+				unsafe { parent.get().borrow().get_var(name) }
+			} else {
+				None
+			}
 		})
 	}
 }
@@ -48,10 +50,10 @@ pub struct World {
 
 impl World {
 	pub fn set_var(&mut self, name: String, v: Primitive) -> Option<Primitive> {
-		self.env.borrow_mut().set_var(name, v)
+		unsafe { self.env.get().borrow_mut().set_var(name, v) }
 	}
 	pub fn get_var(&self, name: &str) -> Option<Primitive> {
-		self.env.borrow().get_var(name)
+		unsafe { self.env.get().borrow().get_var(name) }
 	}
 }