From c8fb3120ff0363391e1c5825d4431262c126cbce Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Thu, 19 Sep 2024 12:57:47 -0400 Subject: [PATCH 1/2] Make `Buffer::apply_ops` infallible --- crates/assistant/src/context.rs | 8 ++-- crates/assistant/src/context/context_tests.rs | 8 +--- crates/assistant/src/context_store.rs | 6 +-- crates/channel/src/channel_buffer.rs | 4 +- crates/channel/src/channel_store.rs | 2 +- crates/collab/src/db/queries/buffers.rs | 4 +- crates/collab/src/db/tests/buffer_tests.rs | 18 ++++---- crates/language/src/buffer.rs | 5 +- crates/language/src/buffer_tests.rs | 46 ++++++++----------- crates/multi_buffer/src/multi_buffer.rs | 12 ++--- crates/project/src/buffer_store.rs | 10 ++-- crates/text/src/tests.rs | 32 ++++++------- crates/text/src/text.rs | 39 +++++++--------- 13 files changed, 86 insertions(+), 108 deletions(-) diff --git a/crates/assistant/src/context.rs b/crates/assistant/src/context.rs index d72b04e3cddb1..830c0980491f7 100644 --- a/crates/assistant/src/context.rs +++ b/crates/assistant/src/context.rs @@ -683,7 +683,7 @@ impl Context { buffer.set_text(saved_context.text.as_str(), cx) }); let operations = saved_context.into_ops(&this.buffer, cx); - this.apply_ops(operations, cx).unwrap(); + this.apply_ops(operations, cx); this } @@ -756,7 +756,7 @@ impl Context { &mut self, ops: impl IntoIterator, cx: &mut ModelContext, - ) -> Result<()> { + ) { let mut buffer_ops = Vec::new(); for op in ops { match op { @@ -765,10 +765,8 @@ impl Context { } } self.buffer - .update(cx, |buffer, cx| buffer.apply_ops(buffer_ops, cx))?; + .update(cx, |buffer, cx| buffer.apply_ops(buffer_ops, cx)); self.flush_ops(cx); - - Ok(()) } fn flush_ops(&mut self, cx: &mut ModelContext) { diff --git a/crates/assistant/src/context/context_tests.rs b/crates/assistant/src/context/context_tests.rs index 842ac05078634..2d6a2894c9521 100644 --- a/crates/assistant/src/context/context_tests.rs +++ b/crates/assistant/src/context/context_tests.rs @@ -1166,9 +1166,7 @@ async fn test_random_context_collaboration(cx: &mut TestAppContext, mut rng: Std ); network.lock().broadcast(replica_id, ops_to_send); - context - .update(cx, |context, cx| context.apply_ops(ops_to_receive, cx)) - .unwrap(); + context.update(cx, |context, cx| context.apply_ops(ops_to_receive, cx)); } else if rng.gen_bool(0.1) && replica_id != 0 { log::info!("Context {}: disconnecting", context_index); network.lock().disconnect_peer(replica_id); @@ -1180,9 +1178,7 @@ async fn test_random_context_collaboration(cx: &mut TestAppContext, mut rng: Std .map(ContextOperation::from_proto) .collect::>>() .unwrap(); - context - .update(cx, |context, cx| context.apply_ops(ops, cx)) - .unwrap(); + context.update(cx, |context, cx| context.apply_ops(ops, cx)); } } } diff --git a/crates/assistant/src/context_store.rs b/crates/assistant/src/context_store.rs index 867d906791485..f57a2fbca613c 100644 --- a/crates/assistant/src/context_store.rs +++ b/crates/assistant/src/context_store.rs @@ -223,7 +223,7 @@ impl ContextStore { if let Some(context) = this.loaded_context_for_id(&context_id, cx) { let operation_proto = envelope.payload.operation.context("invalid operation")?; let operation = ContextOperation::from_proto(operation_proto)?; - context.update(cx, |context, cx| context.apply_ops([operation], cx))?; + context.update(cx, |context, cx| context.apply_ops([operation], cx)); } Ok(()) })? @@ -394,7 +394,7 @@ impl ContextStore { .collect::>>() }) .await?; - context.update(&mut cx, |context, cx| context.apply_ops(operations, cx))??; + context.update(&mut cx, |context, cx| context.apply_ops(operations, cx))?; this.update(&mut cx, |this, cx| { if let Some(existing_context) = this.loaded_context_for_id(&context_id, cx) { existing_context @@ -531,7 +531,7 @@ impl ContextStore { .collect::>>() }) .await?; - context.update(&mut cx, |context, cx| context.apply_ops(operations, cx))??; + context.update(&mut cx, |context, cx| context.apply_ops(operations, cx))?; this.update(&mut cx, |this, cx| { if let Some(existing_context) = this.loaded_context_for_id(&context_id, cx) { existing_context diff --git a/crates/channel/src/channel_buffer.rs b/crates/channel/src/channel_buffer.rs index df3e66483f873..755e7400e1b66 100644 --- a/crates/channel/src/channel_buffer.rs +++ b/crates/channel/src/channel_buffer.rs @@ -66,7 +66,7 @@ impl ChannelBuffer { let capability = channel_store.read(cx).channel_capability(channel.id); language::Buffer::remote(buffer_id, response.replica_id as u16, capability, base_text) })?; - buffer.update(&mut cx, |buffer, cx| buffer.apply_ops(operations, cx))??; + buffer.update(&mut cx, |buffer, cx| buffer.apply_ops(operations, cx))?; let subscription = client.subscribe_to_entity(channel.id.0)?; @@ -151,7 +151,7 @@ impl ChannelBuffer { cx.notify(); this.buffer .update(cx, |buffer, cx| buffer.apply_ops(ops, cx)) - })??; + })?; Ok(()) } diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 9bd5fd564f29d..fc5b12cfae1c3 100644 --- a/crates/channel/src/channel_store.rs +++ b/crates/channel/src/channel_store.rs @@ -1007,7 +1007,7 @@ impl ChannelStore { .into_iter() .map(language::proto::deserialize_operation) .collect::>>()?; - buffer.apply_ops(incoming_operations, cx)?; + buffer.apply_ops(incoming_operations, cx); anyhow::Ok(outgoing_operations) }) .log_err(); diff --git a/crates/collab/src/db/queries/buffers.rs b/crates/collab/src/db/queries/buffers.rs index 7b19dee315476..06ad2b4594651 100644 --- a/crates/collab/src/db/queries/buffers.rs +++ b/crates/collab/src/db/queries/buffers.rs @@ -689,9 +689,7 @@ impl Database { } let mut text_buffer = text::Buffer::new(0, text::BufferId::new(1).unwrap(), base_text); - text_buffer - .apply_ops(operations.into_iter().filter_map(operation_from_wire)) - .unwrap(); + text_buffer.apply_ops(operations.into_iter().filter_map(operation_from_wire)); let base_text = text_buffer.text(); let epoch = buffer.epoch + 1; diff --git a/crates/collab/src/db/tests/buffer_tests.rs b/crates/collab/src/db/tests/buffer_tests.rs index 55a8f216c4940..adc571580a072 100644 --- a/crates/collab/src/db/tests/buffer_tests.rs +++ b/crates/collab/src/db/tests/buffer_tests.rs @@ -96,16 +96,14 @@ async fn test_channel_buffers(db: &Arc) { text::BufferId::new(1).unwrap(), buffer_response_b.base_text, ); - buffer_b - .apply_ops(buffer_response_b.operations.into_iter().map(|operation| { - let operation = proto::deserialize_operation(operation).unwrap(); - if let language::Operation::Buffer(operation) = operation { - operation - } else { - unreachable!() - } - })) - .unwrap(); + buffer_b.apply_ops(buffer_response_b.operations.into_iter().map(|operation| { + let operation = proto::deserialize_operation(operation).unwrap(); + if let language::Operation::Buffer(operation) = operation { + operation + } else { + unreachable!() + } + })); assert_eq!(buffer_b.text(), "hello, cruel world"); diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 43fe1565acb79..08fc1ccdb45d5 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -1972,7 +1972,7 @@ impl Buffer { &mut self, ops: I, cx: &mut ModelContext, - ) -> Result<()> { + ) { self.pending_autoindent.take(); let was_dirty = self.is_dirty(); let old_version = self.version.clone(); @@ -1991,14 +1991,13 @@ impl Buffer { } }) .collect::>(); - self.text.apply_ops(buffer_ops)?; + self.text.apply_ops(buffer_ops); self.deferred_ops.insert(deferred_ops); self.flush_deferred_ops(cx); self.did_edit(&old_version, was_dirty, cx); // Notify independently of whether the buffer was edited as the operations could include a // selection update. cx.notify(); - Ok(()) } fn flush_deferred_ops(&mut self, cx: &mut ModelContext) { diff --git a/crates/language/src/buffer_tests.rs b/crates/language/src/buffer_tests.rs index 50dea8d2562b0..23faa33316da7 100644 --- a/crates/language/src/buffer_tests.rs +++ b/crates/language/src/buffer_tests.rs @@ -308,7 +308,7 @@ fn test_edit_events(cx: &mut gpui::AppContext) { // Incorporating a set of remote ops emits a single edited event, // followed by a dirty changed event. buffer2.update(cx, |buffer, cx| { - buffer.apply_ops(buffer1_ops.lock().drain(..), cx).unwrap(); + buffer.apply_ops(buffer1_ops.lock().drain(..), cx); }); assert_eq!( mem::take(&mut *buffer_1_events.lock()), @@ -332,7 +332,7 @@ fn test_edit_events(cx: &mut gpui::AppContext) { // Incorporating the remote ops again emits a single edited event, // followed by a dirty changed event. buffer2.update(cx, |buffer, cx| { - buffer.apply_ops(buffer1_ops.lock().drain(..), cx).unwrap(); + buffer.apply_ops(buffer1_ops.lock().drain(..), cx); }); assert_eq!( mem::take(&mut *buffer_1_events.lock()), @@ -2274,13 +2274,11 @@ fn test_serialization(cx: &mut gpui::AppContext) { .block(buffer1.read(cx).serialize_ops(None, cx)); let buffer2 = cx.new_model(|cx| { let mut buffer = Buffer::from_proto(1, Capability::ReadWrite, state, None).unwrap(); - buffer - .apply_ops( - ops.into_iter() - .map(|op| proto::deserialize_operation(op).unwrap()), - cx, - ) - .unwrap(); + buffer.apply_ops( + ops.into_iter() + .map(|op| proto::deserialize_operation(op).unwrap()), + cx, + ); buffer }); assert_eq!(buffer2.read(cx).text(), "abcDF"); @@ -2401,13 +2399,11 @@ fn test_random_collaboration(cx: &mut AppContext, mut rng: StdRng) { .block(base_buffer.read(cx).serialize_ops(None, cx)); let mut buffer = Buffer::from_proto(i as ReplicaId, Capability::ReadWrite, state, None).unwrap(); - buffer - .apply_ops( - ops.into_iter() - .map(|op| proto::deserialize_operation(op).unwrap()), - cx, - ) - .unwrap(); + buffer.apply_ops( + ops.into_iter() + .map(|op| proto::deserialize_operation(op).unwrap()), + cx, + ); buffer.set_group_interval(Duration::from_millis(rng.gen_range(0..=200))); let network = network.clone(); cx.subscribe(&cx.handle(), move |buffer, _, event, _| { @@ -2523,14 +2519,12 @@ fn test_random_collaboration(cx: &mut AppContext, mut rng: StdRng) { None, ) .unwrap(); - new_buffer - .apply_ops( - old_buffer_ops - .into_iter() - .map(|op| deserialize_operation(op).unwrap()), - cx, - ) - .unwrap(); + new_buffer.apply_ops( + old_buffer_ops + .into_iter() + .map(|op| deserialize_operation(op).unwrap()), + cx, + ); log::info!( "New replica {} text: {:?}", new_buffer.replica_id(), @@ -2570,7 +2564,7 @@ fn test_random_collaboration(cx: &mut AppContext, mut rng: StdRng) { ops ); new_buffer.update(cx, |new_buffer, cx| { - new_buffer.apply_ops(ops, cx).unwrap(); + new_buffer.apply_ops(ops, cx); }); } } @@ -2598,7 +2592,7 @@ fn test_random_collaboration(cx: &mut AppContext, mut rng: StdRng) { ops.len(), ops ); - buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx).unwrap()); + buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx)); } } _ => {} diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 9dee092dea9f2..29bd9a80682a1 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -5019,13 +5019,11 @@ mod tests { .background_executor() .block(host_buffer.read(cx).serialize_ops(None, cx)); let mut buffer = Buffer::from_proto(1, Capability::ReadWrite, state, None).unwrap(); - buffer - .apply_ops( - ops.into_iter() - .map(|op| language::proto::deserialize_operation(op).unwrap()), - cx, - ) - .unwrap(); + buffer.apply_ops( + ops.into_iter() + .map(|op| language::proto::deserialize_operation(op).unwrap()), + cx, + ); buffer }); let multibuffer = cx.new_model(|cx| MultiBuffer::singleton(guest_buffer.clone(), cx)); diff --git a/crates/project/src/buffer_store.rs b/crates/project/src/buffer_store.rs index ead32359970e2..05b59998f69dd 100644 --- a/crates/project/src/buffer_store.rs +++ b/crates/project/src/buffer_store.rs @@ -644,7 +644,7 @@ impl BufferStore { } hash_map::Entry::Occupied(mut entry) => { if let OpenBuffer::Operations(operations) = entry.get_mut() { - buffer.update(cx, |b, cx| b.apply_ops(operations.drain(..), cx))?; + buffer.update(cx, |b, cx| b.apply_ops(operations.drain(..), cx)); } else if entry.get().upgrade().is_some() { if is_remote { return Ok(()); @@ -1051,12 +1051,12 @@ impl BufferStore { match this.opened_buffers.entry(buffer_id) { hash_map::Entry::Occupied(mut e) => match e.get_mut() { OpenBuffer::Strong(buffer) => { - buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx))?; + buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx)); } OpenBuffer::Operations(operations) => operations.extend_from_slice(&ops), OpenBuffer::Weak(buffer) => { if let Some(buffer) = buffer.upgrade() { - buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx))?; + buffer.update(cx, |buffer, cx| buffer.apply_ops(ops, cx)); } } }, @@ -1217,7 +1217,9 @@ impl BufferStore { .into_iter() .map(language::proto::deserialize_operation) .collect::>>()?; - buffer.update(cx, |buffer, cx| buffer.apply_ops(operations, cx)) + buffer.update(cx, |buffer, cx| { + anyhow::Ok(buffer.apply_ops(operations, cx)) + }) }); if let Err(error) = result { diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index 6f748fb5880b3..8c5d7014eebda 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -515,25 +515,25 @@ fn test_undo_redo() { let entries = buffer.history.undo_stack.clone(); assert_eq!(entries.len(), 3); - buffer.undo_or_redo(entries[0].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[0].transaction.clone()); assert_eq!(buffer.text(), "1cdef234"); - buffer.undo_or_redo(entries[0].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[0].transaction.clone()); assert_eq!(buffer.text(), "1abcdef234"); - buffer.undo_or_redo(entries[1].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[1].transaction.clone()); assert_eq!(buffer.text(), "1abcdx234"); - buffer.undo_or_redo(entries[2].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[2].transaction.clone()); assert_eq!(buffer.text(), "1abx234"); - buffer.undo_or_redo(entries[1].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[1].transaction.clone()); assert_eq!(buffer.text(), "1abyzef234"); - buffer.undo_or_redo(entries[2].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[2].transaction.clone()); assert_eq!(buffer.text(), "1abcdef234"); - buffer.undo_or_redo(entries[2].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[2].transaction.clone()); assert_eq!(buffer.text(), "1abyzef234"); - buffer.undo_or_redo(entries[0].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[0].transaction.clone()); assert_eq!(buffer.text(), "1yzef234"); - buffer.undo_or_redo(entries[1].transaction.clone()).unwrap(); + buffer.undo_or_redo(entries[1].transaction.clone()); assert_eq!(buffer.text(), "1234"); } @@ -692,12 +692,12 @@ fn test_concurrent_edits() { let buf3_op = buffer3.edit([(5..6, "56")]); assert_eq!(buffer3.text(), "abcde56"); - buffer1.apply_op(buf2_op.clone()).unwrap(); - buffer1.apply_op(buf3_op.clone()).unwrap(); - buffer2.apply_op(buf1_op.clone()).unwrap(); - buffer2.apply_op(buf3_op).unwrap(); - buffer3.apply_op(buf1_op).unwrap(); - buffer3.apply_op(buf2_op).unwrap(); + buffer1.apply_op(buf2_op.clone()); + buffer1.apply_op(buf3_op.clone()); + buffer2.apply_op(buf1_op.clone()); + buffer2.apply_op(buf3_op); + buffer3.apply_op(buf1_op); + buffer3.apply_op(buf2_op); assert_eq!(buffer1.text(), "a12c34e56"); assert_eq!(buffer2.text(), "a12c34e56"); @@ -756,7 +756,7 @@ fn test_random_concurrent_edits(mut rng: StdRng) { replica_id, ops.len() ); - buffer.apply_ops(ops).unwrap(); + buffer.apply_ops(ops); } } _ => {} diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index 9630ec5b80334..8d2cd97aacaae 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -38,7 +38,6 @@ pub use subscription::*; pub use sum_tree::Bias; use sum_tree::{FilterCursor, SumTree, TreeMap}; use undo_map::UndoMap; -use util::ResultExt; #[cfg(any(test, feature = "test-support"))] use util::RandomCharIter; @@ -927,23 +926,22 @@ impl Buffer { self.snapshot.line_ending = line_ending; } - pub fn apply_ops>(&mut self, ops: I) -> Result<()> { + pub fn apply_ops>(&mut self, ops: I) { let mut deferred_ops = Vec::new(); for op in ops { self.history.push(op.clone()); if self.can_apply_op(&op) { - self.apply_op(op)?; + self.apply_op(op); } else { self.deferred_replicas.insert(op.replica_id()); deferred_ops.push(op); } } self.deferred_ops.insert(deferred_ops); - self.flush_deferred_ops()?; - Ok(()) + self.flush_deferred_ops(); } - fn apply_op(&mut self, op: Operation) -> Result<()> { + fn apply_op(&mut self, op: Operation) { match op { Operation::Edit(edit) => { if !self.version.observed(edit.timestamp) { @@ -960,7 +958,7 @@ impl Buffer { } Operation::Undo(undo) => { if !self.version.observed(undo.timestamp) { - self.apply_undo(&undo)?; + self.apply_undo(&undo); self.snapshot.version.observe(undo.timestamp); self.lamport_clock.observe(undo.timestamp); } @@ -974,7 +972,6 @@ impl Buffer { true } }); - Ok(()) } fn apply_remote_edit( @@ -1217,7 +1214,7 @@ impl Buffer { fragment_ids } - fn apply_undo(&mut self, undo: &UndoOperation) -> Result<()> { + fn apply_undo(&mut self, undo: &UndoOperation) { self.snapshot.undo_map.insert(undo); let mut edits = Patch::default(); @@ -1268,22 +1265,20 @@ impl Buffer { self.snapshot.visible_text = visible_text; self.snapshot.deleted_text = deleted_text; self.subscriptions.publish_mut(&edits); - Ok(()) } - fn flush_deferred_ops(&mut self) -> Result<()> { + fn flush_deferred_ops(&mut self) { self.deferred_replicas.clear(); let mut deferred_ops = Vec::new(); for op in self.deferred_ops.drain().iter().cloned() { if self.can_apply_op(&op) { - self.apply_op(op)?; + self.apply_op(op); } else { self.deferred_replicas.insert(op.replica_id()); deferred_ops.push(op); } } self.deferred_ops.insert(deferred_ops); - Ok(()) } fn can_apply_op(&self, op: &Operation) -> bool { @@ -1352,7 +1347,7 @@ impl Buffer { if let Some(entry) = self.history.pop_undo() { let transaction = entry.transaction.clone(); let transaction_id = transaction.id; - let op = self.undo_or_redo(transaction).unwrap(); + let op = self.undo_or_redo(transaction); Some((transaction_id, op)) } else { None @@ -1365,7 +1360,7 @@ impl Buffer { .remove_from_undo(transaction_id)? .transaction .clone(); - self.undo_or_redo(transaction).log_err() + Some(self.undo_or_redo(transaction)) } pub fn undo_to_transaction(&mut self, transaction_id: TransactionId) -> Vec { @@ -1378,7 +1373,7 @@ impl Buffer { transactions .into_iter() - .map(|transaction| self.undo_or_redo(transaction).unwrap()) + .map(|transaction| self.undo_or_redo(transaction)) .collect() } @@ -1394,7 +1389,7 @@ impl Buffer { if let Some(entry) = self.history.pop_redo() { let transaction = entry.transaction.clone(); let transaction_id = transaction.id; - let op = self.undo_or_redo(transaction).unwrap(); + let op = self.undo_or_redo(transaction); Some((transaction_id, op)) } else { None @@ -1411,11 +1406,11 @@ impl Buffer { transactions .into_iter() - .map(|transaction| self.undo_or_redo(transaction).unwrap()) + .map(|transaction| self.undo_or_redo(transaction)) .collect() } - fn undo_or_redo(&mut self, transaction: Transaction) -> Result { + fn undo_or_redo(&mut self, transaction: Transaction) -> Operation { let mut counts = HashMap::default(); for edit_id in transaction.edit_ids { counts.insert(edit_id, self.undo_map.undo_count(edit_id) + 1); @@ -1426,11 +1421,11 @@ impl Buffer { version: self.version(), counts, }; - self.apply_undo(&undo)?; + self.apply_undo(&undo); self.snapshot.version.observe(undo.timestamp); let operation = Operation::Undo(undo); self.history.push(operation.clone()); - Ok(operation) + operation } pub fn push_transaction(&mut self, transaction: Transaction, now: Instant) { @@ -1762,7 +1757,7 @@ impl Buffer { self.replica_id, transaction ); - ops.push(self.undo_or_redo(transaction).unwrap()); + ops.push(self.undo_or_redo(transaction)); } } ops From cd162e8bf9bc08c84afdc7b675f242b1fd9c3e90 Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Thu, 19 Sep 2024 13:03:40 -0400 Subject: [PATCH 2/2] Fix Clippy warning --- crates/project/src/buffer_store.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/project/src/buffer_store.rs b/crates/project/src/buffer_store.rs index 05b59998f69dd..0045aba2e89ec 100644 --- a/crates/project/src/buffer_store.rs +++ b/crates/project/src/buffer_store.rs @@ -1217,9 +1217,8 @@ impl BufferStore { .into_iter() .map(language::proto::deserialize_operation) .collect::>>()?; - buffer.update(cx, |buffer, cx| { - anyhow::Ok(buffer.apply_ops(operations, cx)) - }) + buffer.update(cx, |buffer, cx| buffer.apply_ops(operations, cx)); + anyhow::Ok(()) }); if let Err(error) = result {