Add validation in `LspCommand::to_lsp` + check for inverted ranges (#22731)

Michael Sloan created 1 year ago

#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

Change summary

crates/collab/src/tests/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(-)

Detailed changes

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

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<PointUtf16> {
     /// 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<lsp::Diagnostic> {
         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<PointUtf16> {
             message: self.diagnostic.message.clone(),
             data: self.diagnostic.data.clone(),
             ..Default::default()
-        }
+        })
     }
 }
 

crates/language/src/language.rs

@@ -1849,14 +1849,19 @@ pub fn point_from_lsp(point: lsp::Position) -> Unclipped<PointUtf16> {
     Unclipped(PointUtf16::new(point.line, point.character))
 }
 
-pub fn range_to_lsp(range: Range<PointUtf16>) -> 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<PointUtf16>) -> Result<lsp::Range> {
+    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<Unclipped<PointUtf16>> {

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<lsp::Url> {
+    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<lsp::TextDocumentIdentifier> {
+    Ok(lsp::TextDocumentIdentifier {
+        uri: file_path_to_lsp_url(path)?,
+    })
+}
+
+pub(crate) fn make_lsp_text_document_position(
+    path: &Path,
+    position: PointUtf16,
+) -> Result<lsp::TextDocumentPositionParams> {
+    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<LanguageServer>,
         cx: &AppContext,
-    ) -> <Self::LspRequest as lsp::request::Request>::Params;
+    ) -> Result<<Self::LspRequest as lsp::request::Request>::Params>;
 
     async fn response_from_lsp(
         self,
@@ -202,13 +227,8 @@ impl LspCommand for PrepareRename {
         _: &Buffer,
         _: &Arc<LanguageServer>,
         _: &AppContext,
-    ) -> lsp::TextDocumentPositionParams {
-        lsp::TextDocumentPositionParams {
-            text_document: lsp::TextDocumentIdentifier {
-                uri: lsp::Url::from_file_path(path).unwrap(),
-            },
-            position: point_to_lsp(self.position),
-        }
+    ) -> Result<lsp::TextDocumentPositionParams> {
+        make_lsp_text_document_position(path, self.position)
     }
 
     async fn response_from_lsp(
@@ -325,17 +345,12 @@ impl LspCommand for PerformRename {
         _: &Buffer,
         _: &Arc<LanguageServer>,
         _: &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<lsp::RenameParams> {
+        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<LanguageServer>,
         _: &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<lsp::GotoDefinitionParams> {
+        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<LanguageServer>,
         _: &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<lsp::GotoDeclarationParams> {
+        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<LanguageServer>,
         _: &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<lsp::GotoImplementationParams> {
+        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<LanguageServer>,
         _: &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<lsp::GotoTypeDefinitionParams> {
+        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<LanguageServer>,
         _: &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<lsp::ReferenceParams> {
+        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<LanguageServer>,
         _: &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<lsp::DocumentHighlightParams> {
+        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<LanguageServer>,
         _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<lsp::SignatureHelpParams> {
+        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<LanguageServer>,
         _: &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<lsp::HoverParams> {
+        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<LanguageServer>,
         _: &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<lsp::CompletionParams> {
+        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<LanguageServer>,
         _: &AppContext,
-    ) -> lsp::CodeActionParams {
-        let relevant_diagnostics = buffer
+    ) -> Result<lsp::CodeActionParams> {
+        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::<Vec<_>>();
+        {
+            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<LanguageServer>,
         _: &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<lsp::DocumentOnTypeFormattingParams> {
+        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<LanguageServer>,
         _: &AppContext,
-    ) -> lsp::InlayHintParams {
-        lsp::InlayHintParams {
+    ) -> Result<lsp::InlayHintParams> {
+        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<LanguageServer>,
         _: &AppContext,
-    ) -> lsp::LinkedEditingRangeParams {
+    ) -> Result<lsp::LinkedEditingRangeParams> {
         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(

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<LanguageServer>,
         _: &AppContext,
-    ) -> ExpandMacroParams {
-        ExpandMacroParams {
-            text_document: lsp::TextDocumentIdentifier {
-                uri: lsp::Url::from_file_path(path).unwrap(),
-            },
+    ) -> Result<ExpandMacroParams> {
+        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<LanguageServer>,
         _: &AppContext,
-    ) -> OpenDocsParams {
-        OpenDocsParams {
+    ) -> Result<OpenDocsParams> {
+        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<LanguageServer>,
         _: &AppContext,
-    ) -> SwitchSourceHeaderParams {
-        SwitchSourceHeaderParams(lsp::TextDocumentIdentifier {
-            uri: lsp::Url::from_file_path(path).unwrap(),
-        })
+    ) -> Result<SwitchSourceHeaderParams> {
+        Ok(SwitchSourceHeaderParams(make_text_document_identifier(
+            path,
+        )?))
     }
 
     async fn response_from_lsp(

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