From 715bf759c38250cc5b24e2cee39c3803bfbf2b75 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Sat, 9 Nov 2024 01:07:38 +0800 Subject: [PATCH] refactor!: reduce footguns when using for_each(fn) on rust (#540) --- crates/loro-ffi/src/container/list.rs | 2 +- crates/loro-internal/src/handler.rs | 60 +++++++++++++------ crates/loro-internal/src/jsonpath.rs | 45 +++++++------- .../loro-internal/src/state/richtext_state.rs | 11 +++- crates/loro-wasm/src/lib.rs | 7 ++- crates/loro/src/lib.rs | 19 +++++- crates/loro/tests/loro_rust_test.rs | 42 +++++++++++++ 7 files changed, 138 insertions(+), 48 deletions(-) diff --git a/crates/loro-ffi/src/container/list.rs b/crates/loro-ffi/src/container/list.rs index 50092ec5..0863df83 100644 --- a/crates/loro-ffi/src/container/list.rs +++ b/crates/loro-ffi/src/container/list.rs @@ -90,7 +90,7 @@ impl LoroList { // TODO: wrap it in ffi side pub fn for_each(&self, f: I) where - I: FnMut((usize, loro::ValueOrContainer)), + I: FnMut(loro::ValueOrContainer), { self.list.for_each(f) } diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 9c0e2b0f..172e2fa7 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -2574,29 +2574,36 @@ impl ListHandler { pub fn for_each(&self, mut f: I) where - I: FnMut((usize, ValueOrHandler)), + I: FnMut(ValueOrHandler), { match &self.inner { MaybeDetached::Detached(l) => { let l = l.try_lock().unwrap(); - for (i, v) in l.value.iter().enumerate() { - f((i, v.clone())) + for v in l.value.iter() { + f(v.clone()) } } MaybeDetached::Attached(inner) => { + let mut temp = vec![]; inner.with_state(|state| { let a = state.as_list_state().unwrap(); - for (i, v) in a.iter().enumerate() { + for v in a.iter() { match v { LoroValue::Container(c) => { - f((i, ValueOrHandler::Handler(create_handler(inner, c.clone())))); + temp.push(ValueOrHandler::Handler(create_handler( + inner, + c.clone(), + ))); } value => { - f((i, ValueOrHandler::Value(value.clone()))); + temp.push(ValueOrHandler::Value(value.clone())); } } } }); + for v in temp.into_iter() { + f(v); + } } } } @@ -3257,19 +3264,26 @@ impl MovableListHandler { f(v.clone()); } } - MaybeDetached::Attached(m) => m.with_state(|state| { - let a = state.as_movable_list_state().unwrap(); - for v in a.iter() { - match v { - LoroValue::Container(c) => { - f(ValueOrHandler::Handler(create_handler(m, c.clone()))); - } - value => { - f(ValueOrHandler::Value(value.clone())); + MaybeDetached::Attached(m) => { + let mut temp = vec![]; + m.with_state(|state| { + let a = state.as_movable_list_state().unwrap(); + for v in a.iter() { + match v { + LoroValue::Container(c) => { + temp.push(ValueOrHandler::Handler(create_handler(m, c.clone()))); + } + value => { + temp.push(ValueOrHandler::Value(value.clone())); + } } } + }); + + for v in temp.into_iter() { + f(v); } - }), + } } } @@ -3547,19 +3561,29 @@ impl MapHandler { } } MaybeDetached::Attached(inner) => { + let mut temp = vec![]; inner.with_state(|state| { let a = state.as_map_state().unwrap(); for (k, v) in a.iter() { if let Some(v) = &v.value { match v { LoroValue::Container(c) => { - f(k, ValueOrHandler::Handler(create_handler(inner, c.clone()))) + temp.push(( + k.to_string(), + ValueOrHandler::Handler(create_handler(inner, c.clone())), + )); + } + value => { + temp.push((k.to_string(), ValueOrHandler::Value(value.clone()))) } - value => f(k, ValueOrHandler::Value(value.clone())), } } } }); + + for (k, v) in temp.into_iter() { + f(&k, v.clone()); + } } } } diff --git a/crates/loro-internal/src/jsonpath.rs b/crates/loro-internal/src/jsonpath.rs index db4ab2a0..70f8858a 100644 --- a/crates/loro-internal/src/jsonpath.rs +++ b/crates/loro-internal/src/jsonpath.rs @@ -482,15 +482,16 @@ impl PathValue for MapHandler { } fn for_each_for_path(&self, f: &mut dyn FnMut(ValueOrHandler) -> ControlFlow<()>) { - let mut all = vec![]; + let mut done = false; self.for_each(|_, v| { - all.push(v); - }); - for v in all { - if let ControlFlow::Break(_) = f(v) { - break; + if done { + return; } - } + + if let ControlFlow::Break(_) = f(v) { + done = true; + } + }); } fn length_for_path(&self) -> usize { @@ -520,15 +521,16 @@ impl PathValue for ListHandler { } fn for_each_for_path(&self, f: &mut dyn FnMut(ValueOrHandler) -> ControlFlow<()>) { - let mut all = vec![]; + let mut done = false; self.for_each(|v| { - all.push(v.1); - }); - for v in all { - if let ControlFlow::Break(_) = f(v) { - break; + if done { + return; } - } + + if let ControlFlow::Break(_) = f(v) { + done = true; + } + }); } fn length_for_path(&self) -> usize { @@ -562,15 +564,16 @@ impl PathValue for MovableListHandler { } fn for_each_for_path(&self, f: &mut dyn FnMut(ValueOrHandler) -> ControlFlow<()>) { - let mut all = vec![]; + let mut done = false; self.for_each(|v| { - all.push(v); - }); - for v in all { - if let ControlFlow::Break(_) = f(v) { - break; + if done { + return; } - } + + if let ControlFlow::Break(_) = f(v) { + done = true; + } + }) } fn length_for_path(&self) -> usize { diff --git a/crates/loro-internal/src/state/richtext_state.rs b/crates/loro-internal/src/state/richtext_state.rs index 917c7abd..63b10e87 100644 --- a/crates/loro-internal/src/state/richtext_state.rs +++ b/crates/loro-internal/src/state/richtext_state.rs @@ -109,9 +109,14 @@ impl RichtextState { } pub(crate) fn iter(&mut self, mut callback: impl FnMut(&str) -> bool) { - for span in self.state.get_mut().iter() { - if !callback(span.text.as_str()) { - return; + for span in self.state.get_mut().iter_chunk() { + match span { + RichtextStateChunk::Text(text_chunk) => { + if !callback(text_chunk.as_str()) { + return; + } + } + RichtextStateChunk::Style { .. } => {} } } } diff --git a/crates/loro-wasm/src/lib.rs b/crates/loro-wasm/src/lib.rs index 9a74a67a..6ee8f8de 100644 --- a/crates/loro-wasm/src/lib.rs +++ b/crates/loro-wasm/src/lib.rs @@ -1781,11 +1781,14 @@ impl LoroText { JsValue::from_str("Text").into() } - /// Iterate each span(internal storage unit) of the text. + /// Iterate each text span(internal storage unit) /// /// The callback function will be called for each span in the text. /// If the callback returns `false`, the iteration will stop. /// + /// Limitation: you cannot access or alter the doc state when iterating (this is for performance consideration). + /// If you need to access or alter the doc state, please use `toString` instead. + /// /// @example /// ```ts /// import { LoroDoc } from "loro-crdt"; @@ -2632,7 +2635,7 @@ impl LoroList { #[wasm_bindgen(js_name = "toArray", method, skip_typescript)] pub fn to_array(&mut self) -> Vec { let mut arr: Vec = Vec::with_capacity(self.length()); - self.handler.for_each(|(_, x)| { + self.handler.for_each(|x| { arr.push(match x { ValueOrHandler::Value(v) => { let v: JsValue = v.into(); diff --git a/crates/loro/src/lib.rs b/crates/loro/src/lib.rs index 5ae83120..022de4cf 100644 --- a/crates/loro/src/lib.rs +++ b/crates/loro/src/lib.rs @@ -1010,10 +1010,10 @@ impl LoroList { /// Iterate over the elements of the list. pub fn for_each(&self, mut f: I) where - I: FnMut((usize, ValueOrContainer)), + I: FnMut(ValueOrContainer), { - self.handler.for_each(&mut |(index, v)| { - f((index, ValueOrContainer::from(v))); + self.handler.for_each(&mut |v| { + f(ValueOrContainer::from(v)); }) } @@ -1379,6 +1379,9 @@ impl LoroText { /// /// The callback function will be called for each character in the text. /// If the callback returns `false`, the iteration will stop. + /// + /// Limitation: you cannot access or alter the doc state when iterating. + /// If you need to access or alter the doc state, please use `to_string` instead. pub fn iter(&self, callback: impl FnMut(&str) -> bool) { self.handler.iter(callback); } @@ -2217,6 +2220,16 @@ impl LoroMovableList { pub fn clear(&self) -> LoroResult<()> { self.handler.clear() } + + /// Iterate over the elements of the list. + pub fn for_each(&self, mut f: I) + where + I: FnMut(ValueOrContainer), + { + self.handler.for_each(&mut |v| { + f(ValueOrContainer::from(v)); + }) + } } impl Default for LoroMovableList { diff --git a/crates/loro/tests/loro_rust_test.rs b/crates/loro/tests/loro_rust_test.rs index f11e5589..7e86f15f 100644 --- a/crates/loro/tests/loro_rust_test.rs +++ b/crates/loro/tests/loro_rust_test.rs @@ -1994,6 +1994,48 @@ fn test_fork_when_detached() { assert_eq!(doc.get_text("text").to_string(), "Hello, world! Alice!"); } +#[test] +fn test_for_each_movable_list() { + let doc = LoroDoc::new(); + let list = doc.get_movable_list("list"); + list.insert(0, 1).unwrap(); + list.insert(1, "hello").unwrap(); + list.insert(2, true).unwrap(); + let mut vec = vec![]; + list.for_each(|v| { + vec.push(v.into_value().unwrap()); + }); + assert_eq!(vec, vec![1.into(), "hello".into(), true.into()]); +} + +#[test] +fn test_for_each_map() { + let doc = LoroDoc::new(); + let map = doc.get_map("map"); + map.insert("0", 0).unwrap(); + map.insert("1", 1).unwrap(); + map.insert("2", 2).unwrap(); + let mut vec = vec![]; + map.for_each(|_, v| { + vec.push(v.into_value().unwrap()); + }); + assert_eq!(vec, vec![0.into(), 1.into(), 2.into()]); +} + +#[test] +fn test_for_each_list() { + let doc = LoroDoc::new(); + let list = doc.get_list("list"); + list.insert(0, 0).unwrap(); + list.insert(1, 1).unwrap(); + list.insert(2, 2).unwrap(); + let mut vec = vec![]; + list.for_each(|v| { + vec.push(v.into_value().unwrap()); + }); + assert_eq!(vec, vec![0.into(), 1.into(), 2.into()]); +} + #[test] #[should_panic] fn should_avoid_initialize_new_container_accidentally() {