From c87c99a045a1fb4e54228f1e00eaf067b21033de Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Mon, 14 Jun 2021 21:54:32 -0700 Subject: [PATCH 1/6] Add Trace trait --- src/lib.rs | 2 ++ src/trace.rs | 7 +++++++ 2 files changed, 9 insertions(+) create mode 100644 src/trace.rs diff --git a/src/lib.rs b/src/lib.rs index fa382136c..913bcd037 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))] @@ -149,6 +150,7 @@ pub mod implementing_self_referential_data_structures; pub use adopt::Adopt; 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..87d5b53eb --- /dev/null +++ b/src/trace.rs @@ -0,0 +1,7 @@ +use crate::rc::Rc; + +pub trait Trace: Sized { + fn yield_owned_rcs(&self, mark: F) + where + F: for<'a> FnMut(&'a mut Rc); +} From 312b0b45f720f8d9d7f5f3e3138e1932f3419cef Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Mon, 14 Jun 2021 21:54:51 -0700 Subject: [PATCH 2/6] Add a safe adoption API based on the Trace trait --- src/adopt.rs | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/adopt.rs b/src/adopt.rs index ed473fee3..7938a7637 100644 --- a/src/adopt.rs +++ b/src/adopt.rs @@ -1,6 +1,7 @@ use core::ptr; use crate::link::Link; +use crate::trace::Trace; use crate::Rc; mod sealed { @@ -34,6 +35,13 @@ 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 { + /// The smart pointer's inner owned value. + type Inner; + + fn adopt(this: &mut Self, other: &Self) + where + Self::Inner: Trace; + /// Perform bookkeeping to record that `this` has an owned reference to /// `other`. /// @@ -78,6 +86,46 @@ 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; + + fn adopt(this: &mut Self, other: &Self) + 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); + } + } + } + /// Perform bookkeeping to record that `this` has an owned reference to /// `other`. /// From 9d6feb3551ec02a73509caf6eed2bebb43481702 Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Mon, 14 Jun 2021 22:05:24 -0700 Subject: [PATCH 3/6] Implement doubly linked list with forbid_unsafe --- tests/leak_doubly_linked_list.rs | 116 ++++++++++++++++++++----------- 1 file changed, 77 insertions(+), 39 deletions(-) diff --git a/tests/leak_doubly_linked_list.rs b/tests/leak_doubly_linked_list.rs index 2f0e208dc..19d755f21 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: for<'a> FnMut(&'a 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()); From b81dd3437eff04987c96cd801949e5ded5191831 Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Mon, 14 Jun 2021 22:07:34 -0700 Subject: [PATCH 4/6] TODO: revert me; stub docs to get build to compile without warnings for CI dry run --- src/adopt.rs | 2 ++ src/trace.rs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/adopt.rs b/src/adopt.rs index 7938a7637..d5d55bab7 100644 --- a/src/adopt.rs +++ b/src/adopt.rs @@ -38,6 +38,7 @@ pub unsafe trait Adopt: sealed::Sealed { /// The smart pointer's inner owned value. type Inner; + /// TODO: document me! fn adopt(this: &mut Self, other: &Self) where Self::Inner: Trace; @@ -90,6 +91,7 @@ unsafe impl Adopt for Rc { /// `T`. type Inner = T; + /// TODO: document me! fn adopt(this: &mut Self, other: &Self) where Self::Inner: Trace, diff --git a/src/trace.rs b/src/trace.rs index 87d5b53eb..94be255ec 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -1,6 +1,8 @@ use crate::rc::Rc; +/// TODO: document me! pub trait Trace: Sized { + /// TODO: document me! fn yield_owned_rcs(&self, mark: F) where F: for<'a> FnMut(&'a mut Rc); From bfae7c6bdf8f81b20d7e4ed08becfff9927f6250 Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Tue, 15 Jun 2021 06:54:59 -0700 Subject: [PATCH 5/6] Add Adopt::try_adopt --- src/adopt.rs | 33 ++++++++++++++++++++++++++++++--- src/lib.rs | 1 + 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/adopt.rs b/src/adopt.rs index d5d55bab7..cc15c2b18 100644 --- a/src/adopt.rs +++ b/src/adopt.rs @@ -1,3 +1,4 @@ +use core::fmt; use core::ptr; use crate::link::Link; @@ -13,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. /// @@ -34,12 +50,20 @@ 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) + 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; @@ -92,7 +116,7 @@ unsafe impl Adopt for Rc { type Inner = T; /// TODO: document me! - fn adopt(this: &mut Self, other: &Self) + fn try_adopt(this: &mut Self, other: &Self) -> Result where Self::Inner: Trace, { @@ -125,6 +149,9 @@ unsafe impl Adopt for Rc { unsafe { Self::adopt_unchecked(this, &node); } + Ok(node) + } else { + Err(AdoptError(())) } } diff --git a/src/lib.rs b/src/lib.rs index 913bcd037..1227dd5ed 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -148,6 +148,7 @@ mod trace; 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; From a96fd20d779374539e7bfad2f14b4adc60aa9509 Mon Sep 17 00:00:00 2001 From: Ryan Lopopolo Date: Tue, 15 Jun 2021 08:57:19 -0700 Subject: [PATCH 6/6] Remove higher-rank trait bound on mark function in Trace::yield_owned_rcs This ensures that yielded Rcs have the same lifetime as self, which is what we want to assert that self owns the Rc. --- src/trace.rs | 2 +- tests/leak_doubly_linked_list.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/trace.rs b/src/trace.rs index 94be255ec..bd1c4b606 100644 --- a/src/trace.rs +++ b/src/trace.rs @@ -5,5 +5,5 @@ pub trait Trace: Sized { /// TODO: document me! fn yield_owned_rcs(&self, mark: F) where - F: for<'a> FnMut(&'a mut Rc); + F: FnMut(&mut Rc); } diff --git a/tests/leak_doubly_linked_list.rs b/tests/leak_doubly_linked_list.rs index 19d755f21..20730b77b 100644 --- a/tests/leak_doubly_linked_list.rs +++ b/tests/leak_doubly_linked_list.rs @@ -37,7 +37,7 @@ struct Node { impl Trace for NodeCell { fn yield_owned_rcs(&self, mut mark: F) where - F: for<'a> FnMut(&'a mut Rc), + F: FnMut(&mut Rc), { if let Some(ref mut prev) = self.borrow_mut().prev { mark(prev);