Merge pull request #2387 from zed-industries/panic-in-set-selections-from-remote

Fix 'invalid insertion' panic when following

crates/collab/src/tests/integration_tests.rs

@@ -5862,10 +5862,17 @@ async fn test_basic_following(
 
     // Client A updates their selections in those editors
     editor_a1.update(cx_a, |editor, cx| {
-        editor.change_selections(None, cx, |s| s.select_ranges([0..1]))
+        editor.handle_input("a", cx);
+        editor.handle_input("b", cx);
+        editor.handle_input("c", cx);
+        editor.select_left(&Default::default(), cx);
+        assert_eq!(editor.selections.ranges(cx), vec![3..2]);
     });
     editor_a2.update(cx_a, |editor, cx| {
-        editor.change_selections(None, cx, |s| s.select_ranges([2..3]))
+        editor.handle_input("d", cx);
+        editor.handle_input("e", cx);
+        editor.select_left(&Default::default(), cx);
+        assert_eq!(editor.selections.ranges(cx), vec![2..1]);
     });
 
     // When client B starts following client A, all visible view states are replicated to client B.
@@ -5878,6 +5885,27 @@ async fn test_basic_following(
         .await
         .unwrap();
 
+    cx_c.foreground().run_until_parked();
+    let editor_b2 = workspace_b.read_with(cx_b, |workspace, cx| {
+        workspace
+            .active_item(cx)
+            .unwrap()
+            .downcast::<Editor>()
+            .unwrap()
+    });
+    assert_eq!(
+        cx_b.read(|cx| editor_b2.project_path(cx)),
+        Some((worktree_id, "2.txt").into())
+    );
+    assert_eq!(
+        editor_b2.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)),
+        vec![2..1]
+    );
+    assert_eq!(
+        editor_b1.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)),
+        vec![3..2]
+    );
+
     cx_c.foreground().run_until_parked();
     let active_call_c = cx_c.read(ActiveCall::global);
     let project_c = client_c.build_remote_project(project_id, cx_c).await;
@@ -6033,26 +6061,6 @@ async fn test_basic_following(
         });
     }
 
-    let editor_b2 = workspace_b.read_with(cx_b, |workspace, cx| {
-        workspace
-            .active_item(cx)
-            .unwrap()
-            .downcast::<Editor>()
-            .unwrap()
-    });
-    assert_eq!(
-        cx_b.read(|cx| editor_b2.project_path(cx)),
-        Some((worktree_id, "2.txt").into())
-    );
-    assert_eq!(
-        editor_b2.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)),
-        vec![2..3]
-    );
-    assert_eq!(
-        editor_b1.read_with(cx_b, |editor, cx| editor.selections.ranges(cx)),
-        vec![0..1]
-    );
-
     // When client A activates a different editor, client B does so as well.
     workspace_a.update(cx_a, |workspace, cx| {
         workspace.activate_item(&editor_a1, cx)

crates/editor/src/items.rs

@@ -3,12 +3,12 @@ use crate::{
     movement::surrounding_word, persistence::DB, scroll::ScrollAnchor, Anchor, Autoscroll, Editor,
     Event, ExcerptId, ExcerptRange, MultiBuffer, MultiBufferSnapshot, NavigationData, ToPoint as _,
 };
-use anyhow::{anyhow, Context, Result};
+use anyhow::{Context, Result};
 use collections::HashSet;
 use futures::future::try_join_all;
 use gpui::{
-    elements::*, geometry::vector::vec2f, AppContext, Entity, ModelHandle, RenderContext,
+    elements::*, geometry::vector::vec2f, AppContext, AsyncAppContext, Entity, ModelHandle,
-    Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle,
+    RenderContext, Subscription, Task, View, ViewContext, ViewHandle, WeakViewHandle,
 };
 use language::{
     proto::serialize_anchor as serialize_text_anchor, Bias, Buffer, OffsetRangeExt, Point,
@@ -72,11 +72,11 @@ impl FollowableItem for Editor {
             let editor = pane.read_with(&cx, |pane, cx| {
                 let mut editors = pane.items_of_type::<Self>();
                 editors.find(|editor| {
-                    editor.remote_id(&client, cx) == Some(remote_id)
+                    let ids_match = editor.remote_id(&client, cx) == Some(remote_id);
-                        || state.singleton
+                    let singleton_buffer_matches = state.singleton
-                            && buffers.len() == 1
+                        && buffers.first()
-                            && editor.read(cx).buffer.read(cx).as_singleton().as_ref()
+                            == editor.read(cx).buffer.read(cx).as_singleton().as_ref();
-                                == Some(&buffers[0])
+                    ids_match || singleton_buffer_matches
                 })
             });
 
@@ -115,46 +115,29 @@ impl FollowableItem for Editor {
                         multibuffer
                     });
 
-                    cx.add_view(|cx| Editor::for_multibuffer(multibuffer, Some(project), cx))
+                    cx.add_view(|cx| {
+                        let mut editor =
+                            Editor::for_multibuffer(multibuffer, Some(project.clone()), cx);
+                        editor.remote_id = Some(remote_id);
+                        editor
+                    })
                 })
             });
 
-            editor.update(&mut cx, |editor, cx| {
+            update_editor_from_message(
-                editor.remote_id = Some(remote_id);
+                editor.clone(),
-                let buffer = editor.buffer.read(cx).read(cx);
+                project,
-                let selections = state
+                proto::update_view::Editor {
-                    .selections
+                    selections: state.selections,
-                    .into_iter()
+                    pending_selection: state.pending_selection,
-                    .map(|selection| {
+                    scroll_top_anchor: state.scroll_top_anchor,
-                        deserialize_selection(&buffer, selection)
+                    scroll_x: state.scroll_x,
-                            .ok_or_else(|| anyhow!("invalid selection"))
+                    scroll_y: state.scroll_y,
-                    })
+                    ..Default::default()
-                    .collect::<Result<Vec<_>>>()?;
+                },
-                let pending_selection = state
+                &mut cx,
-                    .pending_selection
+            )
-                    .map(|selection| deserialize_selection(&buffer, selection))
+            .await?;
-                    .flatten();
-                let scroll_top_anchor = state
-                    .scroll_top_anchor
-                    .and_then(|anchor| deserialize_anchor(&buffer, anchor));
-                drop(buffer);
-
-                if !selections.is_empty() || pending_selection.is_some() {
-                    editor.set_selections_from_remote(selections, pending_selection, cx);
-                }
-
-                if let Some(scroll_top_anchor) = scroll_top_anchor {
-                    editor.set_scroll_anchor_remote(
-                        ScrollAnchor {
-                            top_anchor: scroll_top_anchor,
-                            offset: vec2f(state.scroll_x, state.scroll_y),
-                        },
-                        cx,
-                    );
-                }
-
-                anyhow::Ok(())
-            })?;
 
             Ok(editor)
         }))
@@ -299,96 +282,9 @@ impl FollowableItem for Editor {
         cx: &mut ViewContext<Self>,
     ) -> Task<Result<()>> {
         let update_view::Variant::Editor(message) = message;
-        let multibuffer = self.buffer.read(cx);
-        let multibuffer = multibuffer.read(cx);
-
-        let buffer_ids = message
-            .inserted_excerpts
-            .iter()
-            .filter_map(|insertion| Some(insertion.excerpt.as_ref()?.buffer_id))
-            .collect::<HashSet<_>>();
-
-        let mut removals = message
-            .deleted_excerpts
-            .into_iter()
-            .map(ExcerptId::from_proto)
-            .collect::<Vec<_>>();
-        removals.sort_by(|a, b| a.cmp(&b, &multibuffer));
-
-        let selections = message
-            .selections
-            .into_iter()
-            .filter_map(|selection| deserialize_selection(&multibuffer, selection))
-            .collect::<Vec<_>>();
-        let pending_selection = message
-            .pending_selection
-            .and_then(|selection| deserialize_selection(&multibuffer, selection));
-
-        let scroll_top_anchor = message
-            .scroll_top_anchor
-            .and_then(|anchor| deserialize_anchor(&multibuffer, anchor));
-        drop(multibuffer);
-
-        let buffers = project.update(cx, |project, cx| {
-            buffer_ids
-                .into_iter()
-                .map(|id| project.open_buffer_by_id(id, cx))
-                .collect::<Vec<_>>()
-        });
-
         let project = project.clone();
         cx.spawn(|this, mut cx| async move {
-            let _buffers = try_join_all(buffers).await?;
+            update_editor_from_message(this, project, message, &mut cx).await
-            this.update(&mut cx, |this, cx| {
-                this.buffer.update(cx, |multibuffer, cx| {
-                    let mut insertions = message.inserted_excerpts.into_iter().peekable();
-                    while let Some(insertion) = insertions.next() {
-                        let Some(excerpt) = insertion.excerpt else { continue };
-                        let Some(previous_excerpt_id) = insertion.previous_excerpt_id else { continue };
-                        let buffer_id = excerpt.buffer_id;
-                        let Some(buffer) = project.read(cx).buffer_for_id(buffer_id, cx) else { continue };
-
-                        let adjacent_excerpts = iter::from_fn(|| {
-                            let insertion = insertions.peek()?;
-                            if insertion.previous_excerpt_id.is_none()
-                                && insertion.excerpt.as_ref()?.buffer_id == buffer_id
-                            {
-                                insertions.next()?.excerpt
-                            } else {
-                                None
-                            }
-                        });
-
-                        multibuffer.insert_excerpts_with_ids_after(
-                            ExcerptId::from_proto(previous_excerpt_id),
-                            buffer,
-                            [excerpt]
-                                .into_iter()
-                                .chain(adjacent_excerpts)
-                                .filter_map(|excerpt| {
-                                    Some((
-                                        ExcerptId::from_proto(excerpt.id),
-                                        deserialize_excerpt_range(excerpt)?,
-                                    ))
-                                }),
-                            cx,
-                        );
-                    }
-
-                    multibuffer.remove_excerpts(removals, cx);
-                });
-
-                if !selections.is_empty() || pending_selection.is_some() {
-                    this.set_selections_from_remote(selections, pending_selection, cx);
-                    this.request_autoscroll_remotely(Autoscroll::newest(), cx);
-                } else if let Some(anchor) = scroll_top_anchor {
-                    this.set_scroll_anchor_remote(ScrollAnchor {
-                        top_anchor: anchor,
-                        offset: vec2f(message.scroll_x, message.scroll_y)
-                    }, cx);
-                }
-            });
-            Ok(())
         })
     }
 
@@ -402,6 +298,128 @@ impl FollowableItem for Editor {
     }
 }
 
+async fn update_editor_from_message(
+    this: ViewHandle<Editor>,
+    project: ModelHandle<Project>,
+    message: proto::update_view::Editor,
+    cx: &mut AsyncAppContext,
+) -> Result<()> {
+    // Open all of the buffers of which excerpts were added to the editor.
+    let inserted_excerpt_buffer_ids = message
+        .inserted_excerpts
+        .iter()
+        .filter_map(|insertion| Some(insertion.excerpt.as_ref()?.buffer_id))
+        .collect::<HashSet<_>>();
+    let inserted_excerpt_buffers = project.update(cx, |project, cx| {
+        inserted_excerpt_buffer_ids
+            .into_iter()
+            .map(|id| project.open_buffer_by_id(id, cx))
+            .collect::<Vec<_>>()
+    });
+    let _inserted_excerpt_buffers = try_join_all(inserted_excerpt_buffers).await?;
+
+    // Update the editor's excerpts.
+    this.update(cx, |editor, cx| {
+        editor.buffer.update(cx, |multibuffer, cx| {
+            let mut removed_excerpt_ids = message
+                .deleted_excerpts
+                .into_iter()
+                .map(ExcerptId::from_proto)
+                .collect::<Vec<_>>();
+            removed_excerpt_ids.sort_by({
+                let multibuffer = multibuffer.read(cx);
+                move |a, b| a.cmp(&b, &multibuffer)
+            });
+
+            let mut insertions = message.inserted_excerpts.into_iter().peekable();
+            while let Some(insertion) = insertions.next() {
+                let Some(excerpt) = insertion.excerpt else { continue };
+                let Some(previous_excerpt_id) = insertion.previous_excerpt_id else { continue };
+                let buffer_id = excerpt.buffer_id;
+                let Some(buffer) = project.read(cx).buffer_for_id(buffer_id, cx) else { continue };
+
+                let adjacent_excerpts = iter::from_fn(|| {
+                    let insertion = insertions.peek()?;
+                    if insertion.previous_excerpt_id.is_none()
+                        && insertion.excerpt.as_ref()?.buffer_id == buffer_id
+                    {
+                        insertions.next()?.excerpt
+                    } else {
+                        None
+                    }
+                });
+
+                multibuffer.insert_excerpts_with_ids_after(
+                    ExcerptId::from_proto(previous_excerpt_id),
+                    buffer,
+                    [excerpt]
+                        .into_iter()
+                        .chain(adjacent_excerpts)
+                        .filter_map(|excerpt| {
+                            Some((
+                                ExcerptId::from_proto(excerpt.id),
+                                deserialize_excerpt_range(excerpt)?,
+                            ))
+                        }),
+                    cx,
+                );
+            }
+
+            multibuffer.remove_excerpts(removed_excerpt_ids, cx);
+        });
+    });
+
+    // Deserialize the editor state.
+    let (selections, pending_selection, scroll_top_anchor) = this.update(cx, |editor, cx| {
+        let buffer = editor.buffer.read(cx).read(cx);
+        let selections = message
+            .selections
+            .into_iter()
+            .filter_map(|selection| deserialize_selection(&buffer, selection))
+            .collect::<Vec<_>>();
+        let pending_selection = message
+            .pending_selection
+            .and_then(|selection| deserialize_selection(&buffer, selection));
+        let scroll_top_anchor = message
+            .scroll_top_anchor
+            .and_then(|anchor| deserialize_anchor(&buffer, anchor));
+        anyhow::Ok((selections, pending_selection, scroll_top_anchor))
+    })?;
+
+    // Wait until the buffer has received all of the operations referenced by
+    // the editor's new state.
+    this.update(cx, |editor, cx| {
+        editor.buffer.update(cx, |buffer, cx| {
+            buffer.wait_for_anchors(
+                selections
+                    .iter()
+                    .chain(pending_selection.as_ref())
+                    .flat_map(|selection| [selection.start, selection.end])
+                    .chain(scroll_top_anchor),
+                cx,
+            )
+        })
+    })
+    .await?;
+
+    // Update the editor's state.
+    this.update(cx, |editor, cx| {
+        if !selections.is_empty() || pending_selection.is_some() {
+            editor.set_selections_from_remote(selections, pending_selection, cx);
+            editor.request_autoscroll_remotely(Autoscroll::newest(), cx);
+        } else if let Some(scroll_top_anchor) = scroll_top_anchor {
+            editor.set_scroll_anchor_remote(
+                ScrollAnchor {
+                    top_anchor: scroll_top_anchor,
+                    offset: vec2f(message.scroll_x, message.scroll_y),
+                },
+                cx,
+            );
+        }
+    });
+    Ok(())
+}
+
 fn serialize_excerpt(
     buffer_id: u64,
     id: &ExcerptId,

crates/editor/src/multi_buffer.rs

@@ -1,6 +1,7 @@
 mod anchor;
 
 pub use anchor::{Anchor, AnchorRangeExt};
+use anyhow::{anyhow, Result};
 use clock::ReplicaId;
 use collections::{BTreeMap, Bound, HashMap, HashSet};
 use futures::{channel::mpsc, SinkExt};
@@ -16,7 +17,9 @@ use language::{
 use std::{
     borrow::Cow,
     cell::{Ref, RefCell},
-    cmp, fmt, io,
+    cmp, fmt,
+    future::Future,
+    io,
     iter::{self, FromIterator},
     mem,
     ops::{Range, RangeBounds, Sub},
@@ -1238,6 +1241,39 @@ impl MultiBuffer {
         cx.notify();
     }
 
+    pub fn wait_for_anchors<'a>(
+        &self,
+        anchors: impl 'a + Iterator<Item = Anchor>,
+        cx: &mut ModelContext<Self>,
+    ) -> impl 'static + Future<Output = Result<()>> {
+        let borrow = self.buffers.borrow();
+        let mut error = None;
+        let mut futures = Vec::new();
+        for anchor in anchors {
+            if let Some(buffer_id) = anchor.buffer_id {
+                if let Some(buffer) = borrow.get(&buffer_id) {
+                    buffer.buffer.update(cx, |buffer, _| {
+                        futures.push(buffer.wait_for_anchors([anchor.text_anchor]))
+                    });
+                } else {
+                    error = Some(anyhow!(
+                        "buffer {buffer_id} is not part of this multi-buffer"
+                    ));
+                    break;
+                }
+            }
+        }
+        async move {
+            if let Some(error) = error {
+                Err(error)?;
+            }
+            for future in futures {
+                future.await?;
+            }
+            Ok(())
+        }
+    }
+
     pub fn text_anchor_for_position<T: ToOffset>(
         &self,
         position: T,

crates/language/src/buffer.rs

@@ -1313,10 +1313,10 @@ impl Buffer {
         self.text.wait_for_edits(edit_ids)
     }
 
-    pub fn wait_for_anchors<'a>(
+    pub fn wait_for_anchors(
         &mut self,
-        anchors: impl IntoIterator<Item = &'a Anchor>,
+        anchors: impl IntoIterator<Item = Anchor>,
-    ) -> impl Future<Output = Result<()>> {
+    ) -> impl 'static + Future<Output = Result<()>> {
         self.text.wait_for_anchors(anchors)
     }
 

crates/project/src/lsp_command.rs

@@ -572,7 +572,7 @@ async fn location_links_from_proto(
                     .and_then(deserialize_anchor)
                     .ok_or_else(|| anyhow!("missing origin end"))?;
                 buffer
-                    .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end]))
+                    .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end]))
                     .await?;
                 Some(Location {
                     buffer,
@@ -597,7 +597,7 @@ async fn location_links_from_proto(
             .and_then(deserialize_anchor)
             .ok_or_else(|| anyhow!("missing target end"))?;
         buffer
-            .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end]))
+            .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end]))
             .await?;
         let target = Location {
             buffer,
@@ -868,7 +868,7 @@ impl LspCommand for GetReferences {
                 .and_then(deserialize_anchor)
                 .ok_or_else(|| anyhow!("missing target end"))?;
             target_buffer
-                .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end]))
+                .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end]))
                 .await?;
             locations.push(Location {
                 buffer: target_buffer,
@@ -1012,7 +1012,7 @@ impl LspCommand for GetDocumentHighlights {
                 .and_then(deserialize_anchor)
                 .ok_or_else(|| anyhow!("missing target end"))?;
             buffer
-                .update(&mut cx, |buffer, _| buffer.wait_for_anchors([&start, &end]))
+                .update(&mut cx, |buffer, _| buffer.wait_for_anchors([start, end]))
                 .await?;
             let kind = match proto::document_highlight::Kind::from_i32(highlight.kind) {
                 Some(proto::document_highlight::Kind::Text) => DocumentHighlightKind::TEXT,

crates/text/src/text.rs

@@ -1331,15 +1331,15 @@ impl Buffer {
         }
     }
 
-    pub fn wait_for_anchors<'a>(
+    pub fn wait_for_anchors(
         &mut self,
-        anchors: impl IntoIterator<Item = &'a Anchor>,
+        anchors: impl IntoIterator<Item = Anchor>,
     ) -> impl 'static + Future<Output = Result<()>> {
         let mut futures = Vec::new();
         for anchor in anchors {
             if !self.version.observed(anchor.timestamp)
-                && *anchor != Anchor::MAX
+                && anchor != Anchor::MAX
-                && *anchor != Anchor::MIN
+                && anchor != Anchor::MIN
             {
                 let (tx, rx) = oneshot::channel();
                 self.edit_id_resolvers