Make `Buffer::apply_ops` infallible (#18089)

Marshall Bowers created

This PR makes the `Buffer::apply_ops` method infallible for
`text::Buffer` and `language::Buffer`.

We discovered that `text::Buffer::apply_ops` was only fallible due to
`apply_undo`, which didn't actually need to be fallible.

Release Notes:

- N/A

Change summary

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            |  9 ++-
crates/text/src/tests.rs                      | 32 +++++++-------
crates/text/src/text.rs                       | 39 +++++++----------
13 files changed, 85 insertions(+), 108 deletions(-)

Detailed changes

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<Item = ContextOperation>,
         cx: &mut ModelContext<Self>,
-    ) -> 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<Context>) {

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::<Result<Vec<_>>>()
                         .unwrap();
-                    context
-                        .update(cx, |context, cx| context.apply_ops(ops, cx))
-                        .unwrap();
+                    context.update(cx, |context, cx| context.apply_ops(ops, cx));
                 }
             }
         }

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::<Result<Vec<_>>>()
                 })
                 .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::<Result<Vec<_>>>()
                 })
                 .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

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(())
     }

crates/channel/src/channel_store.rs 🔗

@@ -1007,7 +1007,7 @@ impl ChannelStore {
                                                 .into_iter()
                                                 .map(language::proto::deserialize_operation)
                                                 .collect::<Result<Vec<_>>>()?;
-                                        buffer.apply_ops(incoming_operations, cx)?;
+                                        buffer.apply_ops(incoming_operations, cx);
                                         anyhow::Ok(outgoing_operations)
                                     })
                                     .log_err();

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;

crates/collab/src/db/tests/buffer_tests.rs 🔗

@@ -96,16 +96,14 @@ async fn test_channel_buffers(db: &Arc<Database>) {
         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");
 

crates/language/src/buffer.rs 🔗

@@ -1972,7 +1972,7 @@ impl Buffer {
         &mut self,
         ops: I,
         cx: &mut ModelContext<Self>,
-    ) -> Result<()> {
+    ) {
         self.pending_autoindent.take();
         let was_dirty = self.is_dirty();
         let old_version = self.version.clone();
@@ -1991,14 +1991,13 @@ impl Buffer {
                 }
             })
             .collect::<Vec<_>>();
-        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<Self>) {

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));
                 }
             }
             _ => {}

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));

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,8 @@ impl BufferStore {
                         .into_iter()
                         .map(language::proto::deserialize_operation)
                         .collect::<Result<Vec<_>>>()?;
-                    buffer.update(cx, |buffer, cx| buffer.apply_ops(operations, cx))
+                    buffer.update(cx, |buffer, cx| buffer.apply_ops(operations, cx));
+                    anyhow::Ok(())
                 });
 
                 if let Err(error) = result {

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);
                 }
             }
             _ => {}

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<I: IntoIterator<Item = Operation>>(&mut self, ops: I) -> Result<()> {
+    pub fn apply_ops<I: IntoIterator<Item = Operation>>(&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<Operation> {
@@ -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<Operation> {
+    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