From 0bbf9e62b693513a79c4ef538a2d5876429949e6 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Fri, 31 Mar 2023 15:26:23 +0100 Subject: [PATCH 01/11] Add server capability for formatting --- lsp/nls/src/server.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index f6e8f695b7..916dfc598b 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -62,6 +62,7 @@ impl Server { ..Default::default() }), document_symbol_provider: Some(OneOf::Left(true)), + document_formatting_provider: Some(OneOf::Left(true)), ..ServerCapabilities::default() } } From 78dd54bb037b0491e60ee2ee15a62403e8f11fff Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Fri, 31 Mar 2023 15:41:19 +0100 Subject: [PATCH 02/11] Add formatting module and handler functions --- lsp/nls/src/requests/formatting.rs | 12 ++++++++++++ lsp/nls/src/requests/mod.rs | 1 + lsp/nls/src/server.rs | 15 +++++++++++---- 3 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 lsp/nls/src/requests/formatting.rs diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs new file mode 100644 index 0000000000..45875dfe26 --- /dev/null +++ b/lsp/nls/src/requests/formatting.rs @@ -0,0 +1,12 @@ +use lsp_server::{RequestId, ResponseError}; +use lsp_types::DocumentFormattingParams; + +use crate::server::Server; + +pub fn handle_format_document( + params: DocumentFormattingParams, + id: RequestId, + server: &mut Server, +) -> Result<(), ResponseError> { + Ok(()) +} diff --git a/lsp/nls/src/requests/mod.rs b/lsp/nls/src/requests/mod.rs index 66c441c46a..e9598ad947 100644 --- a/lsp/nls/src/requests/mod.rs +++ b/lsp/nls/src/requests/mod.rs @@ -1,4 +1,5 @@ pub mod completion; +pub mod formatting; pub mod goto; pub mod hover; pub mod symbols; diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 916dfc598b..70e5b13564 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -11,9 +11,10 @@ use lsp_types::{ notification::{DidChangeTextDocument, DidOpenTextDocument}, request::{Request as RequestTrait, *}, CompletionOptions, CompletionParams, DidChangeTextDocumentParams, DidOpenTextDocumentParams, - DocumentSymbolParams, GotoDefinitionParams, HoverOptions, HoverParams, HoverProviderCapability, - OneOf, ReferenceParams, ServerCapabilities, TextDocumentSyncCapability, TextDocumentSyncKind, - TextDocumentSyncOptions, WorkDoneProgressOptions, + DocumentFormattingParams, DocumentSymbolParams, GotoDefinitionParams, HoverOptions, + HoverParams, HoverProviderCapability, OneOf, ReferenceParams, ServerCapabilities, + TextDocumentSyncCapability, TextDocumentSyncKind, TextDocumentSyncOptions, + WorkDoneProgressOptions, }; use nickel_lang::{ @@ -26,7 +27,7 @@ use nickel_lang::{stdlib, typecheck::Context}; use crate::{ cache::CacheExt, linearization::{completed::Completed, interface::TermKind, Environment, ItemId}, - requests::{completion, goto, hover, symbols}, + requests::{completion, formatting, goto, hover, symbols}, trace::Trace, }; @@ -245,6 +246,12 @@ impl Server { symbols::handle_document_symbols(params, req.id.clone(), self) } + Formatting::METHOD => { + debug!("handle formatting"); + let params: DocumentFormattingParams = serde_json::from_value(req.params).unwrap(); + formatting::handle_format_document(params, req.id.clone(), self) + } + _ => Ok(()), }; From 4fcb51de46a9bf2c5422eaa89bc1c02e70ee86cf Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Fri, 31 Mar 2023 15:55:40 +0100 Subject: [PATCH 03/11] Some structure to the formatting module --- lsp/nls/src/requests/formatting.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index 45875dfe26..c5a317635f 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -1,5 +1,5 @@ -use lsp_server::{RequestId, ResponseError}; -use lsp_types::DocumentFormattingParams; +use lsp_server::{RequestId, Response, ResponseError}; +use lsp_types::{DocumentFormattingParams, Position, Range, TextEdit}; use crate::server::Server; @@ -8,5 +8,26 @@ pub fn handle_format_document( id: RequestId, server: &mut Server, ) -> Result<(), ResponseError> { + let document_id = params.text_document.uri.to_file_path().unwrap(); + let file_id = server.cache.id_of(document_id).unwrap(); + let text = server.cache.files().source(file_id); + + let new_text = String::new(); + let result = Some(vec![TextEdit { + range: Range { + start: Position { + line: 0, + character: 0, + }, + // for now + end: Position { + line: 0, + character: 0, + }, + }, + new_text, + }]); + let response = Response::new_ok(id, result); + Ok(()) } From dd97d95825b043d3db3c4e6c96ff778f785d2010 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Fri, 31 Mar 2023 16:38:21 +0100 Subject: [PATCH 04/11] Add connection to the formatter --- lsp/nls/src/requests/formatting.rs | 32 ++++++++++++++++++++++++------ lsp/nls/src/server.rs | 1 + 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index c5a317635f..95d4c14d6b 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -1,7 +1,9 @@ +use std::process; + use lsp_server::{RequestId, Response, ResponseError}; use lsp_types::{DocumentFormattingParams, Position, Range, TextEdit}; -use crate::server::Server; +use crate::server::{self, Server}; pub fn handle_format_document( params: DocumentFormattingParams, @@ -11,23 +13,41 @@ pub fn handle_format_document( let document_id = params.text_document.uri.to_file_path().unwrap(); let file_id = server.cache.id_of(document_id).unwrap(); let text = server.cache.files().source(file_id); + let document_length = text.lines().count() as u32; + let last_line_length = text.lines().rev().next().unwrap().len() as u32; + + let formatting_command = server::FORMATTING_COMMAND; + let Ok(mut topiary) = process::Command::new(&formatting_command[0]) + .args(&formatting_command[1..]) + .stdin(process::Stdio::piped()) + .stdout(process::Stdio::piped()) + .spawn() else { + // Also give a notification to tell the user that topiary is not + // available + return Ok(()) + }; + + let mut stdin = topiary.stdin.take().unwrap(); + let mut text_bytes = text.as_bytes(); + let _ = std::io::copy(&mut text_bytes, &mut stdin); + + let output = topiary.wait_with_output().unwrap(); - let new_text = String::new(); + let new_text = String::from_utf8(output.stdout).unwrap(); let result = Some(vec![TextEdit { range: Range { start: Position { line: 0, character: 0, }, - // for now end: Position { - line: 0, - character: 0, + line: document_length - 1, + character: last_line_length - 1, }, }, new_text, }]); let response = Response::new_ok(id, result); - + server.reply(response); Ok(()) } diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 70e5b13564..db648d12f7 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -32,6 +32,7 @@ use crate::{ }; pub const DOT_COMPL_TRIGGER: &str = "."; +pub const FORMATTING_COMMAND: [&str; 3] = ["topiary", "-l", "nickel"]; pub struct Server { pub connection: Connection, From 4dc498277d088d11766f87d4d8d5706dc8f564a1 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 4 Apr 2023 14:38:28 +0100 Subject: [PATCH 05/11] Use explicit topiary command --- lsp/nls/src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index db648d12f7..ee82f687bb 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -32,7 +32,7 @@ use crate::{ }; pub const DOT_COMPL_TRIGGER: &str = "."; -pub const FORMATTING_COMMAND: [&str; 3] = ["topiary", "-l", "nickel"]; +pub const FORMATTING_COMMAND: [&str; 3] = ["topiary", "--language", "nickel"]; pub struct Server { pub connection: Connection, From 61652ebf85ed0e452ebb04c0a49740f87e5e4af9 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 4 Apr 2023 14:39:06 +0100 Subject: [PATCH 06/11] Copy data to topiary's stdin concurrently --- lsp/nls/src/requests/formatting.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index 95d4c14d6b..f2e9658aa3 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -1,6 +1,6 @@ use std::process; -use lsp_server::{RequestId, Response, ResponseError}; +use lsp_server::{ErrorCode, RequestId, Response, ResponseError}; use lsp_types::{DocumentFormattingParams, Position, Range, TextEdit}; use crate::server::{self, Server}; @@ -12,7 +12,7 @@ pub fn handle_format_document( ) -> Result<(), ResponseError> { let document_id = params.text_document.uri.to_file_path().unwrap(); let file_id = server.cache.id_of(document_id).unwrap(); - let text = server.cache.files().source(file_id); + let text = server.cache.files().source(file_id).clone(); let document_length = text.lines().count() as u32; let last_line_length = text.lines().rev().next().unwrap().len() as u32; @@ -21,6 +21,7 @@ pub fn handle_format_document( .args(&formatting_command[1..]) .stdin(process::Stdio::piped()) .stdout(process::Stdio::piped()) + .stderr(process::Stdio::piped()) .spawn() else { // Also give a notification to tell the user that topiary is not // available @@ -28,12 +29,25 @@ pub fn handle_format_document( }; let mut stdin = topiary.stdin.take().unwrap(); - let mut text_bytes = text.as_bytes(); - let _ = std::io::copy(&mut text_bytes, &mut stdin); + + std::thread::spawn(move || { + let mut text_bytes = text.as_bytes(); + std::io::copy(&mut text_bytes, &mut stdin).unwrap(); + }); let output = topiary.wait_with_output().unwrap(); + if !output.status.success() { + let error = String::from_utf8_lossy(&output.stderr); + return Err(ResponseError { + code: ErrorCode::InternalError as i32, + message: error.to_string(), + data: None, + }); + } + let new_text = String::from_utf8(output.stdout).unwrap(); + let result = Some(vec![TextEdit { range: Range { start: Position { @@ -42,7 +56,7 @@ pub fn handle_format_document( }, end: Position { line: document_length - 1, - character: last_line_length - 1, + character: last_line_length, }, }, new_text, From 98af76ded39e6a93515f1226095bf181cdc947c8 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 4 Apr 2023 14:47:14 +0100 Subject: [PATCH 07/11] Add error message for formatter failure --- lsp/nls/src/requests/formatting.rs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index f2e9658aa3..094b7aa58e 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -23,9 +23,12 @@ pub fn handle_format_document( .stdout(process::Stdio::piped()) .stderr(process::Stdio::piped()) .spawn() else { - // Also give a notification to tell the user that topiary is not - // available - return Ok(()) + let message = "Executing topiary failed"; + return Err(ResponseError { + code: ErrorCode::InternalError as i32, + message: String::from(message), + data: None, + }); }; let mut stdin = topiary.stdin.take().unwrap(); @@ -61,7 +64,6 @@ pub fn handle_format_document( }, new_text, }]); - let response = Response::new_ok(id, result); - server.reply(response); + server.reply(Response::new_ok(id, result)); Ok(()) } From dca5a64429615594cb399a28024aa84b98728d9f Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Tue, 4 Apr 2023 16:03:51 +0100 Subject: [PATCH 08/11] Remove needless borrow --- lsp/nls/src/requests/formatting.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index 094b7aa58e..c076f1bed5 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -17,7 +17,7 @@ pub fn handle_format_document( let last_line_length = text.lines().rev().next().unwrap().len() as u32; let formatting_command = server::FORMATTING_COMMAND; - let Ok(mut topiary) = process::Command::new(&formatting_command[0]) + let Ok(mut topiary) = process::Command::new(formatting_command[0]) .args(&formatting_command[1..]) .stdin(process::Stdio::piped()) .stdout(process::Stdio::piped()) From b93e15ffe3a749573d44fa6dc39002208befa3d1 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Wed, 5 Apr 2023 15:45:54 +0100 Subject: [PATCH 09/11] Add documentation to `handle_format_document` --- lsp/nls/src/requests/formatting.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lsp/nls/src/requests/formatting.rs b/lsp/nls/src/requests/formatting.rs index c076f1bed5..b240fab7aa 100644 --- a/lsp/nls/src/requests/formatting.rs +++ b/lsp/nls/src/requests/formatting.rs @@ -5,6 +5,9 @@ use lsp_types::{DocumentFormattingParams, Position, Range, TextEdit}; use crate::server::{self, Server}; +/// Handle the LSP formatting request from a client using an external binary as a formatter. +/// If this succeds, it sends a reponse to the server and returns `Ok(..)`, otherwise, +/// it only returns an `Err(..)`. pub fn handle_format_document( params: DocumentFormattingParams, id: RequestId, From 72749050ec6bbff8a64c1fca8029bfbe29a0f4a8 Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Wed, 12 Apr 2023 14:34:08 +0100 Subject: [PATCH 10/11] Add documentation for using the formatter. --- lsp/README.md | 2 +- lsp/nls/README.md | 47 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/lsp/README.md b/lsp/README.md index ab6d316e76..d9924a834c 100644 --- a/lsp/README.md +++ b/lsp/README.md @@ -166,4 +166,4 @@ edit `coc-settings.json`) and add: ### Emacs -TODO +Follow the instructions on the the `nickel-mode` [repo](https://github.com/nickel-lang/nickel-mode). diff --git a/lsp/nls/README.md b/lsp/nls/README.md index 34b113b9e4..3b251403a6 100644 --- a/lsp/nls/README.md +++ b/lsp/nls/README.md @@ -8,4 +8,49 @@ editor. NLS is a stand-alone binary. Once built, you must then configure you code editor to use it for Nickel source files. This document covers building NLS and using -it in VSCode and (Neo)Vim. +it in VSCode, (Neo)Vim and Emacs. + +## Formatting Capabilities + +Formatting in `nls` is currently based on [topiary](https://github.com/tweag/topiary). + +To use it successfully, you need ensure you follow these steps: + +1. Have the `topiary` binary installed in your `PATH`. See [here](https://github.com/tweag/topiary#installing). +2. Have the [topiary](https://github.com/tweag/topiary) repo cloned locally. +3. Set the environment variable `TOPIARY_REPO_DIR` to point to the local copy +of the repo. +4. And finally, an environment variable `TOPIARY_LANGUAGE_DIR` to point to `$TOPIARY_REPO_DIR/languages`. + +Steps 2-4 are necessary because, for now, `topiary` cannot be used outside its +repo directory. + +## Alternatives + +I think making a user fetch the `topiary` and set those environment variables, +just to have the formatting capability in `nls` is a bit too much. I can think +of the following alternatives, but I don't know if they are ideal. + +### Keep a cache of the topiary repo + +Keep a cache of the `topiary` repo. Fetch the repo from GitHub if it is not +available in the local cache or not up to date. + +* Pros + * Automatic updates of the repo (and hence the formatting rules for Nickel) +* Cons + * We still have to set environment variables at runtime + * `nls` has to download a potentially large repo + +### Embedded Nickel formatting rules as a string in the `nls` binary + + Since `topiary` just needs a single `nickel.scm` to be able to format nickel + files we could just point `TOPIARY_LANGUAGE_DIR` to `/language` + , and put the embedded file in that directory. + +* Pros: + * It's just a single file, so it will be small + * No need to download/fetch anything +* Cons: + * We still have to set environment variables at runtime + * We have to ensure the formatting rules are up to date with `topiary` From fa3dfada253d52897510ca406bfe5a2af71b312f Mon Sep 17 00:00:00 2001 From: Oghenevwogaga Ebresafe Date: Thu, 13 Apr 2023 12:27:49 +0100 Subject: [PATCH 11/11] Add formatting module back --- lsp/nls/src/server.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lsp/nls/src/server.rs b/lsp/nls/src/server.rs index 26f3b3f1a9..7e7a612190 100644 --- a/lsp/nls/src/server.rs +++ b/lsp/nls/src/server.rs @@ -27,7 +27,7 @@ use nickel_lang::{stdlib, typecheck::Context}; use crate::{ cache::CacheExt, linearization::{completed::Completed, Environment, ItemId}, - requests::{completion, goto, hover, symbols}, + requests::{completion, formatting, goto, hover, symbols}, trace::Trace, };