From 1073dd632932d0c9131f6413a5ced69ee7096e60 Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Mon, 3 Apr 2023 03:58:50 +0200 Subject: [PATCH] robustly handle invalid LSP ranges (#6512) --- helix-lsp/src/lib.rs | 25 ++++++++++++++++++++----- helix-term/src/ui/completion.rs | 16 ++++++---------- 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index c206ac1d6..a59fa31e4 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -132,7 +132,11 @@ pub mod util { ) -> Option { let pos_line = pos.line as usize; if pos_line > doc.len_lines() - 1 { - return None; + // If it extends past the end, truncate it to the end. This is because the + // way the LSP describes the range including the last newline is by + // specifying a line number after what we would call the last line. + log::warn!("LSP position {pos:?} out of range assuming EOF"); + return Some(doc.len_chars()); } // We need to be careful here to fully comply ith the LSP spec. @@ -242,9 +246,20 @@ pub mod util { pub fn lsp_range_to_range( doc: &Rope, - range: lsp::Range, + mut range: lsp::Range, offset_encoding: OffsetEncoding, ) -> Option { + // This is sort of an edgecase. It's not clear from the spec how to deal with + // ranges where end < start. They don't make much sense but vscode simply caps start to end + // and because it's not specified quite a few LS rely on this as a result (for example the TS server) + if range.start > range.end { + log::error!( + "Invalid LSP range start {:?} > end {:?}, using an empty range at the end instead", + range.start, + range.end + ); + range.start = range.end; + } let start = lsp_pos_to_pos(doc, range.start, offset_encoding)?; let end = lsp_pos_to_pos(doc, range.end, offset_encoding)?; @@ -951,16 +966,16 @@ mod tests { test_case!("", (0, 0) => Some(0)); test_case!("", (0, 1) => Some(0)); - test_case!("", (1, 0) => None); + test_case!("", (1, 0) => Some(0)); test_case!("\n\n", (0, 0) => Some(0)); test_case!("\n\n", (1, 0) => Some(1)); test_case!("\n\n", (1, 1) => Some(1)); test_case!("\n\n", (2, 0) => Some(2)); - test_case!("\n\n", (3, 0) => None); + test_case!("\n\n", (3, 0) => Some(2)); test_case!("test\n\n\n\ncase", (4, 3) => Some(11)); test_case!("test\n\n\n\ncase", (4, 4) => Some(12)); test_case!("test\n\n\n\ncase", (4, 5) => Some(12)); - test_case!("", (u32::MAX, u32::MAX) => None); + test_case!("", (u32::MAX, u32::MAX) => Some(0)); } #[test] diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index e0b1419c5..bc216509f 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -141,16 +141,12 @@ impl Completion { } }; - let start_offset = - match util::lsp_pos_to_pos(doc.text(), edit.range.start, offset_encoding) { - Some(start) => start as i128 - primary_cursor as i128, - None => return Transaction::new(doc.text()), - }; - let end_offset = - match util::lsp_pos_to_pos(doc.text(), edit.range.end, offset_encoding) { - Some(end) => end as i128 - primary_cursor as i128, - None => return Transaction::new(doc.text()), - }; + let Some(range) = util::lsp_range_to_range(doc.text(), edit.range, offset_encoding) else{ + return Transaction::new(doc.text()); + }; + + let start_offset = range.anchor as i128 - primary_cursor as i128; + let end_offset = range.head as i128 - primary_cursor as i128; (Some((start_offset, end_offset)), edit.new_text) } else {