From 9240ad12ee03033e7f20df4642461b3e9449205d Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 20 Sep 2022 15:31:18 +0800 Subject: [PATCH] fix: add safety comment to rle --- crates/loro-core/Cargo.toml | 1 + crates/rle/Cargo.toml | 1 + crates/rle/src/lib.rs | 1 + crates/rle/src/range_map.rs | 4 ++ crates/rle/src/rle_tree/cursor.rs | 43 +++++++++++++++---- crates/rle/src/rle_tree/node.rs | 18 +++++--- crates/rle/src/rle_tree/node/internal_impl.rs | 23 ++++++---- crates/rle/src/rle_tree/node/leaf_impl.rs | 10 ++++- crates/rle/src/rle_tree/test/notify_test.rs | 2 + 9 files changed, 78 insertions(+), 25 deletions(-) diff --git a/crates/loro-core/Cargo.toml b/crates/loro-core/Cargo.toml index dd43c690..e508b8e0 100644 --- a/crates/loro-core/Cargo.toml +++ b/crates/loro-core/Cargo.toml @@ -24,6 +24,7 @@ num = "0.4.0" proptest = "1.0.0" proptest-derive = "0.3.0" rand = "0.8.5" +static_assertions = "1.1.0" # See https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html [lib] diff --git a/crates/rle/Cargo.toml b/crates/rle/Cargo.toml index e93b4099..08b83089 100644 --- a/crates/rle/Cargo.toml +++ b/crates/rle/Cargo.toml @@ -17,3 +17,4 @@ ctor = "0.1.23" proptest = "1.0.0" smartstring = "1.0.1" rand = "0.8.5" +static_assertions = "1.1.0" diff --git a/crates/rle/src/lib.rs b/crates/rle/src/lib.rs index caed08eb..d1bc35a1 100644 --- a/crates/rle/src/lib.rs +++ b/crates/rle/src/lib.rs @@ -20,6 +20,7 @@ //! //! #![allow(dead_code)] +#![deny(clippy::undocumented_unsafe_blocks)] pub mod range_map; mod rle_trait; pub mod rle_tree; diff --git a/crates/rle/src/range_map.rs b/crates/rle/src/range_map.rs index 89d60ae7..7c78d62a 100644 --- a/crates/rle/src/range_map.rs +++ b/crates/rle/src/range_map.rs @@ -152,6 +152,8 @@ impl Mergable for WithStartEnd #[cfg(test)] mod test { + use std::ops::Range; + use super::*; #[derive(Debug, PartialEq, Eq)] struct V { @@ -205,4 +207,6 @@ mod test { map.set(5, V::new(5, 20)); assert_eq!(map.get_range(9, 15), vec![&V::new(5, 20)]); } + + static_assertions::assert_not_impl_any!(RangeMap>: Sync, Send); } diff --git a/crates/rle/src/rle_tree/cursor.rs b/crates/rle/src/rle_tree/cursor.rs index 968a76c8..2c0c1797 100644 --- a/crates/rle/src/rle_tree/cursor.rs +++ b/crates/rle/src/rle_tree/cursor.rs @@ -62,6 +62,9 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> UnsafeCursor<'tree, 'bump, } } + /// # Safety + /// + /// we need to make sure that the cursor is still valid #[inline] pub unsafe fn as_ref(&self) -> &'tree T { self.leaf.as_ref().children[self.index] @@ -86,6 +89,9 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> UnsafeCursor<'tree, 'bump, } } + /// # Safety + /// + /// we need to make sure that the cursor is still valid pub unsafe fn next(&self) -> Option { let leaf = self.leaf.as_ref(); if leaf.children.len() > self.index + 1 { @@ -95,6 +101,9 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> UnsafeCursor<'tree, 'bump, leaf.next.map(|next| Self::new(next, 0, 0, Position::Start)) } + /// # Safety + /// + /// we need to make sure that the cursor is still valid pub unsafe fn prev(&self) -> Option { let leaf = self.leaf.as_ref(); if self.index > 0 { @@ -109,6 +118,7 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> UnsafeCursor<'tree, 'bump, impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> AsRef for SafeCursor<'tree, 'bump, T, A> { #[inline] fn as_ref(&self) -> &'tree T { + // SAFETY: SafeCursor is a shared reference to the tree unsafe { self.0.as_ref() } } } @@ -116,21 +126,25 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> AsRef for SafeCursor<'t impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> SafeCursor<'tree, 'bump, T, A> { #[inline] pub fn as_tree_ref(&self) -> &'tree T { + // SAFETY: SafeCursor is a shared reference to the tree unsafe { self.0.as_ref() } } #[inline] pub fn next(&self) -> Option { + // SAFETY: SafeCursor is a shared reference to the tree unsafe { self.0.next().map(|x| Self(x)) } } #[inline] pub fn prev(&self) -> Option { + // SAFETY: SafeCursor is a shared reference to the tree unsafe { self.0.prev().map(|x| Self(x)) } } #[inline] pub fn leaf(&self) -> &'tree LeafNode<'bump, T, A> { + // SAFETY: SafeCursor has shared reference lifetime to the tree unsafe { self.0.leaf.as_ref() } } @@ -170,6 +184,7 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> AsRef { #[inline] fn as_ref(&self) -> &T { + // SAFETY: SafeCursorMut is a exclusive reference to the tree unsafe { self.0.as_ref() } } } @@ -177,11 +192,13 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> AsRef impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> SafeCursorMut<'tree, 'bump, T, A> { #[inline] pub fn as_ref_(&self) -> &'tree T { + // SAFETY: SafeCursorMut is a exclusive reference to the tree unsafe { self.0.as_ref() } } #[inline] pub fn leaf(&self) -> &'tree LeafNode<'bump, T, A> { + // SAFETY: SafeCursorMut is a exclusive reference to the tree unsafe { self.0.leaf.as_ref() } } @@ -204,19 +221,23 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> SafeCursorMut<'tree, 'bump #[inline] fn as_tree_mut(&mut self) -> &'tree mut T { + // SAFETY: SafeCursorMut is a exclusive reference to the tree unsafe { self.0.as_mut() } } #[inline] pub fn update_cache_recursively(&mut self) { - let leaf = unsafe { self.0.leaf.as_mut() }; - A::update_cache_leaf(leaf); - let mut node = unsafe { leaf.parent.as_mut() }; - loop { - A::update_cache_internal(node); - match node.parent { - Some(mut parent) => node = unsafe { parent.as_mut() }, - None => return, + // SAFETY: SafeCursorMut is a exclusive reference to the tree + unsafe { + let leaf = self.0.leaf.as_mut(); + A::update_cache_leaf(leaf); + let mut node = leaf.parent.as_mut(); + loop { + A::update_cache_internal(node); + match node.parent { + Some(mut parent) => node = parent.as_mut(), + None => return, + } } } } @@ -228,11 +249,15 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> SafeCursorMut<'tree, 'bump #[inline] pub fn next(&self) -> Option { + // SAFETY: SafeCursorMut is a exclusive reference to the tree so we are safe to + // get a reference to the element unsafe { self.0.next().map(|x| Self(x)) } } #[inline] pub fn prev(&self) -> Option { + // SAFETY: SafeCursorMut is a exclusive reference to the tree so we are safe to + // get a reference to the element unsafe { self.0.prev().map(|x| Self(x)) } } @@ -257,6 +282,8 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait> AsMut { #[inline] fn as_mut(&mut self) -> &mut T { + // SAFETY: SafeCursorMut is a exclusive reference to the tree so we are safe to + // get a exclusive reference to the element unsafe { self.0.as_mut() } } } diff --git a/crates/rle/src/rle_tree/node.rs b/crates/rle/src/rle_tree/node.rs index da775d14..2f248bc5 100644 --- a/crates/rle/src/rle_tree/node.rs +++ b/crates/rle/src/rle_tree/node.rs @@ -88,17 +88,23 @@ impl<'a, T: Rle, A: RleTreeTrait> Node<'a, T, A> { #[inline] pub(crate) fn parent_mut(&mut self) -> Option<&mut InternalNode<'a, T, A>> { - match self { - Node::Internal(node) => unsafe { node.parent.map(|mut x| x.as_mut()) }, - Node::Leaf(node) => Some(unsafe { node.parent.as_mut() }), + // SAFETY: all tree data is pinned and can only be operating on single thread + unsafe { + match self { + Node::Internal(node) => node.parent.map(|mut x| x.as_mut()), + Node::Leaf(node) => Some(node.parent.as_mut()), + } } } #[inline] pub(crate) fn parent(&self) -> Option<&InternalNode<'a, T, A>> { - match self { - Node::Internal(node) => node.parent.map(|x| unsafe { x.as_ref() }), - Node::Leaf(node) => Some(unsafe { node.parent.as_ref() }), + // SAFETY: all tree data is pinned and can only be operating on single thread + unsafe { + match self { + Node::Internal(node) => node.parent.map(|x| x.as_ref()), + Node::Leaf(node) => Some(node.parent.as_ref()), + } } } diff --git a/crates/rle/src/rle_tree/node/internal_impl.rs b/crates/rle/src/rle_tree/node/internal_impl.rs index 2b55348f..a70f2923 100644 --- a/crates/rle/src/rle_tree/node/internal_impl.rs +++ b/crates/rle/src/rle_tree/node/internal_impl.rs @@ -220,13 +220,16 @@ impl<'a, T: Rle, A: RleTreeTrait> InternalNode<'a, T, A> { let next = self.children[right_index] .get_last_leaf() .and_then(|x| x.next); - if let Some(mut prev) = prev { - let prev = unsafe { prev.as_mut() }; - prev.next = next; - } - if let Some(mut next) = next { - let next = unsafe { next.as_mut() }; - next.prev = prev; + // SAFETY: rle_tree is single threaded + unsafe { + if let Some(mut prev) = prev { + let prev = prev.as_mut(); + prev.next = next; + } + if let Some(mut next) = next { + let next = next.as_mut(); + next.prev = prev; + } } } @@ -372,12 +375,13 @@ impl<'a, T: Rle, A: RleTreeTrait> InternalNode<'a, T, A> { let mut any_delete: bool; loop { any_delete = false; - for (_, mut node_ptr) in zipper.iter() { + for (_, mut node_ptr) in zipper.iter_mut() { if should_skip.contains(&node_ptr) { continue; } - let node = unsafe { node_ptr.as_mut() }; + // SAFETY: node_ptr points to a valid descendant of self + let node: &mut Node<'a, T, A> = unsafe { node_ptr.as_mut() }; if let Some(node) = node.as_internal() { let ptr = node as *const InternalNode<'a, T, A>; if removed.contains(&ptr) { @@ -395,6 +399,7 @@ impl<'a, T: Rle, A: RleTreeTrait> InternalNode<'a, T, A> { if let Some((sibling, either)) = node.get_a_sibling() { // if has sibling, borrow or merge to it let sibling: &mut Node<'a, T, A> = + // SAFETY: node's sibling points to a valid descendant of self unsafe { &mut *((sibling as *const _) as usize as *mut _) }; if node.children_num() + sibling.children_num() <= A::MAX_CHILDREN_NUM { node.merge_to_sibling(sibling, either, notify); diff --git a/crates/rle/src/rle_tree/node/leaf_impl.rs b/crates/rle/src/rle_tree/node/leaf_impl.rs index 5b2ea9e5..07d3668f 100644 --- a/crates/rle/src/rle_tree/node/leaf_impl.rs +++ b/crates/rle/src/rle_tree/node/leaf_impl.rs @@ -42,6 +42,7 @@ impl<'bump, T: Rle, A: RleTreeTrait> LeafNode<'bump, T, A> { ans_inner.next = self.next; ans_inner.prev = Some(NonNull::new(self).unwrap()); if let Some(mut next) = self.next { + // SAFETY: ans_inner is a valid pointer unsafe { next.as_mut().prev = Some(NonNull::new_unchecked(ans_inner)) }; } self.next = Some(NonNull::new(&mut *ans_inner).unwrap()); @@ -98,10 +99,12 @@ impl<'bump, T: Rle, A: RleTreeTrait> LeafNode<'bump, T, A> { assert!(self.children.len() <= A::MAX_CHILDREN_NUM); A::check_cache_leaf(self); if let Some(next) = self.next { + // SAFETY: this is only for testing, and next must be a valid pointer let self_ptr = unsafe { next.as_ref().prev.unwrap().as_ptr() }; assert!(std::ptr::eq(self, self_ptr)); } if let Some(prev) = self.prev { + // SAFETY: this is only for testing, and prev must be a valid pointer let self_ptr = unsafe { prev.as_ref().next.unwrap().as_ptr() }; assert!(std::ptr::eq(self, self_ptr)); } @@ -128,6 +131,7 @@ impl<'bump, T: Rle, A: RleTreeTrait> LeafNode<'bump, T, A> { } pub fn is_deleted(&self) -> bool { + // SAFETY: we used bumpalo here, so even if current node is deleted we unsafe { let mut node = self.parent.as_ref(); if !node @@ -269,12 +273,14 @@ impl<'bump, T: Rle, A: RleTreeTrait> LeafNode<'bump, T, A> { #[inline] pub fn next(&self) -> Option<&Self> { - self.next.map(|p| unsafe { p.as_ref() }) + // SAFETY: internal variant ensure prev and next are valid reference + unsafe { self.next.map(|p| p.as_ref()) } } #[inline] pub fn prev(&self) -> Option<&Self> { - self.prev.map(|p| unsafe { p.as_ref() }) + // SAFETY: internal variant ensure prev and next are valid reference + unsafe { self.prev.map(|p| p.as_ref()) } } #[inline] diff --git a/crates/rle/src/rle_tree/test/notify_test.rs b/crates/rle/src/rle_tree/test/notify_test.rs index 9733af1e..9c74f7be 100644 --- a/crates/rle/src/rle_tree/test/notify_test.rs +++ b/crates/rle/src/rle_tree/test/notify_test.rs @@ -91,6 +91,7 @@ fn test(interactions: &[Interaction]) { let mut range_map: RangeMap> = Default::default(); for interaction in interactions.iter() { let mut func = |value: &Value, node: *mut LeafNode<'_, Value, ValueTreeTrait>| { + // SAFETY: this is safe because node must be valid let ptr = unsafe { NonNull::new_unchecked(node as usize as *mut _) }; range_map.set( value.value, @@ -126,6 +127,7 @@ fn test(interactions: &[Interaction]) { origin_value, range ); + // SAFETY: this is a test assert!(!unsafe { origin_cursor.0.leaf.as_ref().is_deleted() }); let origin_leaf_ptr = origin_cursor.0.leaf.as_ptr() as usize; let range_map_ptr = range_map_out.value.as_ptr() as usize;