From c754df12b38595b29575e7b58911386a16ac9951 Mon Sep 17 00:00:00 2001 From: Wojciech Kępka Date: Sat, 12 Jun 2021 09:04:30 +0200 Subject: lsp: Check bounds when converting lsp positions (#204) * lsp: Make position conversion funcs return `Option` * Add tests * Fixes * Revert pos_to_lsp_pos to panic--- helix-lsp/src/lib.rs | 86 +++++++++++++++++++++++++++++++++++++++---- helix-term/src/application.rs | 25 +++++++++---- helix-term/src/commands.rs | 13 ++++--- 3 files changed, 104 insertions(+), 20 deletions(-) diff --git a/helix-lsp/src/lib.rs b/helix-lsp/src/lib.rs index 08676066..e16ad765 100644 --- a/helix-lsp/src/lib.rs +++ b/helix-lsp/src/lib.rs @@ -55,23 +55,54 @@ pub mod util { use super::*; use helix_core::{Range, Rope, Transaction}; + /// Converts [`lsp::Position`] to a position in the document. + /// + /// Returns `None` if position exceeds document length or an operation overflows. pub fn lsp_pos_to_pos( doc: &Rope, pos: lsp::Position, offset_encoding: OffsetEncoding, - ) -> usize { + ) -> Option { + let max_line = doc.lines().count().saturating_sub(1); + let pos_line = pos.line as usize; + let pos_line = if pos_line > max_line { + return None; + } else { + pos_line + }; match offset_encoding { OffsetEncoding::Utf8 => { - let line = doc.line_to_char(pos.line as usize); - line + pos.character as usize + let max_char = doc + .line_to_char(max_line) + .checked_add(doc.line(max_line).len_chars())?; + let line = doc.line_to_char(pos_line); + let pos = line.checked_add(pos.character as usize)?; + if pos <= max_char { + Some(pos) + } else { + None + } } OffsetEncoding::Utf16 => { - let line = doc.line_to_char(pos.line as usize); + let max_char = doc + .line_to_char(max_line) + .checked_add(doc.line(max_line).len_chars())?; + let max_cu = doc.char_to_utf16_cu(max_char); + let line = doc.line_to_char(pos_line); let line_start = doc.char_to_utf16_cu(line); - doc.utf16_cu_to_char(line_start + pos.character as usize) + let pos = line_start.checked_add(pos.character as usize)?; + if pos <= max_cu { + Some(doc.utf16_cu_to_char(pos)) + } else { + None + } } } } + + /// Converts position in the document to [`lsp::Position`]. + /// + /// Panics when `pos` is out of `doc` bounds or operation overflows. pub fn pos_to_lsp_pos( doc: &Rope, pos: usize, @@ -95,6 +126,7 @@ pub mod util { } } + /// Converts a range in the document to [`lsp::Range`]. pub fn range_to_lsp_range( doc: &Rope, range: Range, @@ -121,8 +153,17 @@ pub mod util { None }; - let start = lsp_pos_to_pos(doc, edit.range.start, offset_encoding); - let end = lsp_pos_to_pos(doc, edit.range.end, offset_encoding); + let start = + if let Some(start) = lsp_pos_to_pos(doc, edit.range.start, offset_encoding) { + start + } else { + return (0, 0, None); + }; + let end = if let Some(end) = lsp_pos_to_pos(doc, edit.range.end, offset_encoding) { + end + } else { + return (0, 0, None); + }; (start, end, replacement) }), ) @@ -248,3 +289,34 @@ impl Registry { // there needs to be a way to process incoming lsp messages from all clients. // -> notifications need to be dispatched to wherever // -> requests need to generate a reply and travel back to the same lsp! + +#[cfg(test)] +mod tests { + use super::{lsp, util::*, OffsetEncoding}; + use helix_core::Rope; + + #[test] + fn converts_lsp_pos_to_pos() { + macro_rules! test_case { + ($doc:expr, ($x:expr, $y:expr) => $want:expr) => { + let doc = Rope::from($doc); + let pos = lsp::Position::new($x, $y); + assert_eq!($want, lsp_pos_to_pos(&doc, pos, OffsetEncoding::Utf16)); + assert_eq!($want, lsp_pos_to_pos(&doc, pos, OffsetEncoding::Utf8)) + }; + } + + test_case!("", (0, 0) => Some(0)); + test_case!("", (0, 1) => None); + test_case!("", (1, 0) => None); + test_case!("\n\n", (0, 0) => Some(0)); + test_case!("\n\n", (1, 0) => Some(1)); + test_case!("\n\n", (1, 1) => Some(2)); + test_case!("\n\n", (2, 0) => Some(2)); + test_case!("\n\n", (3, 0) => None); + 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) => None); + test_case!("", (u32::MAX, u32::MAX) => None); + } +} diff --git a/helix-term/src/application.rs b/helix-term/src/application.rs index bf829f2c..3d043441 100644 --- a/helix-term/src/application.rs +++ b/helix-term/src/application.rs @@ -178,7 +178,7 @@ impl Application { let diagnostics = params .diagnostics .into_iter() - .map(|diagnostic| { + .filter_map(|diagnostic| { use helix_core::{ diagnostic::{Range, Severity, Severity::*}, Diagnostic, @@ -189,18 +189,29 @@ impl Application { let language_server = doc.language_server().unwrap(); // TODO: convert inside server - let start = lsp_pos_to_pos( + let start = if let Some(start) = lsp_pos_to_pos( text, diagnostic.range.start, language_server.offset_encoding(), - ); - let end = lsp_pos_to_pos( + ) { + start + } else { + log::warn!("lsp position out of bounds - {:?}", diagnostic); + return None; + }; + + let end = if let Some(end) = lsp_pos_to_pos( text, diagnostic.range.end, language_server.offset_encoding(), - ); + ) { + end + } else { + log::warn!("lsp position out of bounds - {:?}", diagnostic); + return None; + }; - Diagnostic { + Some(Diagnostic { range: Range { start, end }, line: diagnostic.range.start.line as usize, message: diagnostic.message, @@ -214,7 +225,7 @@ impl Application { ), // code // source - } + }) }) .collect(); diff --git a/helix-term/src/commands.rs b/helix-term/src/commands.rs index edc3dcfd..f9db5581 100644 --- a/helix-term/src/commands.rs +++ b/helix-term/src/commands.rs @@ -1407,7 +1407,12 @@ fn _goto( let (view, doc) = editor.current(); let definition_pos = location.range.start; // TODO: convert inside server - let new_pos = lsp_pos_to_pos(doc.text(), definition_pos, offset_encoding); + let new_pos = + if let Some(new_pos) = lsp_pos_to_pos(doc.text(), definition_pos, offset_encoding) { + new_pos + } else { + return; + }; doc.set_selection(view.id, Selection::point(new_pos)); align_view(doc, view, Align::Center); } @@ -2297,11 +2302,7 @@ pub fn completion(cx: &mut Context) { let offset_encoding = language_server.offset_encoding(); - let pos = pos_to_lsp_pos( - doc.text(), - doc.selection(view.id).cursor(), - language_server.offset_encoding(), - ); + let pos = pos_to_lsp_pos(doc.text(), doc.selection(view.id).cursor(), offset_encoding); // TODO: handle fails let future = language_server.completion(doc.identifier(), pos); -- cgit v1.2.3-70-g09d2