From 477fc865f5b6f2b1bf43cb4350f49eced80e4e29 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Mon, 21 Aug 2023 20:21:38 +0300 Subject: [PATCH] Properly resolve inlay label parts' locations and buffers --- crates/editor/src/display_map/inlay_map.rs | 1 + crates/editor/src/element.rs | 33 +-- crates/editor/src/link_go_to_definition.rs | 43 ++-- crates/project/src/lsp_command.rs | 256 ++++++++++++++------- crates/project/src/project.rs | 21 +- 5 files changed, 222 insertions(+), 132 deletions(-) diff --git a/crates/editor/src/display_map/inlay_map.rs b/crates/editor/src/display_map/inlay_map.rs index 026f1fc2c2..34181a5769 100644 --- a/crates/editor/src/display_map/inlay_map.rs +++ b/crates/editor/src/display_map/inlay_map.rs @@ -1010,6 +1010,7 @@ impl InlaySnapshot { }) { Ok(i) | Err(i) => i, }; + // TODO kb add a way to highlight inlay hints through here. for range in &ranges[start_ix..] { if range.start.cmp(&transform_end, &self.buffer).is_ge() { break; diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 15b28914af..c28f14d98a 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -358,18 +358,15 @@ impl EditorElement { } if !pending_nonempty_selections && cmd && text_bounds.contains_point(position) { - if let Some(point) = position_map - .point_for_position(text_bounds, position) - .as_valid() - { - if shift { - go_to_fetched_type_definition(editor, point, alt, cx); - } else { - go_to_fetched_definition(editor, point, alt, cx); - } - - return true; + let point = position_map.point_for_position(text_bounds, position); + let could_be_inlay = point.as_valid().is_none(); + if shift || could_be_inlay { + go_to_fetched_type_definition(editor, point, alt, cx); + } else { + go_to_fetched_definition(editor, point, alt, cx); } + + return true; } end_selection @@ -2818,14 +2815,24 @@ struct PositionMap { } #[derive(Debug)] -struct PointForPosition { +pub struct PointForPosition { previous_valid: DisplayPoint, - next_valid: DisplayPoint, + pub next_valid: DisplayPoint, exact_unclipped: DisplayPoint, column_overshoot_after_line_end: u32, } impl PointForPosition { + #[cfg(test)] + pub fn valid(valid: DisplayPoint) -> Self { + Self { + previous_valid: valid, + next_valid: valid, + exact_unclipped: valid, + column_overshoot_after_line_end: 0, + } + } + fn as_valid(&self) -> Option { if self.previous_valid == self.exact_unclipped && self.next_valid == self.exact_unclipped { Some(self.previous_valid) diff --git a/crates/editor/src/link_go_to_definition.rs b/crates/editor/src/link_go_to_definition.rs index a001433727..b043af6132 100644 --- a/crates/editor/src/link_go_to_definition.rs +++ b/crates/editor/src/link_go_to_definition.rs @@ -1,4 +1,4 @@ -use crate::{Anchor, DisplayPoint, Editor, EditorSnapshot, SelectPhase}; +use crate::{element::PointForPosition, Anchor, DisplayPoint, Editor, EditorSnapshot, SelectPhase}; use gpui::{Task, ViewContext}; use language::{Bias, ToOffset}; use project::LocationLink; @@ -7,7 +7,7 @@ use util::TryFutureExt; #[derive(Debug, Default)] pub struct LinkGoToDefinitionState { - pub last_trigger_point: Option, + pub last_trigger_point: Option, pub symbol_range: Option>, pub kind: Option, pub definitions: Vec, @@ -21,29 +21,29 @@ pub enum GoToDefinitionTrigger { } #[derive(Debug, Clone)] -pub enum TriggerPoint { +pub enum TriggerAnchor { Text(Anchor), InlayHint(Anchor, LocationLink), } -impl TriggerPoint { +impl TriggerAnchor { fn anchor(&self) -> &Anchor { match self { - TriggerPoint::Text(anchor) => anchor, - TriggerPoint::InlayHint(anchor, _) => anchor, + TriggerAnchor::Text(anchor) => anchor, + TriggerAnchor::InlayHint(anchor, _) => anchor, } } pub fn definition_kind(&self, shift: bool) -> LinkDefinitionKind { match self { - TriggerPoint::Text(_) => { + TriggerAnchor::Text(_) => { if shift { LinkDefinitionKind::Type } else { LinkDefinitionKind::Symbol } } - TriggerPoint::InlayHint(_, link) => LinkDefinitionKind::Type, + TriggerAnchor::InlayHint(_, _) => LinkDefinitionKind::Type, } } } @@ -61,11 +61,11 @@ pub fn update_go_to_definition_link( let snapshot = editor.snapshot(cx); let trigger_point = match origin { GoToDefinitionTrigger::Text(p) => { - Some(TriggerPoint::Text(snapshot.buffer_snapshot.anchor_before( + Some(TriggerAnchor::Text(snapshot.buffer_snapshot.anchor_before( p.to_offset(&snapshot.display_snapshot, Bias::Left), ))) } - GoToDefinitionTrigger::InlayHint(p, target) => Some(TriggerPoint::InlayHint(p, target)), + GoToDefinitionTrigger::InlayHint(p, target) => Some(TriggerAnchor::InlayHint(p, target)), GoToDefinitionTrigger::None => None, }; @@ -109,7 +109,7 @@ pub enum LinkDefinitionKind { pub fn show_link_definition( definition_kind: LinkDefinitionKind, editor: &mut Editor, - trigger_point: TriggerPoint, + trigger_point: TriggerAnchor, snapshot: EditorSnapshot, cx: &mut ViewContext, ) { @@ -170,7 +170,7 @@ pub fn show_link_definition( let task = cx.spawn(|this, mut cx| { async move { let result = match trigger_point { - TriggerPoint::Text(_) => { + TriggerAnchor::Text(_) => { // query the LSP for definition info cx.update(|cx| { project.update(cx, |project, cx| match definition_kind { @@ -203,8 +203,9 @@ pub fn show_link_definition( ) }) } - TriggerPoint::InlayHint(trigger_source, trigger_target) => { - // TODO kb range is wrong, should be in inlay coordinates + TriggerAnchor::InlayHint(trigger_source, trigger_target) => { + // TODO kb range is wrong, should be in inlay coordinates have a proper inlay range. + // Or highlight inlays differently, in their layer? Some((Some(trigger_source..trigger_source), vec![trigger_target])) } }; @@ -293,7 +294,7 @@ pub fn hide_link_definition(editor: &mut Editor, cx: &mut ViewContext) { pub fn go_to_fetched_definition( editor: &mut Editor, - point: DisplayPoint, + point: PointForPosition, split: bool, cx: &mut ViewContext, ) { @@ -302,7 +303,7 @@ pub fn go_to_fetched_definition( pub fn go_to_fetched_type_definition( editor: &mut Editor, - point: DisplayPoint, + point: PointForPosition, split: bool, cx: &mut ViewContext, ) { @@ -312,7 +313,7 @@ pub fn go_to_fetched_type_definition( fn go_to_fetched_definition_of_kind( kind: LinkDefinitionKind, editor: &mut Editor, - point: DisplayPoint, + point: PointForPosition, split: bool, cx: &mut ViewContext, ) { @@ -330,7 +331,7 @@ fn go_to_fetched_definition_of_kind( } else { editor.select( SelectPhase::Begin { - position: point, + position: point.next_valid, add: false, click_count: 1, }, @@ -460,7 +461,7 @@ mod tests { }); cx.update_editor(|editor, cx| { - go_to_fetched_type_definition(editor, hover_point, false, cx); + go_to_fetched_type_definition(editor, PointForPosition::valid(hover_point), false, cx); }); requests.next().await; cx.foreground().run_until_parked(); @@ -707,7 +708,7 @@ mod tests { // Cmd click with existing definition doesn't re-request and dismisses highlight cx.update_editor(|editor, cx| { - go_to_fetched_definition(editor, hover_point, false, cx); + go_to_fetched_definition(editor, PointForPosition::valid(hover_point), false, cx); }); // Assert selection moved to to definition cx.lsp @@ -748,7 +749,7 @@ mod tests { ]))) }); cx.update_editor(|editor, cx| { - go_to_fetched_definition(editor, hover_point, false, cx); + go_to_fetched_definition(editor, PointForPosition::valid(hover_point), false, cx); }); requests.next().await; cx.foreground().run_until_parked(); diff --git a/crates/project/src/lsp_command.rs b/crates/project/src/lsp_command.rs index 7b4d689a81..20bb302b5b 100644 --- a/crates/project/src/lsp_command.rs +++ b/crates/project/src/lsp_command.rs @@ -7,14 +7,15 @@ use anyhow::{anyhow, Context, Result}; use async_trait::async_trait; use client::proto::{self, PeerId}; use fs::LineEnding; +use futures::future; use gpui::{AppContext, AsyncAppContext, ModelHandle}; use language::{ language_settings::{language_settings, InlayHintKind}, point_from_lsp, point_to_lsp, proto::{deserialize_anchor, deserialize_version, serialize_anchor, serialize_version}, range_from_lsp, range_to_lsp, Anchor, Bias, Buffer, BufferSnapshot, CachedLspAdapter, CharKind, - CodeAction, Completion, OffsetRangeExt, PointUtf16, ToOffset, ToPointUtf16, Transaction, - Unclipped, + CodeAction, Completion, LanguageServerName, OffsetRangeExt, PointUtf16, ToOffset, ToPointUtf16, + Transaction, Unclipped, }; use lsp::{DocumentHighlightKind, LanguageServer, LanguageServerId, ServerCapabilities}; use std::{cmp::Reverse, ops::Range, path::Path, sync::Arc}; @@ -1432,7 +1433,7 @@ impl LspCommand for GetCompletions { }) }); - Ok(futures::future::join_all(completions).await) + Ok(future::join_all(completions).await) } fn to_proto(&self, project_id: u64, buffer: &Buffer) -> proto::GetCompletions { @@ -1500,7 +1501,7 @@ impl LspCommand for GetCompletions { let completions = message.completions.into_iter().map(|completion| { language::proto::deserialize_completion(completion, language.clone()) }); - futures::future::try_join_all(completions).await + future::try_join_all(completions).await } fn buffer_id_from_proto(message: &proto::GetCompletions) -> u64 { @@ -1778,73 +1779,50 @@ impl LspCommand for OnTypeFormatting { } impl InlayHints { - pub fn lsp_to_project_hint( + pub async fn lsp_to_project_hint( lsp_hint: lsp::InlayHint, + project: &ModelHandle, buffer_handle: &ModelHandle, + server_id: LanguageServerId, resolve_state: ResolveState, force_no_type_left_padding: bool, - cx: &AppContext, - ) -> InlayHint { + cx: &mut AsyncAppContext, + ) -> anyhow::Result { let kind = lsp_hint.kind.and_then(|kind| match kind { lsp::InlayHintKind::TYPE => Some(InlayHintKind::Type), lsp::InlayHintKind::PARAMETER => Some(InlayHintKind::Parameter), _ => None, }); - let buffer = buffer_handle.read(cx); - let position = buffer.clip_point_utf16(point_from_lsp(lsp_hint.position), Bias::Left); + + let position = cx.update(|cx| { + let buffer = buffer_handle.read(cx); + let position = buffer.clip_point_utf16(point_from_lsp(lsp_hint.position), Bias::Left); + if kind == Some(InlayHintKind::Parameter) { + buffer.anchor_before(position) + } else { + buffer.anchor_after(position) + } + }); + let label = Self::lsp_inlay_label_to_project( + &buffer_handle, + project, + server_id, + lsp_hint.label, + cx, + ) + .await + .context("lsp to project inlay hint conversion")?; let padding_left = if force_no_type_left_padding && kind == Some(InlayHintKind::Type) { false } else { lsp_hint.padding_left.unwrap_or(false) }; - InlayHint { - position: if kind == Some(InlayHintKind::Parameter) { - buffer.anchor_before(position) - } else { - buffer.anchor_after(position) - }, + + Ok(InlayHint { + position, padding_left, padding_right: lsp_hint.padding_right.unwrap_or(false), - label: match lsp_hint.label { - lsp::InlayHintLabel::String(s) => InlayHintLabel::String(s), - lsp::InlayHintLabel::LabelParts(lsp_parts) => InlayHintLabel::LabelParts( - lsp_parts - .into_iter() - .map(|label_part| InlayHintLabelPart { - value: label_part.value, - tooltip: label_part.tooltip.map(|tooltip| match tooltip { - lsp::InlayHintLabelPartTooltip::String(s) => { - InlayHintLabelPartTooltip::String(s) - } - lsp::InlayHintLabelPartTooltip::MarkupContent(markup_content) => { - InlayHintLabelPartTooltip::MarkupContent(MarkupContent { - kind: match markup_content.kind { - lsp::MarkupKind::PlainText => HoverBlockKind::PlainText, - lsp::MarkupKind::Markdown => HoverBlockKind::Markdown, - }, - value: markup_content.value, - }) - } - }), - location: label_part.location.map(|lsp_location| { - let target_start = buffer.clip_point_utf16( - point_from_lsp(lsp_location.range.start), - Bias::Left, - ); - let target_end = buffer.clip_point_utf16( - point_from_lsp(lsp_location.range.end), - Bias::Left, - ); - Location { - buffer: buffer_handle.clone(), - range: buffer.anchor_after(target_start) - ..buffer.anchor_before(target_end), - } - }), - }) - .collect(), - ), - }, + label, kind, tooltip: lsp_hint.tooltip.map(|tooltip| match tooltip { lsp::InlayHintTooltip::String(s) => InlayHintTooltip::String(s), @@ -1859,7 +1837,100 @@ impl InlayHints { } }), resolve_state, - } + }) + } + + async fn lsp_inlay_label_to_project( + buffer: &ModelHandle, + project: &ModelHandle, + server_id: LanguageServerId, + lsp_label: lsp::InlayHintLabel, + cx: &mut AsyncAppContext, + ) -> anyhow::Result { + let label = match lsp_label { + lsp::InlayHintLabel::String(s) => InlayHintLabel::String(s), + lsp::InlayHintLabel::LabelParts(lsp_parts) => { + let mut parts_data = Vec::with_capacity(lsp_parts.len()); + buffer.update(cx, |buffer, cx| { + for lsp_part in lsp_parts { + let location_buffer_task = match &lsp_part.location { + Some(lsp_location) => { + let location_buffer_task = project.update(cx, |project, cx| { + let language_server_name = project + .language_server_for_buffer(buffer, server_id, cx) + .map(|(_, lsp_adapter)| { + LanguageServerName(Arc::from(lsp_adapter.name())) + }); + language_server_name.map(|language_server_name| { + project.open_local_buffer_via_lsp( + lsp_location.uri.clone(), + server_id, + language_server_name, + cx, + ) + }) + }); + Some(lsp_location.clone()).zip(location_buffer_task) + } + None => None, + }; + + parts_data.push((lsp_part, location_buffer_task)); + } + }); + + let mut parts = Vec::with_capacity(parts_data.len()); + for (lsp_part, location_buffer_task) in parts_data { + let location = match location_buffer_task { + Some((lsp_location, target_buffer_handle_task)) => { + let target_buffer_handle = target_buffer_handle_task + .await + .context("resolving location for label part buffer")?; + let range = cx.read(|cx| { + let target_buffer = target_buffer_handle.read(cx); + let target_start = target_buffer.clip_point_utf16( + point_from_lsp(lsp_location.range.start), + Bias::Left, + ); + let target_end = target_buffer.clip_point_utf16( + point_from_lsp(lsp_location.range.end), + Bias::Left, + ); + target_buffer.anchor_after(target_start) + ..target_buffer.anchor_before(target_end) + }); + Some(Location { + buffer: target_buffer_handle, + range, + }) + } + None => None, + }; + + parts.push(InlayHintLabelPart { + value: lsp_part.value, + tooltip: lsp_part.tooltip.map(|tooltip| match tooltip { + lsp::InlayHintLabelPartTooltip::String(s) => { + InlayHintLabelPartTooltip::String(s) + } + lsp::InlayHintLabelPartTooltip::MarkupContent(markup_content) => { + InlayHintLabelPartTooltip::MarkupContent(MarkupContent { + kind: match markup_content.kind { + lsp::MarkupKind::PlainText => HoverBlockKind::PlainText, + lsp::MarkupKind::Markdown => HoverBlockKind::Markdown, + }, + value: markup_content.value, + }) + } + }), + location, + }); + } + InlayHintLabel::LabelParts(parts) + } + }; + + Ok(label) } pub fn project_to_proto_hint(response_hint: InlayHint, cx: &AppContext) -> proto::InlayHint { @@ -2115,15 +2186,18 @@ impl InlayHints { }) }), location: part.location.and_then(|location| { - let path = cx.read(|cx| { - let project_path = location.buffer.read(cx).project_path(cx)?; - project.read(cx).absolute_path(&project_path, cx) + let (path, location_snapshot) = cx.read(|cx| { + let buffer = location.buffer.read(cx); + let project_path = buffer.project_path(cx)?; + let location_snapshot = buffer.snapshot(); + let path = project.read(cx).absolute_path(&project_path, cx); + path.zip(Some(location_snapshot)) })?; Some(lsp::Location::new( lsp::Url::from_file_path(path).unwrap(), range_to_lsp( - location.range.start.to_point_utf16(snapshot) - ..location.range.end.to_point_utf16(snapshot), + location.range.start.to_point_utf16(&location_snapshot) + ..location.range.end.to_point_utf16(&location_snapshot), ), )) }), @@ -2182,7 +2256,7 @@ impl LspCommand for InlayHints { buffer: ModelHandle, server_id: LanguageServerId, mut cx: AsyncAppContext, - ) -> Result> { + ) -> anyhow::Result> { let (lsp_adapter, lsp_server) = language_server_for_buffer(&project, &buffer, server_id, &mut cx)?; // `typescript-language-server` adds padding to the left for type hints, turning @@ -2194,32 +2268,38 @@ impl LspCommand for InlayHints { // Hence let's use a heuristic first to handle the most awkward case and look for more. let force_no_type_left_padding = lsp_adapter.name.0.as_ref() == "typescript-language-server"; - cx.read(|cx| { - Ok(message - .unwrap_or_default() - .into_iter() - .map(|lsp_hint| { - let resolve_state = match lsp_server.capabilities().inlay_hint_provider { - Some(lsp::OneOf::Right(lsp::InlayHintServerCapabilities::Options( - lsp::InlayHintOptions { - resolve_provider: Some(true), - .. - }, - ))) => { - ResolveState::CanResolve(lsp_server.server_id(), lsp_hint.data.clone()) - } - _ => ResolveState::Resolved, - }; - InlayHints::lsp_to_project_hint( - lsp_hint, - &buffer, - resolve_state, - force_no_type_left_padding, - cx, - ) - }) - .collect()) - }) + + let hints = message.unwrap_or_default().into_iter().map(|lsp_hint| { + let resolve_state = match lsp_server.capabilities().inlay_hint_provider { + Some(lsp::OneOf::Right(lsp::InlayHintServerCapabilities::Options( + lsp::InlayHintOptions { + resolve_provider: Some(true), + .. + }, + ))) => ResolveState::CanResolve(lsp_server.server_id(), lsp_hint.data.clone()), + _ => ResolveState::Resolved, + }; + + let project = project.clone(); + let buffer = buffer.clone(); + cx.spawn(|mut cx| async move { + InlayHints::lsp_to_project_hint( + lsp_hint, + &project, + &buffer, + server_id, + resolve_state, + force_no_type_left_padding, + &mut cx, + ) + .await + }) + }); + future::join_all(hints) + .await + .into_iter() + .collect::>() + .context("lsp to project inlay hints conversion") } fn to_proto(&self, project_id: u64, buffer: &Buffer) -> proto::InlayHints { diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index 65be7cceb1..fbdbd04664 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -5054,22 +5054,23 @@ impl Project { } let buffer_snapshot = buffer.snapshot(); - cx.spawn(|project, cx| async move { + cx.spawn(|project, mut cx| async move { let resolve_task = lang_server.request::( InlayHints::project_to_lsp_hint(hint, &project, &buffer_snapshot, &cx), ); let resolved_hint = resolve_task .await .context("inlay hint resolve LSP request")?; - let resolved_hint = cx.read(|cx| { - InlayHints::lsp_to_project_hint( - resolved_hint, - &buffer_handle, - ResolveState::Resolved, - false, - cx, - ) - }); + let resolved_hint = InlayHints::lsp_to_project_hint( + resolved_hint, + &project, + &buffer_handle, + server_id, + ResolveState::Resolved, + false, + &mut cx, + ) + .await?; Ok(Some(resolved_hint)) }) } else if let Some(project_id) = self.remote_id() {