From 2f690250aee38441aed4eb2ba4280ba53089e3c2 Mon Sep 17 00:00:00 2001
From: bad <badatnames@tutanota.com>
Date: Fri, 16 Sep 2022 23:27:18 +0200
Subject: [PATCH] Make deref on GcRef into an unsafe get to avoid unsoundness

---
 gc/src/allocator.rs  |  4 +++-
 gc/src/gc_ref.rs     | 35 +++++++++++++++++++++++------------
 gc/src/lib.rs        |  5 ++++-
 gc/src/trace_impl.rs |  1 +
 4 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/gc/src/allocator.rs b/gc/src/allocator.rs
index da98f0b..1959bba 100644
--- a/gc/src/allocator.rs
+++ b/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;
@@ -61,7 +63,7 @@ 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 }
diff --git a/gc/src/gc_ref.rs b/gc/src/gc_ref.rs
index 8c7ac57..da77d17 100644
--- a/gc/src/gc_ref.rs
+++ b/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()
 	}
diff --git a/gc/src/lib.rs b/gc/src/lib.rs
index 2fc164c..b8b1372 100644
--- a/gc/src/lib.rs
+++ b/gc/src/lib.rs
@@ -11,11 +11,14 @@ pub mod test_utils;
 
 #[cfg(test)]
 pub(crate) mod tests {
+
 	use super::allocator::GCAllocator;
+	use super::gc_ref::GcRef;
 	use super::test_utils::GotDropped;
+	use super::trace::GCTrace;
 
 	#[test]
-	fn it_works() {
+	fn gc_allocates_and_frees_structs() {
 		let got_dropped = GotDropped::default();
 
 		let mut gc = GCAllocator::default();
diff --git a/gc/src/trace_impl.rs b/gc/src/trace_impl.rs
index eab2547..4fb8dcb 100644
--- a/gc/src/trace_impl.rs
+++ b/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 {}