From 7f4ba09c75c59de76bfe3e428c9ec63f6ed4b764 Mon Sep 17 00:00:00 2001 From: jneem Date: Thu, 14 Nov 2024 16:31:51 +0700 Subject: [PATCH] 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 --- lsp/nls/src/diagnostic.rs | 84 ++++++++++--------- lsp/nls/src/requests/goto.rs | 2 +- lsp/nls/src/requests/hover.rs | 2 +- lsp/nls/src/requests/rename.rs | 10 ++- lsp/nls/tests/inputs/hover-stdlib-type.ncl | 4 + ..._tests__inputs__hover-stdlib-type.ncl.snap | 1 + 6 files changed, 58 insertions(+), 45 deletions(-) diff --git a/lsp/nls/src/diagnostic.rs b/lsp/nls/src/diagnostic.rs index ac4083c5ca..af0ccc84e5 100644 --- 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, files: &Files) -> Self; + fn from_codespan(file_id: &FileId, range: &Range, files: &Files) -> Option; - fn from_span(span: &RawSpan, files: &Files) -> Self { + fn from_span(span: &RawSpan, files: &Files) -> Option { 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, 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, files: &Files) -> Option { + byte_span_to_range(files, *file_id, range.clone()).ok() } } impl LocationCompat for lsp_types::Location { - fn from_codespan(file_id: &FileId, range: &Range, 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, files: &Files) -> Option { + 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 index d69aacf449..147d7acf77 100644 --- 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, 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 index 8106ab69f4..da05898fe0 100644 --- 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 index 8f39f3d297..5c6b63adf1 100644 --- 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::>::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 index da05d0dfdd..f6367a132c 100644 --- 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 index c20eb1c500..98fb6dd31a 100644 --- 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