on master help sign in
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

commitc754df12b38595b29575e7b58911386a16ac9951 authorWojciech Kępka <46892771+wojciechkepka@users.noreply.github.com> dateSat 12 Jun 2021, 09:04 (5 years ago) parent1bf5b10
3 files changed+104−20
helix-lsp/src/lib.rs+79−7 helix-term/src/application.rs+18−7 helix-term/src/commands.rs+7−6
helix-lsp/src/lib.rs+79−7
@@ -55,23 +55,54 @@ pub mod util {
5555 use super::*;
5656 use helix_core::{Range, Rope, Transaction};
5757
58+ /// Converts [`lsp::Position`] to a position in the document.
59+ ///
60+ /// Returns `None` if position exceeds document length or an operation overflows.
5861 pub fn lsp_pos_to_pos(
5962 doc: &Rope,
6063 pos: lsp::Position,
6164 offset_encoding: OffsetEncoding,
62− ) -> usize {
65+ ) -> Option<usize> {
66+ let max_line = doc.lines().count().saturating_sub(1);
67+ let pos_line = pos.line as usize;
68+ let pos_line = if pos_line > max_line {
69+ return None;
70+ } else {
71+ pos_line
72+ };
6373 match offset_encoding {
6474 OffsetEncoding::Utf8 => {
65− let line = doc.line_to_char(pos.line as usize);
66− line + pos.character as usize
75+ let max_char = doc
76+ .line_to_char(max_line)
77+ .checked_add(doc.line(max_line).len_chars())?;
78+ let line = doc.line_to_char(pos_line);
79+ let pos = line.checked_add(pos.character as usize)?;
80+ if pos <= max_char {
81+ Some(pos)
82+ } else {
83+ None
84+ }
6785 }
6886 OffsetEncoding::Utf16 => {
69− let line = doc.line_to_char(pos.line as usize);
87+ let max_char = doc
88+ .line_to_char(max_line)
89+ .checked_add(doc.line(max_line).len_chars())?;
90+ let max_cu = doc.char_to_utf16_cu(max_char);
91+ let line = doc.line_to_char(pos_line);
7092 let line_start = doc.char_to_utf16_cu(line);
71− doc.utf16_cu_to_char(line_start + pos.character as usize)
93+ let pos = line_start.checked_add(pos.character as usize)?;
94+ if pos <= max_cu {
95+ Some(doc.utf16_cu_to_char(pos))
96+ } else {
97+ None
98+ }
7299 }
73100 }
74101 }
102+
103+ /// Converts position in the document to [`lsp::Position`].
104+ ///
105+ /// Panics when `pos` is out of `doc` bounds or operation overflows.
75106 pub fn pos_to_lsp_pos(
76107 doc: &Rope,
77108 pos: usize,
@@ -95,6 +126,7 @@ pub mod util {
95126 }
96127 }
97128
129+ /// Converts a range in the document to [`lsp::Range`].
98130 pub fn range_to_lsp_range(
99131 doc: &Rope,
100132 range: Range,
@@ -121,8 +153,17 @@ pub mod util {
121153 None
122154 };
123155
124− let start = lsp_pos_to_pos(doc, edit.range.start, offset_encoding);
125− let end = lsp_pos_to_pos(doc, edit.range.end, offset_encoding);
156+ let start =
157+ if let Some(start) = lsp_pos_to_pos(doc, edit.range.start, offset_encoding) {
158+ start
159+ } else {
160+ return (0, 0, None);
161+ };
162+ let end = if let Some(end) = lsp_pos_to_pos(doc, edit.range.end, offset_encoding) {
163+ end
164+ } else {
165+ return (0, 0, None);
166+ };
126167 (start, end, replacement)
127168 }),
128169 )
@@ -248,3 +289,34 @@ impl Registry {
248289// there needs to be a way to process incoming lsp messages from all clients.
249290// -> notifications need to be dispatched to wherever
250291// -> requests need to generate a reply and travel back to the same lsp!
292+
293+#[cfg(test)]
294+mod tests {
295+ use super::{lsp, util::*, OffsetEncoding};
296+ use helix_core::Rope;
297+
298+ #[test]
299+ fn converts_lsp_pos_to_pos() {
300+ macro_rules! test_case {
301+ ($doc:expr, ($x:expr, $y:expr) => $want:expr) => {
302+ let doc = Rope::from($doc);
303+ let pos = lsp::Position::new($x, $y);
304+ assert_eq!($want, lsp_pos_to_pos(&doc, pos, OffsetEncoding::Utf16));
305+ assert_eq!($want, lsp_pos_to_pos(&doc, pos, OffsetEncoding::Utf8))
306+ };
307+ }
308+
309+ test_case!("", (0, 0) => Some(0));
310+ test_case!("", (0, 1) => None);
311+ test_case!("", (1, 0) => None);
312+ test_case!("\n\n", (0, 0) => Some(0));
313+ test_case!("\n\n", (1, 0) => Some(1));
314+ test_case!("\n\n", (1, 1) => Some(2));
315+ test_case!("\n\n", (2, 0) => Some(2));
316+ test_case!("\n\n", (3, 0) => None);
317+ test_case!("test\n\n\n\ncase", (4, 3) => Some(11));
318+ test_case!("test\n\n\n\ncase", (4, 4) => Some(12));
319+ test_case!("test\n\n\n\ncase", (4, 5) => None);
320+ test_case!("", (u32::MAX, u32::MAX) => None);
321+ }
322+}
helix-term/src/application.rs+18−7
@@ -178,7 +178,7 @@ impl Application {
178178 let diagnostics = params
179179 .diagnostics
180180 .into_iter()
181− .map(|diagnostic| {
181+ .filter_map(|diagnostic| {
182182 use helix_core::{
183183 diagnostic::{Range, Severity, Severity::*},
184184 Diagnostic,
@@ -189,18 +189,29 @@ impl Application {
189189 let language_server = doc.language_server().unwrap();
190190
191191 // TODO: convert inside server
192− let start = lsp_pos_to_pos(
192+ let start = if let Some(start) = lsp_pos_to_pos(
193193 text,
194194 diagnostic.range.start,
195195 language_server.offset_encoding(),
196− );
197− let end = lsp_pos_to_pos(
196+ ) {
197+ start
198+ } else {
199+ log::warn!("lsp position out of bounds - {:?}", diagnostic);
200+ return None;
201+ };
202+
203+ let end = if let Some(end) = lsp_pos_to_pos(
198204 text,
199205 diagnostic.range.end,
200206 language_server.offset_encoding(),
201− );
207+ ) {
208+ end
209+ } else {
210+ log::warn!("lsp position out of bounds - {:?}", diagnostic);
211+ return None;
212+ };
202213
203− Diagnostic {
214+ Some(Diagnostic {
204215 range: Range { start, end },
205216 line: diagnostic.range.start.line as usize,
206217 message: diagnostic.message,
@@ -214,7 +225,7 @@ impl Application {
214225 ),
215226 // code
216227 // source
217− }
228+ })
218229 })
219230 .collect();
220231
helix-term/src/commands.rs+7−6
@@ -1407,7 +1407,12 @@ fn _goto(
14071407 let (view, doc) = editor.current();
14081408 let definition_pos = location.range.start;
14091409 // TODO: convert inside server
1410− let new_pos = lsp_pos_to_pos(doc.text(), definition_pos, offset_encoding);
1410+ let new_pos =
1411+ if let Some(new_pos) = lsp_pos_to_pos(doc.text(), definition_pos, offset_encoding) {
1412+ new_pos
1413+ } else {
1414+ return;
1415+ };
14111416 doc.set_selection(view.id, Selection::point(new_pos));
14121417 align_view(doc, view, Align::Center);
14131418 }
@@ -2297,11 +2302,7 @@ pub fn completion(cx: &mut Context) {
22972302
22982303 let offset_encoding = language_server.offset_encoding();
22992304
2300− let pos = pos_to_lsp_pos(
2301− doc.text(),
2302− doc.selection(view.id).cursor(),
2303− language_server.offset_encoding(),
2304− );
2305+ let pos = pos_to_lsp_pos(doc.text(), doc.selection(view.id).cursor(), offset_encoding);
23052306
23062307 // TODO: handle fails
23072308 let future = language_server.completion(doc.identifier(), pos);