Merge pull request #2423 from zed-industries/fix-panic-in-go-to-definition

Fix panic when clicking on a definition

crates/editor/src/editor.rs

@@ -1331,6 +1331,10 @@ impl Editor {
         &self.buffer
     }
 
+    fn workspace(&self, cx: &AppContext) -> Option<ViewHandle<Workspace>> {
+        self.workspace.as_ref()?.0.upgrade(cx)
+    }
+
     pub fn title<'a>(&self, cx: &'a AppContext) -> Cow<'a, str> {
         self.buffer().read(cx).title(cx)
     }
@@ -5558,93 +5562,77 @@ impl Editor {
         }
     }
 
-    pub fn go_to_definition(
-        workspace: &mut Workspace,
-        _: &GoToDefinition,
-        cx: &mut ViewContext<Workspace>,
-    ) {
-        Self::go_to_definition_of_kind(GotoDefinitionKind::Symbol, workspace, cx);
+    pub fn go_to_definition(&mut self, _: &GoToDefinition, cx: &mut ViewContext<Self>) {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Symbol, cx);
     }
 
-    pub fn go_to_type_definition(
-        workspace: &mut Workspace,
-        _: &GoToTypeDefinition,
-        cx: &mut ViewContext<Workspace>,
-    ) {
-        Self::go_to_definition_of_kind(GotoDefinitionKind::Type, workspace, cx);
+    pub fn go_to_type_definition(&mut self, _: &GoToTypeDefinition, cx: &mut ViewContext<Self>) {
+        self.go_to_definition_of_kind(GotoDefinitionKind::Type, cx);
     }
 
-    fn go_to_definition_of_kind(
-        kind: GotoDefinitionKind,
-        workspace: &mut Workspace,
-        cx: &mut ViewContext<Workspace>,
-    ) {
-        let active_item = workspace.active_item(cx);
-        let editor_handle = if let Some(editor) = active_item
-            .as_ref()
-            .and_then(|item| item.act_as::<Self>(cx))
-        {
-            editor
-        } else {
-            return;
-        };
-
-        let editor = editor_handle.read(cx);
-        let buffer = editor.buffer.read(cx);
-        let head = editor.selections.newest::<usize>(cx).head();
+    fn go_to_definition_of_kind(&mut self, kind: GotoDefinitionKind, cx: &mut ViewContext<Self>) {
+        let Some(workspace) = self.workspace(cx) else { return };
+        let buffer = self.buffer.read(cx);
+        let head = self.selections.newest::<usize>(cx).head();
         let (buffer, head) = if let Some(text_anchor) = buffer.text_anchor_for_position(head, cx) {
             text_anchor
         } else {
             return;
         };
 
-        let project = workspace.project().clone();
+        let project = workspace.read(cx).project().clone();
         let definitions = project.update(cx, |project, cx| match kind {
             GotoDefinitionKind::Symbol => project.definition(&buffer, head, cx),
             GotoDefinitionKind::Type => project.type_definition(&buffer, head, cx),
         });
 
-        cx.spawn_labeled("Fetching Definition...", |workspace, mut cx| async move {
+        cx.spawn_labeled("Fetching Definition...", |editor, mut cx| async move {
             let definitions = definitions.await?;
-            workspace.update(&mut cx, |workspace, cx| {
-                Editor::navigate_to_definitions(workspace, editor_handle, definitions, cx);
+            editor.update(&mut cx, |editor, cx| {
+                editor.navigate_to_definitions(definitions, cx);
             })?;
-
             Ok::<(), anyhow::Error>(())
         })
         .detach_and_log_err(cx);
     }
 
     pub fn navigate_to_definitions(
-        workspace: &mut Workspace,
-        editor_handle: ViewHandle<Editor>,
-        definitions: Vec<LocationLink>,
-        cx: &mut ViewContext<Workspace>,
+        &mut self,
+        mut definitions: Vec<LocationLink>,
+        cx: &mut ViewContext<Editor>,
     ) {
-        let pane = workspace.active_pane().clone();
+        let Some(workspace) = self.workspace(cx) else { return };
+        let pane = workspace.read(cx).active_pane().clone();
         // If there is one definition, just open it directly
-        if let [definition] = definitions.as_slice() {
+        if definitions.len() == 1 {
+            let definition = definitions.pop().unwrap();
             let range = definition
                 .target
                 .range
                 .to_offset(definition.target.buffer.read(cx));
 
-            let target_editor_handle =
-                workspace.open_project_item(definition.target.buffer.clone(), cx);
-            target_editor_handle.update(cx, |target_editor, cx| {
-                // When selecting a definition in a different buffer, disable the nav history
-                // to avoid creating a history entry at the previous cursor location.
-                if editor_handle != target_editor_handle {
-                    pane.update(cx, |pane, _| pane.disable_history());
-                }
-                target_editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
+            if Some(&definition.target.buffer) == self.buffer.read(cx).as_singleton().as_ref() {
+                self.change_selections(Some(Autoscroll::fit()), cx, |s| {
                     s.select_ranges([range]);
                 });
-
-                pane.update(cx, |pane, _| pane.enable_history());
-            });
+            } else {
+                cx.window_context().defer(move |cx| {
+                    let target_editor: ViewHandle<Self> = workspace.update(cx, |workspace, cx| {
+                        workspace.open_project_item(definition.target.buffer.clone(), cx)
+                    });
+                    target_editor.update(cx, |target_editor, cx| {
+                        // When selecting a definition in a different buffer, disable the nav history
+                        // to avoid creating a history entry at the previous cursor location.
+                        pane.update(cx, |pane, _| pane.disable_history());
+                        target_editor.change_selections(Some(Autoscroll::fit()), cx, |s| {
+                            s.select_ranges([range]);
+                        });
+                        pane.update(cx, |pane, _| pane.enable_history());
+                    });
+                });
+            }
         } else if !definitions.is_empty() {
-            let replica_id = editor_handle.read(cx).replica_id(cx);
+            let replica_id = self.replica_id(cx);
             let title = definitions
                 .iter()
                 .find(|definition| definition.origin.is_some())
@@ -5664,7 +5652,9 @@ impl Editor {
                 .into_iter()
                 .map(|definition| definition.target)
                 .collect();
-            Self::open_locations_in_multibuffer(workspace, locations, replica_id, title, cx)
+            workspace.update(cx, |workspace, cx| {
+                Self::open_locations_in_multibuffer(workspace, locations, replica_id, title, cx)
+            })
         }
     }
 

crates/editor/src/element.rs

@@ -309,25 +309,17 @@ impl EditorElement {
             editor.select(SelectPhase::End, cx);
         }
 
-        if let Some(workspace) = editor
-            .workspace
-            .as_ref()
-            .and_then(|(workspace, _)| workspace.upgrade(cx))
-        {
-            if !pending_nonempty_selections && cmd && text_bounds.contains_point(position) {
-                let (point, target_point) = position_map.point_for_position(text_bounds, position);
+        if !pending_nonempty_selections && cmd && text_bounds.contains_point(position) {
+            let (point, target_point) = position_map.point_for_position(text_bounds, position);
 
-                if point == target_point {
-                    workspace.update(cx, |workspace, cx| {
-                        if shift {
-                            go_to_fetched_type_definition(workspace, point, cx);
-                        } else {
-                            go_to_fetched_definition(workspace, point, cx);
-                        }
-                    });
-
-                    return true;
+            if point == target_point {
+                if shift {
+                    go_to_fetched_type_definition(editor, point, cx);
+                } else {
+                    go_to_fetched_definition(editor, point, cx);
                 }
+
+                return true;
             }
         }
 

crates/editor/src/link_go_to_definition.rs

@@ -1,15 +1,11 @@
 use std::ops::Range;
 
+use crate::{Anchor, DisplayPoint, Editor, EditorSnapshot, SelectPhase};
 use gpui::{Task, ViewContext};
 use language::{Bias, ToOffset};
 use project::LocationLink;
 use settings::Settings;
 use util::TryFutureExt;
-use workspace::Workspace;
-
-use crate::{
-    Anchor, DisplayPoint, Editor, EditorSnapshot, GoToDefinition, GoToTypeDefinition, SelectPhase,
-};
 
 #[derive(Debug, Default)]
 pub struct LinkGoToDefinitionState {
@@ -250,70 +246,51 @@ pub fn hide_link_definition(editor: &mut Editor, cx: &mut ViewContext<Editor>) {
 }
 
 pub fn go_to_fetched_definition(
-    workspace: &mut Workspace,
+    editor: &mut Editor,
     point: DisplayPoint,
-    cx: &mut ViewContext<Workspace>,
+    cx: &mut ViewContext<Editor>,
 ) {
-    go_to_fetched_definition_of_kind(LinkDefinitionKind::Symbol, workspace, point, cx);
+    go_to_fetched_definition_of_kind(LinkDefinitionKind::Symbol, editor, point, cx);
 }
 
 pub fn go_to_fetched_type_definition(
-    workspace: &mut Workspace,
+    editor: &mut Editor,
     point: DisplayPoint,
-    cx: &mut ViewContext<Workspace>,
+    cx: &mut ViewContext<Editor>,
 ) {
-    go_to_fetched_definition_of_kind(LinkDefinitionKind::Type, workspace, point, cx);
+    go_to_fetched_definition_of_kind(LinkDefinitionKind::Type, editor, point, cx);
 }
 
 fn go_to_fetched_definition_of_kind(
     kind: LinkDefinitionKind,
-    workspace: &mut Workspace,
+    editor: &mut Editor,
     point: DisplayPoint,
-    cx: &mut ViewContext<Workspace>,
+    cx: &mut ViewContext<Editor>,
 ) {
-    let active_item = workspace.active_item(cx);
-    let editor_handle = if let Some(editor) = active_item
-        .as_ref()
-        .and_then(|item| item.act_as::<Editor>(cx))
-    {
-        editor
-    } else {
-        return;
-    };
-
-    let (cached_definitions, cached_definitions_kind) = editor_handle.update(cx, |editor, cx| {
-        let definitions = editor.link_go_to_definition_state.definitions.clone();
-        hide_link_definition(editor, cx);
-        (definitions, editor.link_go_to_definition_state.kind)
-    });
+    let cached_definitions = editor.link_go_to_definition_state.definitions.clone();
+    hide_link_definition(editor, cx);
+    let cached_definitions_kind = editor.link_go_to_definition_state.kind;
 
     let is_correct_kind = cached_definitions_kind == Some(kind);
     if !cached_definitions.is_empty() && is_correct_kind {
-        editor_handle.update(cx, |editor, cx| {
-            if !editor.focused {
-                cx.focus_self();
-            }
-        });
+        if !editor.focused {
+            cx.focus_self();
+        }
 
-        Editor::navigate_to_definitions(workspace, editor_handle, cached_definitions, cx);
+        editor.navigate_to_definitions(cached_definitions, cx);
     } else {
-        editor_handle.update(cx, |editor, cx| {
-            editor.select(
-                SelectPhase::Begin {
-                    position: point,
-                    add: false,
-                    click_count: 1,
-                },
-                cx,
-            );
-        });
+        editor.select(
+            SelectPhase::Begin {
+                position: point,
+                add: false,
+                click_count: 1,
+            },
+            cx,
+        );
 
         match kind {
-            LinkDefinitionKind::Symbol => Editor::go_to_definition(workspace, &GoToDefinition, cx),
-
-            LinkDefinitionKind::Type => {
-                Editor::go_to_type_definition(workspace, &GoToTypeDefinition, cx)
-            }
+            LinkDefinitionKind::Symbol => editor.go_to_definition(&Default::default(), cx),
+            LinkDefinitionKind::Type => editor.go_to_type_definition(&Default::default(), cx),
         }
     }
 }
@@ -426,8 +403,8 @@ mod tests {
                 ])))
             });
 
-        cx.update_workspace(|workspace, cx| {
-            go_to_fetched_type_definition(workspace, hover_point, cx);
+        cx.update_editor(|editor, cx| {
+            go_to_fetched_type_definition(editor, hover_point, cx);
         });
         requests.next().await;
         cx.foreground().run_until_parked();
@@ -635,8 +612,8 @@ mod tests {
         "});
 
         // Cmd click with existing definition doesn't re-request and dismisses highlight
-        cx.update_workspace(|workspace, cx| {
-            go_to_fetched_definition(workspace, hover_point, cx);
+        cx.update_editor(|editor, cx| {
+            go_to_fetched_definition(editor, hover_point, cx);
         });
         // Assert selection moved to to definition
         cx.lsp
@@ -676,8 +653,8 @@ mod tests {
                 },
             ])))
         });
-        cx.update_workspace(|workspace, cx| {
-            go_to_fetched_definition(workspace, hover_point, cx);
+        cx.update_editor(|editor, cx| {
+            go_to_fetched_definition(editor, hover_point, cx);
         });
         requests.next().await;
         cx.foreground().run_until_parked();