on master help sign in
properly handle LSP position encoding (#5711)

* properly handle LSP position encoding * add debug assertion to Transaction::change * Apply suggestions from code review Co-authored-by: Michael Davis <mcarsondavis@gmail.com> --------- Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

commit7ebcf4e919a27b60610ba8f3a24a213282eb9ee6 authorPascal Kuthe <pascal.kuthe@semimod.de> dateThu 9 Feb 2023, 08:19 (4 years ago) parent8a60299
4 files changed+107−19
helix-core/src/line_ending.rs+7 helix-core/src/transaction.rs+5 helix-lsp/src/client.rs+1−1 helix-lsp/src/lib.rs+94−18
helix-core/src/line_ending.rs+7
@@ -203,6 +203,13 @@ pub fn line_end_char_index(slice: &RopeSlice, line: usize) -> usize {
203203 .unwrap_or(0)
204204}
205205
206+pub fn line_end_byte_index(slice: &RopeSlice, line: usize) -> usize {
207+ slice.line_to_byte(line + 1)
208+ - get_line_ending(&slice.line(line))
209+ .map(|le| le.as_str().len())
210+ .unwrap_or(0)
211+}
212+
206213/// Fetches line `line_idx` from the passed rope slice, sans any line ending.
207214pub fn line_without_line_ending<'a>(slice: &'a RopeSlice, line_idx: usize) -> RopeSlice<'a> {
208215 let start = slice.line_to_char(line_idx);
helix-core/src/transaction.rs+5
@@ -481,6 +481,11 @@ impl Transaction {
481481 for (from, to, tendril) in changes {
482482 // Verify ranges are ordered and not overlapping
483483 debug_assert!(last <= from);
484+ // Verify ranges are correct
485+ debug_assert!(
486+ from <= to,
487+ "Edit end must end before it starts (should {from} <= {to})"
488+ );
484489
485490 // Retain from last "to" to current "from"
486491 changeset.retain(from - last);
helix-lsp/src/client.rs+1−1
@@ -104,7 +104,7 @@ impl Client {
104104 server_tx,
105105 request_counter: AtomicU64::new(0),
106106 capabilities: OnceCell::new(),
107− offset_encoding: OffsetEncoding::Utf8,
107+ offset_encoding: OffsetEncoding::Utf16,
108108 config,
109109 req_timeout,
110110
helix-lsp/src/lib.rs+94−18
@@ -57,6 +57,7 @@ pub enum OffsetEncoding {
5757
5858pub mod util {
5959 use super::*;
60+ use helix_core::line_ending::{line_end_byte_index, line_end_char_index};
6061 use helix_core::{diagnostic::NumberOrString, Range, Rope, Selection, Tendril, Transaction};
6162
6263 /// Converts a diagnostic in the document to [`lsp::Diagnostic`].
@@ -117,7 +118,7 @@ pub mod util {
117118
118119 /// Converts [`lsp::Position`] to a position in the document.
119120 ///
120− /// Returns `None` if position exceeds document length or an operation overflows.
121+ /// Returns `None` if position.line is out of bounds or an overflow occurs
121122 pub fn lsp_pos_to_pos(
122123 doc: &Rope,
123124 pos: lsp::Position,
@@ -128,22 +129,58 @@ pub mod util {
128129 return None;
129130 }
130131
131− match offset_encoding {
132+ // We need to be careful here to fully comply ith the LSP spec.
133+ // Two relevant quotes from the spec:
134+ //
135+ // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#position
136+ // > If the character value is greater than the line length it defaults back
137+ // > to the line length.
138+ //
139+ // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocuments
140+ // > To ensure that both client and server split the string into the same
141+ // > line representation the protocol specifies the following end-of-line sequences:
142+ // > ‘\n’, ‘\r\n’ and ‘\r’. Positions are line end character agnostic.
143+ // > So you can not specify a position that denotes \r|\n or \n| where | represents the character offset.
144+ //
145+ // This means that while the line must be in bounds the `charater`
146+ // must be capped to the end of the line.
147+ // Note that the end of the line here is **before** the line terminator
148+ // so we must use `line_end_char_index` istead of `doc.line_to_char(pos_line + 1)`
149+ //
150+ // FIXME: Helix does not fully comply with the LSP spec for line terminators.
151+ // The LSP standard requires that line terminators are ['\n', '\r\n', '\r'].
152+ // Without the unicode-linebreak feature disabled, the `\r` terminator is not handled by helix.
153+ // With the unicode-linebreak feature, helix recognizes multiple extra line break chars
154+ // which means that positions will be decoded/encoded incorrectly in their presence
155+
156+ let line = match offset_encoding {
132157 OffsetEncoding::Utf8 => {
133− let line = doc.line_to_char(pos_line);
134− let pos = line.checked_add(pos.character as usize)?;
135− if pos <= doc.len_chars() {
136− Some(pos)
137− } else {
138− None
139− }
158+ let line_start = doc.line_to_byte(pos_line);
159+ let line_end = line_end_byte_index(&doc.slice(..), pos_line);
160+ line_start..line_end
140161 }
141162 OffsetEncoding::Utf16 => {
142− let line = doc.line_to_char(pos_line);
143− let line_start = doc.char_to_utf16_cu(line);
144− let pos = line_start.checked_add(pos.character as usize)?;
145− doc.try_utf16_cu_to_char(pos).ok()
163+ // TODO directly translate line index to char-idx
164+ // ropey can do this just as easily as utf-8 byte translation
165+ // but the functions are just missing.
166+ // Translate to char first and then utf-16 as a workaround
167+ let line_start = doc.line_to_char(pos_line);
168+ let line_end = line_end_char_index(&doc.slice(..), pos_line);
169+ doc.char_to_utf16_cu(line_start)..doc.char_to_utf16_cu(line_end)
146170 }
171+ };
172+
173+ // The LSP spec demands that the offset is capped to the end of the line
174+ let pos = line
175+ .start
176+ .checked_add(pos.character as usize)
177+ .unwrap_or(line.end)
178+ .min(line.end);
179+
180+ // TODO prefer UTF32/char indices to avoid this step
181+ match offset_encoding {
182+ OffsetEncoding::Utf8 => doc.try_byte_to_char(pos).ok(),
183+ OffsetEncoding::Utf16 => doc.try_utf16_cu_to_char(pos).ok(),
147184 }
148185 }
149186
@@ -158,8 +195,8 @@ pub mod util {
158195 match offset_encoding {
159196 OffsetEncoding::Utf8 => {
160197 let line = doc.char_to_line(pos);
161− let line_start = doc.line_to_char(line);
162− let col = pos - line_start;
198+ let line_start = doc.line_to_byte(line);
199+ let col = doc.char_to_byte(pos) - line_start;
163200
164201 lsp::Position::new(line as u32, col as u32)
165202 }
@@ -606,16 +643,55 @@ mod tests {
606643 }
607644
608645 test_case!("", (0, 0) => Some(0));
609− test_case!("", (0, 1) => None);
646+ test_case!("", (0, 1) => Some(0));
610647 test_case!("", (1, 0) => None);
611648 test_case!("\n\n", (0, 0) => Some(0));
612649 test_case!("\n\n", (1, 0) => Some(1));
613− test_case!("\n\n", (1, 1) => Some(2));
650+ test_case!("\n\n", (1, 1) => Some(1));
614651 test_case!("\n\n", (2, 0) => Some(2));
615652 test_case!("\n\n", (3, 0) => None);
616653 test_case!("test\n\n\n\ncase", (4, 3) => Some(11));
617654 test_case!("test\n\n\n\ncase", (4, 4) => Some(12));
618− test_case!("test\n\n\n\ncase", (4, 5) => None);
655+ test_case!("test\n\n\n\ncase", (4, 5) => Some(12));
619656 test_case!("", (u32::MAX, u32::MAX) => None);
620657 }
658+
659+ #[test]
660+ fn emoji_format_gh_4791() {
661+ use lsp_types::{Position, Range, TextEdit};
662+
663+ let edits = vec![
664+ TextEdit {
665+ range: Range {
666+ start: Position {
667+ line: 0,
668+ character: 1,
669+ },
670+ end: Position {
671+ line: 1,
672+ character: 0,
673+ },
674+ },
675+ new_text: "\n ".to_string(),
676+ },
677+ TextEdit {
678+ range: Range {
679+ start: Position {
680+ line: 1,
681+ character: 7,
682+ },
683+ end: Position {
684+ line: 2,
685+ character: 0,
686+ },
687+ },
688+ new_text: "\n ".to_string(),
689+ },
690+ ];
691+
692+ let mut source = Rope::from_str("[\n\"🇺🇸\",\n\"🎄\",\n]");
693+
694+ let transaction = generate_transaction_from_edits(&source, edits, OffsetEncoding::Utf8);
695+ assert!(transaction.apply(&mut source));
696+ }
621697}