Merge pull request #32 from frengor/dev

* Update to 0.6.0
* Replace `Cc::into_inner` with `Cc::try_unwrap`, which now correctly handles weak pointers
* Remove `Cc::is_unique`
* Rework cleaners implementation

Author: frengor

CONTRIBUTING.md

@@ -52,16 +52,15 @@ Thus, marking should be done only:
 
 ## Writing tests
 
-Every unit test should start with a call to `tests::reset_state()` to make sure errors in other tests don't impact the current one.
-
-Also, functions marked as `pub(crate)` and used only in unit tests should have the `for_tests` suffix, like `Cc::new_for_tests`.
+Every unit test (which makes use of a part of the collector) should start with a call to `tests::reset_state()` to make
+sure errors in other tests don't impact the current one.
 
 ## Writing documentation
 
 Docs are always built with every feature enabled. This makes it easier to write and maintain the documentation.
 
-However, this also makes it more difficult to write examples, as those must pass CI even when some of the features they 
-require are disabled. As such, examples are marked as `ignore`d if a feature they need is missing:
+However, this also makes it more difficult to write examples, as those must pass CI even when a features they require is disabled.
+As such, examples are marked as `ignore`d if a feature they need is missing:
 ```rust
 #![cfg_attr(
     feature = "derive",

Cargo.toml

@@ -14,7 +14,7 @@ edition.workspace = true
 members = ["derive"]
 
 [workspace.package]
-version = "0.5.0" # Also update in [dependencies.rust-cc-derive.version]
+version = "0.6.0" # Also update in [dependencies.rust-cc-derive.version]
 authors = ["fren_gor <goro@frengor.com>"]
 repository = "https://github.com/frengor/rust-cc"
 categories = ["memory-management", "no-std"]
@@ -49,7 +49,7 @@ std = ["slotmap?/std", "thiserror/std"]
 pedantic-debug-assertions = []
 
 [dependencies]
-rust-cc-derive = { path = "./derive", version = "=0.5.0", optional = true }
+rust-cc-derive = { path = "./derive", version = "=0.6.0", optional = true }
 slotmap = {  version = "1.0", optional = true }
 thiserror = { version = "1.0", package = "thiserror-core", default-features = false }
 

src/cc.rs

@@ -2,7 +2,7 @@ use alloc::alloc::Layout;
 use alloc::rc::Rc;
 use core::cell::UnsafeCell;
 use core::marker::PhantomData;
-use core::mem;
+use core::mem::ManuallyDrop;
 use core::ops::Deref;
 use core::ptr::{self, drop_in_place, NonNull};
 use core::borrow::Borrow;
@@ -66,32 +66,52 @@ impl<T: Trace> Cc<T> {
         })
     }
 
-    /// Takes out the value inside a [`Cc`].
-    ///
-    /// # Panics
-    /// Panics if the [`Cc`] is not unique (see [`is_unique`]).
-    ///
-    /// [`is_unique`]: fn@Cc::is_unique
+    /// Returns the inner value, if the [`Cc`] has exactly one strong reference and the collector is not collecting, finalizing or dropping.
+    /// 
+    /// Otherwise, an [`Err`] is returned with the same [`Cc`] this method was called on.
+    /// 
+    /// This will succeed even if there are outstanding weak references.
     #[inline]
     #[track_caller]
-    pub fn into_inner(self) -> T {
-        assert!(self.is_unique(), "Cc<_> is not unique");
+    pub fn try_unwrap(self) -> Result<T, Self> {
+        let cc = ManuallyDrop::new(self); // Never drop the Cc
 
-        assert!(
-            !self.counter_marker().is_in_list_or_queue(),
-            "Cc<_> is being used by the collector and inner value cannot be taken out (this might have happen inside Trace, Finalize or Drop implementations)."
-        );
+        if cc.strong_count() != 1 {
+            // No need to access the state here
+            return Err(ManuallyDrop::into_inner(cc));
+        }
 
-        // Make sure self is not into POSSIBLE_CYCLES before deallocating
-        remove_from_list(self.inner.cast());
+        let res = try_state(|state| {
+            if state.is_collecting() || state.is_dropping() {
+                return None;
+            }
 
-        // SAFETY: self is unique and is not inside any list
-        unsafe {
-            let t = ptr::read(self.inner().get_elem());
-            let layout = self.inner().layout();
-            let _ = try_state(|state| cc_dealloc(self.inner, layout, state));
-            mem::forget(self); // Don't call drop on this Cc
-            t
+            #[cfg(feature = "finalization")]
+            if state.is_finalizing() {
+                // To make this method behave the same outside of collections, unwrapping while finalizing is prohibited
+                return None;
+            }
+
+            remove_from_list(cc.inner.cast());
+
+            // Disable upgrading weak ptrs
+            #[cfg(feature = "weak-ptrs")]
+            cc.inner().drop_metadata();
+            // There's no reason here to call CounterMarker::set_dropped, since the pointed value will not be dropped
+
+            // SAFETY: cc is unique and no weak pointer can be upgraded
+            let t = unsafe { ptr::read(cc.inner().get_elem()) };
+            let layout = cc.inner().layout();
+            // SAFETY: cc is unique, is not inside any list and no weak pointer can be upgraded
+            unsafe {
+                cc_dealloc(cc.inner, layout, state);
+            }
+            Some(t)
+        });
+
+        match res {
+            Ok(Some(t)) => Ok(t),
+            _ => Err(ManuallyDrop::into_inner(cc)),
         }
     }
 }
@@ -109,12 +129,6 @@ impl<T: ?Sized + Trace> Cc<T> {
         self.counter_marker().counter() as u32
     }
 
-    /// Returns `true` if the strong reference count is `1`, `false` otherwise.
-    #[inline]
-    pub fn is_unique(&self) -> bool {
-        self.strong_count() == 1
-    }
-
     /// Makes the value in the managed allocation finalizable again.
     /// 
     /// # Panics

src/cleaners/mod.rs

@@ -25,8 +25,8 @@
 //! As such, *when possible* it's suggested to prefer cleaners and disable finalization.
 
 use alloc::boxed::Box;
-use core::cell::RefCell;
 use core::fmt::{self, Debug, Formatter};
+use core::cell::{RefCell, UnsafeCell};
 use slotmap::{new_key_type, SlotMap};
 
 use crate::{Cc, Context, Finalize, Trace};
@@ -37,7 +37,7 @@ new_key_type! {
 }
 
 struct CleanerMap {
-    map: RefCell<Option<SlotMap<CleanerKey, CleaningAction>>>, // The Option is used to avoid allocating until a cleaning action is registered
+    map: RefCell<SlotMap<CleanerKey, CleaningAction>>,
 }
 
 unsafe impl Trace for CleanerMap {
@@ -63,17 +63,15 @@ impl Drop for CleaningAction {
 ///
 /// All the cleaning actions registered in a `Cleaner` are run when it is dropped, unless they have been manually executed before.
 pub struct Cleaner {
-    cleaner_map: Cc<CleanerMap>,
+    cleaner_map: UnsafeCell<Option<Cc<CleanerMap>>>, // The Option is used to avoid allocating until a cleaning action is registered
 }
 
 impl Cleaner {
     /// Creates a new [`Cleaner`].
     #[inline]
     pub fn new() -> Cleaner {
         Cleaner {
-            cleaner_map: Cc::new(CleanerMap {
-                map: RefCell::new(None),
-            }),
+            cleaner_map: UnsafeCell::new(None),
         }
     }
 
@@ -87,21 +85,27 @@ impl Cleaner {
     /// be leaked and the cleaning action will never be executed.
     #[inline]
     pub fn register(&self, action: impl FnOnce() + 'static) -> Cleanable {
-        let map_key = self.cleaner_map
-            .map
-            .borrow_mut()
-            .get_or_insert_with(|| SlotMap::with_capacity_and_key(3))
-            .insert(CleaningAction(Some(Box::new(action))));
+        let cc = {
+            // SAFETY: no reference to the Option already exists
+            let map = unsafe { &mut *self.cleaner_map.get() };
+
+            map.get_or_insert_with(|| Cc::new(CleanerMap {
+                map: RefCell::new(SlotMap::with_capacity_and_key(3)),
+            }))
+        };
+
+        let map_key = cc.map.borrow_mut().insert(CleaningAction(Some(Box::new(action))));
 
         Cleanable {
-            cleaner_map: self.cleaner_map.downgrade(),
+            cleaner_map: cc.downgrade(),
             key: map_key,
         }
     }
 
     #[cfg(all(test, feature = "std"))] // Only used in unit tests
     pub(crate) fn has_allocated(&self) -> bool {
-        self.cleaner_map.map.borrow().is_some()
+        // SAFETY: no reference to the Option already exists
+        unsafe { (*self.cleaner_map.get()).is_some() }
     }
 }
 
@@ -146,10 +150,11 @@ impl Cleanable {
         // Try upgrading to see if the CleanerMap hasn't been deallocated
         let Some(cc) = self.cleaner_map.upgrade() else { return };
 
-        // Just return in case try_borrow_mut fails or the map is None
-        // (the latter shouldn't happen, but better be sure)
-        let Ok(mut ref_mut) = cc.map.try_borrow_mut() else { return };
-        let Some(map) = &mut *ref_mut else { return };
+        // Just return in case try_borrow_mut fails
+        let Ok(mut map) = cc.map.try_borrow_mut() else {
+            crate::utils::cold(); // Should never happen
+            return;
+        };
         let _ = map.remove(self.key);
     }
 }

src/tests/cc.rs

@@ -448,3 +448,116 @@ fn buffered_objects_count_test() {
     drop(cc);
     collect_cycles();
 }
+
+#[test]
+fn try_unwrap_test() {
+    reset_state();
+
+    let cc = Cc::new(5u32);
+    
+    #[cfg(feature = "weak-ptrs")]
+    let weak = cc.downgrade();
+    
+    let unwrapped = cc.try_unwrap();
+    assert_eq!(5, unwrapped.unwrap());
+
+    #[cfg(feature = "weak-ptrs")]
+    assert!(weak.upgrade().is_none());
+}
+
+#[test]
+fn fail_try_unwrap_test() {
+    reset_state();
+
+    let cc = Cc::new(5u32);
+    let copy = cc.clone();
+
+    #[cfg(feature = "weak-ptrs")]
+    let weak = cc.downgrade();
+
+    assert!(cc.try_unwrap().is_err()); // cc dropped here
+
+    #[cfg(feature = "weak-ptrs")]
+    assert!(weak.upgrade().is_some());
+
+    let unwrapped = copy.try_unwrap();
+    assert_eq!(5, unwrapped.unwrap());
+
+    #[cfg(feature = "weak-ptrs")]
+    assert!(weak.upgrade().is_none());
+}
+
+#[cfg(feature = "finalization")]
+#[test]
+fn finalization_try_unwrap_test() {
+    reset_state();
+
+    struct Finalizable {
+        other: RefCell<Option<Cc<u32>>>,
+    }
+
+    unsafe impl Trace for Finalizable {
+        fn trace(&self, ctx: &mut Context<'_>) {
+            self.other.trace(ctx);
+        }
+    }
+
+    impl Finalize for Finalizable {
+        fn finalize(&self) {
+            let res = self.other.take().unwrap().try_unwrap();
+            match res {
+                Err(cc) => assert_eq!(5, *cc),
+                _ => panic!("try_unwrap returned an Ok(...) value during finalization."),
+            }
+        }
+    }
+
+    let _ = Cc::new(Finalizable {
+        other: RefCell::new(Some(Cc::new(5u32))),
+    });
+}
+
+#[cfg(feature = "finalization")]
+#[test]
+fn cyclic_finalization_try_unwrap_test() {
+    reset_state();
+
+    thread_local! {
+        static FINALIZED: Cell<bool> = Cell::new(false);
+    }
+
+    struct Cyclic {
+        cyclic: RefCell<Option<Cc<Self>>>,
+        other: RefCell<Option<Cc<u32>>>,
+    }
+
+    unsafe impl Trace for Cyclic {
+        fn trace(&self, ctx: &mut Context<'_>) {
+            self.cyclic.trace(ctx);
+            self.other.trace(ctx);
+        }
+    }
+
+    impl Finalize for Cyclic {
+        fn finalize(&self) {
+            FINALIZED.with(|fin| fin.set(true));
+            state(|state| assert!(state.is_collecting()));
+
+            let res = self.other.take().unwrap().try_unwrap();
+            match res {
+                Err(cc) => assert_eq!(5, *cc),
+                _ => panic!("try_unwrap returned an Ok(...) value during collection."),
+            }
+        }
+    }
+
+    let cc = Cc::new(Cyclic {
+        cyclic: RefCell::new(None),
+        other: RefCell::new(Some(Cc::new(5u32))),
+    });
+    *cc.cyclic.borrow_mut() = Some(cc.clone());
+    drop(cc);
+    collect_cycles();
+    
+    FINALIZED.with(|fin| assert!(fin.get()));
+}