From ef61c31eb9667da0b4d611b1418fc1e837f6f295 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 02:26:29 +0800 Subject: [PATCH 1/6] fix: restore deleted mergeable children once on revert/apply_diff/undo A revival diff carries the child's full state; a re-activated mergeable child reuses its deterministic cid with the hidden state, so applying it appended the content ("hello" -> "hellohello", 7 -> 14). `_apply_diff` now reconciles such children against their current state instead. Rationale: context/mergeable-containers.md ("Revert, Apply-Diff, And Undo"). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../mergeable-revert-duplicate-content.md | 5 + context/mergeable-containers.md | 37 +- crates/loro-internal/src/handler.rs | 11 +- .../loro-internal/src/handler/full_state.rs | 194 +++++++ crates/loro-internal/src/loro.rs | 54 +- crates/loro-internal/src/state.rs | 10 + .../tests/mergeable_container/delete.rs | 41 ++ crates/loro-wasm/tests/mergeable.test.ts | 27 + crates/loro/tests/mergeable_revert.rs | 473 ++++++++++++++++++ 9 files changed, 848 insertions(+), 4 deletions(-) create mode 100644 .changeset/mergeable-revert-duplicate-content.md create mode 100644 crates/loro-internal/src/handler/full_state.rs create mode 100644 crates/loro/tests/mergeable_revert.rs diff --git a/.changeset/mergeable-revert-duplicate-content.md b/.changeset/mergeable-revert-duplicate-content.md new file mode 100644 index 000000000..ca1390abd --- /dev/null +++ b/.changeset/mergeable-revert-duplicate-content.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. The restored child now matches the target version. When its hidden content already matches, no child ops are written. diff --git a/context/mergeable-containers.md b/context/mergeable-containers.md index 7341722b0..b4a34d4df 100644 --- a/context/mergeable-containers.md +++ b/context/mergeable-containers.md @@ -1,6 +1,6 @@ # Mergeable Container Context -Verified against code 2026-07-21. +Verified against code 2026-09-28. Mergeable containers let two peers independently create the same child container under a map key and converge to one deterministic container id. The source of @@ -79,6 +79,37 @@ rewrites the marker. - Detached map handlers cannot ensure mergeable children, because the deterministic child cid depends on the attached parent cid. +## Revert, Apply-Diff, And Undo + +Recorded diffs (events, `DiffBatch` from `LoroDoc::diff`, undo/revert diffs) describe a +container that becomes reachable again with its *full* state as a from-empty diff +("revival", `DocState::apply_diff` in `state.rs`). `LoroDoc::_apply_diff` recreates regular +containers under fresh ids, so a full state is exactly right for them. A mergeable child is +re-activated at its deterministic cid, which still holds the hidden state, so applying the +full state on top used to duplicate it (`"hello"` -> `"hellohello"`, counter 7 -> 14). + +The fix lives on the apply side, and the diff shape is unchanged: +`handler::apply_map_container_diff_value` records every re-activated mergeable cid in +`full_state_targets`. `_apply_diff` then passes that cid's diff to +`handler::full_state::reconcile_full_state` together with the child's current state +(`DocState::container_full_diff`): + +- Map: reconciled per key. Unchanged entries are kept. Child containers they hold are + added to `full_state_targets`, because their batch diffs are full states too. +- Counter: increments by `target - current`. +- Text/List/MovableList/Tree: if the canonical content is equal, nothing is written and + child containers are handed down. Otherwise the child is cleared and the full state + is applied. +- A full-state target with no diff in the batch is empty at the target version and gets + cleared. + +The events and `DiffBatch` still carry full state on purpose. Consumers that mirror the +document (JS mirrors, loro-js) never saw the hidden state, so they need it. Undo sets a +re-activated child to its state at the undone version. As with revived regular +containers, a remote edit made to the child while it was hidden is not kept. Covered +by `crates/loro/tests/mergeable_revert.rs` and +`tests/mergeable_container/delete.rs`. + ## Snapshot And Retention Rules Snapshot and shallow snapshot export must preserve mergeable child state even @@ -103,6 +134,8 @@ raw/shallow storage may still show the binary marker for forward compatibility. `cargo test -p loro-internal --test mergeable_container convergence` - Delete/hide/reactivate behavior: `cargo test -p loro-internal --test mergeable_container delete` +- Revert / diff + apply_diff / undo / checkout after delete: + `cargo test -p loro --test mergeable_revert` - Different-kind conflicts: `cargo test -p loro-internal --test mergeable_container type_conflict` - Snapshot and shallow snapshot retention: @@ -118,6 +151,8 @@ raw/shallow storage may still show the binary marker for forward compatibility. by the parent marker. - "Deleting the key deletes the child state." False; it hides the child by removing the marker. +- "A revival diff can be applied as an increment." False for mergeable children: it is + the full target state and must be reconciled with the hidden state. - "Kind conflict discards the loser." False; the loser is hidden but should stay recoverable by deterministic cid. - "The marker is the child cid." False; the marker activates a kind at a diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 4342ec7f7..9c02c3b35 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -23,14 +23,16 @@ use loro_common::{ ContainerID, ContainerType, IdFull, InternalString, LoroError, LoroResult, LoroValue, PeerID, TreeID, ID, }; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use std::{borrow::Cow, cmp::Reverse, collections::BinaryHeap, fmt::Debug, ops::Deref, sync::Arc}; use tracing::{error, instrument}; pub use crate::diff::diff_impl::UpdateOptions; pub use tree::TreeHandler; +mod full_state; mod movable_list_apply_delta; +pub(crate) use full_state::reconcile_full_state; mod tree; const REGULAR_CONTAINER_VALUE_ARG_ERROR: &str = @@ -1167,6 +1169,7 @@ impl Handler { key: &str, old_id: ContainerID, on_container_remap: &mut dyn FnMut(ContainerID, ContainerID), + full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { if old_id.is_mergeable() { let parent_id = map.id(); @@ -1174,6 +1177,9 @@ impl Handler { let new_id = ContainerID::new_mergeable(&parent_id, key, kind); let marker = loro_common::mergeable_marker(&parent_id, key, kind); map.insert_without_skipping(key, marker)?; + // The deterministic cid may still hold hidden state, while the child's diff in + // this batch is its full target state. Reconcile instead of appending. + full_state_targets.insert(new_id.clone()); on_container_remap(old_id, new_id); return Ok(()); } @@ -1286,6 +1292,7 @@ impl Handler { &self, diff: Diff, container_remap: &mut FxHashMap, + full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { // In this method we will not clone the values of the containers if // they are remapped. It's the caller's duty to do so @@ -1312,6 +1319,7 @@ impl Handler { &key, h.id(), on_container_remap, + full_state_targets, )?; } Some(ValueOrHandler::Value(LoroValue::Container(old_id))) => { @@ -1320,6 +1328,7 @@ impl Handler { &key, old_id, on_container_remap, + full_state_targets, )?; } Some(ValueOrHandler::Value(v)) => { diff --git a/crates/loro-internal/src/handler/full_state.rs b/crates/loro-internal/src/handler/full_state.rs new file mode 100644 index 000000000..07acfca40 --- /dev/null +++ b/crates/loro-internal/src/handler/full_state.rs @@ -0,0 +1,194 @@ +//! Applying a "revival" diff to a container that may already hold state. +//! +//! When a container becomes reachable again between two versions, the recorded diff +//! (events, [`crate::undo::DiffBatch`]) carries the container's *full* target state as a +//! from-empty diff. `apply_diff` recreates regular containers under fresh ids, so that is +//! enough for them. A mergeable child, however, is re-activated at its deterministic cid, +//! which still holds the hidden state; applying the full diff on top of it would add the +//! content twice. [`reconcile_full_state`] turns such a diff into the edit that moves the +//! current state to the target. See `context/mergeable-containers.md`. + +use super::{Handler, ValueOrHandler}; +use crate::{ + delta::{ResolvedMapDelta, ResolvedMapValue, TreeExternalDiff}, + event::{Diff, TextMeta}, + state::TreeParentId, +}; +use fractional_index::FractionalIndex; +use loro_common::{ContainerID, InternalString, LoroResult, LoroValue, TreeID}; +use loro_delta::DeltaItem; +use rustc_hash::{FxHashMap, FxHashSet}; + +/// Canonical form of a from-empty container diff, independent of how the diff is chunked. +#[derive(PartialEq)] +enum FullState { + Text(Vec<(String, TextMeta)>), + List(Vec), + Tree(FxHashMap), + Unknown, +} + +impl FullState { + /// Returns `None` if `diff` is not a pure insertion diff. + fn from_diff(diff: &Diff) -> Option { + match diff { + Diff::Text(delta) => { + let mut ans: Vec<(String, TextMeta)> = Vec::new(); + for item in delta.iter() { + let DeltaItem::Replace { + value, + attr, + delete: 0, + } = item + else { + return None; + }; + match ans.last_mut() { + Some((s, last_attr)) if last_attr == attr => s.push_str(value.as_str()), + _ => ans.push((value.as_str().to_string(), attr.clone())), + } + } + Some(Self::Text(ans)) + } + Diff::List(delta) => { + let mut ans = Vec::new(); + for item in delta.iter() { + let DeltaItem::Replace { + value, delete: 0, .. + } = item + else { + return None; + }; + ans.extend(value.iter().map(ValueOrHandler::to_value)); + } + Some(Self::List(ans)) + } + Diff::Tree(tree) => { + let mut ans = FxHashMap::default(); + for item in tree.iter() { + let TreeExternalDiff::Create { + parent, position, .. + } = &item.action + else { + return None; + }; + ans.insert(item.target, (*parent, position.clone())); + } + Some(Self::Tree(ans)) + } + Diff::Unknown => Some(Self::Unknown), + Diff::Map(_) => unreachable!("maps are reconciled per key"), + #[cfg(feature = "counter")] + Diff::Counter(_) => unreachable!("counters are reconciled by value"), + } + } +} + +/// Containers referenced by a full-state diff. Their own diffs in the same batch are full +/// states as well. +fn collect_children(diff: &Diff, out: &mut FxHashSet) { + match diff { + Diff::List(delta) => { + for item in delta.iter() { + if let DeltaItem::Replace { value, .. } = item { + for v in value.iter() { + if let ValueOrHandler::Handler(h) = v { + out.insert(h.id()); + } + } + } + } + } + Diff::Tree(tree) => { + for item in tree.iter() { + out.insert(item.target.associated_meta_container()); + } + } + _ => {} + } +} + +fn map_value(v: &ResolvedMapValue) -> Option { + v.value.as_ref().map(ValueOrHandler::to_value) +} + +/// Given `target`, a from-empty diff describing the state `handler` should end up in, and +/// `current`, the handler's current state as a from-empty diff, returns the diff that +/// still has to be applied (or applies it directly when that is simpler). +/// +/// Child containers that keep their id and whose diffs in the batch are therefore also +/// full states are added to `full_state_targets`. +pub(crate) fn reconcile_full_state( + handler: &Handler, + target: Diff, + current: Diff, + full_state_targets: &mut FxHashSet, +) -> LoroResult> { + match (target, current) { + (Diff::Map(target), Diff::Map(current)) => { + // Per key, so unchanged entries (and the child containers they hold) survive. + let current: FxHashMap = current + .updated + .iter() + .filter_map(|(k, v)| Some((k.clone(), map_value(v)?))) + .collect(); + let mut updated = FxHashMap::default(); + let mut target_keys = FxHashSet::default(); + for (key, value) in target.updated { + let Some(target_value) = map_value(&value) else { + continue; + }; + target_keys.insert(key.clone()); + if current.get(&key) == Some(&target_value) { + if let LoroValue::Container(id) = target_value { + full_state_targets.insert(id); + } + } else { + updated.insert(key, value); + } + } + for key in current.keys() { + if !target_keys.contains(key) { + updated.insert(key.clone(), ResolvedMapValue::new_unset()); + } + } + Ok((!updated.is_empty()).then_some(Diff::Map(ResolvedMapDelta { updated }))) + } + #[cfg(feature = "counter")] + (Diff::Counter(target), Diff::Counter(current)) => { + if let Handler::Counter(counter) = handler { + let delta = target - current; + if delta != 0.0 { + counter.increment(delta)?; + } + } + Ok(None) + } + (target, current) => { + let (Some(target_state), Some(current_state)) = ( + FullState::from_diff(&target), + FullState::from_diff(¤t), + ) else { + // Not a revival diff; apply it as is. + return Ok(Some(target)); + }; + if target_state == current_state { + collect_children(&target, full_state_targets); + return Ok(None); + } + if !is_empty(¤t_state) { + handler.clear()?; + } + Ok(Some(target)) + } + } +} + +fn is_empty(state: &FullState) -> bool { + match state { + FullState::Text(t) => t.is_empty(), + FullState::List(l) => l.is_empty(), + FullState::Tree(t) => t.is_empty(), + FullState::Unknown => true, + } +} diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 58684d5f9..f19dbe3d8 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -1368,6 +1368,12 @@ impl LoroDoc { let mut ans: LoroResult<()> = Ok(()); let mut missing_containers: Vec = Vec::new(); + // Containers whose diff in this batch is a full (from-empty) state but that keep an + // id which may already hold state: re-activated mergeable children and, transitively, + // the children of revived containers that were left untouched. See + // `handler::full_state` and context/mergeable-containers.md. + let mut full_state_targets: FxHashSet = FxHashSet::default(); + let mut in_batch: FxHashSet = FxHashSet::default(); for (mut id, diff) in diff.into_iter() { let mut remapped = false; while let Some(rid) = container_remap.get(&id) { @@ -1375,6 +1381,9 @@ impl LoroDoc { id = rid.clone(); } + let is_full_state = full_state_targets.remove(&id); + in_batch.insert(id.clone()); + if matches!(&id, ContainerID::Normal { .. }) && self.arena.id_to_idx(&id).is_none() { // Not in arena does not imply non-existent; consult state/kv and register lazily let exists = self.state.lock().does_container_exist(&id); @@ -1386,7 +1395,11 @@ impl LoroDoc { self.state.lock().ensure_container(&id); } - if skip_unreachable && !remapped && !self.state.lock().get_reachable(&id) { + if skip_unreachable + && !remapped + && !is_full_state + && !self.state.lock().get_reachable(&id) + { continue; } @@ -1395,11 +1408,48 @@ impl LoroDoc { containers: Box::new(vec![id]), }); }; - if let Err(e) = h.apply_diff(diff, container_remap) { + let diff = if is_full_state { + let current = self.state.lock().container_full_diff(h.container_idx()); + match crate::handler::reconcile_full_state( + &h, + diff, + current, + &mut full_state_targets, + ) { + Ok(Some(diff)) => diff, + Ok(None) => continue, + Err(e) => { + ans = Err(e); + continue; + } + } + } else { + diff + }; + if let Err(e) = h.apply_diff(diff, container_remap, &mut full_state_targets) { ans = Err(e); } } + // A full-state container without a diff in the batch is empty at the target version. + for id in full_state_targets { + if in_batch.contains(&id) { + continue; + } + let Some(h) = self.get_handler(id) else { + continue; + }; + let is_empty = self + .state + .lock() + .is_container_state_empty(h.container_idx()); + if !is_empty { + if let Err(e) = h.clear() { + ans = Err(e); + } + } + } + if !missing_containers.is_empty() { return Err(LoroError::ContainersNotFound { containers: Box::new(missing_containers), diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index 8dad24ef0..11245ee53 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -1306,6 +1306,16 @@ impl DocState { f(state) } + /// The container's current state as a from-empty diff, the shape revival events use. + pub(crate) fn container_full_diff(&mut self, idx: ContainerIdx) -> Diff { + let doc = self.doc.clone(); + self.store.get_or_create_mut(idx).to_diff(&doc) + } + + pub(crate) fn is_container_state_empty(&mut self, idx: ContainerIdx) -> bool { + self.store.get_or_create_mut(idx).is_state_empty() + } + pub(super) fn is_in_txn(&self) -> bool { self.in_txn } diff --git a/crates/loro-internal/tests/mergeable_container/delete.rs b/crates/loro-internal/tests/mergeable_container/delete.rs index 0ca6f2a2a..14f843620 100644 --- a/crates/loro-internal/tests/mergeable_container/delete.rs +++ b/crates/loro-internal/tests/mergeable_container/delete.rs @@ -340,3 +340,44 @@ fn delete_on_mergeable_key_emits_only_existing_op_types() { "delete must emit exactly one op (the MapSet clearing the slot); got {new_ops}" ); } + +/// `revert_to` a version before the delete re-activates the hidden child. The revival diff +/// carries the child's full state, which must replace the preserved hidden state rather than +/// be appended to it (it used to yield "hellohello" / 14 / ["keep", "keep"]). +#[test] +fn revert_to_before_delete_restores_hidden_content_once() { + let doc = doc(1); + let root = doc.get_map("m"); + root.ensure_mergeable_text("t") + .unwrap() + .insert(0, "hello", loro_internal::cursor::PosType::Unicode) + .unwrap(); + root.ensure_mergeable_list("l") + .unwrap() + .push("keep") + .unwrap(); + #[cfg(feature = "counter")] + root.ensure_mergeable_counter("c") + .unwrap() + .increment(7.0) + .unwrap(); + doc.commit_then_renew(); + let before_delete = doc.state_frontiers(); + let expected = doc.get_deep_value().to_json_value(); + + for key in ["t", "l", "c"] { + let _ = root.delete(key); + } + doc.commit_then_renew(); + assert_eq!(doc.get_deep_value().to_json_value(), json!({ "m": {} })); + + doc.revert_to(&before_delete).unwrap(); + doc.commit_then_renew(); + assert_eq!(doc.get_deep_value().to_json_value(), expected); + + let replay = loro_internal::LoroDoc::new_auto_commit(); + replay + .import(&doc.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(replay.get_deep_value().to_json_value(), expected); +} diff --git a/crates/loro-wasm/tests/mergeable.test.ts b/crates/loro-wasm/tests/mergeable.test.ts index 77a14e8ee..ecac80642 100644 --- a/crates/loro-wasm/tests/mergeable.test.ts +++ b/crates/loro-wasm/tests/mergeable.test.ts @@ -222,4 +222,31 @@ describe("mergeable containers (WASM bindings)", () => { errorSpy.mockRestore(); } }); + test("revertTo / applyDiff restore a deleted mergeable child once", () => { + const setup = () => { + const d = new LoroDoc(); + d.setPeerId(1); + const m = d.getMap("m"); + m.ensureMergeableText("t").insert(0, "hello"); + m.ensureMergeableCounter("c").increment(7); + m.ensureMergeableList("l").push("keep"); + m.ensureMergeableMovableList("ml").push("keep"); + d.commit(); + const a = d.frontiers(); + for (const key of ["t", "c", "l", "ml"]) m.delete(key); + d.commit(); + return { d, a, b: d.frontiers() }; + }; + const expected = { m: { t: "hello", c: 7, l: ["keep"], ml: ["keep"] } }; + + const r = setup(); + r.d.revertTo(r.a); + r.d.commit(); + expect(r.d.toJSON()).toEqual(expected); + + const p = setup(); + p.d.applyDiff(p.d.diff(p.b, p.a)); + p.d.commit(); + expect(p.d.toJSON()).toEqual(expected); + }); }); diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs new file mode 100644 index 000000000..8faceefc6 --- /dev/null +++ b/crates/loro/tests/mergeable_revert.rs @@ -0,0 +1,473 @@ +//! Reverting to a version where a mergeable child was visible must restore the child's +//! content exactly once. +//! +//! A mergeable child keeps its state at a deterministic cid while the parent marker is +//! gone. The diff that re-activates it carries the child's *full* target state (the same +//! "revival" shape events use), so `apply_diff` has to set the hidden child to that +//! state instead of appending it. See `context/mergeable-containers.md`. + +use loro::{ExpandType, Frontiers, LoroDoc, StyleConfig, ToJson, TreeParentId, UndoManager}; +use serde_json::Value; + +fn doc() -> LoroDoc { + let d = LoroDoc::new(); + d.set_peer_id(1).unwrap(); + d.config_default_text_style(Some(StyleConfig { + expand: ExpandType::After, + })); + d +} + +/// Deep JSON with tree node ids stripped: reviving a deleted regular tree node through +/// `apply_diff` creates a fresh node id, which is unrelated to this bug. +fn json(d: &LoroDoc) -> Value { + fn strip(v: &mut Value) { + match v { + Value::Object(o) => { + if o.contains_key("fractional_index") { + o.remove("id"); + o.remove("parent"); + } + o.values_mut().for_each(strip); + } + Value::Array(a) => a.iter_mut().for_each(strip), + _ => {} + } + } + let mut v = d.get_deep_value().to_json_value(); + strip(&mut v); + v +} + +fn assert_replays_to(d: &LoroDoc, expected: &Value, what: &str) { + assert_eq!(&json(d), expected, "{what}: wrong state"); + let replay = LoroDoc::new(); + replay + .import(&d.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + // Empty root containers only exist in docs that touched them. + let non_empty_roots = |v: &Value| { + let mut v = v.clone(); + v.as_object_mut().unwrap().retain(|_, root| match root { + Value::Array(a) => !a.is_empty(), + Value::Object(o) => !o.is_empty(), + _ => true, + }); + v + }; + assert_eq!( + non_empty_roots(&json(&replay)), + non_empty_roots(expected), + "{what}: ops replay to a different state" + ); +} + +/// Runs `setup`, records version `a`, runs `mutate`, then checks that every way of +/// going back to `a` (revert_to, diff + apply_diff, undo, checkout) yields `a`'s state. +/// +/// With `reuses_children`, the reverted subtree hangs off a mergeable root, so revert must +/// reuse the hidden containers: reverting to `a` a second time emits no ops. (Regular +/// containers revived by `apply_diff` get fresh ids, so a second revert rewrites them.) +fn check(setup: impl Fn(&LoroDoc), mutate: impl Fn(&LoroDoc), reuses_children: bool) { + let d = doc(); + setup(&d); + d.commit(); + let a = d.state_frontiers(); + let expected_a = json(&d); + mutate(&d); + d.commit(); + let b = d.state_frontiers(); + let expected_b = json(&d); + assert_ne!(expected_a, expected_b, "mutation must change the state"); + + // revert_to + let r = d.fork(); + r.revert_to(&a).unwrap(); + r.commit(); + assert_replays_to(&r, &expected_a, "revert_to"); + let len = r.len_ops(); + r.revert_to(&a).unwrap(); + r.commit(); + if reuses_children { + assert_eq!(r.len_ops(), len, "second revert_to must not emit ops"); + } + assert_replays_to(&r, &expected_a, "revert_to twice"); + // Revert forward again. + r.revert_to(&b).unwrap(); + r.commit(); + assert_replays_to(&r, &expected_b, "revert_to back to b"); + + // diff + apply_diff + let r = d.fork(); + let diff = r.diff(&b, &a).unwrap(); + r.apply_diff(diff).unwrap(); + r.commit(); + assert_replays_to(&r, &expected_a, "diff + apply_diff"); + + // checkout + let r = d.fork(); + r.checkout(&a).unwrap(); + assert_eq!(json(&r), expected_a, "checkout a"); + r.checkout(&b).unwrap(); + assert_eq!(json(&r), expected_b, "checkout b"); + r.checkout_to_latest(); + assert_eq!(json(&r), expected_b, "checkout latest"); + + // undo / redo + let u = doc(); + setup(&u); + u.commit(); + let mut undo = UndoManager::new(&u); + mutate(&u); + u.commit(); + assert_eq!(json(&u), expected_b); + assert!(undo.undo().unwrap()); + u.commit(); + assert_replays_to(&u, &expected_a, "undo"); + assert!(undo.redo().unwrap()); + u.commit(); + assert_replays_to(&u, &expected_b, "redo"); + assert!(undo.undo().unwrap()); + u.commit(); + assert_replays_to(&u, &expected_a, "undo after redo"); +} + +fn delete_s(d: &LoroDoc) { + d.get_map("m").delete("s").unwrap(); +} + +#[test] +fn mergeable_text_revert_after_delete() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn mergeable_text_with_styles_revert_after_delete() { + check( + |d| { + let t = d.get_map("m").ensure_mergeable_text("s").unwrap(); + t.insert(0, "hello world").unwrap(); + t.mark(0..5, "bold", true).unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +#[cfg(feature = "counter")] +fn mergeable_counter_revert_after_delete() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_counter("s") + .unwrap() + .increment(7.0) + .unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn mergeable_list_revert_after_delete() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_list("s") + .unwrap() + .push("keep") + .unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn mergeable_movable_list_revert_after_delete() { + check( + |d| { + let l = d.get_map("m").ensure_mergeable_movable_list("s").unwrap(); + l.push("keep").unwrap(); + l.push("second").unwrap(); + l.mov(1, 0).unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn mergeable_map_revert_after_delete() { + check( + |d| { + let m = d.get_map("m").ensure_mergeable_map("s").unwrap(); + m.insert("k", "v").unwrap(); + let t = m.insert_container("t", loro::LoroText::new()).unwrap(); + t.insert(0, "inner").unwrap(); + let l = m.insert_container("l", loro::LoroList::new()).unwrap(); + l.push(1).unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn mergeable_tree_revert_after_delete() { + check( + |d| { + let tree = d.get_map("m").ensure_mergeable_tree("s").unwrap(); + let root = tree.create(TreeParentId::Root).unwrap(); + let child = tree.create(root).unwrap(); + tree.get_meta(root).unwrap().insert("name", "root").unwrap(); + let t = tree + .get_meta(child) + .unwrap() + .insert_container("t", loro::LoroText::new()) + .unwrap(); + t.insert(0, "child text").unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn nested_mergeable_revert_after_outer_delete() { + check( + |d| { + let outer = d.get_map("m").ensure_mergeable_map("s").unwrap(); + outer + .ensure_mergeable_text("inner") + .unwrap() + .insert(0, "hi") + .unwrap(); + let deeper = outer.ensure_mergeable_map("deeper").unwrap(); + deeper + .ensure_mergeable_list("items") + .unwrap() + .push("x") + .unwrap(); + }, + delete_s, + true, + ); +} + +#[test] +fn nested_mergeable_revert_after_inner_delete() { + check( + |d| { + let outer = d.get_map("m").ensure_mergeable_map("s").unwrap(); + outer + .ensure_mergeable_text("inner") + .unwrap() + .insert(0, "hi") + .unwrap(); + }, + |d| { + let outer = d.get_map("m").ensure_mergeable_map("s").unwrap(); + outer.delete("inner").unwrap(); + }, + true, + ); +} + +#[test] +fn mergeable_under_list_element_revert_after_delete() { + check( + |d| { + let list = d.get_list("l"); + let m = list.insert_container(0, loro::LoroMap::new()).unwrap(); + m.ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| d.get_list("l").delete(0, 1).unwrap(), + false, + ); +} + +#[test] +fn mergeable_under_movable_list_element_revert_after_delete() { + check( + |d| { + let list = d.get_movable_list("l"); + let m = list.insert_container(0, loro::LoroMap::new()).unwrap(); + m.ensure_mergeable_counter("s") + .unwrap() + .increment(3.0) + .unwrap(); + }, + |d| d.get_movable_list("l").delete(0, 1).unwrap(), + false, + ); +} + +#[test] +fn mergeable_under_tree_node_revert_after_delete() { + check( + |d| { + let tree = d.get_tree("tree"); + let node = tree.create(TreeParentId::Root).unwrap(); + tree.get_meta(node) + .unwrap() + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| { + let tree = d.get_tree("tree"); + let node = tree.roots()[0]; + tree.delete(node).unwrap(); + }, + false, + ); +} + +#[test] +fn mergeable_text_edited_before_delete() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| { + let t = d.get_map("m").ensure_mergeable_text("s").unwrap(); + t.insert(5, " world").unwrap(); + t.delete(0, 1).unwrap(); + delete_s(d); + }, + // The hidden text differs from the target, so the first revert rewrites it and a + // second revert re-syncs character identity like any text revert. + false, + ); +} + +#[test] +fn empty_mergeable_text_revert_after_edit_and_delete() { + check( + |d| { + d.get_map("m").ensure_mergeable_text("s").unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hi") + .unwrap(); + delete_s(d); + }, + true, + ); +} + +#[test] +#[cfg(feature = "counter")] +fn mergeable_kind_change_revert() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_counter("s") + .unwrap() + .increment(3.0) + .unwrap(); + }, + true, + ); +} + +#[test] +fn mergeable_overwritten_by_value_revert() { + check( + |d| { + d.get_map("m") + .ensure_mergeable_list("s") + .unwrap() + .push("keep") + .unwrap(); + }, + |d| d.get_map("m").insert("s", "plain").unwrap(), + true, + ); +} + +/// The exact reproduction from the loro-js cross-check. +#[test] +fn revert_restores_mergeable_text_once() { + let d = doc(); + let m = d.get_map("m"); + m.ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + d.commit(); + let a: Frontiers = d.state_frontiers(); + m.delete("s").unwrap(); + d.commit(); + let before = d.len_ops(); + d.revert_to(&a).unwrap(); + d.commit(); + assert_eq!(d.get_deep_value(), loro::loro_value!({"m": {"s": "hello"}}),); + // Only the parent marker is rewritten; unchanged hidden content is reused as-is. + assert_eq!(d.len_ops() - before, 1); +} + +/// Undoing a local delete while a remote peer edited the (now hidden) child must not +/// duplicate content. The revived child is set to its state at the undone version, so the +/// concurrent remote edit is dropped, the same as for a revived regular container, whose +/// concurrent edits stay in the dead original. +#[test] +fn undo_delete_with_concurrent_remote_edit_does_not_duplicate() { + let a = doc(); + let b = LoroDoc::new(); + b.set_peer_id(2).unwrap(); + a.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + a.commit(); + b.import(&a.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + let mut undo = UndoManager::new(&a); + delete_s(&a); + a.commit(); + b.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(5, "!") + .unwrap(); + b.commit(); + a.import(&b.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(json(&a), serde_json::json!({"m": {}})); + assert!(undo.undo().unwrap()); + a.commit(); + assert_replays_to(&a, &serde_json::json!({"m": {"s": "hello"}}), "undo"); +} From b57170cf33dfe9cae6c8b270ccedf223f2ff7512 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 03:01:32 +0800 Subject: [PATCH 2/6] fix: record re-activated mergeable children as deltas in diff/undo Replace the apply-side full-state reconcile with a recording-side rule: while `LoroDoc::diff` and undo record, a mergeable child re-activated by a container that keeps its id emits its own delta instead of a full-state revival. `apply_diff` stays incremental, so forwarded local events stay consistent, and revert keeps char/element/TreeID identity. Rationale: context/mergeable-containers.md ("Revert, Apply-Diff, And Undo"). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../mergeable-revert-duplicate-content.md | 2 +- context/mergeable-containers.md | 71 ++-- crates/loro-internal/src/handler.rs | 11 +- .../loro-internal/src/handler/full_state.rs | 194 --------- crates/loro-internal/src/loro.rs | 73 +--- crates/loro-internal/src/state.rs | 72 ++-- .../tests/mergeable_container/delete.rs | 6 +- crates/loro-wasm/tests/mergeable.test.ts | 75 ++++ crates/loro/tests/mergeable_revert.rs | 370 ++++++++++++++++-- 9 files changed, 524 insertions(+), 350 deletions(-) delete mode 100644 crates/loro-internal/src/handler/full_state.rs diff --git a/.changeset/mergeable-revert-duplicate-content.md b/.changeset/mergeable-revert-duplicate-content.md index ca1390abd..b11cb83e2 100644 --- a/.changeset/mergeable-revert-duplicate-content.md +++ b/.changeset/mergeable-revert-duplicate-content.md @@ -2,4 +2,4 @@ "loro-crdt": patch --- -Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. The restored child now matches the target version. When its hidden content already matches, no child ops are written. +Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. The restored child now matches the target version and keeps its characters, list elements and tree node ids, so only the real difference is written. `diff()` now returns a delta relative to the hidden child's state for a re-activated mergeable child, not its full content. Checkout and import events are unchanged. diff --git a/context/mergeable-containers.md b/context/mergeable-containers.md index b4a34d4df..92f8e2ec0 100644 --- a/context/mergeable-containers.md +++ b/context/mergeable-containers.md @@ -81,34 +81,44 @@ rewrites the marker. ## Revert, Apply-Diff, And Undo -Recorded diffs (events, `DiffBatch` from `LoroDoc::diff`, undo/revert diffs) describe a -container that becomes reachable again with its *full* state as a from-empty diff -("revival", `DocState::apply_diff` in `state.rs`). `LoroDoc::_apply_diff` recreates regular -containers under fresh ids, so a full state is exactly right for them. A mergeable child is -re-activated at its deterministic cid, which still holds the hidden state, so applying the -full state on top used to duplicate it (`"hello"` -> `"hellohello"`, counter 7 -> 14). - -The fix lives on the apply side, and the diff shape is unchanged: -`handler::apply_map_container_diff_value` records every re-activated mergeable cid in -`full_state_targets`. `_apply_diff` then passes that cid's diff to -`handler::full_state::reconcile_full_state` together with the child's current state -(`DocState::container_full_diff`): - -- Map: reconciled per key. Unchanged entries are kept. Child containers they hold are - added to `full_state_targets`, because their batch diffs are full states too. -- Counter: increments by `target - current`. -- Text/List/MovableList/Tree: if the canonical content is equal, nothing is written and - child containers are handed down. Otherwise the child is cleared and the full state - is applied. -- A full-state target with no diff in the batch is empty at the target version and gets - cleared. - -The events and `DiffBatch` still carry full state on purpose. Consumers that mirror the -document (JS mirrors, loro-js) never saw the hidden state, so they need it. Undo sets a -re-activated child to its state at the undone version. As with revived regular -containers, a remote edit made to the child while it was hidden is not kept. Covered -by `crates/loro/tests/mergeable_revert.rs` and -`tests/mergeable_container/delete.rs`. +When a container becomes reachable again, `DocState::apply_diff` (`state.rs`) +normally records its *full* state as a from-empty diff. This is called revival, and it +is what checkout/import subscribers need. `LoroDoc::_apply_diff` recreates regular +containers under fresh ids, so full state is right for them. A mergeable child is +different: it is re-activated at its deterministic cid, and a doc at the diff's start +version still holds its hidden state there. Applying a full state on top used to +duplicate it (`"hello"` -> `"hellohello"`, counter 7 -> 14). + +`LoroDoc::diff` and the undo diff closure record with +`DocState::mergeable_revival_as_delta` set. In that mode, a mergeable child that is +re-activated by the diff of a container that keeps its id (not itself revived) is not +revived. Its own calculator delta, or nothing when it has no ops in range, becomes the +batch entry. `_apply_diff` stays purely incremental. So: + +- revert/diff/undo keep char, element and `TreeID` identity. Only the real difference is + written; hidden content that already matches costs only the parent marker op. +- Concurrent reverts of the same state delete the same elements and merge to the + target. Concurrent edits to kept elements (e.g. a tree node's meta) survive. +- Local events of a revert are increments on the hidden state. Forwarding them to a doc + in the same state through `apply_diff` reproduces the source state. +- A mergeable child whose parent *is* revived under a fresh id (e.g. a map in a deleted + list element) resolves to a fresh deterministic cid, so it still gets full state. + +Consequences: + +- `LoroDoc::diff` output for a re-activated mergeable child is a delta relative to its + hidden state, not a mirror-friendly full state. Checkout/import events are unchanged + and still revive with full state. +- `revert_to` behaves like it does for a root container: a remote edit to the hidden + child that was already observed is reverted, and one that arrives later merges. +- Undo of a delete only restores the marker, so remote edits made while the child was + hidden survive in either arrival order. +- Counters stay additive: two peers reverting 10 -> 7 concurrently give 4, as for a + root counter. + +Covered by `crates/loro/tests/mergeable_revert.rs` (including event forwarding, concurrent +reverts, and tree meta edits), `tests/mergeable_container/delete.rs`, and +`crates/loro-wasm/tests/mergeable.test.ts`. ## Snapshot And Retention Rules @@ -151,8 +161,9 @@ raw/shallow storage may still show the binary marker for forward compatibility. by the parent marker. - "Deleting the key deletes the child state." False; it hides the child by removing the marker. -- "A revival diff can be applied as an increment." False for mergeable children: it is - the full target state and must be reconciled with the hidden state. +- "A re-activated mergeable child needs a full-state revival in a diff that will be + applied to a Loro doc." False; the target doc still holds the hidden state, so + `LoroDoc::diff`/undo record the child's delta instead. - "Kind conflict discards the loser." False; the loser is hidden but should stay recoverable by deterministic cid. - "The marker is the child cid." False; the marker activates a kind at a diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 9c02c3b35..4342ec7f7 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -23,16 +23,14 @@ use loro_common::{ ContainerID, ContainerType, IdFull, InternalString, LoroError, LoroResult, LoroValue, PeerID, TreeID, ID, }; -use rustc_hash::{FxHashMap, FxHashSet}; +use rustc_hash::FxHashMap; use serde::{Deserialize, Serialize}; use std::{borrow::Cow, cmp::Reverse, collections::BinaryHeap, fmt::Debug, ops::Deref, sync::Arc}; use tracing::{error, instrument}; pub use crate::diff::diff_impl::UpdateOptions; pub use tree::TreeHandler; -mod full_state; mod movable_list_apply_delta; -pub(crate) use full_state::reconcile_full_state; mod tree; const REGULAR_CONTAINER_VALUE_ARG_ERROR: &str = @@ -1169,7 +1167,6 @@ impl Handler { key: &str, old_id: ContainerID, on_container_remap: &mut dyn FnMut(ContainerID, ContainerID), - full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { if old_id.is_mergeable() { let parent_id = map.id(); @@ -1177,9 +1174,6 @@ impl Handler { let new_id = ContainerID::new_mergeable(&parent_id, key, kind); let marker = loro_common::mergeable_marker(&parent_id, key, kind); map.insert_without_skipping(key, marker)?; - // The deterministic cid may still hold hidden state, while the child's diff in - // this batch is its full target state. Reconcile instead of appending. - full_state_targets.insert(new_id.clone()); on_container_remap(old_id, new_id); return Ok(()); } @@ -1292,7 +1286,6 @@ impl Handler { &self, diff: Diff, container_remap: &mut FxHashMap, - full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { // In this method we will not clone the values of the containers if // they are remapped. It's the caller's duty to do so @@ -1319,7 +1312,6 @@ impl Handler { &key, h.id(), on_container_remap, - full_state_targets, )?; } Some(ValueOrHandler::Value(LoroValue::Container(old_id))) => { @@ -1328,7 +1320,6 @@ impl Handler { &key, old_id, on_container_remap, - full_state_targets, )?; } Some(ValueOrHandler::Value(v)) => { diff --git a/crates/loro-internal/src/handler/full_state.rs b/crates/loro-internal/src/handler/full_state.rs deleted file mode 100644 index 07acfca40..000000000 --- a/crates/loro-internal/src/handler/full_state.rs +++ /dev/null @@ -1,194 +0,0 @@ -//! Applying a "revival" diff to a container that may already hold state. -//! -//! When a container becomes reachable again between two versions, the recorded diff -//! (events, [`crate::undo::DiffBatch`]) carries the container's *full* target state as a -//! from-empty diff. `apply_diff` recreates regular containers under fresh ids, so that is -//! enough for them. A mergeable child, however, is re-activated at its deterministic cid, -//! which still holds the hidden state; applying the full diff on top of it would add the -//! content twice. [`reconcile_full_state`] turns such a diff into the edit that moves the -//! current state to the target. See `context/mergeable-containers.md`. - -use super::{Handler, ValueOrHandler}; -use crate::{ - delta::{ResolvedMapDelta, ResolvedMapValue, TreeExternalDiff}, - event::{Diff, TextMeta}, - state::TreeParentId, -}; -use fractional_index::FractionalIndex; -use loro_common::{ContainerID, InternalString, LoroResult, LoroValue, TreeID}; -use loro_delta::DeltaItem; -use rustc_hash::{FxHashMap, FxHashSet}; - -/// Canonical form of a from-empty container diff, independent of how the diff is chunked. -#[derive(PartialEq)] -enum FullState { - Text(Vec<(String, TextMeta)>), - List(Vec), - Tree(FxHashMap), - Unknown, -} - -impl FullState { - /// Returns `None` if `diff` is not a pure insertion diff. - fn from_diff(diff: &Diff) -> Option { - match diff { - Diff::Text(delta) => { - let mut ans: Vec<(String, TextMeta)> = Vec::new(); - for item in delta.iter() { - let DeltaItem::Replace { - value, - attr, - delete: 0, - } = item - else { - return None; - }; - match ans.last_mut() { - Some((s, last_attr)) if last_attr == attr => s.push_str(value.as_str()), - _ => ans.push((value.as_str().to_string(), attr.clone())), - } - } - Some(Self::Text(ans)) - } - Diff::List(delta) => { - let mut ans = Vec::new(); - for item in delta.iter() { - let DeltaItem::Replace { - value, delete: 0, .. - } = item - else { - return None; - }; - ans.extend(value.iter().map(ValueOrHandler::to_value)); - } - Some(Self::List(ans)) - } - Diff::Tree(tree) => { - let mut ans = FxHashMap::default(); - for item in tree.iter() { - let TreeExternalDiff::Create { - parent, position, .. - } = &item.action - else { - return None; - }; - ans.insert(item.target, (*parent, position.clone())); - } - Some(Self::Tree(ans)) - } - Diff::Unknown => Some(Self::Unknown), - Diff::Map(_) => unreachable!("maps are reconciled per key"), - #[cfg(feature = "counter")] - Diff::Counter(_) => unreachable!("counters are reconciled by value"), - } - } -} - -/// Containers referenced by a full-state diff. Their own diffs in the same batch are full -/// states as well. -fn collect_children(diff: &Diff, out: &mut FxHashSet) { - match diff { - Diff::List(delta) => { - for item in delta.iter() { - if let DeltaItem::Replace { value, .. } = item { - for v in value.iter() { - if let ValueOrHandler::Handler(h) = v { - out.insert(h.id()); - } - } - } - } - } - Diff::Tree(tree) => { - for item in tree.iter() { - out.insert(item.target.associated_meta_container()); - } - } - _ => {} - } -} - -fn map_value(v: &ResolvedMapValue) -> Option { - v.value.as_ref().map(ValueOrHandler::to_value) -} - -/// Given `target`, a from-empty diff describing the state `handler` should end up in, and -/// `current`, the handler's current state as a from-empty diff, returns the diff that -/// still has to be applied (or applies it directly when that is simpler). -/// -/// Child containers that keep their id and whose diffs in the batch are therefore also -/// full states are added to `full_state_targets`. -pub(crate) fn reconcile_full_state( - handler: &Handler, - target: Diff, - current: Diff, - full_state_targets: &mut FxHashSet, -) -> LoroResult> { - match (target, current) { - (Diff::Map(target), Diff::Map(current)) => { - // Per key, so unchanged entries (and the child containers they hold) survive. - let current: FxHashMap = current - .updated - .iter() - .filter_map(|(k, v)| Some((k.clone(), map_value(v)?))) - .collect(); - let mut updated = FxHashMap::default(); - let mut target_keys = FxHashSet::default(); - for (key, value) in target.updated { - let Some(target_value) = map_value(&value) else { - continue; - }; - target_keys.insert(key.clone()); - if current.get(&key) == Some(&target_value) { - if let LoroValue::Container(id) = target_value { - full_state_targets.insert(id); - } - } else { - updated.insert(key, value); - } - } - for key in current.keys() { - if !target_keys.contains(key) { - updated.insert(key.clone(), ResolvedMapValue::new_unset()); - } - } - Ok((!updated.is_empty()).then_some(Diff::Map(ResolvedMapDelta { updated }))) - } - #[cfg(feature = "counter")] - (Diff::Counter(target), Diff::Counter(current)) => { - if let Handler::Counter(counter) = handler { - let delta = target - current; - if delta != 0.0 { - counter.increment(delta)?; - } - } - Ok(None) - } - (target, current) => { - let (Some(target_state), Some(current_state)) = ( - FullState::from_diff(&target), - FullState::from_diff(¤t), - ) else { - // Not a revival diff; apply it as is. - return Ok(Some(target)); - }; - if target_state == current_state { - collect_children(&target, full_state_targets); - return Ok(None); - } - if !is_empty(¤t_state) { - handler.clear()?; - } - Ok(Some(target)) - } - } -} - -fn is_empty(state: &FullState) -> bool { - match state { - FullState::Text(t) => t.is_empty(), - FullState::List(l) => l.is_empty(), - FullState::Tree(t) => t.is_empty(), - FullState::Unknown => true, - } -} diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 5e19d5aa6..fea588466 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -1230,9 +1230,14 @@ impl LoroDoc { }, |from, to| { self._checkout_without_emitting(from, false, false).unwrap(); - self.state.lock().start_recording(); + { + let mut state = self.state.lock(); + state.start_recording(); + state.set_mergeable_revival_as_delta(true); + } self._checkout_without_emitting(to, false, false).unwrap(); let mut state = self.state.lock(); + state.set_mergeable_revival_as_delta(false); let e = state.take_events(); state.stop_and_clear_recording(); DiffBatch::new(e) @@ -1317,9 +1322,17 @@ impl LoroDoc { }; let result = (|| { self._checkout_without_emitting(a, true, false)?; - self.state.lock().start_recording(); - self._checkout_without_emitting(b, true, false)?; + { + let mut state = self.state.lock(); + state.start_recording(); + // The batch is applied to a doc at `a`, which still holds the hidden state of + // mergeable children that `b` re-activates. + state.set_mergeable_revival_as_delta(true); + } + let checkout = self._checkout_without_emitting(b, true, false); let mut state = self.state.lock(); + state.set_mergeable_revival_as_delta(false); + checkout?; let e = state.take_events(); state.stop_and_clear_recording(); Ok::<_, LoroError>(e) @@ -1368,12 +1381,6 @@ impl LoroDoc { let mut ans: LoroResult<()> = Ok(()); let mut missing_containers: Vec = Vec::new(); - // Containers whose diff in this batch is a full (from-empty) state but that keep an - // id which may already hold state: re-activated mergeable children and, transitively, - // the children of revived containers that were left untouched. See - // `handler::full_state` and context/mergeable-containers.md. - let mut full_state_targets: FxHashSet = FxHashSet::default(); - let mut in_batch: FxHashSet = FxHashSet::default(); for (mut id, diff) in diff.into_iter() { let mut remapped = false; while let Some(rid) = container_remap.get(&id) { @@ -1381,9 +1388,6 @@ impl LoroDoc { id = rid.clone(); } - let is_full_state = full_state_targets.remove(&id); - in_batch.insert(id.clone()); - if matches!(&id, ContainerID::Normal { .. }) && self.arena.id_to_idx(&id).is_none() { // Not in arena does not imply non-existent; consult state/kv and register lazily let exists = self.state.lock().does_container_exist(&id); @@ -1395,11 +1399,7 @@ impl LoroDoc { self.state.lock().ensure_container(&id); } - if skip_unreachable - && !remapped - && !is_full_state - && !self.state.lock().get_reachable(&id) - { + if skip_unreachable && !remapped && !self.state.lock().get_reachable(&id) { continue; } @@ -1408,48 +1408,11 @@ impl LoroDoc { containers: Box::new(vec![id]), }); }; - let diff = if is_full_state { - let current = self.state.lock().container_full_diff(h.container_idx()); - match crate::handler::reconcile_full_state( - &h, - diff, - current, - &mut full_state_targets, - ) { - Ok(Some(diff)) => diff, - Ok(None) => continue, - Err(e) => { - ans = Err(e); - continue; - } - } - } else { - diff - }; - if let Err(e) = h.apply_diff(diff, container_remap, &mut full_state_targets) { + if let Err(e) = h.apply_diff(diff, container_remap) { ans = Err(e); } } - // A full-state container without a diff in the batch is empty at the target version. - for id in full_state_targets { - if in_batch.contains(&id) { - continue; - } - let Some(h) = self.get_handler(id) else { - continue; - }; - let is_empty = self - .state - .lock() - .is_container_state_empty(h.container_idx()); - if !is_empty { - if let Err(e) = h.clear() { - ans = Err(e); - } - } - } - if !missing_containers.is_empty() { return Err(LoroError::ContainersNotFound { containers: Box::new(missing_containers), diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index d266a8fab..78c565225 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -174,6 +174,12 @@ pub struct DocState { dead_containers_cache: DeadContainersCache, alive_containers_cache: Option, + /// Set while recording a diff that will be applied to a doc at the start version + /// (`LoroDoc::diff`, undo). A mergeable child re-activated inside a container that + /// keeps its id then records its own delta instead of a full-state revival, because the + /// target doc still holds the child's hidden state at the same deterministic cid. See + /// context/mergeable-containers.md. + mergeable_revival_as_delta: bool, } struct AliveContainersCache { @@ -499,6 +505,7 @@ impl DocState { event_recorder: Default::default(), dead_containers_cache: Default::default(), alive_containers_cache: None, + mergeable_revival_as_delta: false, }, crate::lock::LockKind::DocState, )) @@ -524,6 +531,7 @@ impl DocState { event_recorder: Default::default(), dead_containers_cache: Default::default(), alive_containers_cache: None, + mergeable_revival_as_delta: false, })) } @@ -785,32 +793,46 @@ impl DocState { } let state = self.store.get_or_create_mut(idx); if is_recording { + let is_revived = + diff.bring_back || to_revive_in_this_layer.contains(&idx); // process bring_back before apply - let external_diff = - if diff.bring_back || to_revive_in_this_layer.contains(&idx) { - state.apply_diff( - internal_diff.into_internal().unwrap(), - DiffApplyContext { - mode: diff.diff_mode, - doc: &self.doc, - }, - )?; - state.to_diff(&self.doc) - } else { - state.apply_diff_and_convert( - internal_diff.into_internal().unwrap(), - DiffApplyContext { - mode: diff.diff_mode, - doc: &self.doc, - }, - ) - }; + let external_diff = if is_revived { + state.apply_diff( + internal_diff.into_internal().unwrap(), + DiffApplyContext { + mode: diff.diff_mode, + doc: &self.doc, + }, + )?; + state.to_diff(&self.doc) + } else { + state.apply_diff_and_convert( + internal_diff.into_internal().unwrap(), + DiffApplyContext { + mode: diff.diff_mode, + doc: &self.doc, + }, + ) + }; + let arena = &self.arena; + let mergeable_as_delta = + self.mergeable_revival_as_delta && !is_revived; trigger_on_new_container( &external_diff, |cid| { + // This container keeps its id, so a mergeable child + // it re-activates keeps its hidden state; its own + // delta (if any) is what the target doc needs. + if mergeable_as_delta + && arena + .idx_to_id(cid) + .is_some_and(|id| id.is_mergeable()) + { + return; + } to_revive_in_next_layer.insert(cid); }, - &self.arena, + arena, ); diff.diff = external_diff.into(); } else { @@ -1306,14 +1328,8 @@ impl DocState { f(state) } - /// The container's current state as a from-empty diff, the shape revival events use. - pub(crate) fn container_full_diff(&mut self, idx: ContainerIdx) -> Diff { - let doc = self.doc.clone(); - self.store.get_or_create_mut(idx).to_diff(&doc) - } - - pub(crate) fn is_container_state_empty(&mut self, idx: ContainerIdx) -> bool { - self.store.get_or_create_mut(idx).is_state_empty() + pub(crate) fn set_mergeable_revival_as_delta(&mut self, on: bool) { + self.mergeable_revival_as_delta = on; } pub(super) fn is_in_txn(&self) -> bool { diff --git a/crates/loro-internal/tests/mergeable_container/delete.rs b/crates/loro-internal/tests/mergeable_container/delete.rs index 14f843620..79b58021f 100644 --- a/crates/loro-internal/tests/mergeable_container/delete.rs +++ b/crates/loro-internal/tests/mergeable_container/delete.rs @@ -341,9 +341,9 @@ fn delete_on_mergeable_key_emits_only_existing_op_types() { ); } -/// `revert_to` a version before the delete re-activates the hidden child. The revival diff -/// carries the child's full state, which must replace the preserved hidden state rather than -/// be appended to it (it used to yield "hellohello" / 14 / ["keep", "keep"]). +/// `revert_to` a version before the delete re-activates the hidden child. The diff must carry +/// the child's delta relative to its preserved hidden state, not a full-state revival that +/// would be appended to it (it used to yield "hellohello" / 14 / ["keep", "keep"]). #[test] fn revert_to_before_delete_restores_hidden_content_once() { let doc = doc(1); diff --git a/crates/loro-wasm/tests/mergeable.test.ts b/crates/loro-wasm/tests/mergeable.test.ts index ecac80642..706cf329c 100644 --- a/crates/loro-wasm/tests/mergeable.test.ts +++ b/crates/loro-wasm/tests/mergeable.test.ts @@ -249,4 +249,79 @@ describe("mergeable containers (WASM bindings)", () => { p.d.commit(); expect(p.d.toJSON()).toEqual(expected); }); + describe("revert of hidden mergeable children that diverged from the target", () => { + // Target: t="hello", l=["keep"], c=7, tree with one node. Then the hidden children + // diverge (t="hello!", l=["keep","extra"], c=10, an extra tree node) and are deleted. + const setup = (peer: number) => { + const d = new LoroDoc(); + d.setPeerId(peer); + const m = d.getMap("m"); + m.ensureMergeableText("t").insert(0, "hello"); + m.ensureMergeableList("l").push("keep"); + m.ensureMergeableCounter("c").increment(7); + const tree = m.ensureMergeableTree("tree"); + const n = tree.createNode(); + n.data.set("v", "keep"); + d.commit(); + const target = d.frontiers(); + m.ensureMergeableText("t").insert(5, "!"); + m.ensureMergeableList("l").push("extra"); + m.ensureMergeableCounter("c").increment(3); + tree.createNode(); + d.commit(); + for (const key of ["t", "l", "c", "tree"]) m.delete(key); + d.commit(); + return { d, target, nodeId: n.id }; + }; + const expected = { + t: "hello", + l: ["keep"], + c: 7, + }; + const visible = (d: LoroDoc) => { + const { tree, ...rest } = d.toJSON().m; + return { rest, tree }; + }; + + test("revertTo keeps tree node identity", () => { + const { d, target, nodeId } = setup(1); + d.revertTo(target); + d.commit(); + const { rest, tree } = visible(d); + expect(rest).toEqual(expected); + expect(tree.map((n: any) => n.id)).toEqual([nodeId]); + }); + + test("local revert events forwarded through applyDiff match the source", () => { + const { d, target } = setup(1); + const mirror = d.fork(); + const batches: [string, any][][] = []; + const unsub = d.subscribe((e) => { + batches.push(e.events.map((ev) => [ev.target, ev.diff])); + }); + d.revertTo(target); + d.commit(); + unsub(); + for (const batch of batches) mirror.applyDiff(batch as any); + mirror.commit(); + expect(mirror.toJSON()).toEqual(d.toJSON()); + expect(visible(mirror).rest).toEqual(expected); + }); + + test("two peers reverting concurrently merge to the target", () => { + const { d: a, target, nodeId } = setup(1); + const b = a.fork(); + b.setPeerId(2); + a.revertTo(target); + a.commit(); + b.revertTo(target); + b.commit(); + sync(a, b); + const { rest, tree } = visible(a); + // The counter is additive: both peers compensate 10 -> 7, like a root counter. + expect(rest).toEqual({ ...expected, c: 4 }); + expect(tree.map((n: any) => n.id)).toEqual([nodeId]); + expect(b.toJSON()).toEqual(a.toJSON()); + }); + }); }); diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs index 8faceefc6..b6bc14960 100644 --- a/crates/loro/tests/mergeable_revert.rs +++ b/crates/loro/tests/mergeable_revert.rs @@ -2,12 +2,60 @@ //! content exactly once. //! //! A mergeable child keeps its state at a deterministic cid while the parent marker is -//! gone. The diff that re-activates it carries the child's *full* target state (the same -//! "revival" shape events use), so `apply_diff` has to set the hidden child to that -//! state instead of appending it. See `context/mergeable-containers.md`. +//! gone, and a doc at the diff's start version still holds that state. So `LoroDoc::diff` +//! and undo record the re-activated child's own delta instead of a full-state revival, +//! which keeps char/element/TreeID identity. See `context/mergeable-containers.md`. -use loro::{ExpandType, Frontiers, LoroDoc, StyleConfig, ToJson, TreeParentId, UndoManager}; -use serde_json::Value; +use loro::{ + event::{Diff, DiffBatch, MapDelta}, + ExpandType, Frontiers, LoroDoc, StyleConfig, ToJson, TreeParentId, UndoManager, +}; +use serde_json::{json, Value}; +use std::{ + borrow::Cow, + sync::{Arc, Mutex}, +}; + +fn owned_diff(diff: Diff<'_>) -> Diff<'static> { + match diff { + Diff::List(l) => Diff::List(l), + Diff::Text(t) => Diff::Text(t), + Diff::Map(m) => Diff::Map(MapDelta { + updated: m + .updated + .into_iter() + .map(|(k, v)| (Cow::Owned(k.into_owned()), v)) + .collect(), + }), + Diff::Tree(t) => Diff::Tree(Cow::Owned(t.into_owned())), + #[cfg(feature = "counter")] + Diff::Counter(c) => Diff::Counter(c), + _ => Diff::Unknown, + } +} + +/// Runs `op` on `source` and forwards the local events it emits to `mirror` through +/// `apply_diff`. `mirror` must start in the same state as `source`. +fn forward_events(source: &LoroDoc, mirror: &LoroDoc, op: impl FnOnce(&LoroDoc)) { + let batches: Arc>> = Default::default(); + let sink = batches.clone(); + let sub = source.subscribe_root(Arc::new(move |e| { + let mut batch = DiffBatch::default(); + for d in e.events { + batch + .push(d.target.clone(), owned_diff(d.diff)) + .unwrap_or_else(|_| panic!("duplicate container in one event")); + } + sink.lock().unwrap().push(batch); + })); + op(source); + source.commit(); + drop(sub); + for batch in batches.lock().unwrap().drain(..) { + mirror.apply_diff(batch).unwrap(); + } + mirror.commit(); +} fn doc() -> LoroDoc { let d = LoroDoc::new(); @@ -97,6 +145,13 @@ fn check(setup: impl Fn(&LoroDoc), mutate: impl Fn(&LoroDoc), reuses_children: b r.commit(); assert_replays_to(&r, &expected_b, "revert_to back to b"); + // Local events of a revert, forwarded to a doc in the same state. + let source = d.fork(); + let mirror = d.fork(); + forward_events(&source, &mirror, |s| s.revert_to(&a).unwrap()); + assert_eq!(json(&source), expected_a, "event forwarding: source"); + assert_eq!(json(&mirror), expected_a, "event forwarding: mirror"); + // diff + apply_diff let r = d.fork(); let diff = r.diff(&b, &a).unwrap(); @@ -438,36 +493,293 @@ fn revert_restores_mergeable_text_once() { assert_eq!(d.len_ops() - before, 1); } -/// Undoing a local delete while a remote peer edited the (now hidden) child must not -/// duplicate content. The revived child is set to its state at the undone version, so the -/// concurrent remote edit is dropped, the same as for a revived regular container, whose -/// concurrent edits stay in the dead original. -#[test] -fn undo_delete_with_concurrent_remote_edit_does_not_duplicate() { - let a = doc(); - let b = LoroDoc::new(); - b.set_peer_id(2).unwrap(); - a.get_map("m") - .ensure_mergeable_text("s") - .unwrap() - .insert(0, "hello") +fn peer(d: &LoroDoc, id: u64) -> LoroDoc { + let f = d.fork(); + f.set_peer_id(id).unwrap(); + f +} + +fn sync(a: &LoroDoc, b: &LoroDoc) { + a.import(&b.export(loro::ExportMode::all_updates()).unwrap()) .unwrap(); - a.commit(); b.import(&a.export(loro::ExportMode::all_updates()).unwrap()) .unwrap(); - let mut undo = UndoManager::new(&a); - delete_s(&a); +} + +/// A doc whose mergeable children under `m` were visible with `target` content, then got +/// `diverge` applied and their keys deleted. Returns the doc and the target version. +fn deleted_after_divergence( + target: impl Fn(&LoroDoc), + diverge: impl Fn(&LoroDoc), +) -> (LoroDoc, Frontiers, Value) { + let d = doc(); + target(&d); + d.commit(); + let a = d.state_frontiers(); + let expected = json(&d); + diverge(&d); + d.commit(); + let keys: Vec = d.get_map("m").keys().map(|k| k.to_string()).collect(); + for k in keys { + d.get_map("m").delete(&k).unwrap(); + } + d.commit(); + (d, a, expected) +} + +fn diverged_children() -> (LoroDoc, Frontiers, Value) { + deleted_after_divergence( + |d| { + let m = d.get_map("m"); + m.ensure_mergeable_text("t") + .unwrap() + .insert(0, "hello") + .unwrap(); + m.ensure_mergeable_list("l").unwrap().push("keep").unwrap(); + m.ensure_mergeable_movable_list("ml") + .unwrap() + .push("keep") + .unwrap(); + m.ensure_mergeable_map("nested") + .unwrap() + .ensure_mergeable_text("t") + .unwrap() + .insert(0, "hello") + .unwrap(); + let tree = m.ensure_mergeable_tree("tree").unwrap(); + let n = tree.create(TreeParentId::Root).unwrap(); + tree.get_meta(n).unwrap().insert("v", "keep").unwrap(); + }, + |d| { + let m = d.get_map("m"); + m.ensure_mergeable_text("t") + .unwrap() + .insert(5, "!") + .unwrap(); + m.ensure_mergeable_list("l").unwrap().push("extra").unwrap(); + m.ensure_mergeable_movable_list("ml") + .unwrap() + .push("extra") + .unwrap(); + m.ensure_mergeable_map("nested") + .unwrap() + .ensure_mergeable_text("t") + .unwrap() + .insert(5, "!") + .unwrap(); + m.ensure_mergeable_tree("tree") + .unwrap() + .create(TreeParentId::Root) + .unwrap(); + }, + ) +} + +fn tree_ids(d: &LoroDoc) -> Vec { + let m = d.get_map("m"); + let tree = m.ensure_mergeable_tree("tree").unwrap(); + tree.nodes() + .into_iter() + .filter(|n| !tree.is_node_deleted(n).unwrap()) + .collect() +} + +/// Revert rewrites hidden content that differs from the target with identity-preserving +/// edits: only `!` / `extra` / the extra tree node are removed. +#[test] +fn revert_diverged_hidden_children_keeps_identity() { + let (d, a, expected) = diverged_children(); + let tree_before = d.fork(); + tree_before.checkout(&a).unwrap(); + let original_node = tree_ids(&tree_before); + let before = d.len_ops(); + d.revert_to(&a).unwrap(); + d.commit(); + assert_replays_to(&d, &expected, "revert"); + assert_eq!(tree_ids(&d), original_node, "tree node identity is kept"); + // Markers (5), plus deleting "!" twice, "extra" twice and one tree node. + assert_eq!(d.len_ops() - before, 10); +} + +/// Local revert events forwarded to another doc through `apply_diff` reproduce the source +/// state, including hidden children whose content matched or differed from the target. +#[test] +fn revert_events_forward_through_apply_diff() { + let (d, a, expected) = diverged_children(); + let source = d.fork(); + let mirror = d.fork(); + forward_events(&source, &mirror, |s| s.revert_to(&a).unwrap()); + assert_eq!(json(&source), expected); + assert_eq!(json(&mirror), expected); +} + +#[test] +#[cfg(feature = "counter")] +fn revert_counter_events_forward_through_apply_diff() { + // Hidden value equal to the target (7) and larger than it (10). + for extra in [0.0, 3.0] { + let (d, a, expected) = deleted_after_divergence( + |d| { + d.get_map("m") + .ensure_mergeable_counter("c") + .unwrap() + .increment(7.0) + .unwrap(); + }, + |d| { + if extra != 0.0 { + d.get_map("m") + .ensure_mergeable_counter("c") + .unwrap() + .increment(extra) + .unwrap(); + } + }, + ); + assert_eq!(expected, json!({"m": {"c": 7.0}})); + let source = d.fork(); + let mirror = d.fork(); + forward_events(&source, &mirror, |s| s.revert_to(&a).unwrap()); + assert_eq!(json(&source), expected, "source, hidden extra {extra}"); + assert_eq!(json(&mirror), expected, "mirror, hidden extra {extra}"); + } +} + +/// Two peers revert the same deleted state concurrently. Identity-preserving edits make +/// the two reverts delete the same elements, so the merge equals the target. +#[test] +fn concurrent_reverts_of_diverged_hidden_children_converge_to_target() { + let (a, target, expected) = diverged_children(); + let b = peer(&a, 2); + a.revert_to(&target).unwrap(); + a.commit(); + b.revert_to(&target).unwrap(); + b.commit(); + sync(&a, &b); + assert_replays_to(&a, &expected, "peer a"); + assert_replays_to(&b, &expected, "peer b"); + assert_eq!(tree_ids(&a).len(), 1); +} + +/// A revert that keeps a tree node must keep its TreeID, so a concurrent edit to that node +/// on another peer survives the merge. +#[test] +fn revert_keeps_tree_node_for_concurrent_meta_edit() { + let a = doc(); + let tree = a.get_map("m").ensure_mergeable_tree("tree").unwrap(); + let n = tree.create(TreeParentId::Root).unwrap(); + tree.get_meta(n).unwrap().insert("v", "keep").unwrap(); + a.commit(); + let target = a.state_frontiers(); + tree.create(TreeParentId::Root).unwrap(); + a.commit(); + let b = peer(&a, 2); + + a.get_map("m").delete("tree").unwrap(); + a.commit(); + a.revert_to(&target).unwrap(); a.commit(); b.get_map("m") - .ensure_mergeable_text("s") + .ensure_mergeable_tree("tree") + .unwrap() + .get_meta(n) .unwrap() - .insert(5, "!") + .insert("remote", "REMOTE") .unwrap(); b.commit(); - a.import(&b.export(loro::ExportMode::all_updates()).unwrap()) - .unwrap(); - assert_eq!(json(&a), serde_json::json!({"m": {}})); - assert!(undo.undo().unwrap()); - a.commit(); - assert_replays_to(&a, &serde_json::json!({"m": {"s": "hello"}}), "undo"); + sync(&a, &b); + + for d in [&a, &b] { + assert_eq!(tree_ids(d), vec![n]); + let meta = d + .get_map("m") + .ensure_mergeable_tree("tree") + .unwrap() + .get_meta(n) + .unwrap(); + assert_eq!( + meta.get_deep_value().to_json_value(), + json!({"v": "keep", "remote": "REMOTE"}) + ); + } +} + +/// Undoing a local delete only restores the parent marker, so a remote edit made to the +/// hidden child survives regardless of whether it arrives before or after the undo. +#[test] +fn undo_delete_keeps_concurrent_remote_edit_in_either_order() { + for import_before_undo in [true, false] { + let a = doc(); + a.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + a.commit(); + let b = peer(&a, 2); + let mut undo = UndoManager::new(&a); + delete_s(&a); + a.commit(); + b.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(5, "!") + .unwrap(); + b.commit(); + let remote = b.export(loro::ExportMode::all_updates()).unwrap(); + if import_before_undo { + a.import(&remote).unwrap(); + assert_eq!(json(&a), json!({"m": {}})); + } + assert!(undo.undo().unwrap()); + a.commit(); + if !import_before_undo { + a.import(&remote).unwrap(); + } + assert_replays_to( + &a, + &json!({"m": {"s": "hello!"}}), + &format!("undo, import_before_undo={import_before_undo}"), + ); + } +} + +/// `revert_to` targets a version: a remote edit already observed is reverted, one that +/// arrives afterwards merges, exactly as for a root text. +#[test] +fn revert_after_remote_edit_to_hidden_child_behaves_like_root_text() { + for import_before_revert in [true, false] { + let a = doc(); + a.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + a.commit(); + let target = a.state_frontiers(); + let b = peer(&a, 2); + delete_s(&a); + a.commit(); + b.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(5, "!") + .unwrap(); + b.commit(); + let remote = b.export(loro::ExportMode::all_updates()).unwrap(); + if import_before_revert { + a.import(&remote).unwrap(); + } + a.revert_to(&target).unwrap(); + a.commit(); + if !import_before_revert { + a.import(&remote).unwrap(); + } + let expected = if import_before_revert { + "hello" + } else { + "hello!" + }; + assert_replays_to(&a, &json!({"m": {"s": expected}}), "revert"); + } } From 1605c7be44829e6a6afb84797c86d0410ba57656 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 04:00:50 +0800 Subject: [PATCH 3/6] fix: keep full-state events for re-activated mergeable children Restore the public `diff()`/event contract (a child that becomes visible carries its full state) and record the actual change of containers that kept their state (`KeptChange`) next to it. Revert and undo apply the changes; `apply_diff` aligns full states with the target's hidden state; the undo manager transforms against the changes; local transactions that re-activate a mergeable child now emit its full state too. Rationale: context/mergeable-containers.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../mergeable-revert-duplicate-content.md | 2 +- context/mergeable-containers.md | 103 +++--- crates/loro-internal/src/diff_calc.rs | 2 + crates/loro-internal/src/event.rs | 47 +++ crates/loro-internal/src/handler.rs | 11 +- .../loro-internal/src/handler/full_state.rs | 278 ++++++++++++++++ crates/loro-internal/src/loro.rs | 99 +++++- crates/loro-internal/src/state.rs | 307 +++++++++++++----- crates/loro-internal/src/txn.rs | 1 + crates/loro-internal/src/undo.rs | 36 +- crates/loro-wasm/tests/mergeable.test.ts | 46 ++- crates/loro/tests/mergeable_revert.rs | 280 +++++++++++++++- 12 files changed, 1064 insertions(+), 148 deletions(-) create mode 100644 crates/loro-internal/src/handler/full_state.rs diff --git a/.changeset/mergeable-revert-duplicate-content.md b/.changeset/mergeable-revert-duplicate-content.md index b11cb83e2..03ac73daf 100644 --- a/.changeset/mergeable-revert-duplicate-content.md +++ b/.changeset/mergeable-revert-duplicate-content.md @@ -2,4 +2,4 @@ "loro-crdt": patch --- -Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. The restored child now matches the target version and keeps its characters, list elements and tree node ids, so only the real difference is written. `diff()` now returns a delta relative to the hidden child's state for a re-activated mergeable child, not its full content. Checkout and import events are unchanged. +Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. `revertTo` and undo now write only the real difference and keep the child's characters, list elements and tree node ids. `diff()` and events still report the restored child with its full content, and `applyDiff` now reconciles that content with whatever hidden state the receiving doc has, so it also works on a doc that never saw the child. Local events for a restored mergeable child now carry its full content too, like import and checkout events. `UndoManager` keeps a peer's own edits undoable after a remote undo restores the child. diff --git a/context/mergeable-containers.md b/context/mergeable-containers.md index 92f8e2ec0..84c94ef0e 100644 --- a/context/mergeable-containers.md +++ b/context/mergeable-containers.md @@ -79,46 +79,68 @@ rewrites the marker. - Detached map handlers cannot ensure mergeable children, because the deterministic child cid depends on the attached parent cid. -## Revert, Apply-Diff, And Undo - -When a container becomes reachable again, `DocState::apply_diff` (`state.rs`) -normally records its *full* state as a from-empty diff. This is called revival, and it -is what checkout/import subscribers need. `LoroDoc::_apply_diff` recreates regular -containers under fresh ids, so full state is right for them. A mergeable child is -different: it is re-activated at its deterministic cid, and a doc at the diff's start -version still holds its hidden state there. Applying a full state on top used to -duplicate it (`"hello"` -> `"hellohello"`, counter 7 -> 14). - -`LoroDoc::diff` and the undo diff closure record with -`DocState::mergeable_revival_as_delta` set. In that mode, a mergeable child that is -re-activated by the diff of a container that keeps its id (not itself revived) is not -revived. Its own calculator delta, or nothing when it has no ops in range, becomes the -batch entry. `_apply_diff` stays purely incremental. So: - -- revert/diff/undo keep char, element and `TreeID` identity. Only the real difference is - written; hidden content that already matches costs only the parent marker op. -- Concurrent reverts of the same state delete the same elements and merge to the - target. Concurrent edits to kept elements (e.g. a tree node's meta) survive. -- Local events of a revert are increments on the hidden state. Forwarding them to a doc - in the same state through `apply_diff` reproduces the source state. -- A mergeable child whose parent *is* revived under a fresh id (e.g. a map in a deleted - list element) resolves to a fresh deterministic cid, so it still gets full state. +## Revert, Apply-Diff, Events, And Undo + +A mergeable child that becomes visible again keeps its deterministic cid and its hidden +state. Two consumers need different things: + +- Anything that did not see the hidden state (JS mirrors, loro-js, another doc, a doc + built only from the visible state) needs the child's **full state**. +- The doc that kept the state (its own revert and undo) needs the **actual change**. So + does the undo manager's transform base, because undo positions live in the kept + content. Applying a full state there duplicated content (`"hello"` -> `"hellohello"`, + counter 7 -> 14), and transforming against it broke later undo steps. + +The contract keeps both: + +- **Events and `LoroDoc::diff` report full state**, as on main. That covers import, + checkout, and local transactions. A re-activated mergeable child, and every container it + holds that kept its id, appears with its full state; no entry means empty. Import and + checkout get this from revival in `DocState::apply_diff`. Local transactions get it from + `DocState::revive_mergeable_in_local_diff` (in `commit_txn`), triggered by a local map + diff whose value is a mergeable container. +- **`KeptChange`** (`event.rs`) sits next to such full states on `InternalContainerDiff` + and `ContainerDiff::kept`, and holds the actual change. `DocState::apply_diff` tracks the + `kept` set: a mergeable child re-activated by a container that is not itself revived, plus + what a kept container holds, except containers its own change inserts (those are new, and + their full state is their change). A mergeable child under a parent revived under a fresh + id is not kept: `apply_diff` gives it a fresh cid. `ContainerDiff::change()` returns the + actual change. +- **Revert and undo** build their batch with `DiffBatch::from_changes` and apply it with + alignment off. Only the real difference is written, keeping char/element/`TreeID` + identity. Unchanged hidden content costs only the parent marker op. +- **Public `apply_diff`** (`_apply_diff(.., align_revived_mergeable = true)`) aligns a + re-activated mergeable child's full state with whatever the target doc holds at that cid + (`handler/full_state.rs`), whether that is nothing, the same state, or a different + state: + - Map: per key. + - Counter: adds the difference. + - Text and List/MovableList: keep the common prefix and suffix (chars with equal + styles; values, containers by id) and replace the middle. + - Tree: by `TreeID`. Missing nodes are created, kept nodes are moved if their parent or + position differs, the rest are deleted. + - Kept children are handed down as further full-state targets. A target with no entry is + cleared. +- **UndoManager** composes remote/paused events with `change()`, not `diff`. Consequences: -- `LoroDoc::diff` output for a re-activated mergeable child is a delta relative to its - hidden state, not a mirror-friendly full state. Checkout/import events are unchanged - and still revive with full state. -- `revert_to` behaves like it does for a root container: a remote edit to the hidden - child that was already observed is reverted, and one that arrives later merges. -- Undo of a delete only restores the marker, so remote edits made while the child was - hidden survive in either arrival order. -- Counters stay additive: two peers reverting 10 -> 7 concurrently give 4, as for a - root counter. - -Covered by `crates/loro/tests/mergeable_revert.rs` (including event forwarding, concurrent -reverts, and tree meta edits), `tests/mergeable_container/delete.rs`, and -`crates/loro-wasm/tests/mergeable.test.ts`. +- `source.diff(D, T)` applied to a doc that never saw the child restores it. +- Local events forwarded to another doc (same or no hidden state) reproduce the source. +- A peer can still undo its own edit to a child that a remote undo re-activated. +- Concurrent reverts of the same state delete the same elements and merge to the target. + A concurrent edit to a kept element, such as a tree node's meta, survives. +- `revert_to` behaves like it does for a root container: a remote edit to the hidden child + that was already observed is reverted, and one that arrives later merges. Undo of a delete + only restores the marker, so such remote edits survive in either order. +- Counters stay additive: two peers reverting 10 -> 7 concurrently give 4, as for a root + counter. +- Alignment in `apply_diff` of a different hidden state is content-based. Its edits are + new ops of the applying doc; identity is kept only for the shared prefix/suffix and for + tree nodes. + +Covered by `crates/loro/tests/mergeable_revert.rs`, `tests/mergeable_container/delete.rs`, +and `crates/loro-wasm/tests/mergeable.test.ts`. ## Snapshot And Retention Rules @@ -161,9 +183,10 @@ raw/shallow storage may still show the binary marker for forward compatibility. by the parent marker. - "Deleting the key deletes the child state." False; it hides the child by removing the marker. -- "A re-activated mergeable child needs a full-state revival in a diff that will be - applied to a Loro doc." False; the target doc still holds the hidden state, so - `LoroDoc::diff`/undo record the child's delta instead. +- "A full-state revival of a re-activated mergeable child is its change." False; it is + what a consumer that never saw the child needs. The doc that kept the child uses + `KeptChange` (revert, undo, undo transforms), and `apply_diff` aligns the full state + with the target's hidden state. - "Kind conflict discards the loser." False; the loser is hidden but should stay recoverable by deterministic cid. - "The marker is the child cid." False; the marker activates a kind at a diff --git a/crates/loro-internal/src/diff_calc.rs b/crates/loro-internal/src/diff_calc.rs index 28a620563..9f32035d5 100644 --- a/crates/loro-internal/src/diff_calc.rs +++ b/crates/loro-internal/src/diff_calc.rs @@ -392,6 +392,7 @@ impl DiffCalculator { bring_back, diff: diff.into(), diff_mode, + kept: None, }, ), ); @@ -417,6 +418,7 @@ impl DiffCalculator { bring_back: true, diff: DiffVariant::None, diff_mode: DiffMode::Checkout, + kept: None, }, ), ); diff --git a/crates/loro-internal/src/event.rs b/crates/loro-internal/src/event.rs index ee9abab7a..a41e9291b 100644 --- a/crates/loro-internal/src/event.rs +++ b/crates/loro-internal/src/event.rs @@ -28,6 +28,51 @@ pub struct ContainerDiff { pub(crate) idx: ContainerIdx, pub is_unknown: bool, pub diff: Diff, + /// Set when `diff` is a full-state revival of a container that kept its id and state. + pub(crate) kept: Option, +} + +impl ContainerDiff { + /// The container's actual change in this event, which differs from `diff` when `diff` is + /// a full-state revival of a container that kept its state. `None` means unchanged. + pub(crate) fn change(&self) -> Option<&Diff> { + match &self.kept { + None => Some(&self.diff), + Some(KeptChange::Unchanged) => None, + Some(KeptChange::Changed(d)) => Some(d), + } + } +} + +/// Events and [`crate::undo::DiffBatch`]es describe a container that becomes visible with its +/// full state, which is what a consumer that never saw it needs. A re-activated mergeable child +/// (and every container it holds) keeps its id and its hidden state, so its actual change is +/// recorded next to the full state. Undo, revert and the undo manager's transforms use it. +/// See context/mergeable-containers.md. +#[derive(Debug, Clone)] +pub(crate) enum KeptChange { + Unchanged, + Changed(Diff), +} + +impl KeptChange { + pub(crate) fn from_diff(diff: Diff) -> Self { + if diff.is_empty() { + KeptChange::Unchanged + } else { + KeptChange::Changed(diff) + } + } + + /// `self` followed by `next`. + pub(crate) fn then(self, next: KeptChange) -> Self { + match (self, next) { + (KeptChange::Unchanged, x) | (x, KeptChange::Unchanged) => x, + (KeptChange::Changed(a), KeptChange::Changed(b)) => { + KeptChange::Changed(a.compose(b).unwrap()) + } + } + } } /// The kind of the event trigger. @@ -99,6 +144,8 @@ pub(crate) struct InternalContainerDiff { pub(crate) diff: DiffVariant, /// This mode decides how should we apply the diff. pub(crate) diff_mode: DiffMode, + /// See [`ContainerDiff::kept`]. + pub(crate) kept: Option, } #[derive(Default, Debug, Clone, EnumAsInner)] diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 4342ec7f7..d6c77d4bb 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -23,14 +23,16 @@ use loro_common::{ ContainerID, ContainerType, IdFull, InternalString, LoroError, LoroResult, LoroValue, PeerID, TreeID, ID, }; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use serde::{Deserialize, Serialize}; use std::{borrow::Cow, cmp::Reverse, collections::BinaryHeap, fmt::Debug, ops::Deref, sync::Arc}; use tracing::{error, instrument}; pub use crate::diff::diff_impl::UpdateOptions; pub use tree::TreeHandler; +mod full_state; mod movable_list_apply_delta; +pub(crate) use full_state::align_full_state; mod tree; const REGULAR_CONTAINER_VALUE_ARG_ERROR: &str = @@ -1167,6 +1169,7 @@ impl Handler { key: &str, old_id: ContainerID, on_container_remap: &mut dyn FnMut(ContainerID, ContainerID), + full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { if old_id.is_mergeable() { let parent_id = map.id(); @@ -1174,6 +1177,9 @@ impl Handler { let new_id = ContainerID::new_mergeable(&parent_id, key, kind); let marker = loro_common::mergeable_marker(&parent_id, key, kind); map.insert_without_skipping(key, marker)?; + // The deterministic cid may still hold hidden state, while the child's diff in + // this batch is its full target state. Reconcile instead of appending. + full_state_targets.insert(new_id.clone()); on_container_remap(old_id, new_id); return Ok(()); } @@ -1286,6 +1292,7 @@ impl Handler { &self, diff: Diff, container_remap: &mut FxHashMap, + full_state_targets: &mut FxHashSet, ) -> LoroResult<()> { // In this method we will not clone the values of the containers if // they are remapped. It's the caller's duty to do so @@ -1312,6 +1319,7 @@ impl Handler { &key, h.id(), on_container_remap, + full_state_targets, )?; } Some(ValueOrHandler::Value(LoroValue::Container(old_id))) => { @@ -1320,6 +1328,7 @@ impl Handler { &key, old_id, on_container_remap, + full_state_targets, )?; } Some(ValueOrHandler::Value(v)) => { diff --git a/crates/loro-internal/src/handler/full_state.rs b/crates/loro-internal/src/handler/full_state.rs new file mode 100644 index 000000000..549827433 --- /dev/null +++ b/crates/loro-internal/src/handler/full_state.rs @@ -0,0 +1,278 @@ +//! Applying a full-state diff to a re-activated mergeable child in `LoroDoc::apply_diff`. +//! +//! Events and `LoroDoc::diff` describe a container that becomes visible with its full state. +//! `apply_diff` recreates a regular container under a fresh id, so a full state is exactly +//! right for it. A mergeable child is re-activated at its deterministic cid, which may already +//! hold hidden state in the target doc (the same, different, or none). [`align_full_state`] +//! turns the full state into an edit of that hidden state, keeping the characters, elements, +//! entries and tree nodes the two share. See context/mergeable-containers.md. + +use super::{Handler, ValueOrHandler}; +use crate::{ + delta::{ResolvedMapDelta, ResolvedMapValue, TreeDiff, TreeDiffItem, TreeExternalDiff}, + event::{Diff, ListDeltaMeta, ListDiff, TextDiff, TextMeta}, + state::TreeParentId, + utils::string_slice::StringSlice, +}; +use fractional_index::FractionalIndex; +use loro_common::{ContainerID, InternalString, LoroResult, LoroValue, TreeID}; +use loro_delta::{array_vec::ArrayVec, DeltaItem, DeltaRope}; +use rustc_hash::{FxHashMap, FxHashSet}; + +/// Given `target`, a full-state diff for `handler`, and `current`, the handler's current state +/// as a full-state diff, returns the edit that turns `current` into `target` (or applies it +/// directly). Returns `target` unchanged if it is not a full-state diff. +/// +/// Child containers that keep their id are added to `full_state_targets`: their own diffs in +/// the batch are full states as well. +pub(crate) fn align_full_state( + handler: &Handler, + target: Diff, + current: Diff, + full_state_targets: &mut FxHashSet, +) -> LoroResult> { + #[cfg(not(feature = "counter"))] + let _ = handler; + match (target, current) { + (Diff::Map(target), Diff::Map(current)) => { + Ok(align_map(target, current, full_state_targets).map(Diff::Map)) + } + #[cfg(feature = "counter")] + (Diff::Counter(target), Diff::Counter(current)) => { + if let Handler::Counter(counter) = handler { + let delta = target - current; + if delta != 0.0 { + counter.increment(delta)?; + } + } + Ok(None) + } + (Diff::Text(target), Diff::Text(current)) => Ok(match align_text(&target, ¤t) { + Some(edit) => (!edit.is_empty()).then_some(Diff::Text(edit)), + None => Some(Diff::Text(target)), + }), + (Diff::List(target), Diff::List(current)) => { + Ok(match align_list(&target, ¤t, full_state_targets) { + Some(edit) => (!edit.is_empty()).then_some(Diff::List(edit)), + None => Some(Diff::List(target)), + }) + } + (Diff::Tree(target), Diff::Tree(current)) => { + Ok(match align_tree(&target, ¤t, full_state_targets) { + Some(edit) => (!edit.diff.is_empty()).then_some(Diff::Tree(edit)), + None => Some(Diff::Tree(target)), + }) + } + (Diff::Unknown, _) => Ok(None), + (target, _) => Ok(Some(target)), + } +} + +fn map_value(v: &ResolvedMapValue) -> Option { + v.value.as_ref().map(ValueOrHandler::to_value) +} + +/// Per key, so entries (and the child containers they hold) that already match are kept. +fn align_map( + target: ResolvedMapDelta, + current: ResolvedMapDelta, + full_state_targets: &mut FxHashSet, +) -> Option { + let current: FxHashMap = current + .updated + .iter() + .filter_map(|(k, v)| Some((k.clone(), map_value(v)?))) + .collect(); + let mut updated = FxHashMap::default(); + let mut target_keys = FxHashSet::default(); + for (key, value) in target.updated { + let Some(target_value) = map_value(&value) else { + continue; + }; + target_keys.insert(key.clone()); + if current.get(&key) == Some(&target_value) { + if let LoroValue::Container(id) = target_value { + full_state_targets.insert(id); + } + } else { + updated.insert(key, value); + } + } + for key in current.keys() { + if !target_keys.contains(key) { + updated.insert(key.clone(), ResolvedMapValue::new_unset()); + } + } + (!updated.is_empty()).then_some(ResolvedMapDelta { updated }) +} + +/// Length of one char in the units of text diffs (see `StringSlice::rle_len`). +fn char_len(c: char) -> usize { + if cfg!(feature = "wasm") { + c.len_utf16() + } else { + 1 + } +} + +/// `None` if `diff` is not a pure insertion. +fn text_chars(diff: &TextDiff) -> Option> { + let mut ans = Vec::new(); + for item in diff.iter() { + let DeltaItem::Replace { + value, + attr, + delete: 0, + } = item + else { + return None; + }; + ans.extend(value.as_str().chars().map(|c| (c, attr))); + } + Some(ans) +} + +/// Lengths of the longest common prefix and of the longest common suffix of the rest. +fn common_ends(a: &[T], b: &[T]) -> (usize, usize) { + let prefix = a.iter().zip(b).take_while(|(x, y)| x == y).count(); + let suffix = a[prefix..] + .iter() + .rev() + .zip(b[prefix..].iter().rev()) + .take_while(|(x, y)| x == y) + .count(); + (prefix, suffix) +} + +/// Keeps the common prefix and suffix (chars with equal styles) and replaces the middle. +fn align_text(target: &TextDiff, current: &TextDiff) -> Option { + let target = text_chars(target)?; + let current = text_chars(current)?; + let (prefix, suffix) = common_ends(&target, ¤t); + let mut edit = TextDiff::new(); + if prefix == target.len() && prefix == current.len() { + return Some(edit); + } + let retain: usize = current[..prefix].iter().map(|(c, _)| char_len(*c)).sum(); + let delete: usize = current[prefix..current.len() - suffix] + .iter() + .map(|(c, _)| char_len(*c)) + .sum(); + edit.push_retain(retain, TextMeta::default()); + edit.push_delete(delete); + let middle = &target[prefix..target.len() - suffix]; + let mut i = 0; + while i < middle.len() { + let attr = middle[i].1; + let run: String = middle[i..] + .iter() + .take_while(|(_, a)| *a == attr) + .map(|(c, _)| *c) + .collect(); + i += run.chars().count(); + edit.push_insert(StringSlice::from(run), attr.clone()); + } + Some(edit) +} + +/// `None` if `diff` is not a pure insertion. +fn list_items(diff: &ListDiff) -> Option> { + let mut ans = Vec::new(); + for item in diff.iter() { + let DeltaItem::Replace { + value, delete: 0, .. + } = item + else { + return None; + }; + ans.extend(value.iter()); + } + Some(ans) +} + +/// Keeps the common prefix and suffix (equal values, containers by id) and replaces the middle. +fn align_list( + target: &ListDiff, + current: &ListDiff, + full_state_targets: &mut FxHashSet, +) -> Option { + let target = list_items(target)?; + let current = list_items(current)?; + let target_values: Vec = target.iter().map(|v| v.to_value()).collect(); + let current_values: Vec = current.iter().map(|v| v.to_value()).collect(); + let (prefix, suffix) = common_ends(&target_values, ¤t_values); + let kept = target_values[..prefix] + .iter() + .chain(&target_values[target_values.len() - suffix..]); + for v in kept { + if let LoroValue::Container(id) = v { + full_state_targets.insert(id.clone()); + } + } + let mut edit: ListDiff = DeltaRope::new(); + if prefix == target.len() && prefix == current.len() { + return Some(edit); + } + edit.push_retain(prefix, ListDeltaMeta::default()); + edit.push_delete(current.len() - prefix - suffix); + for v in &target[prefix..target.len() - suffix] { + edit.push_insert(ArrayVec::from([(*v).clone()]), ListDeltaMeta::default()); + } + Some(edit) +} + +/// Keeps nodes by `TreeID`: creates missing ones, moves kept ones whose parent or position +/// differ, and deletes the rest. +fn align_tree( + target: &TreeDiff, + current: &TreeDiff, + full_state_targets: &mut FxHashSet, +) -> Option { + let mut current_nodes: FxHashMap = + FxHashMap::default(); + for item in current.diff.iter() { + let TreeExternalDiff::Create { + parent, + index, + position, + } = &item.action + else { + return None; + }; + current_nodes.insert(item.target, (*parent, position, *index)); + } + let mut edit = TreeDiff::default(); + let mut target_nodes = FxHashSet::default(); + for item in target.diff.iter() { + let TreeExternalDiff::Create { + parent, position, .. + } = &item.action + else { + return None; + }; + target_nodes.insert(item.target); + match current_nodes.get(&item.target) { + Some((p, pos, _)) if p == parent && *pos == position => { + full_state_targets.insert(item.target.associated_meta_container()); + } + Some(_) => { + // `apply_diff` moves a `Create` target that is still alive. + full_state_targets.insert(item.target.associated_meta_container()); + edit.diff.push(item.clone()); + } + None => edit.diff.push(item.clone()), + } + } + for (id, (parent, _, index)) in current_nodes { + if !target_nodes.contains(&id) { + edit.diff.push(TreeDiffItem { + target: id, + action: TreeExternalDiff::Delete { + old_parent: parent, + old_index: index, + }, + }); + } + } + Some(edit) +} diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index fea588466..6fbaeafa3 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -1233,14 +1233,15 @@ impl LoroDoc { { let mut state = self.state.lock(); state.start_recording(); - state.set_mergeable_revival_as_delta(true); + state.set_record_changes_only(true); } self._checkout_without_emitting(to, false, false).unwrap(); let mut state = self.state.lock(); - state.set_mergeable_revival_as_delta(false); + state.set_record_changes_only(false); let e = state.take_events(); state.stop_and_clear_recording(); - DiffBatch::new(e) + // Applied to this doc, which kept the state of re-activated mergeable children. + DiffBatch::from_changes(e) }, before_diff, ); @@ -1258,7 +1259,7 @@ impl LoroDoc { // Try applying the diff, but ignore the error if it happens. // MovableList's undo behavior is too tricky to handle in a collaborative env // so in edge cases this may be an Error - if let Err(e) = self._apply_diff(diff, container_remap, true) { + if let Err(e) = self._apply_diff(diff, container_remap, true, false) { warn!("Undo Failed {:?}", e); } @@ -1280,8 +1281,15 @@ impl LoroDoc { // TODO: test when the doc is readonly // TODO: test when the doc is detached but enabled editing let f = self.state_frontiers(); - let diff = self.diff(&f, target)?; - self._apply_diff(diff, &mut Default::default(), false) + let diff = self.diff_events(&f, target, true)?; + // This doc kept the state of the mergeable children the target re-activates, so apply + // their actual changes rather than the full states `diff` reports. + self._apply_diff( + DiffBatch::from_changes(diff), + &mut Default::default(), + false, + false, + ) } /// Calculate the diff between two versions so that apply diff on a will make the state same as b. @@ -1289,6 +1297,16 @@ impl LoroDoc { /// NOTE: This method will make the doc enter the **detached mode**. // FIXME: This method needs testing (no event should be emitted during processing this) pub fn diff(&self, a: &Frontiers, b: &Frontiers) -> LoroResult { + self.diff_events(a, b, false).map(DiffBatch::new) + } + + /// With `changes_only`, only [`DiffBatch::from_changes`] of the result is meaningful. + fn diff_events( + &self, + a: &Frontiers, + b: &Frontiers, + changes_only: bool, + ) -> LoroResult> { { // Check whether a and b are valid before checkout so this returns a normal error // instead of panicking on shallow docs. @@ -1325,13 +1343,11 @@ impl LoroDoc { { let mut state = self.state.lock(); state.start_recording(); - // The batch is applied to a doc at `a`, which still holds the hidden state of - // mergeable children that `b` re-activates. - state.set_mergeable_revival_as_delta(true); + state.set_record_changes_only(changes_only); } let checkout = self._checkout_without_emitting(b, true, false); let mut state = self.state.lock(); - state.set_mergeable_revival_as_delta(false); + state.set_record_changes_only(false); checkout?; let e = state.take_events(); state.stop_and_clear_recording(); @@ -1349,13 +1365,13 @@ impl LoroDoc { if was_recording { self.state.lock().start_recording(); } - result.map(DiffBatch::new) + result } /// Apply a diff to the current state. #[inline(always)] pub fn apply_diff(&self, diff: DiffBatch) -> LoroResult<()> { - self._apply_diff(diff, &mut Default::default(), true) + self._apply_diff(diff, &mut Default::default(), true, true) } /// Apply a diff to the current state. @@ -1369,11 +1385,18 @@ impl LoroDoc { /// /// However, the diff may contain operations that depend on container IDs. /// Therefore, users need to provide a `container_remap` to record and retrieve the container ID remapping. + /// + /// With `align_revived_mergeable`, `diff` follows the event contract: a mergeable child the + /// batch re-activates carries its full state (no entry means empty), which is aligned with + /// whatever hidden state this doc has at its deterministic cid. Revert and undo pass + /// batches of actual changes ([`DiffBatch::from_changes`]) and disable it. See + /// context/mergeable-containers.md. pub(crate) fn _apply_diff( &self, diff: DiffBatch, container_remap: &mut FxHashMap, skip_unreachable: bool, + align_revived_mergeable: bool, ) -> LoroResult<()> { if !self.can_edit() { return Err(LoroError::EditWhenDetached); @@ -1381,6 +1404,11 @@ impl LoroDoc { let mut ans: LoroResult<()> = Ok(()); let mut missing_containers: Vec = Vec::new(); + // Containers whose diff in this batch is a full state while they keep an id that may + // already hold state: re-activated mergeable children and, transitively, the children + // of aligned containers that were kept. + let mut full_state_targets: FxHashSet = FxHashSet::default(); + let mut applied: FxHashSet = FxHashSet::default(); for (mut id, diff) in diff.into_iter() { let mut remapped = false; while let Some(rid) = container_remap.get(&id) { @@ -1388,6 +1416,11 @@ impl LoroDoc { id = rid.clone(); } + let is_full_state = align_revived_mergeable && full_state_targets.remove(&id); + if align_revived_mergeable { + applied.insert(id.clone()); + } + if matches!(&id, ContainerID::Normal { .. }) && self.arena.id_to_idx(&id).is_none() { // Not in arena does not imply non-existent; consult state/kv and register lazily let exists = self.state.lock().does_container_exist(&id); @@ -1399,7 +1432,11 @@ impl LoroDoc { self.state.lock().ensure_container(&id); } - if skip_unreachable && !remapped && !self.state.lock().get_reachable(&id) { + if skip_unreachable + && !remapped + && !is_full_state + && !self.state.lock().get_reachable(&id) + { continue; } @@ -1408,11 +1445,45 @@ impl LoroDoc { containers: Box::new(vec![id]), }); }; - if let Err(e) = h.apply_diff(diff, container_remap) { + let diff = if is_full_state { + let current = self.state.lock().container_full_diff(h.container_idx()); + match crate::handler::align_full_state(&h, diff, current, &mut full_state_targets) { + Ok(Some(diff)) => diff, + Ok(None) => continue, + Err(e) => { + ans = Err(e); + continue; + } + } + } else { + diff + }; + if let Err(e) = h.apply_diff(diff, container_remap, &mut full_state_targets) { ans = Err(e); } } + if align_revived_mergeable { + // A full-state container without an entry in the batch is empty. + for id in full_state_targets { + if applied.contains(&id) { + continue; + } + let Some(h) = self.get_handler(id) else { + continue; + }; + let is_empty = self + .state + .lock() + .is_container_state_empty(h.container_idx()); + if !is_empty { + if let Err(e) = h.clear() { + ans = Err(e); + } + } + } + } + if !missing_containers.is_empty() { return Err(LoroError::ContainersNotFound { containers: Box::new(missing_containers), diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index 78c565225..7b6cb7970 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -22,7 +22,7 @@ use crate::{ cursor::{Cursor, PosType}, delta::TreeExternalDiff, diff_calc::{DiffCalculator, DiffMode}, - event::{Diff, EventTriggerKind, Index, InternalContainerDiff, InternalDiff}, + event::{Diff, EventTriggerKind, Index, InternalContainerDiff, InternalDiff, KeptChange}, fx_map, handler::ValueOrHandler, id::PeerID, @@ -174,12 +174,10 @@ pub struct DocState { dead_containers_cache: DeadContainersCache, alive_containers_cache: Option, - /// Set while recording a diff that will be applied to a doc at the start version - /// (`LoroDoc::diff`, undo). A mergeable child re-activated inside a container that - /// keeps its id then records its own delta instead of a full-state revival, because the - /// target doc still holds the child's hidden state at the same deterministic cid. See - /// context/mergeable-containers.md. - mergeable_revival_as_delta: bool, + /// Set while recording for a caller that only uses actual changes + /// ([`crate::undo::DiffBatch::from_changes`]: revert, undo). Containers that keep their + /// state are then not revived at all, which skips building full states nobody reads. + record_changes_only: bool, } struct AliveContainersCache { @@ -505,7 +503,7 @@ impl DocState { event_recorder: Default::default(), dead_containers_cache: Default::default(), alive_containers_cache: None, - mergeable_revival_as_delta: false, + record_changes_only: false, }, crate::lock::LockKind::DocState, )) @@ -531,7 +529,7 @@ impl DocState { event_recorder: Default::default(), dead_containers_cache: Default::default(), alive_containers_cache: None, - mergeable_revival_as_delta: false, + record_changes_only: false, })) } @@ -727,6 +725,10 @@ impl DocState { diffs.sort_by_cached_key(|diff| self.arena.get_depth(diff.idx)); let mut to_revive_in_next_layer: FxHashSet = FxHashSet::default(); let mut to_revive_in_this_layer: FxHashSet = FxHashSet::default(); + // Revived containers that kept their id and state: a mergeable child re-activated by + // a container that is not itself revived, and what such a container holds. Their event + // still carries the full state; `KeptChange` records their actual change. + let mut kept: FxHashSet = FxHashSet::default(); let mut last_depth = 0; let len = diffs.len(); for mut diff in std::mem::replace(&mut diffs, Vec::with_capacity(len)) { @@ -741,26 +743,11 @@ impl DocState { let to_create = std::mem::take(&mut to_revive_in_this_layer); to_revive_in_this_layer = std::mem::take(&mut to_revive_in_next_layer); for new in to_create { - let state = self.store.get_or_create_mut(new); - if state.is_state_empty() { - continue; + if let Some(d) = + self.revive_without_diff(new, &mut to_revive_in_this_layer, &mut kept) + { + diffs.push(d); } - - let external_diff = state.to_diff(&self.doc); - trigger_on_new_container( - &external_diff, - |cid| { - to_revive_in_this_layer.insert(cid); - }, - &self.arena, - ); - - diffs.push(InternalContainerDiff { - idx: new, - bring_back: true, - diff: external_diff.into(), - diff_mode: DiffMode::Checkout, - }); } last_depth += 1; @@ -795,42 +782,56 @@ impl DocState { if is_recording { let is_revived = diff.bring_back || to_revive_in_this_layer.contains(&idx); + let is_kept = is_revived && kept.contains(&idx); + let ctx = DiffApplyContext { + mode: diff.diff_mode, + doc: &self.doc, + }; + let internal_diff = internal_diff.into_internal().unwrap(); + // Containers the actual change inserts are new to a doc that + // kept this container, so they are revived with full state. + let mut inserted: FxHashSet = FxHashSet::default(); // process bring_back before apply - let external_diff = if is_revived { - state.apply_diff( - internal_diff.into_internal().unwrap(), - DiffApplyContext { - mode: diff.diff_mode, - doc: &self.doc, + let external_diff = if is_kept { + let change = state.apply_diff_and_convert(internal_diff, ctx); + trigger_on_new_container( + &change, + |cid| { + inserted.insert(cid); }, - )?; + &self.arena, + ); + diff.kept = Some(KeptChange::from_diff(change)); + state.to_diff(&self.doc) + } else if is_revived { + state.apply_diff(internal_diff, ctx)?; state.to_diff(&self.doc) } else { - state.apply_diff_and_convert( - internal_diff.into_internal().unwrap(), - DiffApplyContext { - mode: diff.diff_mode, - doc: &self.doc, - }, - ) + state.apply_diff_and_convert(internal_diff, ctx) }; let arena = &self.arena; - let mergeable_as_delta = - self.mergeable_revival_as_delta && !is_revived; + let changes_only = self.record_changes_only; trigger_on_new_container( &external_diff, |cid| { - // This container keeps its id, so a mergeable child - // it re-activates keeps its hidden state; its own - // delta (if any) is what the target doc needs. - if mergeable_as_delta - && arena - .idx_to_id(cid) - .is_some_and(|id| id.is_mergeable()) - { + if changes_only && !is_revived && is_mergeable(arena, cid) { + // It keeps its state; its own diff (if any) is + // its change. return; } to_revive_in_next_layer.insert(cid); + let keeps_state = if is_revived { + is_kept + && (!inserted.contains(&cid) + || is_mergeable(arena, cid)) + } else { + // A container that keeps its id re-activates a + // mergeable child at its deterministic cid. + is_mergeable(arena, cid) + }; + if keeps_state { + kept.insert(cid); + } }, arena, ); @@ -852,7 +853,7 @@ impl DocState { } to_revive_in_this_layer.remove(&idx); - if !diff.diff.is_empty() { + if !diff.diff.is_empty() || matches!(diff.kept, Some(KeptChange::Changed(_))) { diffs.push(diff); } } @@ -861,27 +862,10 @@ impl DocState { while !to_revive_in_this_layer.is_empty() || !to_revive_in_next_layer.is_empty() { let to_create = std::mem::take(&mut to_revive_in_this_layer); for new in to_create { - let state = self.store.get_or_create_mut(new); - if state.is_state_empty() { - continue; - } - - let external_diff = state.to_diff(&self.doc); - trigger_on_new_container( - &external_diff, - |cid| { - to_revive_in_next_layer.insert(cid); - }, - &self.arena, - ); - - if !external_diff.is_empty() { - diffs.push(InternalContainerDiff { - idx: new, - bring_back: true, - diff: external_diff.into(), - diff_mode: DiffMode::Checkout, - }); + if let Some(d) = + self.revive_without_diff(new, &mut to_revive_in_next_layer, &mut kept) + { + diffs.push(d); } } @@ -1068,13 +1052,102 @@ impl DocState { pub(crate) fn commit_txn(&mut self, new_frontiers: Frontiers, diff: Option) { self.in_txn = false; self.frontiers = new_frontiers; - if let Some(diff) = diff { + if let Some(mut diff) = diff { if self.is_recording() { + if let Some(revived) = self.revive_mergeable_in_local_diff(&diff.diff) { + diff.diff = Cow::Owned(revived); + } self.record_diff(diff); } } } + /// A local transaction that writes a mergeable marker makes the child visible with the + /// state it kept. Like import/checkout revival, its event carries that full state (and so + /// do the containers it holds), with the transaction's own ops as the [`KeptChange`]. + /// Returns `None` if the transaction re-activates no mergeable child. + fn revive_mergeable_in_local_diff( + &mut self, + diffs: &[InternalContainerDiff], + ) -> Option> { + let mut queue: Vec = Vec::new(); + for d in diffs { + if let crate::event::DiffVariant::External(Diff::Map(map)) = &d.diff { + for v in map.updated.values() { + if let Some(ValueOrHandler::Handler(h)) = &v.value { + if h.id().is_mergeable() { + queue.push(h.container_idx()); + } + } + } + } + } + if queue.is_empty() { + return None; + } + + let mut rest = Vec::with_capacity(diffs.len()); + for d in diffs { + rest.push(d.clone()); + } + let mut seen: FxHashSet = FxHashSet::default(); + let mut revived = Vec::new(); + while let Some(idx) = queue.pop() { + if !seen.insert(idx) { + continue; + } + // The transaction's own ops on this container, composed. + let mut change: Option = None; + rest.retain(|d| { + if d.idx != idx { + return true; + } + let crate::event::DiffVariant::External(diff) = &d.diff else { + return true; + }; + change = Some(match change.take() { + None => diff.clone(), + Some(c) => c.compose(diff.clone()).unwrap(), + }); + false + }); + // Containers the transaction inserted here are new, and their own diffs are full. + let mut inserted: FxHashSet = FxHashSet::default(); + if let Some(change) = &change { + trigger_on_new_container( + change, + |cid| { + inserted.insert(cid); + }, + &self.arena, + ); + } + let full = self.container_full_diff(idx); + trigger_on_new_container( + &full, + |cid| { + if !inserted.contains(&cid) || is_mergeable(&self.arena, cid) { + queue.push(cid); + } + }, + &self.arena, + ); + let kept = change.map_or(KeptChange::Unchanged, KeptChange::from_diff); + if full.is_empty() && matches!(kept, KeptChange::Unchanged) { + continue; + } + revived.push(InternalContainerDiff { + idx, + bring_back: true, + diff: full.into(), + diff_mode: DiffMode::Linear, + kept: Some(kept), + }); + } + rest.extend(revived); + Some(rest) + } + /// Ensure the container is created and will be encoded in the next `encode` call #[inline] pub(crate) fn ensure_container(&mut self, id: &ContainerID) { @@ -1293,6 +1366,7 @@ impl DocState { .to_diff(&self.doc) .into(), diff_mode: DiffMode::Checkout, + kept: None, }) .collect(); @@ -1328,8 +1402,53 @@ impl DocState { f(state) } - pub(crate) fn set_mergeable_revival_as_delta(&mut self, on: bool) { - self.mergeable_revival_as_delta = on; + pub(crate) fn set_record_changes_only(&mut self, on: bool) { + self.record_changes_only = on; + } + + /// The container's current state as a full-state diff, the shape revival events use. + pub(crate) fn container_full_diff(&mut self, idx: ContainerIdx) -> Diff { + let doc = self.doc.clone(); + self.store.get_or_create_mut(idx).to_diff(&doc) + } + + pub(crate) fn is_container_state_empty(&mut self, idx: ContainerIdx) -> bool { + self.store.get_or_create_mut(idx).is_state_empty() + } + + /// Revives a container that has no diff of its own in this batch: its event is its full + /// state. Children it holds are queued into `to_revive`; if it kept its state, they did too. + fn revive_without_diff( + &mut self, + idx: ContainerIdx, + to_revive: &mut FxHashSet, + kept: &mut FxHashSet, + ) -> Option { + let state = self.store.get_or_create_mut(idx); + if state.is_state_empty() { + return None; + } + + let is_kept = kept.contains(&idx); + let external_diff = state.to_diff(&self.doc); + trigger_on_new_container( + &external_diff, + |cid| { + to_revive.insert(cid); + if is_kept { + kept.insert(cid); + } + }, + &self.arena, + ); + + Some(InternalContainerDiff { + idx, + bring_back: true, + diff: external_diff.into(), + diff_mode: DiffMode::Checkout, + kept: is_kept.then_some(KeptChange::Unchanged), + }) } pub(super) fn is_in_txn(&self) -> bool { @@ -2092,9 +2211,14 @@ impl DocState { for diff in diffs { #[allow(clippy::unnecessary_to_owned)] for container_diff in diff.diff.into_owned() { - let Some((last_container_diff, _)) = containers.get_mut(&container_diff.idx) else { + let Some((last_container_diff, last_kept, _)) = + containers.get_mut(&container_diff.idx) + else { if let Some(path) = self.get_path(container_diff.idx) { - containers.insert(container_diff.idx, (container_diff.diff, path)); + containers.insert( + container_diff.idx, + (container_diff.diff, container_diff.kept, path), + ); } else { // if we cannot find the path to the container, the container must be overwritten afterwards. // So we can ignore the diff from it. @@ -2119,12 +2243,28 @@ impl DocState { // subscriber is attached and many edits land on one container // in a single event batch. let prev = std::mem::take(last_container_diff); - *last_container_diff = prev.compose(container_diff.diff).unwrap(); + if last_kept.is_some() || container_diff.kept.is_some() { + // Track the actual change next to the full-state revival. + let as_change = |kept: Option, diff: &crate::event::DiffVariant| { + kept.unwrap_or_else(|| { + KeptChange::from_diff(diff.clone().into_external().unwrap()) + }) + }; + let prev_change = as_change(last_kept.take(), &prev); + let next_change = as_change(container_diff.kept.clone(), &container_diff.diff); + *last_kept = Some(prev_change.then(next_change)); + } + *last_container_diff = if container_diff.kept.is_some() { + // A full-state revival supersedes what the container showed before. + container_diff.diff + } else { + prev.compose(container_diff.diff).unwrap() + }; } } let mut diff: Vec<_> = containers .into_iter() - .map(|(container, (diff, path))| { + .map(|(container, (diff, kept, path))| { let idx = container; let id = self.arena.get_container_id(idx).unwrap(); let is_unknown = id.is_unknown(); @@ -2135,6 +2275,7 @@ impl DocState { diff: diff.into_external().unwrap(), is_unknown, path, + kept, } }) .collect(); @@ -2588,6 +2729,10 @@ fn create_state_(idx: ContainerIdx, config: &Configure, peer: u64) -> State { } } +fn is_mergeable(arena: &SharedArena, idx: ContainerIdx) -> bool { + arena.idx_to_id(idx).is_some_and(|id| id.is_mergeable()) +} + fn trigger_on_new_container( state_diff: &Diff, mut listener: impl FnMut(ContainerIdx), diff --git a/crates/loro-internal/src/txn.rs b/crates/loro-internal/src/txn.rs index ff207e7fb..9c2ce8ae4 100644 --- a/crates/loro-internal/src/txn.rs +++ b/crates/loro-internal/src/txn.rs @@ -516,6 +516,7 @@ impl Transaction { bring_back: false, diff: (x.diff.into()), diff_mode: crate::diff_calc::DiffMode::Linear, + kept: None, }) .collect(), ), diff --git a/crates/loro-internal/src/undo.rs b/crates/loro-internal/src/undo.rs index f60fa1d32..d960842df 100644 --- a/crates/loro-internal/src/undo.rs +++ b/crates/loro-internal/src/undo.rs @@ -13,7 +13,7 @@ use crate::{ change::{get_sys_timestamp, Timestamp}, cursor::{AbsolutePosition, Cursor}, delta::TreeExternalDiff, - event::{Diff, EventTriggerKind}, + event::{Diff, EventTriggerKind, KeptChange}, version::Frontiers, ContainerDiff, DiffEvent, DocDiff, LoroDoc, Subscription, }; @@ -45,6 +45,31 @@ impl DiffBatch { } } + /// Like [`DiffBatch::new`], but a container that kept its state while its event reports a + /// full-state revival contributes its actual change. Use it for a batch applied to the doc + /// it was computed on (revert, undo). See [`crate::event::KeptChange`]. + pub(crate) fn from_changes(diff: Vec) -> Self { + let mut map: FxHashMap = Default::default(); + let mut order: Vec = Vec::with_capacity(diff.len()); + for d in diff.into_iter() { + for item in d.diff.into_iter() { + let change = match item.kept { + None => item.diff, + Some(KeptChange::Unchanged) => continue, + Some(KeptChange::Changed(change)) => change, + }; + let old = map.insert(item.id.clone(), change); + assert!(old.is_none(), "Duplicate container ID in diff events"); + order.push(item.id); + } + } + + Self { + cid_to_events: map, + order, + } + } + pub fn compose(&mut self, other: &Self) { if other.cid_to_events.is_empty() { return; @@ -420,12 +445,17 @@ impl Stack { let remote_diff = &mut self.stack.back_mut().unwrap().1; let mut remote_diff = remote_diff.lock(); for e in diff { + // Transform against what actually changed. A re-activated mergeable child's event + // is its full state, but its content (and the undo items' positions in it) stayed. + let Some(change) = e.change() else { + continue; + }; if let Some(d) = remote_diff.cid_to_events.get_mut(&e.id) { - d.compose_ref(&e.diff); + d.compose_ref(change); } else { remote_diff .cid_to_events - .insert(e.id.clone(), e.diff.clone()); + .insert(e.id.clone(), change.clone()); remote_diff.order.push(e.id.clone()); } } diff --git a/crates/loro-wasm/tests/mergeable.test.ts b/crates/loro-wasm/tests/mergeable.test.ts index 706cf329c..cedf5b69b 100644 --- a/crates/loro-wasm/tests/mergeable.test.ts +++ b/crates/loro-wasm/tests/mergeable.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test, vi } from "vitest"; -import { LoroDoc } from "../bundler/index"; +import { LoroDoc, UndoManager } from "../bundler/index"; function sync(a: LoroDoc, b: LoroDoc) { const aBytes = a.export({ mode: "update", from: b.version() }); @@ -324,4 +324,48 @@ describe("mergeable containers (WASM bindings)", () => { expect(b.toJSON()).toEqual(a.toJSON()); }); }); + test("diff() restores a deleted mergeable child on a doc that never saw it", () => { + const source = new LoroDoc(); + source.setPeerId(1); + const m = source.getMap("m"); + m.ensureMergeableText("t").insert(0, "hello"); + m.ensureMergeableCounter("c").increment(7); + m.ensureMergeableList("l").push("keep"); + source.commit(); + const target = source.frontiers(); + m.ensureMergeableCounter("c").increment(3); + for (const key of ["t", "c", "l"]) m.delete(key); + source.commit(); + + const mirror = new LoroDoc(); + mirror.getMap("m"); + mirror.applyDiff(source.diff(source.frontiers(), target)); + mirror.commit(); + expect(mirror.toJSON()).toEqual({ m: { t: "hello", c: 7, l: ["keep"] } }); + }); + + test("a peer can undo its edit after a remote undo re-activates the child", () => { + const a = new LoroDoc(); + a.setPeerId(1); + const t = a.getMap("m").ensureMergeableText("s"); + t.insert(0, "hello"); + a.commit(); + const b = a.fork(); + b.setPeerId(2); + const ua = new UndoManager(a, {}); + const ub = new UndoManager(b, {}); + a.getMap("m").delete("s"); + a.commit(); + b.getMap("m").ensureMergeableText("s").insert(5, "!"); + b.commit(); + a.import(b.export({ mode: "update" })); + expect(ua.undo()).toBe(true); + a.commit(); + b.import(a.export({ mode: "update" })); + expect(b.toJSON()).toEqual({ m: { s: "hello!" } }); + expect(ub.canUndo()).toBe(true); + expect(ub.undo()).toBe(true); + b.commit(); + expect(b.toJSON()).toEqual({ m: { s: "hello" } }); + }); }); diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs index b6bc14960..1baadaa82 100644 --- a/crates/loro/tests/mergeable_revert.rs +++ b/crates/loro/tests/mergeable_revert.rs @@ -2,9 +2,10 @@ //! content exactly once. //! //! A mergeable child keeps its state at a deterministic cid while the parent marker is -//! gone, and a doc at the diff's start version still holds that state. So `LoroDoc::diff` -//! and undo record the re-activated child's own delta instead of a full-state revival, -//! which keeps char/element/TreeID identity. See `context/mergeable-containers.md`. +//! gone. Events and `LoroDoc::diff` report a re-activated child with its full state, so any +//! doc or mirror can apply them; `apply_diff` aligns that state with whatever hidden state +//! the target has. Revert and undo apply the child's actual change, which keeps +//! char/element/TreeID identity. See `context/mergeable-containers.md`. use loro::{ event::{Diff, DiffBatch, MapDelta}, @@ -57,6 +58,16 @@ fn forward_events(source: &LoroDoc, mirror: &LoroDoc, op: impl FnOnce(&LoroDoc)) mirror.commit(); } +/// A new doc with `d`'s visible state and none of its history or hidden state. +fn visible_copy(d: &LoroDoc) -> LoroDoc { + let copy = LoroDoc::new(); + copy.set_peer_id(9).unwrap(); + copy.apply_diff(d.diff(&Frontiers::default(), &d.state_frontiers()).unwrap()) + .unwrap(); + copy.commit(); + copy +} + fn doc() -> LoroDoc { let d = LoroDoc::new(); d.set_peer_id(1).unwrap(); @@ -66,8 +77,8 @@ fn doc() -> LoroDoc { d } -/// Deep JSON with tree node ids stripped: reviving a deleted regular tree node through -/// `apply_diff` creates a fresh node id, which is unrelated to this bug. +/// Deep JSON with tree node ids and empty roots stripped: reviving a deleted regular tree +/// node through `apply_diff` creates a fresh node id, which is unrelated to this bug. fn json(d: &LoroDoc) -> Value { fn strip(v: &mut Value) { match v { @@ -84,6 +95,12 @@ fn json(d: &LoroDoc) -> Value { } let mut v = d.get_deep_value().to_json_value(); strip(&mut v); + // Empty root containers only exist in docs that touched them. + v.as_object_mut().unwrap().retain(|_, root| match root { + Value::Array(a) => !a.is_empty(), + Value::Object(o) => !o.is_empty(), + _ => true, + }); v } @@ -145,12 +162,28 @@ fn check(setup: impl Fn(&LoroDoc), mutate: impl Fn(&LoroDoc), reuses_children: b r.commit(); assert_replays_to(&r, &expected_b, "revert_to back to b"); - // Local events of a revert, forwarded to a doc in the same state. + // Local events of a revert, forwarded to a doc in the same state, and to one that only + // has the visible state (built from `diff`, so it never saw the hidden children). let source = d.fork(); let mirror = d.fork(); forward_events(&source, &mirror, |s| s.revert_to(&a).unwrap()); assert_eq!(json(&source), expected_a, "event forwarding: source"); assert_eq!(json(&mirror), expected_a, "event forwarding: mirror"); + let source = d.fork(); + let visible_only = visible_copy(&d); + assert_eq!(json(&visible_only), expected_b, "visible-only copy"); + forward_events(&source, &visible_only, |s| s.revert_to(&a).unwrap()); + assert_eq!( + json(&visible_only), + expected_a, + "event forwarding: visible-only mirror" + ); + + // Public diff applied to a doc that only has the visible state. + let visible_only = visible_copy(&d); + visible_only.apply_diff(d.diff(&b, &a).unwrap()).unwrap(); + visible_only.commit(); + assert_eq!(json(&visible_only), expected_a, "diff to visible-only doc"); // diff + apply_diff let r = d.fork(); @@ -729,7 +762,7 @@ fn undo_delete_keeps_concurrent_remote_edit_in_either_order() { let remote = b.export(loro::ExportMode::all_updates()).unwrap(); if import_before_undo { a.import(&remote).unwrap(); - assert_eq!(json(&a), json!({"m": {}})); + assert_eq!(json(&a), json!({})); } assert!(undo.undo().unwrap()); a.commit(); @@ -783,3 +816,236 @@ fn revert_after_remote_edit_to_hidden_child_behaves_like_root_text() { assert_replays_to(&a, &json!({"m": {"s": expected}}), "revert"); } } + +/// `diff` keeps reporting a re-activated mergeable child with its full state, so applying it +/// restores the target content on docs whose hidden state is absent, the same, or different. +#[test] +fn public_diff_restores_children_whatever_the_target_hidden_state() { + let (d, target, expected) = diverged_children(); + let deleted = d.state_frontiers(); + let diff = || d.diff(&deleted, &target).unwrap(); + + // Never saw the children. + let absent = LoroDoc::new(); + absent.get_map("m"); + absent.apply_diff(diff()).unwrap(); + absent.commit(); + assert_eq!(json(&absent), expected, "absent hidden state"); + + // Same hidden state. + let same = d.fork(); + same.apply_diff(diff()).unwrap(); + same.commit(); + assert_replays_to(&same, &expected, "same hidden state"); + assert_eq!(tree_ids(&same).len(), 1); + + // Different hidden state at the same deterministic cids. + let (other, _, _) = deleted_after_divergence( + |d| { + let m = d.get_map("m"); + m.ensure_mergeable_text("t") + .unwrap() + .insert(0, "xhellz") + .unwrap(); + m.ensure_mergeable_list("l").unwrap().push("other").unwrap(); + m.ensure_mergeable_movable_list("ml") + .unwrap() + .push(1) + .unwrap(); + m.ensure_mergeable_map("nested") + .unwrap() + .ensure_mergeable_text("t") + .unwrap() + .insert(0, "zz") + .unwrap(); + let tree = m.ensure_mergeable_tree("tree").unwrap(); + tree.create(TreeParentId::Root).unwrap(); + }, + |_| {}, + ); + other.set_peer_id(3).unwrap(); + other.apply_diff(diff()).unwrap(); + other.commit(); + assert_replays_to(&other, &expected, "different hidden state"); +} + +#[test] +#[cfg(feature = "counter")] +fn public_diff_restores_counter_whatever_the_target_hidden_state() { + for extra in [0.0, 3.0] { + let (d, target, expected) = deleted_after_divergence( + |d| { + d.get_map("m") + .ensure_mergeable_counter("c") + .unwrap() + .increment(7.0) + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_counter("c") + .unwrap() + .increment(extra) + .unwrap(); + }, + ); + let diff = d.diff(&d.state_frontiers(), &target).unwrap(); + let absent = LoroDoc::new(); + absent.get_map("m"); + absent.apply_diff(diff.clone()).unwrap(); + absent.commit(); + assert_eq!(json(&absent), expected, "absent, extra {extra}"); + let same = d.fork(); + same.apply_diff(diff).unwrap(); + same.commit(); + assert_eq!(json(&same), expected, "same, extra {extra}"); + } +} + +/// A local re-activation (here `ensure_mergeable_*` over a deleted key) reports the child's +/// full state, like import and checkout do, so a mirror without hidden state can follow. +#[test] +fn local_reactivation_event_carries_full_state() { + let d = doc(); + let m = d.get_map("m"); + m.ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + m.ensure_mergeable_list("l").unwrap().push("keep").unwrap(); + d.commit(); + delete_s(&d); + m.delete("l").unwrap(); + d.commit(); + let mirror = visible_copy(&d); + forward_events(&d, &mirror, |d| { + let m = d.get_map("m"); + m.ensure_mergeable_text("s").unwrap(); + m.ensure_mergeable_list("l").unwrap().push("more").unwrap(); + }); + let expected = json!({"m": {"s": "hello", "l": ["keep", "more"]}}); + assert_eq!(json(&d), expected); + assert_eq!(json(&mirror), expected); +} + +/// A re-activates a child that B edited while it was hidden. B's import event reports the +/// child's full state, but its content did not change, so B can still undo its own edit. +#[test] +fn peer_can_undo_own_edit_after_remote_undo_reactivates_child() { + type Edit = fn(&LoroDoc); + let cases: [(&str, Edit, Edit, Value, Value); 4] = [ + ( + "text", + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(5, "!") + .unwrap(); + }, + json!({"m": {"s": "hello!"}}), + json!({"m": {"s": "hello"}}), + ), + ( + "list", + |d| { + d.get_map("m") + .ensure_mergeable_list("s") + .unwrap() + .push("keep") + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_list("s") + .unwrap() + .push("mine") + .unwrap(); + }, + json!({"m": {"s": ["keep", "mine"]}}), + json!({"m": {"s": ["keep"]}}), + ), + ( + "movable list", + |d| { + d.get_map("m") + .ensure_mergeable_movable_list("s") + .unwrap() + .push("keep") + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_movable_list("s") + .unwrap() + .push("mine") + .unwrap(); + }, + json!({"m": {"s": ["keep", "mine"]}}), + json!({"m": {"s": ["keep"]}}), + ), + ( + "nested text", + |d| { + d.get_map("m") + .ensure_mergeable_map("s") + .unwrap() + .ensure_mergeable_text("t") + .unwrap() + .insert(0, "hello") + .unwrap(); + }, + |d| { + d.get_map("m") + .ensure_mergeable_map("s") + .unwrap() + .ensure_mergeable_text("t") + .unwrap() + .insert(5, "!") + .unwrap(); + }, + json!({"m": {"s": {"t": "hello!"}}}), + json!({"m": {"s": {"t": "hello"}}}), + ), + ]; + for (name, setup, edit, merged, after_undo) in cases { + for delete_arrives_first in [false, true] { + let a = doc(); + setup(&a); + a.commit(); + let b = peer(&a, 2); + let mut ua = UndoManager::new(&a); + let mut ub = UndoManager::new(&b); + delete_s(&a); + a.commit(); + edit(&b); + b.commit(); + if delete_arrives_first { + b.import(&a.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + } + a.import(&b.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + assert!(ua.undo().unwrap(), "{name}: A undoes its delete"); + a.commit(); + b.import(&a.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + let what = format!("{name}, delete_arrives_first={delete_arrives_first}"); + assert_eq!(json(&a), merged, "{what}: A"); + assert_eq!(json(&b), merged, "{what}: B"); + assert!(ub.can_undo(), "{what}"); + assert!(ub.undo().unwrap(), "{what}: B undoes its edit"); + b.commit(); + assert_replays_to(&b, &after_undo, &what); + sync(&a, &b); + assert_eq!(json(&a), after_undo, "{what}: A after sync"); + } + } +} From 6574efd5a3080e8d2c5b17560779329e71c3519f Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 09:24:25 +0800 Subject: [PATCH 4/6] fix: make mergeable full-state alignment opt-in; keep main's event shapes Local events go back to main's shapes (a re-ensure emits only the parent marker), so mirrors that share the source's hidden state keep working. `LoroDoc::diff` marks its batch as full-state (`DiffBatch::is_full_state`; WASM `applyDiff(diff, { fullState: true })`), and only such batches are aligned with the target's hidden state. Moved movable-list children are full-state targets, style-only differences are re-marked instead of rewritten. Rationale: context/mergeable-containers.md. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../mergeable-revert-duplicate-content.md | 6 +- context/mergeable-containers.md | 119 ++++--- .../loro-internal/src/handler/full_state.rs | 108 +++++- crates/loro-internal/src/loro.rs | 20 +- crates/loro-internal/src/state.rs | 91 +---- crates/loro-internal/src/undo.rs | 8 + crates/loro-wasm/src/lib.rs | 53 ++- crates/loro-wasm/tests/mergeable.test.ts | 86 ++++- crates/loro/src/event.rs | 19 ++ crates/loro/src/lib.rs | 6 + crates/loro/tests/mergeable_revert.rs | 320 ++++++++++++++++-- 11 files changed, 642 insertions(+), 194 deletions(-) diff --git a/.changeset/mergeable-revert-duplicate-content.md b/.changeset/mergeable-revert-duplicate-content.md index 03ac73daf..a8ecbef30 100644 --- a/.changeset/mergeable-revert-duplicate-content.md +++ b/.changeset/mergeable-revert-duplicate-content.md @@ -2,4 +2,8 @@ "loro-crdt": patch --- -Fix `revertTo`, `applyDiff(diff(a, b))` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. `revertTo` and undo now write only the real difference and keep the child's characters, list elements and tree node ids. `diff()` and events still report the restored child with its full content, and `applyDiff` now reconciles that content with whatever hidden state the receiving doc has, so it also works on a doc that never saw the child. Local events for a restored mergeable child now carry its full content too, like import and checkout events. `UndoManager` keeps a peer's own edits undoable after a remote undo restores the child. +Fix `revertTo` and undo adding a mergeable child's content twice after its map key was deleted. For example, `revertTo` a version before `map.delete("s")` turned a mergeable text `"hello"` into `"hellohello"`, a counter `7` into `14`, and a list `["keep"]` into `["keep", "keep"]`. They now write only the real difference and keep the child's characters, list elements and tree node ids. + +`applyDiff` accepts a new option, `applyDiff(diff, { fullState: true })`. Use it when applying the result of `doc.diff()` to a document that may still keep a deleted mergeable child the diff restores, such as the same document or a fork: the child's full content is then aligned with that hidden state instead of appended to it. Without the option, `applyDiff` behaves as before: that is right for documents without such hidden state, and for events forwarded to a document that shares the source's hidden state. Event shapes are unchanged. To build a mirror from scratch, use `diff()` or a snapshot. + +`UndoManager` also keeps a peer's own edit undoable after a remote undo restores the child it edited. diff --git a/context/mergeable-containers.md b/context/mergeable-containers.md index 84c94ef0e..67b7fd85b 100644 --- a/context/mergeable-containers.md +++ b/context/mergeable-containers.md @@ -82,62 +82,73 @@ rewrites the marker. ## Revert, Apply-Diff, Events, And Undo A mergeable child that becomes visible again keeps its deterministic cid and its hidden -state. Two consumers need different things: - -- Anything that did not see the hidden state (JS mirrors, loro-js, another doc, a doc - built only from the visible state) needs the child's **full state**. -- The doc that kept the state (its own revert and undo) needs the **actual change**. So - does the undo manager's transform base, because undo positions live in the kept - content. Applying a full state there duplicated content (`"hello"` -> `"hellohello"`, - counter 7 -> 14), and transforming against it broke later undo steps. - -The contract keeps both: - -- **Events and `LoroDoc::diff` report full state**, as on main. That covers import, - checkout, and local transactions. A re-activated mergeable child, and every container it - holds that kept its id, appears with its full state; no entry means empty. Import and - checkout get this from revival in `DocState::apply_diff`. Local transactions get it from - `DocState::revive_mergeable_in_local_diff` (in `commit_txn`), triggered by a local map - diff whose value is a mergeable container. -- **`KeptChange`** (`event.rs`) sits next to such full states on `InternalContainerDiff` - and `ContainerDiff::kept`, and holds the actual change. `DocState::apply_diff` tracks the - `kept` set: a mergeable child re-activated by a container that is not itself revived, plus - what a kept container holds, except containers its own change inserts (those are new, and - their full state is their change). A mergeable child under a parent revived under a fresh - id is not kept: `apply_diff` gives it a fresh cid. `ContainerDiff::change()` returns the - actual change. -- **Revert and undo** build their batch with `DiffBatch::from_changes` and apply it with - alignment off. Only the real difference is written, keeping char/element/`TreeID` - identity. Unchanged hidden content costs only the parent marker op. -- **Public `apply_diff`** (`_apply_diff(.., align_revived_mergeable = true)`) aligns a - re-activated mergeable child's full state with whatever the target doc holds at that cid - (`handler/full_state.rs`), whether that is nothing, the same state, or a different - state: +state. What a diff consumer needs depends on whether it holds that hidden state: + +- the doc that kept it (its own revert/undo, or a mirror that shares its history) needs + the **actual change**; +- a doc without it (a fresh doc, a JSON mirror) needs the **full state**; +- a doc with a *different* hidden state needs the full state **aligned** with what it has. + +A batch alone cannot say which it holds. In JS, `[cid, diff][]` has no room for that. For a +plain re-ensure, "no child entry" means "unchanged" in a local event but "empty" in a +full-state diff. So the mode is explicit: + +- **Events keep main's shapes.** + - Local transactions report the parent marker plus the transaction's own ops. A plain + `ensure_mergeable_*` over a deleted key emits only the marker. + - Import/checkout revival reports full state, as on main. It also records the actual + change as `KeptChange` (`event.rs`: `InternalContainerDiff`/`ContainerDiff::kept`, + `ContainerDiff::change()`). `DocState::apply_diff` tracks the `kept` set: a mergeable + child re-activated by a container that is not itself revived, plus what a kept + container holds, minus containers its own change inserts. A mergeable child under a + parent revived under a fresh id is not kept, because `apply_diff` gives it a fresh cid. +- **`LoroDoc::diff`** reports full state and sets `DiffBatch::full_state` + (`loro::event::DiffBatch::is_full_state`/`set_full_state`). +- **`apply_diff` is incremental by default**, as on main. It aligns only for a + `full_state` batch (WASM: `applyDiff(diff, { fullState: true })`). In `_apply_diff` with + `align_revived_mergeable`, a re-activated mergeable child's full state is aligned with + what the doc holds at that cid (`handler/full_state.rs`): - Map: per key. - Counter: adds the difference. - - Text and List/MovableList: keep the common prefix and suffix (chars with equal - styles; values, containers by id) and replace the middle. - - Tree: by `TreeID`. Missing nodes are created, kept nodes are moved if their parent or - position differs, the rest are deleted. - - Kept children are handed down as further full-state targets. A target with no entry is - cleared. -- **UndoManager** composes remote/paused events with `change()`, not `diff`. - -Consequences: - -- `source.diff(D, T)` applied to a doc that never saw the child restores it. -- Local events forwarded to another doc (same or no hidden state) reproduce the source. -- A peer can still undo its own edit to a child that a remote undo re-activated. -- Concurrent reverts of the same state delete the same elements and merge to the target. - A concurrent edit to a kept element, such as a tree node's meta, survives. -- `revert_to` behaves like it does for a root container: a remote edit to the hidden child - that was already observed is reverted, and one that arrives later merges. Undo of a delete - only restores the marker, so such remote edits survive in either order. + - Text: keeps the common prefix/suffix by content; re-marks kept chars whose style + differs (`null` unmarks); replaces the middle. + - List/MovableList: keeps the common prefix/suffix (values; containers by id) and + replaces the middle. In a movable list, `apply_delta` turns deleting and re-inserting + a child into a move (`movable_list_apply_delta.rs`, #1138), so middle children present + in both keep their id and are full-state targets too. A plain list recreates them. + - Tree: by `TreeID` (create/move/delete). + - Kept children become further full-state targets. A target with no entry is cleared. +- **Revert and undo** apply `DiffBatch::from_changes` without alignment, recorded with + `record_changes_only` (an optimization that skips unused full states). Only the real + difference is written, keeping char/element/`TreeID` identity. +- **UndoManager** composes remote events with `ContainerDiff::change()`, so a peer can still + undo its edit after a remote undo re-activates the child. + +When to opt in: + +- **Mirroring events** (subscribe β†’ `applyDiff`) requires the receiver to share the + source's hidden state, as on main. Use the default. To build a mirror from scratch, use + `diff()` or a snapshot. +- **Applying `diff()` to a doc that may hold hidden state** for a child the diff + re-activates (the same doc, a fork, a doc with another history) needs + `{ fullState: true }` in JS. In Rust the flag comes with `LoroDoc::diff`. Without it, such + a doc gets main's behavior: the full state is appended to the hidden one. On a doc + without hidden state both modes give the same result. + +Known limitations: + +- Alignment is content-based and each receiver writes its own ops. Two receivers that each + align the same full-state batch against the same hidden state and then merge can apply + it twice (e.g. `hello` over `hXYZo` gives `hellello`, `[keep]` over `[other]` gives two + `keep`, and two new tree nodes). That converges, but it is not idempotent. To restore + once across peers, have one peer apply and sync the updates. +- Identity is kept for the shared prefix/suffix, moved movable-list children, and tree + nodes by `TreeID`. Plain-list middle children and middle text are recreated. - Counters stay additive: two peers reverting 10 -> 7 concurrently give 4, as for a root counter. -- Alignment in `apply_diff` of a different hidden state is content-based. Its edits are - new ops of the applying doc; identity is kept only for the shared prefix/suffix and for - tree nodes. +- `revert_to` treats a remote edit to the hidden child like a root container does: already + observed means reverted, arriving later means merged. Undo of a delete keeps such edits + in either order. Covered by `crates/loro/tests/mergeable_revert.rs`, `tests/mergeable_container/delete.rs`, and `crates/loro-wasm/tests/mergeable.test.ts`. @@ -185,8 +196,8 @@ raw/shallow storage may still show the binary marker for forward compatibility. removing the marker. - "A full-state revival of a re-activated mergeable child is its change." False; it is what a consumer that never saw the child needs. The doc that kept the child uses - `KeptChange` (revert, undo, undo transforms), and `apply_diff` aligns the full state - with the target's hidden state. + `KeptChange` (revert, undo, undo transforms), and `apply_diff` aligns a full state with + the target's hidden state only for `full_state` batches. - "Kind conflict discards the loser." False; the loser is hidden but should stay recoverable by deterministic cid. - "The marker is the child cid." False; the marker activates a kind at a diff --git a/crates/loro-internal/src/handler/full_state.rs b/crates/loro-internal/src/handler/full_state.rs index 549827433..f7648b10d 100644 --- a/crates/loro-internal/src/handler/full_state.rs +++ b/crates/loro-internal/src/handler/full_state.rs @@ -31,8 +31,6 @@ pub(crate) fn align_full_state( current: Diff, full_state_targets: &mut FxHashSet, ) -> LoroResult> { - #[cfg(not(feature = "counter"))] - let _ = handler; match (target, current) { (Diff::Map(target), Diff::Map(current)) => { Ok(align_map(target, current, full_state_targets).map(Diff::Map)) @@ -52,10 +50,13 @@ pub(crate) fn align_full_state( None => Some(Diff::Text(target)), }), (Diff::List(target), Diff::List(current)) => { - Ok(match align_list(&target, ¤t, full_state_targets) { - Some(edit) => (!edit.is_empty()).then_some(Diff::List(edit)), - None => Some(Diff::List(target)), - }) + let movable = matches!(handler, Handler::MovableList(_)); + Ok( + match align_list(&target, ¤t, movable, full_state_targets) { + Some(edit) => (!edit.is_empty()).then_some(Diff::List(edit)), + None => Some(Diff::List(target)), + }, + ) } (Diff::Tree(target), Diff::Tree(current)) => { Ok(match align_tree(&target, ¤t, full_state_targets) { @@ -134,31 +135,78 @@ fn text_chars(diff: &TextDiff) -> Option> { /// Lengths of the longest common prefix and of the longest common suffix of the rest. fn common_ends(a: &[T], b: &[T]) -> (usize, usize) { - let prefix = a.iter().zip(b).take_while(|(x, y)| x == y).count(); + common_ends_by(a, b, |x, y| x == y) +} + +fn common_ends_by(a: &[T], b: &[T], eq: impl Fn(&T, &T) -> bool) -> (usize, usize) { + let prefix = a.iter().zip(b).take_while(|(x, y)| eq(x, y)).count(); let suffix = a[prefix..] .iter() .rev() .zip(b[prefix..].iter().rev()) - .take_while(|(x, y)| x == y) + .take_while(|(x, y)| eq(x, y)) .count(); (prefix, suffix) } -/// Keeps the common prefix and suffix (chars with equal styles) and replaces the middle. +/// The styles to mark on a kept char so that `current` becomes `target` (`null` unmarks). +fn style_change(current: &TextMeta, target: &TextMeta) -> TextMeta { + let mut change = TextMeta::default(); + for (key, value) in target.0.iter() { + if current.0.get(key) != Some(value) { + change.0.insert(key.clone(), value.clone()); + } + } + for key in current.0.keys() { + if !target.0.contains_key(key) { + change.0.insert(key.clone(), LoroValue::Null); + } + } + change +} + +/// Retains `current[i]` for each `(current[i], target[i])` pair, restyling chars whose styles +/// differ. +fn push_restyled_retain( + edit: &mut TextDiff, + current: &[(char, &TextMeta)], + target: &[(char, &TextMeta)], +) { + let same_style = |i: usize| current[i].1 == target[i].1; + let mut i = 0; + while i < current.len() { + let start = i; + let mut len = 0; + if same_style(i) { + while i < current.len() && same_style(i) { + len += char_len(current[i].0); + i += 1; + } + edit.push_retain(len, TextMeta::default()); + } else { + let (from, to) = (current[i].1, target[i].1); + while i < current.len() && current[i].1 == from && target[i].1 == to { + len += char_len(current[i].0); + i += 1; + } + debug_assert!(i > start); + edit.push_retain(len, style_change(from, to)); + } + } +} + +/// Keeps the chars of the common prefix and suffix (compared by content; differing styles are +/// re-marked) and replaces the middle. fn align_text(target: &TextDiff, current: &TextDiff) -> Option { let target = text_chars(target)?; let current = text_chars(current)?; - let (prefix, suffix) = common_ends(&target, ¤t); + let (prefix, suffix) = common_ends_by(&target, ¤t, |(a, _), (b, _)| a == b); let mut edit = TextDiff::new(); - if prefix == target.len() && prefix == current.len() { - return Some(edit); - } - let retain: usize = current[..prefix].iter().map(|(c, _)| char_len(*c)).sum(); + push_restyled_retain(&mut edit, ¤t[..prefix], &target[..prefix]); let delete: usize = current[prefix..current.len() - suffix] .iter() .map(|(c, _)| char_len(*c)) .sum(); - edit.push_retain(retain, TextMeta::default()); edit.push_delete(delete); let middle = &target[prefix..target.len() - suffix]; let mut i = 0; @@ -172,6 +220,11 @@ fn align_text(target: &TextDiff, current: &TextDiff) -> Option { i += run.chars().count(); edit.push_insert(StringSlice::from(run), attr.clone()); } + push_restyled_retain( + &mut edit, + ¤t[current.len() - suffix..], + &target[target.len() - suffix..], + ); Some(edit) } @@ -191,9 +244,14 @@ fn list_items(diff: &ListDiff) -> Option> { } /// Keeps the common prefix and suffix (equal values, containers by id) and replaces the middle. +/// +/// In a movable list, `apply_delta` turns deleting a child container and re-inserting it into +/// a move (see `movable_list_apply_delta.rs`), so middle children that exist in both lists +/// keep their id as well. A plain list recreates them under fresh ids. fn align_list( target: &ListDiff, current: &ListDiff, + movable: bool, full_state_targets: &mut FxHashSet, ) -> Option { let target = list_items(target)?; @@ -201,9 +259,25 @@ fn align_list( let target_values: Vec = target.iter().map(|v| v.to_value()).collect(); let current_values: Vec = current.iter().map(|v| v.to_value()).collect(); let (prefix, suffix) = common_ends(&target_values, ¤t_values); - let kept = target_values[..prefix] + let mut kept: Vec<&LoroValue> = target_values[..prefix] .iter() - .chain(&target_values[target_values.len() - suffix..]); + .chain(&target_values[target_values.len() - suffix..]) + .collect(); + if movable { + let current_middle: FxHashSet<&ContainerID> = current_values + [prefix..current_values.len() - suffix] + .iter() + .filter_map(|v| v.as_container()) + .collect(); + kept.extend( + target_values[prefix..target_values.len() - suffix] + .iter() + .filter(|v| { + v.as_container() + .is_some_and(|id| current_middle.contains(id)) + }), + ); + } for v in kept { if let LoroValue::Container(id) = v { full_state_targets.insert(id.clone()); diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 6fbaeafa3..7c712be5b 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -1297,7 +1297,10 @@ impl LoroDoc { /// NOTE: This method will make the doc enter the **detached mode**. // FIXME: This method needs testing (no event should be emitted during processing this) pub fn diff(&self, a: &Frontiers, b: &Frontiers) -> LoroResult { - self.diff_events(a, b, false).map(DiffBatch::new) + self.diff_events(a, b, false).map(|e| DiffBatch { + full_state: true, + ..DiffBatch::new(e) + }) } /// With `changes_only`, only [`DiffBatch::from_changes`] of the result is meaningful. @@ -1370,8 +1373,13 @@ impl LoroDoc { /// Apply a diff to the current state. #[inline(always)] + /// + /// A mergeable child that the batch re-activates is aligned with this doc's hidden state + /// only if `diff.full_state` is set (batches from [`LoroDoc::diff`]); otherwise its entry + /// is applied as an increment. See [`DiffBatch::full_state`]. pub fn apply_diff(&self, diff: DiffBatch) -> LoroResult<()> { - self._apply_diff(diff, &mut Default::default(), true, true) + let align = diff.full_state; + self._apply_diff(diff, &mut Default::default(), true, align) } /// Apply a diff to the current state. @@ -1386,11 +1394,11 @@ impl LoroDoc { /// However, the diff may contain operations that depend on container IDs. /// Therefore, users need to provide a `container_remap` to record and retrieve the container ID remapping. /// - /// With `align_revived_mergeable`, `diff` follows the event contract: a mergeable child the + /// With `align_revived_mergeable` (a [`DiffBatch::full_state`] batch), a mergeable child the /// batch re-activates carries its full state (no entry means empty), which is aligned with - /// whatever hidden state this doc has at its deterministic cid. Revert and undo pass - /// batches of actual changes ([`DiffBatch::from_changes`]) and disable it. See - /// context/mergeable-containers.md. + /// whatever hidden state this doc has at its deterministic cid. Otherwise entries are + /// increments; revert and undo pass batches of actual changes ([`DiffBatch::from_changes`]). + /// See context/mergeable-containers.md. pub(crate) fn _apply_diff( &self, diff: DiffBatch, diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index 7b6cb7970..f44d487f7 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -1052,102 +1052,13 @@ impl DocState { pub(crate) fn commit_txn(&mut self, new_frontiers: Frontiers, diff: Option) { self.in_txn = false; self.frontiers = new_frontiers; - if let Some(mut diff) = diff { + if let Some(diff) = diff { if self.is_recording() { - if let Some(revived) = self.revive_mergeable_in_local_diff(&diff.diff) { - diff.diff = Cow::Owned(revived); - } self.record_diff(diff); } } } - /// A local transaction that writes a mergeable marker makes the child visible with the - /// state it kept. Like import/checkout revival, its event carries that full state (and so - /// do the containers it holds), with the transaction's own ops as the [`KeptChange`]. - /// Returns `None` if the transaction re-activates no mergeable child. - fn revive_mergeable_in_local_diff( - &mut self, - diffs: &[InternalContainerDiff], - ) -> Option> { - let mut queue: Vec = Vec::new(); - for d in diffs { - if let crate::event::DiffVariant::External(Diff::Map(map)) = &d.diff { - for v in map.updated.values() { - if let Some(ValueOrHandler::Handler(h)) = &v.value { - if h.id().is_mergeable() { - queue.push(h.container_idx()); - } - } - } - } - } - if queue.is_empty() { - return None; - } - - let mut rest = Vec::with_capacity(diffs.len()); - for d in diffs { - rest.push(d.clone()); - } - let mut seen: FxHashSet = FxHashSet::default(); - let mut revived = Vec::new(); - while let Some(idx) = queue.pop() { - if !seen.insert(idx) { - continue; - } - // The transaction's own ops on this container, composed. - let mut change: Option = None; - rest.retain(|d| { - if d.idx != idx { - return true; - } - let crate::event::DiffVariant::External(diff) = &d.diff else { - return true; - }; - change = Some(match change.take() { - None => diff.clone(), - Some(c) => c.compose(diff.clone()).unwrap(), - }); - false - }); - // Containers the transaction inserted here are new, and their own diffs are full. - let mut inserted: FxHashSet = FxHashSet::default(); - if let Some(change) = &change { - trigger_on_new_container( - change, - |cid| { - inserted.insert(cid); - }, - &self.arena, - ); - } - let full = self.container_full_diff(idx); - trigger_on_new_container( - &full, - |cid| { - if !inserted.contains(&cid) || is_mergeable(&self.arena, cid) { - queue.push(cid); - } - }, - &self.arena, - ); - let kept = change.map_or(KeptChange::Unchanged, KeptChange::from_diff); - if full.is_empty() && matches!(kept, KeptChange::Unchanged) { - continue; - } - revived.push(InternalContainerDiff { - idx, - bring_back: true, - diff: full.into(), - diff_mode: DiffMode::Linear, - kept: Some(kept), - }); - } - rest.extend(revived); - Some(rest) - } - /// Ensure the container is created and will be encoded in the next `encode` call #[inline] pub(crate) fn ensure_container(&mut self, id: &ContainerID) { diff --git a/crates/loro-internal/src/undo.rs b/crates/loro-internal/src/undo.rs index d960842df..c335fd385 100644 --- a/crates/loro-internal/src/undo.rs +++ b/crates/loro-internal/src/undo.rs @@ -25,6 +25,12 @@ use crate::{ pub struct DiffBatch { pub cid_to_events: FxHashMap, pub order: Vec, + /// Set on batches from [`LoroDoc::diff`]: a mergeable child the batch re-activates carries + /// its full state (no entry means empty), which `apply_diff` aligns with whatever hidden + /// state the target doc keeps at that deterministic cid. Unset batches (events, hand-built + /// ones) are applied incrementally, as a doc that shares the source's hidden state needs. + /// See context/mergeable-containers.md. + pub full_state: bool, } impl DiffBatch { @@ -42,6 +48,7 @@ impl DiffBatch { Self { cid_to_events: map, order, + full_state: false, } } @@ -67,6 +74,7 @@ impl DiffBatch { Self { cid_to_events: map, order, + full_state: false, } } diff --git a/crates/loro-wasm/src/lib.rs b/crates/loro-wasm/src/lib.rs index 7bd4e822b..7f94b0472 100644 --- a/crates/loro-wasm/src/lib.rs +++ b/crates/loro-wasm/src/lib.rs @@ -438,6 +438,8 @@ extern "C" { pub type JsVersionVectorDiff; #[wasm_bindgen(typescript_type = "[ContainerID, Diff|JsonDiff][]")] pub type JsDiffBatch; + #[wasm_bindgen(typescript_type = "ApplyDiffOptions")] + pub type JsApplyDiffOptions; } mod observer { @@ -2320,8 +2322,41 @@ impl LoroDoc { /// doc2.applyDiff(diff); /// console.log(doc2.getText("text").toString()); // "Hello" /// ``` + /// + /// Pass `{ fullState: true }` when applying the result of `doc.diff()` to a document that may + /// keep hidden state for a mergeable child the diff makes visible again (e.g. applying + /// `doc.diff(afterDelete, beforeDelete)` to `doc` itself or to a copy of it). The child's + /// entry is then read as its full content and aligned with that hidden state. Without it + /// (the default), entries are applied as increments, which is right for events forwarded to + /// a document that shares the source's hidden state. + /// + /// ```ts + /// const text = doc.getMap("m").ensureMergeableText("t"); + /// text.insert(0, "hello"); + /// const before = doc.frontiers(); + /// doc.getMap("m").delete("t"); + /// doc.applyDiff(doc.diff(doc.frontiers(), before), { fullState: true }); + /// console.log(doc.getMap("m").get("t").toString()); // "hello" + /// ``` #[wasm_bindgen(js_name = "applyDiff")] - pub fn apply_diff(&self, diff: JsDiffBatch) -> JsResult<()> { + pub fn apply_diff( + &self, + diff: JsDiffBatch, + options: Option, + ) -> JsResult<()> { + let full_state = match options { + Some(options) => { + let full_state = Reflect::get(&options, &JsValue::from_str("fullState"))?; + if full_state.is_undefined() || full_state.is_null() { + false + } else { + full_state + .as_bool() + .ok_or("`fullState` must be a boolean")? + } + } + None => false, + }; let diff: JsValue = diff.into(); let arr: js_sys::Array = diff.into(); let mut cid_to_events = FxHashMap::default(); @@ -2343,6 +2378,7 @@ impl LoroDoc { self.doc.apply_diff(DiffBatch { cid_to_events, order, + full_state, })?; Ok(()) } @@ -6548,6 +6584,21 @@ interface LoroDoc { diff(from: OpId[], to: OpId[], for_json?: boolean): [ContainerID, JsonDiff|Diff][]; } +/** + * Options for `LoroDoc.applyDiff`. + */ +export interface ApplyDiffOptions { + /** + * Read the batch as the result of `doc.diff()`: a mergeable child that the batch makes + * visible again carries its full content, which is aligned with whatever hidden state the + * target document keeps for it (none, the same, or different). + * + * Leave it unset (the default) for batches built from events: those entries are + * increments, which is right when the target shares the source's hidden state. + */ + fullState?: boolean; +} + /** * Represents a `Delta` type which is a union of different operations that can be performed. * diff --git a/crates/loro-wasm/tests/mergeable.test.ts b/crates/loro-wasm/tests/mergeable.test.ts index cedf5b69b..8db5c1690 100644 --- a/crates/loro-wasm/tests/mergeable.test.ts +++ b/crates/loro-wasm/tests/mergeable.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test, vi } from "vitest"; -import { LoroDoc, UndoManager } from "../bundler/index"; +import { LoroDoc, LoroMap, LoroText, UndoManager } from "../bundler/index"; function sync(a: LoroDoc, b: LoroDoc) { const aBytes = a.export({ mode: "update", from: b.version() }); @@ -245,7 +245,8 @@ describe("mergeable containers (WASM bindings)", () => { expect(r.d.toJSON()).toEqual(expected); const p = setup(); - p.d.applyDiff(p.d.diff(p.b, p.a)); + // The doc keeps the hidden children, so the full-state diff needs `fullState`. + p.d.applyDiff(p.d.diff(p.b, p.a), { fullState: true }); p.d.commit(); expect(p.d.toJSON()).toEqual(expected); }); @@ -368,4 +369,85 @@ describe("mergeable containers (WASM bindings)", () => { b.commit(); expect(b.toJSON()).toEqual({ m: { s: "hello" } }); }); + test("applyDiff is incremental by default and aligns with fullState", () => { + const setup = () => { + const d = new LoroDoc(); + d.setPeerId(1); + d.getMap("m").ensureMergeableText("t").insert(0, "hello"); + d.commit(); + const target = d.frontiers(); + d.getMap("m").delete("t"); + d.commit(); + return { d, diff: d.diff(d.frontiers(), target) }; + }; + // Same hidden state: the default applies the entry as an increment, as before. + const a = setup(); + a.d.applyDiff(a.diff); + a.d.commit(); + expect(a.d.toJSON()).toEqual({ m: { t: "hellohello" } }); + // With fullState it is aligned with the hidden state. + const b = setup(); + b.d.applyDiff(b.diff, { fullState: true }); + b.d.commit(); + expect(b.d.toJSON()).toEqual({ m: { t: "hello" } }); + // A doc without hidden state gets the same result either way. + for (const options of [undefined, { fullState: true }]) { + const fresh = new LoroDoc(); + fresh.getMap("m"); + fresh.applyDiff(setup().diff, options); + fresh.commit(); + expect(fresh.toJSON()).toEqual({ m: { t: "hello" } }); + } + expect(() => + setup().d.applyDiff(setup().diff, { fullState: "yes" } as any), + ).toThrow(); + }); + + test("fullState applyDiff keeps reordered movable-list children", () => { + for (const kind of ["map", "text"]) { + const d = new LoroDoc(); + d.setPeerId(1); + const list = d.getMap("m").ensureMergeableMovableList("s"); + for (const [i, v] of ["A", "B"].entries()) { + if (kind === "map") list.insertContainer(i, new LoroMap()).set("v", v); + else list.insertContainer(i, new LoroText()).insert(0, v); + } + d.commit(); + const target = d.frontiers(); + const expected = d.toJSON(); + list.move(0, 1); + d.getMap("m").delete("s"); + d.commit(); + d.applyDiff(d.diff(d.frontiers(), target), { fullState: true }); + d.commit(); + expect(d.toJSON()).toEqual(expected); + } + }); + + test("local re-ensure events mirror onto a doc sharing the hidden state", () => { + const d = new LoroDoc(); + d.setPeerId(1); + const m = d.getMap("m"); + m.ensureMergeableCounter("c").increment(7); + m.ensureMergeableText("t").insert(0, "hello"); + m.ensureMergeableList("l").push("keep"); + d.commit(); + for (const key of ["c", "t", "l"]) m.delete(key); + d.commit(); + const mirror = d.fork(); + const batches: [string, any][][] = []; + const unsub = d.subscribe((e) => { + batches.push(e.events.map((ev) => [ev.target, ev.diff])); + }); + m.ensureMergeableCounter("c"); + m.ensureMergeableText("t"); + m.ensureMergeableList("l").push("more"); + d.commit(); + unsub(); + for (const batch of batches) mirror.applyDiff(batch as any); + mirror.commit(); + const expected = { m: { c: 7, t: "hello", l: ["keep", "more"] } }; + expect(d.toJSON()).toEqual(expected); + expect(mirror.toJSON()).toEqual(expected); + }); }); diff --git a/crates/loro/src/event.rs b/crates/loro/src/event.rs index 4273b2770..e2954fe32 100644 --- a/crates/loro/src/event.rs +++ b/crates/loro/src/event.rs @@ -254,6 +254,7 @@ impl From for ValueOrContainer { pub struct DiffBatch { cid_to_events: FxHashMap>, order: Vec, + full_state: bool, } impl std::fmt::Debug for DiffBatch { @@ -289,6 +290,22 @@ impl DiffBatch { } } + /// Whether a mergeable child this batch re-activates carries its full state. + /// + /// Batches from [`crate::LoroDoc::diff`] set it. [`crate::LoroDoc::apply_diff`] then aligns + /// that full state with whatever hidden state the target doc keeps for the child (none, + /// the same, or different), so the batch applies to any doc. Otherwise (the default, e.g. + /// a batch built from events) the child's entry is applied as an increment, which is right + /// for a doc that shares the source's hidden state. + pub fn is_full_state(&self) -> bool { + self.full_state + } + + /// Sets [`DiffBatch::is_full_state`]. + pub fn set_full_state(&mut self, full_state: bool) { + self.full_state = full_state; + } + pub(crate) fn validate_for_apply(&self) -> LoroResult<()> { for (_, diff) in self.iter() { validate_diff_for_apply(diff)?; @@ -354,6 +371,7 @@ impl From for DiffBatch { DiffBatch { cid_to_events: map, order: value.order, + full_state: value.full_state, } } } @@ -426,6 +444,7 @@ impl From for InnerDiffBatch { InnerDiffBatch { cid_to_events: map, order: value.order, + full_state: value.full_state, } } } diff --git a/crates/loro/src/lib.rs b/crates/loro/src/lib.rs index 2b4933097..a3cfb864d 100644 --- a/crates/loro/src/lib.rs +++ b/crates/loro/src/lib.rs @@ -1473,6 +1473,12 @@ impl LoroDoc { /// Apply a diff to the current document state. /// /// Internally, it will apply the diff to the current state. + /// + /// A mergeable child that the batch makes visible again is handled according to + /// [`DiffBatch::is_full_state`]. Batches from [`LoroDoc::diff`] carry the child's full state + /// and are aligned with whatever hidden state this doc keeps for it, so they apply to any + /// doc. Other batches (e.g. built from events) are applied as increments, which is right + /// when this doc shares the source's hidden state. #[inline] pub fn apply_diff(&self, diff: DiffBatch) -> LoroResult<()> { diff.validate_for_apply()?; diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs index 1baadaa82..d01c52ddd 100644 --- a/crates/loro/tests/mergeable_revert.rs +++ b/crates/loro/tests/mergeable_revert.rs @@ -2,10 +2,11 @@ //! content exactly once. //! //! A mergeable child keeps its state at a deterministic cid while the parent marker is -//! gone. Events and `LoroDoc::diff` report a re-activated child with its full state, so any -//! doc or mirror can apply them; `apply_diff` aligns that state with whatever hidden state -//! the target has. Revert and undo apply the child's actual change, which keeps -//! char/element/TreeID identity. See `context/mergeable-containers.md`. +//! gone. Revert and undo apply the child's actual change, which keeps char/element/TreeID +//! identity. `LoroDoc::diff` reports a re-activated child with its full state and marks the +//! batch (`DiffBatch::is_full_state`), so `apply_diff` aligns it with whatever hidden state +//! the target has. Local events keep main's shape (increments), so forwarding them needs a +//! target that shares the source's hidden state. See `context/mergeable-containers.md`. use loro::{ event::{Diff, DiffBatch, MapDelta}, @@ -162,28 +163,30 @@ fn check(setup: impl Fn(&LoroDoc), mutate: impl Fn(&LoroDoc), reuses_children: b r.commit(); assert_replays_to(&r, &expected_b, "revert_to back to b"); - // Local events of a revert, forwarded to a doc in the same state, and to one that only - // has the visible state (built from `diff`, so it never saw the hidden children). + // Local events of a revert, forwarded to a doc that shares the hidden state. let source = d.fork(); let mirror = d.fork(); forward_events(&source, &mirror, |s| s.revert_to(&a).unwrap()); assert_eq!(json(&source), expected_a, "event forwarding: source"); assert_eq!(json(&mirror), expected_a, "event forwarding: mirror"); - let source = d.fork(); - let visible_only = visible_copy(&d); - assert_eq!(json(&visible_only), expected_b, "visible-only copy"); - forward_events(&source, &visible_only, |s| s.revert_to(&a).unwrap()); - assert_eq!( - json(&visible_only), - expected_a, - "event forwarding: visible-only mirror" - ); - // Public diff applied to a doc that only has the visible state. + // Public (full-state) diff applied to a doc that only has the visible state. let visible_only = visible_copy(&d); visible_only.apply_diff(d.diff(&b, &a).unwrap()).unwrap(); visible_only.commit(); assert_eq!(json(&visible_only), expected_a, "diff to visible-only doc"); + // Without the flag, it is applied as on main, which is right for a doc without hidden + // state. + let visible_only = visible_copy(&d); + let mut diff = d.diff(&b, &a).unwrap(); + diff.set_full_state(false); + visible_only.apply_diff(diff).unwrap(); + visible_only.commit(); + assert_eq!( + json(&visible_only), + expected_a, + "incremental diff to visible-only doc" + ); // diff + apply_diff let r = d.fork(); @@ -902,10 +905,11 @@ fn public_diff_restores_counter_whatever_the_target_hidden_state() { } } -/// A local re-activation (here `ensure_mergeable_*` over a deleted key) reports the child's -/// full state, like import and checkout do, so a mirror without hidden state can follow. +/// A local re-activation (`ensure_mergeable_*` over a deleted key) keeps main's event shape: +/// only the parent marker, plus the transaction's own ops on the child. Forwarded to a doc that +/// shares the hidden state, the default (incremental) `apply_diff` reproduces the source. #[test] -fn local_reactivation_event_carries_full_state() { +fn local_reensure_event_keeps_main_shape() { let d = doc(); let m = d.get_map("m"); m.ensure_mergeable_text("s") @@ -913,19 +917,45 @@ fn local_reactivation_event_carries_full_state() { .insert(0, "hello") .unwrap(); m.ensure_mergeable_list("l").unwrap().push("keep").unwrap(); + #[cfg(feature = "counter")] + m.ensure_mergeable_counter("c") + .unwrap() + .increment(7.0) + .unwrap(); d.commit(); - delete_s(&d); - m.delete("l").unwrap(); + for key in ["s", "l", "c"] { + m.delete(key).unwrap(); + } d.commit(); - let mirror = visible_copy(&d); + let mirror = d.fork(); + let seen: Arc>> = Default::default(); + let sink = seen.clone(); + let sub = d.subscribe_root(Arc::new(move |e| { + for ev in e.events { + sink.lock().unwrap().push(format!("{:?}", ev.target)); + } + })); forward_events(&d, &mirror, |d| { let m = d.get_map("m"); m.ensure_mergeable_text("s").unwrap(); + #[cfg(feature = "counter")] + m.ensure_mergeable_counter("c").unwrap(); m.ensure_mergeable_list("l").unwrap().push("more").unwrap(); }); - let expected = json!({"m": {"s": "hello", "l": ["keep", "more"]}}); + drop(sub); + let mut expected = json!({"m": {"s": "hello", "l": ["keep", "more"]}}); + #[cfg(feature = "counter")] + { + expected["m"]["c"] = json!(7.0); + } assert_eq!(json(&d), expected); assert_eq!(json(&mirror), expected); + // The unchanged text and counter get no entry of their own; the list only its push. + let seen = seen.lock().unwrap(); + assert!( + seen.iter().all(|t| !t.contains(">s") && !t.contains(">c")), + "{seen:?}" + ); } /// A re-activates a child that B edited while it was hidden. B's import event reports the @@ -1049,3 +1079,247 @@ fn peer_can_undo_own_edit_after_remote_undo_reactivates_child() { } } } + +/// Container children of a mergeable (movable) list, reordered/removed/added after the target +/// version and then hidden. The full-state `diff` restores them on docs with the same hidden +/// state, none, or a different one: movable-list children that exist in both are moved and keep +/// their content (no loss, no duplication). +#[test] +fn public_diff_restores_reordered_container_children() { + type Mutation = fn(&LoroDoc, bool); + fn children(d: &LoroDoc, movable: bool) -> usize { + let m = d.get_map("m"); + if movable { + m.ensure_mergeable_movable_list("s").unwrap().len() + } else { + m.ensure_mergeable_list("s").unwrap().len() + } + } + let mutations: [(&str, Mutation); 5] = [ + ("swap first two", |d, movable| { + if movable { + d.get_map("m") + .ensure_mergeable_movable_list("s") + .unwrap() + .mov(0, 1) + .unwrap(); + } else { + d.get_map("m") + .ensure_mergeable_list("s") + .unwrap() + .delete(0, 1) + .unwrap(); + } + }), + ("rotate", |d, movable| { + if movable { + d.get_map("m") + .ensure_mergeable_movable_list("s") + .unwrap() + .mov(2, 0) + .unwrap(); + } + }), + ("reverse", |d, movable| { + if movable { + let l = d.get_map("m").ensure_mergeable_movable_list("s").unwrap(); + l.mov(2, 0).unwrap(); + l.mov(2, 1).unwrap(); + } + }), + ("delete middle", |d, movable| { + let m = d.get_map("m"); + if movable { + m.ensure_mergeable_movable_list("s") + .unwrap() + .delete(1, 1) + .unwrap(); + } else { + m.ensure_mergeable_list("s").unwrap().delete(1, 1).unwrap(); + } + }), + ("insert new child", |d, movable| { + let m = d.get_map("m"); + if movable { + m.ensure_mergeable_movable_list("s") + .unwrap() + .insert_container(1, loro::LoroText::new()) + .unwrap() + .insert(0, "new") + .unwrap(); + } else { + m.ensure_mergeable_list("s") + .unwrap() + .insert_container(1, loro::LoroText::new()) + .unwrap() + .insert(0, "new") + .unwrap(); + } + }), + ]; + for movable in [true, false] { + for texts in [false, true] { + for (name, mutate) in mutations { + let setup = |d: &LoroDoc| { + let m = d.get_map("m"); + for (i, v) in ["A", "B", "C"].into_iter().enumerate() { + if texts { + let t = if movable { + m.ensure_mergeable_movable_list("s") + .unwrap() + .insert_container(i, loro::LoroText::new()) + .unwrap() + } else { + m.ensure_mergeable_list("s") + .unwrap() + .insert_container(i, loro::LoroText::new()) + .unwrap() + }; + t.insert(0, v).unwrap(); + } else { + let c = if movable { + m.ensure_mergeable_movable_list("s") + .unwrap() + .insert_container(i, loro::LoroMap::new()) + .unwrap() + } else { + m.ensure_mergeable_list("s") + .unwrap() + .insert_container(i, loro::LoroMap::new()) + .unwrap() + }; + c.insert("v", v).unwrap(); + } + } + }; + let (d, target, expected) = deleted_after_divergence(setup, |d| mutate(d, movable)); + let what = format!("movable={movable} texts={texts} {name}"); + let diff = || d.diff(&d.state_frontiers(), &target).unwrap(); + + let same = d.fork(); + same.apply_diff(diff()).unwrap(); + same.commit(); + assert_replays_to(&same, &expected, &format!("{what}: same hidden")); + assert_eq!(children(&same, movable), 3, "{what}"); + + let absent = LoroDoc::new(); + absent.get_map("m"); + absent.apply_diff(diff()).unwrap(); + absent.commit(); + assert_eq!(json(&absent), expected, "{what}: absent hidden"); + + let (other, _, _) = deleted_after_divergence( + |d| { + let m = d.get_map("m"); + if movable { + m.ensure_mergeable_movable_list("s") + .unwrap() + .push(1) + .unwrap(); + } else { + m.ensure_mergeable_list("s").unwrap().push(1).unwrap(); + } + }, + |_| {}, + ); + other.set_peer_id(3).unwrap(); + other.apply_diff(diff()).unwrap(); + other.commit(); + assert_replays_to(&other, &expected, &format!("{what}: different hidden")); + + let r = d.fork(); + r.revert_to(&target).unwrap(); + r.commit(); + assert_replays_to(&r, &expected, &format!("{what}: revert_to")); + } + } + } +} + +/// The default `apply_diff` is incremental, as on main: a full-state batch applied without the +/// flag to a doc that keeps the child's hidden state appends to it. `LoroDoc::diff` sets the +/// flag, so this only happens to batches whose flag was cleared (or rebuilt from JS without +/// `{ fullState: true }`). +#[test] +fn default_apply_diff_is_incremental() { + let d = doc(); + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + d.commit(); + let target = d.state_frontiers(); + delete_s(&d); + d.commit(); + let mut diff = d.diff(&d.state_frontiers(), &target).unwrap(); + assert!(diff.is_full_state()); + diff.set_full_state(false); + d.apply_diff(diff).unwrap(); + d.commit(); + assert_eq!(json(&d), json!({"m": {"s": "hellohello"}})); +} + +/// Aligning text compares content and styles separately: kept chars whose style differs are +/// re-marked, not deleted and re-inserted, so a concurrent edit to them survives. +#[test] +fn public_diff_restyles_kept_chars() { + let d = doc(); + let t = d.get_map("m").ensure_mergeable_text("s").unwrap(); + t.insert(0, "hello").unwrap(); + d.commit(); + let b = peer(&d, 3); + t.mark(0..5, "bold", true).unwrap(); + d.commit(); + let bold = d.state_frontiers(); + t.unmark(0..5, "bold").unwrap(); + d.commit(); + delete_s(&d); + d.commit(); + + let r = d.fork(); + r.set_peer_id(4).unwrap(); + let before = r.len_ops(); + r.apply_diff(d.diff(&d.state_frontiers(), &bold).unwrap()) + .unwrap(); + r.commit(); + let restored = r.get_map("m").ensure_mergeable_text("s").unwrap(); + assert_eq!( + restored.get_richtext_value().to_json_value(), + json!([{"insert": "hello", "attributes": {"bold": true}}]) + ); + // The marker and the mark: no text is rewritten. + assert!(r.len_ops() - before <= 3, "{} ops", r.len_ops() - before); + + // B concurrently replaces the old "h" with "H". + let bt = b.get_map("m").ensure_mergeable_text("s").unwrap(); + bt.delete(0, 1).unwrap(); + bt.insert(0, "H").unwrap(); + b.commit(); + sync(&r, &b); + assert_eq!(restored.to_string(), "Hello"); +} + +/// Committing many new mergeable children with a subscriber attached stays linear. A previous +/// revision rebuilt local events per re-activated child and scanned the whole transaction for +/// each (20k children: ~50 ms -> ~1.3-2.2 s). +/// +/// `cargo test -p loro --test mergeable_revert perf_commit_many_mergeable_children -- --ignored --nocapture` +#[test] +#[ignore] +fn perf_commit_many_mergeable_children_with_subscriber() { + for n in [5_000usize, 20_000] { + let d = doc(); + let _sub = d.subscribe_root(Arc::new(|_| {})); + let m = d.get_map("m"); + for i in 0..n { + m.ensure_mergeable_text(&i.to_string()) + .unwrap() + .insert(0, "x") + .unwrap(); + } + let start = std::time::Instant::now(); + d.commit(); + println!("{n} children: commit {:?}", start.elapsed()); + } +} From a3bcbbc2c8ff467667ad971637fb0a559c87e8a7 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 10:00:36 +0800 Subject: [PATCH 5/6] perf: align full-state text on style runs instead of per char Compare the two texts by bytes and walk style runs for re-marking, so aligning a 1M-char text costs a byte comparison plus the run count. Adds emoji/combining-mark/CJK alignment tests in Rust and WASM (UTF-16 units). Co-Authored-By: Claude Opus 5.5 (1M context) --- .../loro-internal/src/handler/full_state.rs | 139 +++++++++++------- crates/loro-wasm/tests/mergeable.test.ts | 30 ++++ crates/loro/tests/mergeable_revert.rs | 63 ++++++++ 3 files changed, 178 insertions(+), 54 deletions(-) diff --git a/crates/loro-internal/src/handler/full_state.rs b/crates/loro-internal/src/handler/full_state.rs index f7648b10d..f62e0d0a8 100644 --- a/crates/loro-internal/src/handler/full_state.rs +++ b/crates/loro-internal/src/handler/full_state.rs @@ -12,7 +12,10 @@ use crate::{ delta::{ResolvedMapDelta, ResolvedMapValue, TreeDiff, TreeDiffItem, TreeExternalDiff}, event::{Diff, ListDeltaMeta, ListDiff, TextDiff, TextMeta}, state::TreeParentId, - utils::string_slice::StringSlice, + utils::{ + string_slice::StringSlice, + utf16::{count_unicode_chars, count_utf16_len}, + }, }; use fractional_index::FractionalIndex; use loro_common::{ContainerID, InternalString, LoroResult, LoroValue, TreeID}; @@ -107,17 +110,17 @@ fn align_map( (!updated.is_empty()).then_some(ResolvedMapDelta { updated }) } -/// Length of one char in the units of text diffs (see `StringSlice::rle_len`). -fn char_len(c: char) -> usize { +/// Length of `s` in the units of text diffs (see `StringSlice::rle_len`). +fn text_len(s: &str) -> usize { if cfg!(feature = "wasm") { - c.len_utf16() + count_utf16_len(s.as_bytes()) } else { - 1 + count_unicode_chars(s.as_bytes()) } } -/// `None` if `diff` is not a pure insertion. -fn text_chars(diff: &TextDiff) -> Option> { +/// Style runs of a pure-insertion text diff, or `None` if it is not one. +fn text_runs(diff: &TextDiff) -> Option> { let mut ans = Vec::new(); for item in diff.iter() { let DeltaItem::Replace { @@ -128,11 +131,29 @@ fn text_chars(diff: &TextDiff) -> Option> { else { return None; }; - ans.extend(value.as_str().chars().map(|c| (c, attr))); + ans.push((value.as_str(), attr)); } Some(ans) } +/// The pieces of `runs` that cover the byte range `range` of their concatenated text. +fn slice_runs<'a>( + runs: &[(&'a str, &'a TextMeta)], + range: std::ops::Range, +) -> Vec<(&'a str, &'a TextMeta)> { + let mut ans = Vec::new(); + let mut offset = 0; + for &(text, attr) in runs { + let (start, end) = (offset, offset + text.len()); + offset = end; + let (from, to) = (range.start.max(start), range.end.min(end)); + if from < to { + ans.push((&text[from - start..to - start], attr)); + } + } + ans +} + /// Lengths of the longest common prefix and of the longest common suffix of the rest. fn common_ends(a: &[T], b: &[T]) -> (usize, usize) { common_ends_by(a, b, |x, y| x == y) @@ -165,65 +186,75 @@ fn style_change(current: &TextMeta, target: &TextMeta) -> TextMeta { change } -/// Retains `current[i]` for each `(current[i], target[i])` pair, restyling chars whose styles -/// differ. +/// Retains a kept text range given as the `current` and `target` runs over the same text, +/// re-marking the pieces whose styles differ. fn push_restyled_retain( edit: &mut TextDiff, - current: &[(char, &TextMeta)], - target: &[(char, &TextMeta)], + current: &[(&str, &TextMeta)], + target: &[(&str, &TextMeta)], ) { - let same_style = |i: usize| current[i].1 == target[i].1; - let mut i = 0; - while i < current.len() { - let start = i; - let mut len = 0; - if same_style(i) { - while i < current.len() && same_style(i) { - len += char_len(current[i].0); - i += 1; - } - edit.push_retain(len, TextMeta::default()); + let (mut ci, mut ti) = (0, 0); + let (mut c_off, mut t_off) = (0, 0); + while ci < current.len() && ti < target.len() { + let (c_text, c_attr) = current[ci]; + let (t_text, t_attr) = target[ti]; + let take = (c_text.len() - c_off).min(t_text.len() - t_off); + let piece = &c_text[c_off..c_off + take]; + let change = if c_attr == t_attr { + TextMeta::default() } else { - let (from, to) = (current[i].1, target[i].1); - while i < current.len() && current[i].1 == from && target[i].1 == to { - len += char_len(current[i].0); - i += 1; - } - debug_assert!(i > start); - edit.push_retain(len, style_change(from, to)); + style_change(c_attr, t_attr) + }; + edit.push_retain(text_len(piece), change); + c_off += take; + t_off += take; + if c_off == c_text.len() { + ci += 1; + c_off = 0; + } + if t_off == t_text.len() { + ti += 1; + t_off = 0; } } } -/// Keeps the chars of the common prefix and suffix (compared by content; differing styles are -/// re-marked) and replaces the middle. +/// Keeps the text of the common prefix and suffix (compared by content; differing styles are +/// re-marked) and replaces the middle. Works on style runs, so its cost is a byte comparison +/// of the two texts plus the number of runs. fn align_text(target: &TextDiff, current: &TextDiff) -> Option { - let target = text_chars(target)?; - let current = text_chars(current)?; - let (prefix, suffix) = common_ends_by(&target, ¤t, |(a, _), (b, _)| a == b); - let mut edit = TextDiff::new(); - push_restyled_retain(&mut edit, ¤t[..prefix], &target[..prefix]); - let delete: usize = current[prefix..current.len() - suffix] + let target = text_runs(target)?; + let current = text_runs(current)?; + let target_text: String = target.iter().map(|(s, _)| *s).collect(); + let current_text: String = current.iter().map(|(s, _)| *s).collect(); + let (t, c) = (target_text.as_bytes(), current_text.as_bytes()); + let mut prefix = t.iter().zip(c).take_while(|(x, y)| x == y).count(); + while !target_text.is_char_boundary(prefix) { + prefix -= 1; + } + let mut suffix = t[prefix..] .iter() - .map(|(c, _)| char_len(*c)) - .sum(); - edit.push_delete(delete); - let middle = &target[prefix..target.len() - suffix]; - let mut i = 0; - while i < middle.len() { - let attr = middle[i].1; - let run: String = middle[i..] - .iter() - .take_while(|(_, a)| *a == attr) - .map(|(c, _)| *c) - .collect(); - i += run.chars().count(); - edit.push_insert(StringSlice::from(run), attr.clone()); + .rev() + .zip(c[prefix..].iter().rev()) + .take_while(|(x, y)| x == y) + .count(); + while !target_text.is_char_boundary(t.len() - suffix) { + suffix -= 1; + } + let mut edit = TextDiff::new(); + push_restyled_retain( + &mut edit, + &slice_runs(¤t, 0..prefix), + &slice_runs(&target, 0..prefix), + ); + edit.push_delete(text_len(¤t_text[prefix..c.len() - suffix])); + for (text, attr) in slice_runs(&target, prefix..t.len() - suffix) { + edit.push_insert(StringSlice::from(text), attr.clone()); } push_restyled_retain( &mut edit, - ¤t[current.len() - suffix..], - &target[target.len() - suffix..], + &slice_runs(¤t, c.len() - suffix..c.len()), + &slice_runs(&target, t.len() - suffix..t.len()), ); Some(edit) } diff --git a/crates/loro-wasm/tests/mergeable.test.ts b/crates/loro-wasm/tests/mergeable.test.ts index 8db5c1690..772c4a531 100644 --- a/crates/loro-wasm/tests/mergeable.test.ts +++ b/crates/loro-wasm/tests/mergeable.test.ts @@ -450,4 +450,34 @@ describe("mergeable containers (WASM bindings)", () => { expect(d.toJSON()).toEqual(expected); expect(mirror.toJSON()).toEqual(expected); }); + test("fullState applyDiff aligns emoji and styled text in UTF-16 units", () => { + const cases: [string, string][] = [ + ["aπŸ˜€b", "aπŸ˜€c"], + ["πŸ˜€πŸ˜€", "πŸ˜€πŸ˜ƒ"], + ["Γ©δΈ­ζ–‡x", "Γ©δΈ­ζ–‡"], + ["πŸ‘πŸ½ok", "πŸ‘πŸΎok"], + ]; + for (const [targetText, hiddenText] of cases) { + const d = new LoroDoc(); + d.setPeerId(1); + d.configTextStyle({ bold: { expand: "after" } }); + const t = d.getMap("m").ensureMergeableText("s"); + t.insert(0, targetText); + t.mark({ start: 0, end: 1 }, "bold", true); + d.commit(); + const target = d.frontiers(); + const expected = t.toDelta(); + t.delete(0, t.length); + t.insert(0, hiddenText); + d.commit(); + d.getMap("m").delete("s"); + d.commit(); + const same = d.fork(); + same.applyDiff(d.diff(d.frontiers(), target), { fullState: true }); + same.commit(); + expect(same.getMap("m").ensureMergeableText("s").toDelta()).toEqual( + expected, + ); + } + }); }); diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs index d01c52ddd..969d8dcf7 100644 --- a/crates/loro/tests/mergeable_revert.rs +++ b/crates/loro/tests/mergeable_revert.rs @@ -1323,3 +1323,66 @@ fn perf_commit_many_mergeable_children_with_subscriber() { println!("{n} children: commit {:?}", start.elapsed()); } } + +/// Text alignment with multi-byte chars (emoji, combining marks, CJK) and styles, against the +/// same hidden text, a different one, and none. +#[test] +fn public_diff_aligns_non_ascii_styled_text() { + let cases: [(&str, &str); 6] = [ + ("aπŸ˜€b", "aπŸ˜€c"), + ("πŸ˜€πŸ˜€", "πŸ˜€πŸ˜ƒ"), + ("Γ©δΈ­ζ–‡x", "Γ©δΈ­ζ–‡"), + ("πŸ‘πŸ½ok", "πŸ‘πŸΎok"), + ("", "πŸ˜€"), + ("πŸ˜€", ""), + ]; + for (target_text, hidden_text) in cases { + let d = doc(); + let t = d.get_map("m").ensure_mergeable_text("s").unwrap(); + t.insert(0, target_text).unwrap(); + if !target_text.is_empty() { + t.mark(0..1, "bold", true).unwrap(); + } + d.commit(); + let target = d.state_frontiers(); + let expected = t.get_richtext_value().to_json_value(); + t.delete(0, t.len_unicode()).unwrap(); + t.insert(0, hidden_text).unwrap(); + d.commit(); + delete_s(&d); + d.commit(); + let diff = || d.diff(&d.state_frontiers(), &target).unwrap(); + let what = format!("{target_text:?} over {hidden_text:?}"); + + let same = d.fork(); + same.apply_diff(diff()).unwrap(); + same.commit(); + let (other, _, _) = deleted_after_divergence( + |d| { + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "xπŸ˜€y") + .unwrap(); + }, + |_| {}, + ); + other.config_default_text_style(Some(StyleConfig { + expand: ExpandType::After, + })); + other.apply_diff(diff()).unwrap(); + other.commit(); + let absent = doc(); + absent.get_map("m"); + absent.apply_diff(diff()).unwrap(); + absent.commit(); + for (name, r) in [("same", &same), ("different", &other), ("absent", &absent)] { + let text = r.get_map("m").ensure_mergeable_text("s").unwrap(); + assert_eq!( + text.get_richtext_value().to_json_value(), + expected, + "{what}: {name}" + ); + } + } +} From 5ff481a6a72a37deea8ceb26bc807fd169e7cefa Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 28 Sep 2026 17:19:21 +0800 Subject: [PATCH 6/6] fix: keep the full-state flag through DiffBatch::compose; document flag use An empty batch adopts the other side's flag, two full-state batches stay full-state, mixing modes panics, and `clear` resets the flag. Document that only `diff()` results (and import/checkout events) may be marked full-state, never local events. Co-Authored-By: Claude Opus 5.5 (1M context) --- context/mergeable-containers.md | 13 ++++ crates/loro-internal/src/undo.rs | 30 +++++++- crates/loro-wasm/src/lib.rs | 15 +++- crates/loro/src/event.rs | 12 +++ crates/loro/tests/mergeable_revert.rs | 106 ++++++++++++++++++++++++++ 5 files changed, 170 insertions(+), 6 deletions(-) diff --git a/context/mergeable-containers.md b/context/mergeable-containers.md index 67b7fd85b..8a0d13c5c 100644 --- a/context/mergeable-containers.md +++ b/context/mergeable-containers.md @@ -124,6 +124,19 @@ full-state diff. So the mode is explicit: - **UndoManager** composes remote events with `ContainerDiff::change()`, so a peer can still undo its edit after a remote undo re-activates the child. +Which batches may carry the flag: + +- Results of `diff()` do, including after a JSON round trip with `{ fullState: true }`. +- Import/checkout events may, because they revive children with full state. +- **Never** batches built from local events. A local re-ensure has no child entry because + the child is unchanged; read as a full state, that means "empty" and clears it (a counter + at 7 becomes 0). +- `DiffBatch::compose` (internal) keeps the flag. An empty side is the identity, and two + full-state batches stay full-state: a child that the second batch re-activates was hidden + where they meet, so the first has no entry for it. Mixing a full-state and an + incremental batch panics, because neither mode can apply the result. `clear` resets the + flag. + When to opt in: - **Mirroring events** (subscribe β†’ `applyDiff`) requires the receiver to share the diff --git a/crates/loro-internal/src/undo.rs b/crates/loro-internal/src/undo.rs index c335fd385..bd40dc1b9 100644 --- a/crates/loro-internal/src/undo.rs +++ b/crates/loro-internal/src/undo.rs @@ -27,9 +27,12 @@ pub struct DiffBatch { pub order: Vec, /// Set on batches from [`LoroDoc::diff`]: a mergeable child the batch re-activates carries /// its full state (no entry means empty), which `apply_diff` aligns with whatever hidden - /// state the target doc keeps at that deterministic cid. Unset batches (events, hand-built - /// ones) are applied incrementally, as a doc that shares the source's hidden state needs. - /// See context/mergeable-containers.md. + /// state the target doc keeps at that deterministic cid. Unset batches (local events, + /// hand-built ones) are applied incrementally, as a doc that shares the source's hidden + /// state needs. Local events must stay unset: a plain re-ensure has no child entry because + /// the child is unchanged, which a full-state batch would read as "empty". Import and + /// checkout events do carry full states for revived children, so they may be set. See + /// context/mergeable-containers.md. pub full_state: bool, } @@ -78,10 +81,30 @@ impl DiffBatch { } } + /// Composes `other` after `self`. + /// + /// [`DiffBatch::full_state`] composes as follows. An empty batch is the identity, so + /// composing into one adopts `other`'s flag. Two full-state batches stay full-state: a + /// mergeable child that `other` re-activates was hidden at the version where the batches + /// meet, so `self` has no entry for it, and a child `self` re-activated followed by + /// increments is still a full state. Two incremental batches stay incremental. A + /// full-state batch and an incremental one cannot be composed: re-activated children would + /// mix full states and increments on hidden state, which neither mode can apply. + /// + /// # Panics + /// + /// If both batches are non-empty and exactly one of them is full-state. pub fn compose(&mut self, other: &Self) { if other.cid_to_events.is_empty() { return; } + if self.cid_to_events.is_empty() { + self.full_state = other.full_state; + } + assert_eq!( + self.full_state, other.full_state, + "cannot compose a full-state DiffBatch with an incremental one" + ); for (id, diff) in other.iter() { if let Some(this_diff) = self.cid_to_events.get_mut(id) { @@ -108,6 +131,7 @@ impl DiffBatch { pub fn clear(&mut self) { self.cid_to_events.clear(); self.order.clear(); + self.full_state = false; } pub fn iter(&self) -> impl Iterator + '_ { diff --git a/crates/loro-wasm/src/lib.rs b/crates/loro-wasm/src/lib.rs index 7f94b0472..9c9a1ab27 100644 --- a/crates/loro-wasm/src/lib.rs +++ b/crates/loro-wasm/src/lib.rs @@ -2328,7 +2328,8 @@ impl LoroDoc { /// `doc.diff(afterDelete, beforeDelete)` to `doc` itself or to a copy of it). The child's /// entry is then read as its full content and aligned with that hidden state. Without it /// (the default), entries are applied as increments, which is right for events forwarded to - /// a document that shares the source's hidden state. + /// a document that shares the source's hidden state. Never pass it for events from local + /// commits; see `ApplyDiffOptions.fullState`. /// /// ```ts /// const text = doc.getMap("m").ensureMergeableText("t"); @@ -6593,8 +6594,16 @@ export interface ApplyDiffOptions { * visible again carries its full content, which is aligned with whatever hidden state the * target document keeps for it (none, the same, or different). * - * Leave it unset (the default) for batches built from events: those entries are - * increments, which is right when the target shares the source's hidden state. + * Only use it for `doc.diff()` results (also after a JSON round trip), or for events from + * `import`/`checkout`, which report a revived child with its full content. + * + * Never use it for events from local commits. A local re-activation (such as + * `ensureMergeableCounter` over a deleted key) reports only the parent map change plus the + * commit's own ops, because the child's content did not change. With `fullState`, a + * missing child entry means "empty" and an entry means the whole content, so the hidden + * child would be cleared or overwritten (e.g. a counter at 7 becomes 0). Leave it unset + * (the default) for such events: their entries are increments, which is right when the + * target shares the source's hidden state. */ fullState?: boolean; } diff --git a/crates/loro/src/event.rs b/crates/loro/src/event.rs index e2954fe32..79ff0b1eb 100644 --- a/crates/loro/src/event.rs +++ b/crates/loro/src/event.rs @@ -302,6 +302,18 @@ impl DiffBatch { } /// Sets [`DiffBatch::is_full_state`]. + /// + /// Only set it for batches whose re-activated mergeable children carry their full state: + /// results of [`crate::LoroDoc::diff`] (already set), or a batch rebuilt from one, e.g. + /// after serialization. Events from an import or a checkout also qualify, since they + /// report a revived child with its full state. + /// + /// Never set it on a batch built from **local** events (commits of this doc). A local + /// re-activation (such as `ensure_mergeable_counter` over a deleted key) reports only the + /// parent marker plus the transaction's own ops, because the child's content did not + /// change. A full-state batch reads a missing child entry as "empty" and an entry as the + /// whole content, so applying such a batch would clear or overwrite the hidden child + /// (e.g. a counter at 7 becomes 0). pub fn set_full_state(&mut self, full_state: bool) { self.full_state = full_state; } diff --git a/crates/loro/tests/mergeable_revert.rs b/crates/loro/tests/mergeable_revert.rs index 969d8dcf7..612702c91 100644 --- a/crates/loro/tests/mergeable_revert.rs +++ b/crates/loro/tests/mergeable_revert.rs @@ -1386,3 +1386,109 @@ fn public_diff_aligns_non_ascii_styled_text() { } } } + +fn hidden_hello() -> (LoroDoc, Frontiers) { + let d = doc(); + d.get_map("m") + .ensure_mergeable_text("s") + .unwrap() + .insert(0, "hello") + .unwrap(); + d.commit(); + let visible = d.state_frontiers(); + delete_s(&d); + d.commit(); + (d, visible) +} + +/// Composing keeps the full-state flag: an empty batch adopts it, two full-state batches stay +/// full-state, and `clear` resets it. +#[test] +fn diff_batch_compose_keeps_full_state() { + use loro_internal::undo::DiffBatch as InnerDiffBatch; + let (d, visible) = hidden_hello(); + let deleted = d.state_frontiers(); + let full: InnerDiffBatch = d.diff(&deleted, &visible).unwrap().into(); + assert!(full.full_state); + + let mut joined = InnerDiffBatch::default(); + joined.compose(&full); + assert!(joined.full_state); + let mirror = d.fork(); + mirror.apply_diff(joined.clone().into()).unwrap(); + mirror.commit(); + assert_eq!(json(&mirror), json!({"m": {"s": "hello"}})); + + // full-state after full-state: another edit of the visible text. + let later = d.fork(); + later.checkout(&visible).unwrap(); + let t = later.get_map("m").ensure_mergeable_text("s").unwrap(); + later.set_detached_editing(true); + t.insert(5, "!").unwrap(); + later.commit(); + let next: InnerDiffBatch = later + .diff(&visible, &later.state_frontiers()) + .unwrap() + .into(); + assert!(next.full_state); + joined.compose(&next); + assert!(joined.full_state); + let mirror = d.fork(); + mirror.apply_diff(joined.clone().into()).unwrap(); + mirror.commit(); + assert_eq!(json(&mirror), json!({"m": {"s": "hello!"}})); + + joined.clear(); + assert!(!joined.full_state); +} + +#[test] +#[should_panic(expected = "cannot compose a full-state DiffBatch with an incremental one")] +fn diff_batch_compose_rejects_mixed_modes() { + use loro_internal::undo::DiffBatch as InnerDiffBatch; + let (d, visible) = hidden_hello(); + let full: InnerDiffBatch = d.diff(&d.state_frontiers(), &visible).unwrap().into(); + let mut incremental = full.clone(); + incremental.full_state = false; + incremental.compose(&full); +} + +/// Import events report a revived mergeable child with its full state, so a batch built from +/// them may be applied with the full-state flag, like a `diff` result. +#[test] +fn import_events_may_be_applied_as_full_state() { + let (d, visible) = hidden_hello(); + let b = d.fork(); + b.set_peer_id(2).unwrap(); + // A re-activates the child; B imports it and forwards its import events. + d.revert_to(&visible).unwrap(); + d.commit(); + for full_state in [false, true] { + let b = b.fork(); + let mirror = b.fork(); + let batches: Arc>> = Default::default(); + let sink = batches.clone(); + let sub = b.subscribe_root(Arc::new(move |e| { + let mut batch = DiffBatch::default(); + for ev in e.events { + batch.push(ev.target.clone(), owned_diff(ev.diff)).unwrap(); + } + sink.lock().unwrap().push(batch); + })); + b.import(&d.export(loro::ExportMode::all_updates()).unwrap()) + .unwrap(); + drop(sub); + for mut batch in batches.lock().unwrap().drain(..) { + batch.set_full_state(full_state); + mirror.apply_diff(batch).unwrap(); + } + mirror.commit(); + assert_eq!(json(&b), json!({"m": {"s": "hello"}})); + let expected = if full_state { "hello" } else { "hellohello" }; + assert_eq!( + json(&mirror), + json!({"m": {"s": expected}}), + "full_state={full_state}" + ); + } +}