fix: add safety comment to rle

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]

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"

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;

crates/rle/src/range_map.rs

@@ -152,6 +152,8 @@ impl<Index: GlobalIndex, T: PartialEq + Eq> Mergable for WithStartEnd<Index, T>
 
 #[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<usize, Range<usize>>: Sync, Send);
 }

crates/rle/src/rle_tree/cursor.rs

@@ -62,6 +62,9 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> 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<T>> UnsafeCursor<'tree, 'bump,
         }
     }
 
+    /// # Safety
+    ///
+    /// we need to make sure that the cursor is still valid
     pub unsafe fn next(&self) -> Option<Self> {
         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<T>> 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<Self> {
         let leaf = self.leaf.as_ref();
         if self.index > 0 {
@@ -109,6 +118,7 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> UnsafeCursor<'tree, 'bump,
 impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> AsRef<T> 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<T>> AsRef<T> for SafeCursor<'t
 impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> 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<Self> {
+        // SAFETY: SafeCursor is a shared reference to the tree
         unsafe { self.0.next().map(|x| Self(x)) }
     }
 
     #[inline]
     pub fn prev(&self) -> Option<Self> {
+        // 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<T>> AsRef<T>
 {
     #[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<T>> AsRef<T>
 impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> 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<T>> 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() };
+        // SAFETY: SafeCursorMut is a exclusive reference to the tree
-        A::update_cache_leaf(leaf);
+        unsafe {
-        let mut node = unsafe { leaf.parent.as_mut() };
+            let leaf = self.0.leaf.as_mut();
-        loop {
+            A::update_cache_leaf(leaf);
-            A::update_cache_internal(node);
+            let mut node = leaf.parent.as_mut();
-            match node.parent {
+            loop {
-                Some(mut parent) => node = unsafe { parent.as_mut() },
+                A::update_cache_internal(node);
-                None => return,
+                match node.parent {
+                    Some(mut parent) => node = parent.as_mut(),
+                    None => return,
+                }
             }
         }
     }
@@ -228,11 +249,15 @@ impl<'tree, 'bump: 'tree, T: Rle, A: RleTreeTrait<T>> SafeCursorMut<'tree, 'bump
 
     #[inline]
     pub fn next(&self) -> Option<Self> {
+        // 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<Self> {
+        // 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<T>> AsMut<T>
 {
     #[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() }
     }
 }

crates/rle/src/rle_tree/node.rs

@@ -88,17 +88,23 @@ impl<'a, T: Rle, A: RleTreeTrait<T>> Node<'a, T, A> {
 
     #[inline]
     pub(crate) fn parent_mut(&mut self) -> Option<&mut InternalNode<'a, T, A>> {
-        match self {
+        // SAFETY: all tree data is pinned and can only be operating on single thread
-            Node::Internal(node) => unsafe { node.parent.map(|mut x| x.as_mut()) },
+        unsafe {
-            Node::Leaf(node) => Some(unsafe { node.parent.as_mut() }),
+            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 {
+        // SAFETY: all tree data is pinned and can only be operating on single thread
-            Node::Internal(node) => node.parent.map(|x| unsafe { x.as_ref() }),
+        unsafe {
-            Node::Leaf(node) => Some(unsafe { node.parent.as_ref() }),
+            match self {
+                Node::Internal(node) => node.parent.map(|x| x.as_ref()),
+                Node::Leaf(node) => Some(node.parent.as_ref()),
+            }
         }
     }
 

crates/rle/src/rle_tree/node/internal_impl.rs

@@ -220,13 +220,16 @@ impl<'a, T: Rle, A: RleTreeTrait<T>> InternalNode<'a, T, A> {
         let next = self.children[right_index]
             .get_last_leaf()
             .and_then(|x| x.next);
-        if let Some(mut prev) = prev {
+        // SAFETY: rle_tree is single threaded
-            let prev = unsafe { prev.as_mut() };
+        unsafe {
-            prev.next = next;
+            if let Some(mut prev) = prev {
-        }
+                let prev = prev.as_mut();
-        if let Some(mut next) = next {
+                prev.next = next;
-            let next = unsafe { next.as_mut() };
+            }
-            next.prev = prev;
+            if let Some(mut next) = next {
+                let next = next.as_mut();
+                next.prev = prev;
+            }
         }
     }
 
@@ -372,12 +375,13 @@ impl<'a, T: Rle, A: RleTreeTrait<T>> 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<T>> 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);

crates/rle/src/rle_tree/node/leaf_impl.rs

@@ -42,6 +42,7 @@ impl<'bump, T: Rle, A: RleTreeTrait<T>> 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<T>> 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<T>> 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<T>> 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]

crates/rle/src/rle_tree/test/notify_test.rs

@@ -91,6 +91,7 @@ fn test(interactions: &[Interaction]) {
     let mut range_map: RangeMap<u64, ValueIndex<'_>> = 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;