From 141393232e1804a079a6c70152045284c535d7a6 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 6 Jan 2025 15:00:36 -0700 Subject: [PATCH] Add validation in `LspCommand::to_lsp` + check for inverted ranges (#22731) #22690 logged errors and flipped the range in this case. Instead it brings more visibility to the issue to return errors. Release Notes: - N/A --- .../random_project_collaboration_tests.rs | 2 +- crates/language/src/diagnostic_set.rs | 9 +- crates/language/src/language.rs | 19 +- crates/project/src/lsp_command.rs | 221 ++++++++---------- crates/project/src/lsp_ext_command.rs | 26 +-- crates/project/src/lsp_store.rs | 19 +- 6 files changed, 134 insertions(+), 162 deletions(-) diff --git a/crates/collab/src/tests/random_project_collaboration_tests.rs b/crates/collab/src/tests/random_project_collaboration_tests.rs index 351ae0cbe6ae88c5c839bf86fad6013b4f0e8c78..3dd16b9b7a4ba65d6b86c89ece156804a618d11f 100644 --- a/crates/collab/src/tests/random_project_collaboration_tests.rs +++ b/crates/collab/src/tests/random_project_collaboration_tests.rs @@ -1134,7 +1134,7 @@ impl RandomizedTest for ProjectCollaborationTest { let end = PointUtf16::new(end_row, end_column); let range = if start > end { end..start } else { start..end }; highlights.push(lsp::DocumentHighlight { - range: range_to_lsp(range.clone()), + range: range_to_lsp(range.clone()).unwrap(), kind: Some(lsp::DocumentHighlightKind::READ), }); } diff --git a/crates/language/src/diagnostic_set.rs b/crates/language/src/diagnostic_set.rs index 3423d9ad4d0f150f3784026e1539375293b4c374..2319cb1bfb6caaa2063d1e4a51e03c4c6b772639 100644 --- a/crates/language/src/diagnostic_set.rs +++ b/crates/language/src/diagnostic_set.rs @@ -1,4 +1,5 @@ use crate::{range_to_lsp, Diagnostic}; +use anyhow::Result; use collections::HashMap; use lsp::LanguageServerId; use std::{ @@ -54,16 +55,16 @@ pub struct Summary { impl DiagnosticEntry { /// Returns a raw LSP diagnostic used to provide diagnostic context to LSP /// codeAction request - pub fn to_lsp_diagnostic_stub(&self) -> lsp::Diagnostic { + pub fn to_lsp_diagnostic_stub(&self) -> Result { let code = self .diagnostic .code .clone() .map(lsp::NumberOrString::String); - let range = range_to_lsp(self.range.clone()); + let range = range_to_lsp(self.range.clone())?; - lsp::Diagnostic { + Ok(lsp::Diagnostic { code, range, severity: Some(self.diagnostic.severity), @@ -71,7 +72,7 @@ impl DiagnosticEntry { message: self.diagnostic.message.clone(), data: self.diagnostic.data.clone(), ..Default::default() - } + }) } } diff --git a/crates/language/src/language.rs b/crates/language/src/language.rs index 2eb1532ff3373419cc84f538b12b3f0a5c1ba1e7..dfe9be8e268a93b002c67f1f3a3d3c35518d7c54 100644 --- a/crates/language/src/language.rs +++ b/crates/language/src/language.rs @@ -1849,14 +1849,19 @@ pub fn point_from_lsp(point: lsp::Position) -> Unclipped { Unclipped(PointUtf16::new(point.line, point.character)) } -pub fn range_to_lsp(range: Range) -> lsp::Range { - let mut start = point_to_lsp(range.start); - let mut end = point_to_lsp(range.end); - if start > end { - log::error!("range_to_lsp called with inverted range {start:?}-{end:?}"); - mem::swap(&mut start, &mut end); +pub fn range_to_lsp(range: Range) -> Result { + if range.start > range.end { + Err(anyhow!( + "Inverted range provided to an LSP request: {:?}-{:?}", + range.start, + range.end + )) + } else { + Ok(lsp::Range { + start: point_to_lsp(range.start), + end: point_to_lsp(range.end), + }) } - lsp::Range { start, end } } pub fn range_from_lsp(range: lsp::Range) -> Range> { diff --git a/crates/project/src/lsp_command.rs b/crates/project/src/lsp_command.rs index 5b8f7759c08be301b95983f1c961f34b1c170cd4..abc2e0a21d9f04599bd28a1058f6897763cec137 100644 --- a/crates/project/src/lsp_command.rs +++ b/crates/project/src/lsp_command.rs @@ -45,6 +45,31 @@ pub fn lsp_formatting_options(settings: &LanguageSettings) -> lsp::FormattingOpt } } +pub(crate) fn file_path_to_lsp_url(path: &Path) -> Result { + match lsp::Url::from_file_path(path) { + Ok(url) => Ok(url), + Err(()) => Err(anyhow!( + "Invalid file path provided to LSP request: {path:?}" + )), + } +} + +pub(crate) fn make_text_document_identifier(path: &Path) -> Result { + Ok(lsp::TextDocumentIdentifier { + uri: file_path_to_lsp_url(path)?, + }) +} + +pub(crate) fn make_lsp_text_document_position( + path: &Path, + position: PointUtf16, +) -> Result { + Ok(lsp::TextDocumentPositionParams { + text_document: make_text_document_identifier(path)?, + position: point_to_lsp(position), + }) +} + #[async_trait(?Send)] pub trait LspCommand: 'static + Sized + Send + std::fmt::Debug { type Response: 'static + Default + Send + std::fmt::Debug; @@ -65,7 +90,7 @@ pub trait LspCommand: 'static + Sized + Send + std::fmt::Debug { buffer: &Buffer, language_server: &Arc, cx: &AppContext, - ) -> ::Params; + ) -> Result<::Params>; async fn response_from_lsp( self, @@ -202,13 +227,8 @@ impl LspCommand for PrepareRename { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::TextDocumentPositionParams { - lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - } + ) -> Result { + make_lsp_text_document_position(path, self.position) } async fn response_from_lsp( @@ -325,17 +345,12 @@ impl LspCommand for PerformRename { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::RenameParams { - lsp::RenameParams { - text_document_position: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::RenameParams { + text_document_position: make_lsp_text_document_position(path, self.position)?, new_name: self.new_name.clone(), work_done_progress_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -455,17 +470,12 @@ impl LspCommand for GetDefinition { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::GotoDefinitionParams { - lsp::GotoDefinitionParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::GotoDefinitionParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -555,17 +565,12 @@ impl LspCommand for GetDeclaration { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::GotoDeclarationParams { - lsp::GotoDeclarationParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::GotoDeclarationParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -648,17 +653,12 @@ impl LspCommand for GetImplementation { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::GotoImplementationParams { - lsp::GotoImplementationParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::GotoImplementationParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -748,17 +748,12 @@ impl LspCommand for GetTypeDefinition { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::GotoTypeDefinitionParams { - lsp::GotoTypeDefinitionParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::GotoTypeDefinitionParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -1061,20 +1056,15 @@ impl LspCommand for GetReferences { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::ReferenceParams { - lsp::ReferenceParams { - text_document_position: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::ReferenceParams { + text_document_position: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), context: lsp::ReferenceContext { include_declaration: true, }, - } + }) } async fn response_from_lsp( @@ -1237,17 +1227,12 @@ impl LspCommand for GetDocumentHighlights { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::DocumentHighlightParams { - lsp::DocumentHighlightParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::DocumentHighlightParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -1391,22 +1376,12 @@ impl LspCommand for GetSignatureHelp { _: &Buffer, _: &Arc, _cx: &AppContext, - ) -> lsp::SignatureHelpParams { - let url_result = lsp::Url::from_file_path(path); - if url_result.is_err() { - log::error!("an invalid file path has been specified"); - } - - lsp::SignatureHelpParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: url_result.expect("invalid file path"), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::SignatureHelpParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, context: None, work_done_progress_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -1505,16 +1480,11 @@ impl LspCommand for GetHover { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::HoverParams { - lsp::HoverParams { - text_document_position_params: lsp::TextDocumentPositionParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, - position: point_to_lsp(self.position), - }, + ) -> Result { + Ok(lsp::HoverParams { + text_document_position_params: make_lsp_text_document_position(path, self.position)?, work_done_progress_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -1728,16 +1698,13 @@ impl LspCommand for GetCompletions { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::CompletionParams { - lsp::CompletionParams { - text_document_position: lsp::TextDocumentPositionParams::new( - lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path(path).unwrap()), - point_to_lsp(self.position), - ), + ) -> Result { + Ok(lsp::CompletionParams { + text_document_position: make_lsp_text_document_position(path, self.position)?, context: Some(self.context.clone()), work_done_progress_params: Default::default(), partial_result_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -2075,12 +2042,14 @@ impl LspCommand for GetCodeActions { buffer: &Buffer, language_server: &Arc, _: &AppContext, - ) -> lsp::CodeActionParams { - let relevant_diagnostics = buffer + ) -> Result { + let mut relevant_diagnostics = Vec::new(); + for entry in buffer .snapshot() .diagnostics_in_range::<_, language::PointUtf16>(self.range.clone(), false) - .map(|entry| entry.to_lsp_diagnostic_stub()) - .collect::>(); + { + relevant_diagnostics.push(entry.to_lsp_diagnostic_stub()?); + } let supported = Self::supported_code_action_kinds(language_server.adapter_server_capabilities()); @@ -2102,11 +2071,9 @@ impl LspCommand for GetCodeActions { supported }; - lsp::CodeActionParams { - text_document: lsp::TextDocumentIdentifier::new( - lsp::Url::from_file_path(path).unwrap(), - ), - range: range_to_lsp(self.range.to_point_utf16(buffer)), + Ok(lsp::CodeActionParams { + text_document: make_text_document_identifier(path)?, + range: range_to_lsp(self.range.to_point_utf16(buffer))?, work_done_progress_params: Default::default(), partial_result_params: Default::default(), context: lsp::CodeActionContext { @@ -2114,7 +2081,7 @@ impl LspCommand for GetCodeActions { only, ..lsp::CodeActionContext::default() }, - } + }) } async fn response_from_lsp( @@ -2286,15 +2253,12 @@ impl LspCommand for OnTypeFormatting { _: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::DocumentOnTypeFormattingParams { - lsp::DocumentOnTypeFormattingParams { - text_document_position: lsp::TextDocumentPositionParams::new( - lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path(path).unwrap()), - point_to_lsp(self.position), - ), + ) -> Result { + Ok(lsp::DocumentOnTypeFormattingParams { + text_document_position: make_lsp_text_document_position(path, self.position)?, ch: self.trigger.clone(), options: self.options.clone(), - } + }) } async fn response_from_lsp( @@ -2792,14 +2756,14 @@ impl LspCommand for InlayHints { buffer: &Buffer, _: &Arc, _: &AppContext, - ) -> lsp::InlayHintParams { - lsp::InlayHintParams { + ) -> Result { + Ok(lsp::InlayHintParams { text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), + uri: file_path_to_lsp_url(path)?, }, - range: range_to_lsp(self.range.to_point_utf16(buffer)), + range: range_to_lsp(self.range.to_point_utf16(buffer))?, work_done_progress_params: Default::default(), - } + }) } async fn response_from_lsp( @@ -2949,15 +2913,12 @@ impl LspCommand for LinkedEditingRange { buffer: &Buffer, _server: &Arc, _: &AppContext, - ) -> lsp::LinkedEditingRangeParams { + ) -> Result { let position = self.position.to_point_utf16(&buffer.snapshot()); - lsp::LinkedEditingRangeParams { - text_document_position_params: lsp::TextDocumentPositionParams::new( - lsp::TextDocumentIdentifier::new(lsp::Url::from_file_path(path).unwrap()), - point_to_lsp(position), - ), + Ok(lsp::LinkedEditingRangeParams { + text_document_position_params: make_lsp_text_document_position(path, position)?, work_done_progress_params: Default::default(), - } + }) } async fn response_from_lsp( diff --git a/crates/project/src/lsp_ext_command.rs b/crates/project/src/lsp_ext_command.rs index 7890630e315289449ef57262e2c98ff160fd6a0d..2d2610bc314357a31b0eca698a073fa5dd0e2bec 100644 --- a/crates/project/src/lsp_ext_command.rs +++ b/crates/project/src/lsp_ext_command.rs @@ -1,4 +1,4 @@ -use crate::{lsp_command::LspCommand, lsp_store::LspStore}; +use crate::{lsp_command::LspCommand, lsp_store::LspStore, make_text_document_identifier}; use anyhow::{Context, Result}; use async_trait::async_trait; use gpui::{AppContext, AsyncAppContext, Model}; @@ -53,13 +53,11 @@ impl LspCommand for ExpandMacro { _: &Buffer, _: &Arc, _: &AppContext, - ) -> ExpandMacroParams { - ExpandMacroParams { - text_document: lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }, + ) -> Result { + Ok(ExpandMacroParams { + text_document: make_text_document_identifier(path)?, position: point_to_lsp(self.position), - } + }) } async fn response_from_lsp( @@ -179,13 +177,13 @@ impl LspCommand for OpenDocs { _: &Buffer, _: &Arc, _: &AppContext, - ) -> OpenDocsParams { - OpenDocsParams { + ) -> Result { + Ok(OpenDocsParams { text_document: lsp::TextDocumentIdentifier { uri: lsp::Url::from_file_path(path).unwrap(), }, position: point_to_lsp(self.position), - } + }) } async fn response_from_lsp( @@ -292,10 +290,10 @@ impl LspCommand for SwitchSourceHeader { _: &Buffer, _: &Arc, _: &AppContext, - ) -> SwitchSourceHeaderParams { - SwitchSourceHeaderParams(lsp::TextDocumentIdentifier { - uri: lsp::Url::from_file_path(path).unwrap(), - }) + ) -> Result { + Ok(SwitchSourceHeaderParams(make_text_document_identifier( + path, + )?)) } async fn response_from_lsp( diff --git a/crates/project/src/lsp_store.rs b/crates/project/src/lsp_store.rs index e4b8c850d096b84e8da088ccd7786d0347b5e53a..15e775055069c794ce16fb98b124b1f77188c41b 100644 --- a/crates/project/src/lsp_store.rs +++ b/crates/project/src/lsp_store.rs @@ -3487,7 +3487,18 @@ impl LspStore { }; let file = File::from_dyn(buffer.file()).and_then(File::as_local); if let (Some(file), Some(language_server)) = (file, language_server) { - let lsp_params = request.to_lsp(&file.abs_path(cx), buffer, &language_server, cx); + let lsp_params = match request.to_lsp(&file.abs_path(cx), buffer, &language_server, cx) + { + Ok(lsp_params) => lsp_params, + Err(err) => { + log::error!( + "Preparing LSP request to {} failed: {}", + language_server.name(), + err + ); + return Task::ready(Err(err)); + } + }; let status = request.status(); return cx.spawn(move |this, cx| async move { if !request.check_capabilities(language_server.adapter_server_capabilities()) { @@ -3536,11 +3547,7 @@ impl LspStore { let result = lsp_request.await; let response = result.map_err(|err| { - log::warn!( - "Generic lsp request to {} failed: {}", - language_server.name(), - err - ); + log::warn!("LSP request to {} failed: {}", language_server.name(), err); err })?;