feat(lsp): Publish diagnostics on file save (#1676)

* feat(lsp): Publish diagnostics on file save

* chore: Construct the LSP frontend inside the tokio runtime

* chore(errors): Expose properties on CustomDiagnostic and CustomLabel for use by lsp

* update for jakes changes

Author: phated

Cargo.lock

@@ -41,9 +41,11 @@ noir_wasm = { path = "crates/wasm" }
 cfg-if = "1.0.0"
 clap = { version = "4.1.4", features = ["derive"] }
 codespan = "0.11.1"
+codespan-lsp = "0.11.1"
 codespan-reporting = "0.11.1"
 chumsky = { git = "https://github.com/jfecher/chumsky", rev = "ad9d312" }
 dirs = "4"
+lsp-types = "0.94"
 serde = { version = "1.0.136", features = ["derive"] }
 serde_json = "1.0"
 smol_str = "0.1.17"
@@ -52,4 +54,7 @@ toml = "0.7.2"
 tower = "0.4"
 url = "2.2.0"
 wasm-bindgen = { version = "0.2.83", features = ["serde-serialize"] }
-wasm-bindgen-test = "0.3.33"
+wasm-bindgen-test = "0.3.33"
+
+[patch.crates-io]
+async-lsp = { git = "https://github.com/oxalica/async-lsp", rev = "9fc2db84ddcda291a864f044657f68d4377557f7" }

Cargo.toml

@@ -8,10 +8,15 @@ edition.workspace = true
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
+acvm.workspace = true
+codespan-lsp.workspace = true
+lsp-types.workspace = true
+noirc_driver.workspace = true
+noirc_errors.workspace = true
+noirc_frontend.workspace = true
 serde_json.workspace = true
 tower.workspace = true
 async-lsp = { version = "0.0.4", default-features = false, features = ["omni-trait"] }
-lsp-types = "0.94"
 
 [dev-dependencies]
 tokio = { version = "1.0", features = ["macros"] }

crates/lsp/Cargo.toml

@@ -5,28 +5,42 @@ use std::{
     task::{Context, Poll},
 };
 
+use acvm::Language;
 use async_lsp::{
-    router::Router, AnyEvent, AnyNotification, AnyRequest, Error, LspService, ResponseError,
+    router::Router, AnyEvent, AnyNotification, AnyRequest, ClientSocket, Error, LanguageClient,
+    LspService, ResponseError,
 };
 use lsp_types::{
-    notification, request, DidChangeConfigurationParams, DidChangeTextDocumentParams,
-    DidCloseTextDocumentParams, DidOpenTextDocumentParams, InitializeParams, InitializeResult,
-    InitializedParams, ServerCapabilities,
+    notification, request, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams,
+    DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams,
+    DidSaveTextDocumentParams, InitializeParams, InitializeResult, InitializedParams,
+    PublishDiagnosticsParams, Range, ServerCapabilities, TextDocumentSyncOptions,
 };
+use noirc_driver::Driver;
+use noirc_errors::{DiagnosticKind, FileDiagnostic};
+use noirc_frontend::graph::CrateType;
 use serde_json::Value as JsonValue;
 use tower::Service;
 
 // State for the LSP gets implemented on this struct and is internal to the implementation
-#[derive(Debug, Default)]
-struct LspState;
+#[derive(Debug)]
+struct LspState {
+    client: ClientSocket,
+}
+
+impl LspState {
+    fn new(client: &ClientSocket) -> Self {
+        Self { client: client.clone() }
+    }
+}
 
 pub struct NargoLspService {
     router: Router<LspState>,
 }
 
 impl NargoLspService {
-    pub fn new() -> Self {
-        let state = LspState::default();
+    pub fn new(client: &ClientSocket) -> Self {
+        let state = LspState::new(client);
         let mut router = Router::new(state);
         router
             .request::<request::Initialize, _>(on_initialize)
@@ -36,17 +50,12 @@ impl NargoLspService {
             .notification::<notification::DidOpenTextDocument>(on_did_open_text_document)
             .notification::<notification::DidChangeTextDocument>(on_did_change_text_document)
             .notification::<notification::DidCloseTextDocument>(on_did_close_text_document)
+            .notification::<notification::DidSaveTextDocument>(on_did_save_text_document)
             .notification::<notification::Exit>(on_exit);
         Self { router }
     }
 }
 
-impl Default for NargoLspService {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 // This trait implemented as a passthrough to the router, which makes
 // our `NargoLspService` a normal Service as far as Tower is concerned.
 impl Service<AnyRequest> for NargoLspService {
@@ -90,8 +99,14 @@ fn on_initialize(
     _params: InitializeParams,
 ) -> impl Future<Output = Result<InitializeResult, ResponseError>> {
     async {
+        let text_document_sync = TextDocumentSyncOptions {
+            save: Some(true.into()),
+            ..TextDocumentSyncOptions::default()
+        };
+
         Ok(InitializeResult {
             capabilities: ServerCapabilities {
+                text_document_sync: Some(text_document_sync.into()),
                 // Add capabilities before this spread when adding support for one
                 ..ServerCapabilities::default()
             },
@@ -142,22 +157,99 @@ fn on_did_close_text_document(
     ControlFlow::Continue(())
 }
 
+fn on_did_save_text_document(
+    state: &mut LspState,
+    params: DidSaveTextDocumentParams,
+) -> ControlFlow<Result<(), async_lsp::Error>> {
+    // TODO: Requiring `Language` and `is_opcode_supported` to construct a driver makes for some real stinky code
+    // The driver should not require knowledge of the backend; instead should be implemented as an independent pass (in nargo?)
+    let mut driver = Driver::new(&Language::R1CS, Box::new(|_op| false));
+
+    let file_path = &params.text_document.uri.to_file_path().unwrap();
+
+    driver.create_local_crate(file_path, CrateType::Binary);
+
+    let mut diagnostics = Vec::new();
+
+    let file_diagnostics = match driver.check_crate(false) {
+        Ok(warnings) => warnings,
+        Err(errors_and_warnings) => errors_and_warnings,
+    };
+
+    if !file_diagnostics.is_empty() {
+        let fm = driver.file_manager();
+        let files = fm.as_simple_files();
+
+        for FileDiagnostic { file_id, diagnostic } in file_diagnostics {
+            // TODO: This file_id never be 0 because the "path" where it maps is the directory, not a file
+            if file_id.as_usize() != 0 {
+                continue;
+            }
+
+            let mut range = Range::default();
+
+            // TODO: Should this be the first item in secondaries? Should we bail when we find a range?
+            for sec in diagnostic.secondaries {
+                // TODO: Codespan ranges are often (always?) off by some amount of characters
+                if let Ok(codespan_range) =
+                    codespan_lsp::byte_span_to_range(files, file_id.as_usize(), sec.span.into())
+                {
+                    // We have to manually attach each because the codespan_lsp restricts lsp-types to the wrong version range
+                    range.start.line = codespan_range.start.line;
+                    range.start.character = codespan_range.start.character;
+                    range.end.line = codespan_range.end.line;
+                    range.end.character = codespan_range.end.character;
+                }
+            }
+            let severity = match diagnostic.kind {
+                DiagnosticKind::Error => Some(DiagnosticSeverity::ERROR),
+                DiagnosticKind::Warning => Some(DiagnosticSeverity::WARNING),
+            };
+            diagnostics.push(Diagnostic {
+                range,
+                severity,
+                message: diagnostic.message,
+                ..Diagnostic::default()
+            })
+        }
+    }
+
+    let _ = state.client.publish_diagnostics(PublishDiagnosticsParams {
+        uri: params.text_document.uri,
+        version: None,
+        diagnostics,
+    });
+
+    ControlFlow::Continue(())
+}
+
 fn on_exit(_state: &mut LspState, _params: ()) -> ControlFlow<Result<(), async_lsp::Error>> {
     ControlFlow::Continue(())
 }
 
 #[cfg(test)]
 mod lsp_tests {
+    use lsp_types::TextDocumentSyncCapability;
     use tokio::test;
 
     use super::*;
 
     #[test]
     async fn test_on_initialize() {
-        let mut state = LspState::default();
+        // Not available in published release yet
+        let client = ClientSocket::new_closed();
+        let mut state = LspState::new(&client);
         let params = InitializeParams::default();
         let response = on_initialize(&mut state, params).await.unwrap();
-        assert_eq!(response.capabilities, ServerCapabilities::default());
+        assert!(matches!(
+            response.capabilities,
+            ServerCapabilities {
+                text_document_sync: Some(TextDocumentSyncCapability::Options(
+                    TextDocumentSyncOptions { save: Some(_), .. }
+                )),
+                ..
+            }
+        ));
         assert!(response.server_info.is_none());
     }
 }

crates/lsp/src/lib.rs

@@ -28,19 +28,18 @@ pub(crate) fn run<B: Backend>(
 
     let runtime = Builder::new_current_thread().enable_all().build().unwrap();
 
-    let (server, _) = async_lsp::Frontend::new_server(|client| {
-        let router = NargoLspService::new();
-
-        ServiceBuilder::new()
-            .layer(TracingLayer::default())
-            .layer(LifecycleLayer::default())
-            .layer(CatchUnwindLayer::default())
-            .layer(ConcurrencyLayer::default())
-            .layer(ClientProcessMonitorLayer::new(client))
-            .service(router)
-    });
-
     runtime.block_on(async {
+        let (server, _) = async_lsp::Frontend::new_server(|client| {
+            let router = NargoLspService::new(&client);
+
+            ServiceBuilder::new()
+                .layer(TracingLayer::default())
+                .layer(LifecycleLayer::default())
+                .layer(CatchUnwindLayer::default())
+                .layer(ConcurrencyLayer::default())
+                .layer(ClientProcessMonitorLayer::new(client))
+                .service(router)
+        });
         let stdin = BufReader::new(PipeStdin::lock().unwrap());
         let stdout = async_lsp::stdio::PipeStdout::lock().unwrap();
         server.run(stdin, stdout).await.map_err(CliError::LspError)

crates/nargo_cli/src/cli/lsp_cmd.rs

@@ -5,10 +5,10 @@ use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct CustomDiagnostic {
-    message: String,
-    secondaries: Vec<CustomLabel>,
+    pub message: String,
+    pub secondaries: Vec<CustomLabel>,
     notes: Vec<String>,
-    kind: DiagnosticKind,
+    pub kind: DiagnosticKind,
 }
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
@@ -93,9 +93,9 @@ impl std::fmt::Display for CustomDiagnostic {
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
-struct CustomLabel {
+pub struct CustomLabel {
     message: String,
-    span: Span,
+    pub span: Span,
 }
 
 impl CustomLabel {