Repository: tweag/nickel
Pull request #1605 (2023-09-14 13:22:55)

Fix an off-by-one error in the LSP formatter
According to [the LSP specification](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#range) the end of a text range in the protocol is exclusive. When executing a formatting request in the language server we want to replace the entire document with the formatted result, including any trailing newlines. This means we need to specify the beginning of the line after the very last line in the document as the end of the range of text to be replaced.

This also fixes an annoying behaviour when formatting with NLS in Helix. Previously, Helix would insist on selecting (what looked like) the entire document on every formatting request. With this change, Helix refrains from doing this.


Patch:
diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs
index c3dc31f9..e4d555d1 100644
--- a/lsp/nls/src/requests/formatting.rs
+++ b/lsp/nls/src/requests/formatting.rs
@@ -16,7 +16,6 @@ pub fn handle_format_document(
     let file_id = server.cache.id_of(&SourcePath::Path(path)).unwrap();
     let text = server.cache.files().source(file_id).clone();
     let document_length = text.lines().count() as u32;
-    let last_line_length = text.lines().next_back().unwrap().len() as u32;
 
     let mut formatted: Vec<u8> = Vec::new();
     nickel_lang_core::format::format(text.as_bytes(), &mut formatted).map_err(|err| {
@@ -39,9 +38,12 @@ pub fn handle_format_document(
                 line: 0,
                 character: 0,
             },
+            // The end position is exclusive. Since we want to replace the
+            // entire document, we specify the beginning of the line after the
+            // last line in the document.
             end: Position {
-                line: document_length - 1,
-                character: last_line_length,
+                line: document_length,
+                character: 0,
             },
         },
         new_text: formatted,
diff --git a/lsp/nls/src/requests/formatting_external.rs b/lsp/nls/src/requests/formatting_external.rs
index b7a4ea79..842e027a 100644
--- a/lsp/nls/src/requests/formatting_external.rs
+++ b/lsp/nls/src/requests/formatting_external.rs
@@ -20,7 +20,6 @@ pub fn handle_format_document(
     let file_id = server.cache.id_of(&SourcePath::Path(path)).unwrap();
     let text = server.cache.files().source(file_id).clone();
     let document_length = text.lines().count() as u32;
-    let last_line_length = text.lines().next_back().unwrap().len() as u32;
 
     let Ok(mut topiary) = process::Command::new(FORMATTING_COMMAND[0])
         .args(&FORMATTING_COMMAND[1..])
@@ -62,9 +61,12 @@ pub fn handle_format_document(
                 line: 0,
                 character: 0,
             },
+            // The end position is exclusive. Since we want to replace the
+            // entire document, we specify the beginning of the line after the
+            // last line in the document.
             end: Position {
-                line: document_length - 1,
-                character: last_line_length,
+                line: document_length,
+                character: 0,
             },
         },
         new_text,


Test patch:
diff --git a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__formatting.ncl.snap b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__formatting.ncl.snap
index 225ef795..25418037 100644
--- a/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__formatting.ncl.snap
+++ b/lsp/nls/tests/snapshots/main__lsp__nls__tests__inputs__formatting.ncl.snap
@@ -2,7 +2,7 @@
 source: lsp/nls/tests/main.rs
 expression: output
 ---
-[<0:0-1:8> {
+[<0:0-2:0> {
   foo = "bar",
   baz = 7
 }