Fix crash in nls (#2093)

* Fix crash in nls Nls would crash when it failed to convert a nickel location to an lsp location. But this could actually happen in realistic situations, such as when trying to go to a location in the standard library. Now, we just ignore locations that fail to convert. * Update snapshot
tweag · Nov 14, 2024 · 7f4ba09 · 7f4ba09
1 parent 5e1c17d
commit 7f4ba09
Show file tree

Hide file tree

Showing 6 changed files with 58 additions and 45 deletions.
diff --git a/lsp/nls/src/diagnostic.rs b/lsp/nls/src/diagnostic.rs
@@ -97,11 +97,14 @@ pub trait DiagnosticCompat: Sized {
 }
 
 /// Determine the position of a [codespan_reporting::diagnostic::Label] by looking it up
-/// in the file cache
+/// in the file cache.
+///
+/// Lookups are fallible: they fail if the range is invalid, or if the file doesn't have
+/// a path that can be converted to a url (e.g. if it's a generated file).
 pub trait LocationCompat: Sized {
-    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Self;
+    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Option<Self>;
 
-    fn from_span(span: &RawSpan, files: &Files) -> Self {
+    fn from_span(span: &RawSpan, files: &Files) -> Option<Self> {
         Self::from_codespan(
             &span.src_id,
             &(span.start.to_usize()..span.end.to_usize()),
@@ -111,20 +114,17 @@ pub trait LocationCompat: Sized {
 }
 
 impl LocationCompat for lsp_types::Range {
-    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Self {
-        byte_span_to_range(files, *file_id, range.clone()).unwrap_or(lsp_types::Range {
-            start: Default::default(),
-            end: Default::default(),
-        })
+    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Option<Self> {
+        byte_span_to_range(files, *file_id, range.clone()).ok()
     }
 }
 
 impl LocationCompat for lsp_types::Location {
-    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Self {
-        lsp_types::Location {
-            uri: lsp_types::Url::from_file_path(files.name(*file_id)).unwrap(),
-            range: lsp_types::Range::from_codespan(file_id, range, files),
-        }
+    fn from_codespan(file_id: &FileId, range: &Range<usize>, files: &Files) -> Option<Self> {
+        Some(lsp_types::Location {
+            uri: lsp_types::Url::from_file_path(files.name(*file_id)).ok()?,
+            range: lsp_types::Range::from_codespan(file_id, range, files)?,
+        })
     }
 }
 
@@ -165,45 +165,51 @@ impl DiagnosticCompat for SerializableDiagnostic {
                 .or_else(|| within_file_labels.clone().next());
 
             if let Some(label) = maybe_label {
-                let range = lsp_types::Range::from_codespan(&label.file_id, &label.range, files);
-                let message = if diagnostic.notes.is_empty() {
-                    diagnostic.message.clone()
-                } else {
-                    format!("{}\n{}", diagnostic.message, diagnostic.notes.join("\n"))
-                };
-                diagnostics.push(SerializableDiagnostic {
-                    range: OrdRange(range),
-                    severity,
-                    code: code.clone(),
-                    message,
-                    related_information: Some(
-                        cross_file_labels
-                            .map(|label| {
-                                OrdDiagnosticRelatedInformation(DiagnosticRelatedInformation {
-                                    location: lsp_types::Location::from_codespan(
+                if let Some(range) =
+                    lsp_types::Range::from_codespan(&label.file_id, &label.range, files)
+                {
+                    let message = if diagnostic.notes.is_empty() {
+                        diagnostic.message.clone()
+                    } else {
+                        format!("{}\n{}", diagnostic.message, diagnostic.notes.join("\n"))
+                    };
+                    diagnostics.push(SerializableDiagnostic {
+                        range: OrdRange(range),
+                        severity,
+                        code: code.clone(),
+                        message,
+                        related_information: Some(
+                            cross_file_labels
+                                .filter_map(|label| {
+                                    let location = lsp_types::Location::from_codespan(
                                         &label.file_id,
                                         &label.range,
                                         files,
-                                    ),
-                                    message: label.message.clone(),
+                                    )?;
+                                    Some(OrdDiagnosticRelatedInformation(
+                                        DiagnosticRelatedInformation {
+                                            location,
+                                            message: label.message.clone(),
+                                        },
+                                    ))
                                 })
-                            })
-                            .collect(),
-                    ),
-                });
+                                .collect(),
+                        ),
+                    });
+                }
             }
         }
 
-        diagnostics.extend(within_file_labels.map(|label| {
-            let range = lsp_types::Range::from_codespan(&label.file_id, &label.range, files);
+        diagnostics.extend(within_file_labels.filter_map(|label| {
+            let range = lsp_types::Range::from_codespan(&label.file_id, &label.range, files)?;
 
-            SerializableDiagnostic {
+            Some(SerializableDiagnostic {
                 range: OrdRange(range),
                 message: label.message.clone(),
                 severity: Some(lsp_types::DiagnosticSeverity::HINT),
                 code: code.clone(),
                 related_information: None,
-            }
+            })
         }));
         diagnostics
     }

diff --git a/lsp/nls/src/requests/goto.rs b/lsp/nls/src/requests/goto.rs
@@ -16,7 +16,7 @@ fn ids_to_locations(ids: impl IntoIterator<Item = RawSpan>, world: &World) -> Ve
     spans.dedup();
     spans
         .iter()
-        .map(|loc| Location::from_span(loc, world.cache.files()))
+        .filter_map(|loc| Location::from_span(loc, world.cache.files()))
         .collect()
 }
 

diff --git a/lsp/nls/src/requests/hover.rs b/lsp/nls/src/requests/hover.rs
@@ -208,7 +208,7 @@ pub fn handle(
                 contents: HoverContents::Array(contents),
                 range: hover
                     .span
-                    .map(|s| Range::from_span(&s, server.world.cache.files())),
+                    .and_then(|s| Range::from_span(&s, server.world.cache.files())),
             },
         ));
     } else {

diff --git a/lsp/nls/src/requests/rename.rs b/lsp/nls/src/requests/rename.rs
@@ -45,10 +45,12 @@ pub fn handle_rename(
     let mut changes = HashMap::<Url, Vec<TextEdit>>::new();
     for pos in all_positions {
         let url = Url::from_file_path(server.world.cache.files().name(pos.src_id)).unwrap();
-        changes.entry(url).or_default().push(TextEdit {
-            range: Range::from_span(&pos, server.world.cache.files()),
-            new_text: params.new_name.clone(),
-        });
+        if let Some(range) = Range::from_span(&pos, server.world.cache.files()) {
+            changes.entry(url).or_default().push(TextEdit {
+                range,
+                new_text: params.new_name.clone(),
+            });
+        }
     }
 
     server.reply(Response::new_ok(

diff --git a/lsp/nls/tests/inputs/hover-stdlib-type.ncl b/lsp/nls/tests/inputs/hover-stdlib-type.ncl
@@ -10,3 +10,7 @@ foo.bar.baz "0" + std.string.to_number "1"
 ### type = "Hover"
 ### textDocument.uri = "file:///main.ncl"
 ### position = { line = 2, character = 30 }
+### [[request]]
+### type = "GotoDefinition"
+### textDocument.uri = "file:///main.ncl"
+### position = { line = 2, character = 30 }
diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-stdlib-type.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__hover-stdlib-type.ncl.snap
@@ -17,3 +17,4 @@ NumberLiteral -> Dyn
 ```, ```nickel
 String -> Number
 ```]
+None