diff --git a/src/adopt.rs b/src/adopt.rs index ed473fee3..cc15c2b18 100644 --- a/src/adopt.rs +++ b/src/adopt.rs @@ -1,6 +1,8 @@ +use core::fmt; use core::ptr; use crate::link::Link; +use crate::trace::Trace; use crate::Rc; mod sealed { @@ -12,6 +14,21 @@ mod sealed { impl Sealed for Rc {} } +/// The error type for `try_adopt` methods. +/// +/// See [`Adopt::try_adopt`] for more information. +#[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, PartialOrd, Ord)] +pub struct AdoptError(()); + +#[cfg(feature = "std")] +impl std::error::Error for AdoptError {} + +impl fmt::Display for AdoptError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str("Rc adoption failed because the Rc does not own a pointer to the adoptee") + } +} + /// Build a graph of linked [`Rc`] smart pointers to enable busting cycles on /// drop. /// @@ -33,7 +50,23 @@ mod sealed { /// /// [`adopt_unchecked`]: Adopt::adopt_unchecked /// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html -pub unsafe trait Adopt: sealed::Sealed { +pub unsafe trait Adopt: sealed::Sealed + Sized { + /// The smart pointer's inner owned value. + type Inner; + + /// TODO: document me! + fn adopt(this: &mut Self, other: &Self) -> Self + where + Self::Inner: Trace, + { + Self::try_adopt(this, other).unwrap_or_else(|err| panic!("{}", err)) + } + + /// TODO: document me! + fn try_adopt(this: &mut Self, other: &Self) -> Result + where + Self::Inner: Trace; + /// Perform bookkeeping to record that `this` has an owned reference to /// `other`. /// @@ -78,6 +111,50 @@ pub unsafe trait Adopt: sealed::Sealed { /// Implementation of [`Adopt`] for [`Rc`] which enables `Rc`s to form a cycle /// of strong references that are reaped by `Rc`'s [`Drop`] implementation. unsafe impl Adopt for Rc { + /// `Rc`'s inner owned value. For an `Rc`, the inner owned value is a + /// `T`. + type Inner = T; + + /// TODO: document me! + fn try_adopt(this: &mut Self, other: &Self) -> Result + where + Self::Inner: Trace, + { + // Use internal iteration on `this`'s owned `Rc`s to look for `other`. + // + // If `this` can yield a mutable reference to an `Rc` that has the same + // inner allocation as `other`, we can safely assert that `this` owns + // an `Rc` with the same `RcBox` as `other`. + let needle = other.inner() as *const _; + let mut found = None; + Trace::yield_owned_rcs(this.as_ref(), |node| { + if found.is_some() { + return; + } + // If `this` yields an `Rc` with the same `RcBox` as the given + // `other`, `this` owns a `Rc` that points to the same allocation as + // `other`, which fulfills the safety invariant of `adopt_unchecked`. + if ptr::eq(needle, node.inner()) { + // Clone the node we've found that matches the needle so we can + // save a reference to the `RcBox` we want to adopt. + found = Some(Rc::clone(node)); + } + }); + if let Some(node) = found { + // SAFETY: `yield_owned_rcs` yielded a mutable reference to an `Rc` + // matching `other`'s inner allocation, which means `this` must own + // an `Rc` matching `other`. + // + // This uphold's adopt_unchecked's safety invariant. + unsafe { + Self::adopt_unchecked(this, &node); + } + Ok(node) + } else { + Err(AdoptError(())) + } + } + /// Perform bookkeeping to record that `this` has an owned reference to /// `other`. /// diff --git a/src/lib.rs b/src/lib.rs index fa382136c..1227dd5ed 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -139,6 +139,7 @@ mod drop; mod hash; mod link; mod rc; +mod trace; // Doc modules #[cfg(any(doctest, docsrs))] @@ -147,8 +148,10 @@ mod rc; pub mod implementing_self_referential_data_structures; pub use adopt::Adopt; +pub use adopt::AdoptError; pub use rc::Rc; pub use rc::Weak; +pub use trace::Trace; /// Cactus alias for [`Rc`]. pub type CactusRef = Rc; diff --git a/src/trace.rs b/src/trace.rs new file mode 100644 index 000000000..bd1c4b606 --- /dev/null +++ b/src/trace.rs @@ -0,0 +1,9 @@ +use crate::rc::Rc; + +/// TODO: document me! +pub trait Trace: Sized { + /// TODO: document me! + fn yield_owned_rcs(&self, mark: F) + where + F: FnMut(&mut Rc); +} diff --git a/tests/leak_doubly_linked_list.rs b/tests/leak_doubly_linked_list.rs index 2f0e208dc..20730b77b 100644 --- a/tests/leak_doubly_linked_list.rs +++ b/tests/leak_doubly_linked_list.rs @@ -1,48 +1,83 @@ #![warn(clippy::all)] #![warn(clippy::pedantic)] #![allow(clippy::shadow_unrelated)] +#![forbid(unsafe_code)] -use cactusref::{Adopt, Rc}; +use cactusref::{Adopt, Rc, Trace}; use core::cell::RefCell; use core::iter; +use core::ops::Deref; + +struct NodeCell(RefCell>); + +impl NodeCell { + fn new(data: T) -> Self { + Self(RefCell::new(Node { + prev: None, + next: None, + data, + })) + } +} + +impl Deref for NodeCell { + type Target = RefCell>; + + fn deref(&self) -> &Self::Target { + &self.0 + } +} struct Node { - pub prev: Option>>, - pub next: Option>>, + pub prev: Option>>, + pub next: Option>>, pub data: T, } +impl Trace for NodeCell { + fn yield_owned_rcs(&self, mut mark: F) + where + F: FnMut(&mut Rc), + { + if let Some(ref mut prev) = self.borrow_mut().prev { + mark(prev); + } + if let Some(ref mut next) = self.borrow_mut().next { + mark(next); + } + } +} + struct List { - pub head: Option>>>, + pub head: Option>>, } impl List { - fn pop(&mut self) -> Option>>> { + fn pop(&mut self) -> Option>> { let head = self.head.take()?; - let tail = head.borrow_mut().prev.take(); - let next = head.borrow_mut().next.take(); - if let Some(ref tail) = tail { + let mut tail = head.borrow_mut().prev.take(); + let mut next = head.borrow_mut().next.take(); + + if let Some(ref mut tail) = tail { Rc::unadopt(&head, tail); Rc::unadopt(tail, &head); tail.borrow_mut().next = next.as_ref().map(Rc::clone); if let Some(ref next) = next { - unsafe { - Rc::adopt_unchecked(tail, next); - } + Rc::adopt(tail, next); } } - if let Some(ref next) = next { + + if let Some(ref mut next) = next { Rc::unadopt(&head, next); Rc::unadopt(next, &head); next.borrow_mut().prev = tail.as_ref().map(Rc::clone); if let Some(ref tail) = tail { - unsafe { - Rc::adopt_unchecked(next, tail); - } + Rc::adopt(next, tail); } } + self.head = next; Some(head) } @@ -50,35 +85,36 @@ impl List { impl From> for List { fn from(list: Vec) -> Self { - let nodes = list + if list.is_empty() { + return Self { head: None }; + } + let mut nodes = list .into_iter() - .map(|data| { - Rc::new(RefCell::new(Node { - prev: None, - next: None, - data, - })) - }) + .map(|data| Rc::new(NodeCell::new(data))) .collect::>(); + for i in 0..nodes.len() - 1 { - let curr = &nodes[i]; - let next = &nodes[i + 1]; - curr.borrow_mut().next = Some(Rc::clone(next)); - next.borrow_mut().prev = Some(Rc::clone(curr)); - unsafe { - Rc::adopt_unchecked(curr, next); - Rc::adopt_unchecked(next, curr); - } - } - let tail = &nodes[nodes.len() - 1]; - let head = &nodes[0]; - tail.borrow_mut().next = Some(Rc::clone(head)); - head.borrow_mut().prev = Some(Rc::clone(tail)); - unsafe { - Rc::adopt_unchecked(tail, head); - Rc::adopt_unchecked(head, tail); + let next = Rc::clone(&nodes[i + 1]); + let curr = &mut nodes[i]; + curr.borrow_mut().next = Some(Rc::clone(&next)); + Rc::adopt(curr, &next); + + let curr = Rc::clone(&nodes[i]); + let next = &mut nodes[i + 1]; + next.borrow_mut().prev = Some(Rc::clone(&curr)); + Rc::adopt(next, &curr); } + let head = Rc::clone(&nodes[0]); + let tail = nodes.last_mut().unwrap(); + tail.borrow_mut().next = Some(Rc::clone(&head)); + Rc::adopt(tail, &head); + + let tail = Rc::clone(&nodes[nodes.len() - 1]); + let head = &mut nodes[0]; + head.borrow_mut().prev = Some(Rc::clone(&tail)); + Rc::adopt(head, &tail); + let head = Rc::clone(head); Self { head: Some(head) } } @@ -95,10 +131,12 @@ fn leak_doubly_linked_list() { .take(10) .collect::>(); let mut list = List::from(list); + let head = list.pop().unwrap(); assert!(head.borrow().data.starts_with('a')); assert_eq!(Rc::strong_count(&head), 1); assert_eq!(list.head.as_ref().map(Rc::strong_count), Some(3)); + let weak = Rc::downgrade(&head); drop(head); assert!(weak.upgrade().is_none());