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

@@ -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<usize> {
+        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);
+                let max_char = doc
-                line + pos.character as usize
+                    .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 start =
-                let end = lsp_pos_to_pos(doc, edit.range.end, offset_encoding);
+                    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);
+    }
+}

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();
 

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(
+    let pos = pos_to_lsp_pos(doc.text(), doc.selection(view.id).cursor(), offset_encoding);
-        doc.text(),
-        doc.selection(view.id).cursor(),
-        language_server.offset_encoding(),
-    );
 
     // TODO: handle fails
     let future = language_server.completion(doc.identifier(), pos);