From 7714eaab1aedceed9f1683030910eae545abd91d Mon Sep 17 00:00:00 2001 From: Koby Date: Thu, 14 Dec 2023 14:16:29 +0100 Subject: [PATCH 01/14] feat: consider single package in workspace for code lens --- tooling/lsp/src/lib.rs | 5 +- tooling/lsp/src/requests/code_lens_request.rs | 234 ++++++++++++++++++ tooling/lsp/src/requests/mod.rs | 9 +- tooling/lsp/src/types.rs | 11 +- 4 files changed, 253 insertions(+), 6 deletions(-) create mode 100644 tooling/lsp/src/requests/code_lens_request.rs diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 2ad8096a13f..c65f8d459ee 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -7,7 +7,7 @@ use std::{ collections::HashMap, future::Future, ops::{self, ControlFlow}, - path::PathBuf, + path::{Path, PathBuf}, pin::Pin, task::{self, Poll}, }; @@ -27,7 +27,7 @@ use notifications::{ on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized, }; use requests::{ - on_formatting, on_goto_definition_request, on_initialize, on_profile_run_request, on_shutdown, + on_code_lens_request, on_formatting, on_goto_definition_request, on_initialize, on_profile_run_request, on_shutdown, on_test_run_request, on_tests_request, }; use serde_json::Value as JsonValue; @@ -72,6 +72,7 @@ impl NargoLspService { .request::(on_initialize) .request::(on_formatting) .request::(on_shutdown) + .request::(on_code_lens_request) .request::(on_tests_request) .request::(on_test_run_request) .request::(on_profile_run_request) diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs new file mode 100644 index 00000000000..043b9458d53 --- /dev/null +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -0,0 +1,234 @@ +use std::future::{self, Future}; + +use async_lsp::{ErrorCode, LanguageClient, ResponseError}; + +use nargo::{package::Package, prepare_package, workspace::Workspace}; +use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; +use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_frontend::hir::FunctionNameMatch; + +use crate::{ + byte_span_to_range, types::{CodeLens, CodeLensParams, CodeLensResult, Command, LogMessageParams, MessageType}, + LspState, +}; + + +const ARROW: &str = "▶\u{fe0e}"; +const TEST_COMMAND: &str = "nargo.test"; +const TEST_CODELENS_TITLE: &str = "Run Test"; +const COMPILE_COMMAND: &str = "nargo.compile"; +const COMPILE_CODELENS_TITLE: &str = "Compile"; +const INFO_COMMAND: &str = "nargo.info"; +const INFO_CODELENS_TITLE: &str = "Info"; +const EXECUTE_COMMAND: &str = "nargo.execute"; +const EXECUTE_CODELENS_TITLE: &str = "Execute"; + +const PROFILE_COMMAND: &str = "nargo.profile"; +const PROFILE_CODELENS_TITLE: &str = "Profile"; + +fn with_arrow(title: &str) -> String { + format!("{ARROW} {title}") +} + +fn package_selection_args(workspace: &Workspace, package: &Package) -> Vec { + vec![ + "--program-dir".into(), + workspace.root_dir.display().to_string().into(), + "--package".into(), + package.name.to_string().into(), + ] +} + +pub(crate) fn on_code_lens_request( + state: &mut LspState, + params: CodeLensParams, +) -> impl Future> { + future::ready(on_code_lens_request_inner(state, params)) +} + +fn on_code_lens_request_inner( + state: &mut LspState, + params: CodeLensParams, +) -> Result { + let file_path = params.text_document.uri.to_file_path().map_err(|_| { + ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") + })?; + + let package_root = nargo_toml::find_file_manifest(file_path.as_path()); + + let toml_path = match package_root { + Some(toml_path) => toml_path, + None => { + // If we cannot find a manifest, we log a warning but return no diagnostics + // We can reconsider this when we can build a file without the need for a Nargo.toml file to resolve deps + let _ = state.client.log_message(LogMessageParams { + typ: MessageType::WARNING, + message: format!("Nargo.toml not found for file: {:}", file_path.display()), + }); + return Ok(None); + } + }; + + let workspace = resolve_workspace_from_toml( + &toml_path, + PackageSelection::All, + Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), + ) + .map_err(|err| { + // If we found a manifest, but the workspace is invalid, we raise an error about it + ResponseError::new(ErrorCode::REQUEST_FAILED, err) + })?; + + let mut lenses: Vec = vec![]; + + for package in &workspace { + let (mut context, crate_id) = nargo::prepare_package(package); + // We ignore the warnings and errors produced by compilation for producing code lenses + // because we can still get the test functions even if compilation fails + let _ = check_crate(&mut context, crate_id, false, false); + + let fm = &context.file_manager; + let files = fm.as_file_map(); + let tests = context + .get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Anything); + + for (func_name, test_function) in tests { + let location = context.function_meta(&test_function.get_id()).name.location; + let file_id = location.file; + + // Ignore diagnostics for any file that wasn't the file we saved + // TODO: In the future, we could create "related" diagnostics for these files + if fm.path(file_id) != file_path { + continue; + } + + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + + let test_command = Command { + title: with_arrow(TEST_CODELENS_TITLE), + command: TEST_COMMAND.into(), + arguments: Some( + [ + package_selection_args(&workspace, package), + vec!["--exact".into(), func_name.into()], + ] + .concat(), + ), + }; + + let test_lens = CodeLens { range, command: Some(test_command), data: None }; + + lenses.push(test_lens); + } + + if package.is_binary() { + if let Some(main_func_id) = context.get_main_function(&crate_id) { + let location = context.function_meta(&main_func_id).name.location; + let file_id = location.file; + + // Ignore diagnostics for any file that wasn't the file we saved + // TODO: In the future, we could create "related" diagnostics for these files + if fm.path(file_id) != file_path { + continue; + } + + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + + let compile_command = Command { + title: with_arrow(COMPILE_CODELENS_TITLE), + command: COMPILE_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; + + lenses.push(compile_lens); + + let info_command = Command { + title: INFO_CODELENS_TITLE.to_string(), + command: INFO_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let info_lens = CodeLens { range, command: Some(info_command), data: None }; + + lenses.push(info_lens); + + let execute_command = Command { + title: EXECUTE_CODELENS_TITLE.to_string(), + command: EXECUTE_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let execute_lens = CodeLens { range, command: Some(execute_command), data: None }; + + lenses.push(execute_lens); + + let profile_command = Command { + title: PROFILE_CODELENS_TITLE.to_string(), + command: PROFILE_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; + + lenses.push(profile_lens); + } + } + + if package.is_contract() { + // Currently not looking to deduplicate this since we don't have a clear decision on if the Contract stuff is staying + for contract in context.get_all_contracts(&crate_id) { + let location = contract.location; + let file_id = location.file; + + // Ignore diagnostics for any file that wasn't the file we saved + // TODO: In the future, we could create "related" diagnostics for these files + if fm.path(file_id) != file_path { + continue; + } + + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + + let compile_command = Command { + title: with_arrow(COMPILE_CODELENS_TITLE), + command: COMPILE_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; + + lenses.push(compile_lens); + + let info_command = Command { + title: INFO_CODELENS_TITLE.to_string(), + command: INFO_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let info_lens = CodeLens { range, command: Some(info_command), data: None }; + + lenses.push(info_lens); + + let profile_command = Command { + title: PROFILE_CODELENS_TITLE.to_string(), + command: PROFILE_COMMAND.into(), + arguments: Some(package_selection_args(&workspace, package)), + }; + + let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; + + lenses.push(profile_lens); + } + } + } + + if lenses.is_empty() { + Ok(None) + } else { + Ok(Some(lenses)) + } +} diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 309c3dc0a5c..95f92262dbd 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -1,6 +1,6 @@ use std::future::Future; -use crate::types::InitializeParams; +use crate::types::{CodeLensOptions, InitializeParams}; use async_lsp::ResponseError; use lsp_types::{Position, TextDocumentSyncCapability, TextDocumentSyncKind}; use nargo_fmt::Config; @@ -20,13 +20,14 @@ use crate::{ // They are not attached to the `NargoLspService` struct so they can be unit tested with only `LspState` // and params passed in. +mod code_lens_request; mod goto_definition; mod profile_run; mod test_run; mod tests; pub(crate) use { - goto_definition::on_goto_definition_request, profile_run::on_profile_run_request, + code_lens_request::on_code_lens_request, goto_definition::on_goto_definition_request, profile_run::on_profile_run_request, test_run::on_test_run_request, tests::on_tests_request, }; @@ -39,6 +40,8 @@ pub(crate) fn on_initialize( async { let text_document_sync = TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL); + let code_lens = CodeLensOptions { resolve_provider: Some(false) }; + let nargo = NargoCapability { tests: Some(NargoTestsOptions { fetch: Some(true), @@ -50,6 +53,7 @@ pub(crate) fn on_initialize( Ok(InitializeResult { capabilities: ServerCapabilities { text_document_sync: Some(text_document_sync), + code_lens_provider: Some(code_lens), document_formatting_provider: true, nargo: Some(nargo), definition_provider: Some(lsp_types::OneOf::Left(true)), @@ -127,6 +131,7 @@ mod initialization { text_document_sync: Some(TextDocumentSyncCapability::Kind( TextDocumentSyncKind::FULL )), + code_lens_provider: Some(CodeLensOptions { resolve_provider: Some(false) }), document_formatting_provider: true, .. } diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index 48c412eb5ad..755a6405db2 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -9,7 +9,7 @@ use std::collections::{BTreeMap, HashMap}; // Re-providing lsp_types that we don't need to override pub(crate) use lsp_types::{ - Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams, + CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams, InitializeParams, InitializedParams, LogMessageParams, MessageType, Position, PublishDiagnosticsParams, Range, ServerInfo, TextDocumentSyncCapability, Url, @@ -24,7 +24,9 @@ pub(crate) mod request { }; // Re-providing lsp_types that we don't need to override - pub(crate) use lsp_types::request::{Formatting, GotoDefinition, Shutdown}; + pub(crate) use lsp_types::request::{ + CodeLensRequest as CodeLens, Formatting, GotoDefinition, Shutdown, + }; #[derive(Debug)] pub(crate) struct Initialize; @@ -112,6 +114,10 @@ pub(crate) struct ServerCapabilities { #[serde(skip_serializing_if = "Option::is_none")] pub(crate) definition_provider: Option>, + /// The server provides code lens. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) code_lens_provider: Option, + /// The server provides document formatting. pub(crate) document_formatting_provider: bool, @@ -214,4 +220,5 @@ pub(crate) struct NargoProfileRunResult { pub(crate) opcodes_counts: HashMap, } +pub(crate) type CodeLensResult = Option>; pub(crate) type GotoDefinitionResult = Option; From 167bf0616891c841ffa5c9f3f14ffc6c884e9581 Mon Sep 17 00:00:00 2001 From: Koby Date: Thu, 14 Dec 2023 14:20:05 +0100 Subject: [PATCH 02/14] feat: use cached code lenses --- tooling/lsp/src/lib.rs | 9 +- tooling/lsp/src/notifications/mod.rs | 6 + tooling/lsp/src/requests/code_lens_request.rs | 238 ++++++++++-------- tooling/lsp/src/requests/mod.rs | 3 +- tooling/lsp/src/types.rs | 9 +- 5 files changed, 147 insertions(+), 118 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index c65f8d459ee..2fdf75ccfd8 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -7,7 +7,7 @@ use std::{ collections::HashMap, future::Future, ops::{self, ControlFlow}, - path::{Path, PathBuf}, + path::PathBuf, pin::Pin, task::{self, Poll}, }; @@ -18,6 +18,7 @@ use async_lsp::{ ResponseError, }; use fm::codespan_files as files; +use lsp_types::CodeLens; use noirc_frontend::{ graph::{CrateId, CrateName}, hir::{Context, FunctionNameMatch}, @@ -27,8 +28,8 @@ use notifications::{ on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized, }; use requests::{ - on_code_lens_request, on_formatting, on_goto_definition_request, on_initialize, on_profile_run_request, on_shutdown, - on_test_run_request, on_tests_request, + on_code_lens_request, on_formatting, on_goto_definition_request, on_initialize, + on_profile_run_request, on_shutdown, on_test_run_request, on_tests_request, }; use serde_json::Value as JsonValue; use tower::Service; @@ -47,6 +48,7 @@ pub struct LspState { client: ClientSocket, solver: WrapperSolver, input_files: HashMap, + collected_lenses: Option>, } impl LspState { @@ -56,6 +58,7 @@ impl LspState { root_path: None, solver: WrapperSolver(Box::new(solver)), input_files: HashMap::new(), + collected_lenses: None, } } } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 61f0d231738..4c5d3cde282 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -43,6 +43,7 @@ pub(super) fn on_did_change_text_document( ) -> ControlFlow> { let text = params.content_changes.into_iter().next().unwrap().text; state.input_files.insert(params.text_document.uri.to_string(), text); + state.collected_lenses = Some(Vec::new()); ControlFlow::Continue(()) } @@ -118,6 +119,11 @@ pub(super) fn on_did_save_text_document( }); } + let collected_lenses = crate::requests::collect_lenses_for_package( + &context, crate_id, &file_path, &workspace, package, + ); + state.collected_lenses = Some(collected_lenses); + let fm = &context.file_manager; let files = fm.as_file_map(); diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 043b9458d53..04a31288cc0 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -2,17 +2,17 @@ use std::future::{self, Future}; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; -use nargo::{package::Package, prepare_package, workspace::Workspace}; -use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; +use nargo::{package::Package, workspace::Workspace}; +use nargo_toml::{resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING}; use noirc_frontend::hir::FunctionNameMatch; use crate::{ - byte_span_to_range, types::{CodeLens, CodeLensParams, CodeLensResult, Command, LogMessageParams, MessageType}, + byte_span_to_range, + types::{CodeLens, CodeLensParams, CodeLensResult, Command, LogMessageParams, MessageType}, LspState, }; - const ARROW: &str = "▶\u{fe0e}"; const TEST_COMMAND: &str = "nargo.test"; const TEST_CODELENS_TITLE: &str = "Run Test"; @@ -50,6 +50,10 @@ fn on_code_lens_request_inner( state: &mut LspState, params: CodeLensParams, ) -> Result { + if let Some(collected_lenses) = &state.collected_lenses { + return Ok(Some(collected_lenses.clone())); + } + let file_path = params.text_document.uri.to_file_path().map_err(|_| { ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; @@ -87,148 +91,162 @@ fn on_code_lens_request_inner( // because we can still get the test functions even if compilation fails let _ = check_crate(&mut context, crate_id, false, false); - let fm = &context.file_manager; - let files = fm.as_file_map(); - let tests = context - .get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Anything); + let collected_lenses = + collect_lenses_for_package(&context, crate_id, &file_path, &workspace, package); + + lenses.extend(collected_lenses); + } + + if lenses.is_empty() { + Ok(None) + } else { + state.collected_lenses = Some(lenses.clone()); + Ok(Some(lenses)) + } +} + +pub(crate) fn collect_lenses_for_package( + context: &noirc_frontend::macros_api::HirContext, + crate_id: noirc_frontend::macros_api::CrateId, + file_path: &std::path::PathBuf, + workspace: &Workspace, + package: &Package, +) -> Vec { + let mut lenses: Vec = vec![]; + let fm = &context.file_manager; + let files = fm.as_file_map(); + let tests = + context.get_all_test_functions_in_crate_matching(&crate_id, FunctionNameMatch::Anything); + for (func_name, test_function) in tests { + let location = context.function_meta(&test_function.get_id()).name.location; + let file_id = location.file; + + // Ignore diagnostics for any file that wasn't the file we saved + // TODO: In the future, we could create "related" diagnostics for these files + if fm.path(file_id) != *file_path { + continue; + } + + let range = byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + + let test_command = Command { + title: with_arrow(TEST_CODELENS_TITLE), + command: TEST_COMMAND.into(), + arguments: Some( + [ + package_selection_args(workspace, package), + vec!["--exact".into(), func_name.into()], + ] + .concat(), + ), + }; + + let test_lens = CodeLens { range, command: Some(test_command), data: None }; - for (func_name, test_function) in tests { - let location = context.function_meta(&test_function.get_id()).name.location; + lenses.push(test_lens); + } + if package.is_binary() { + if let Some(main_func_id) = context.get_main_function(&crate_id) { + let location = context.function_meta(&main_func_id).name.location; let file_id = location.file; // Ignore diagnostics for any file that wasn't the file we saved // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != file_path { - continue; + if fm.path(file_id) != *file_path { + return lenses; } let range = byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); - let test_command = Command { - title: with_arrow(TEST_CODELENS_TITLE), - command: TEST_COMMAND.into(), - arguments: Some( - [ - package_selection_args(&workspace, package), - vec!["--exact".into(), func_name.into()], - ] - .concat(), - ), + let compile_command = Command { + title: with_arrow(COMPILE_CODELENS_TITLE), + command: COMPILE_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), }; - let test_lens = CodeLens { range, command: Some(test_command), data: None }; - - lenses.push(test_lens); - } - - if package.is_binary() { - if let Some(main_func_id) = context.get_main_function(&crate_id) { - let location = context.function_meta(&main_func_id).name.location; - let file_id = location.file; - - // Ignore diagnostics for any file that wasn't the file we saved - // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != file_path { - continue; - } + let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; - let range = - byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + lenses.push(compile_lens); - let compile_command = Command { - title: with_arrow(COMPILE_CODELENS_TITLE), - command: COMPILE_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; - - let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; - - lenses.push(compile_lens); - - let info_command = Command { - title: INFO_CODELENS_TITLE.to_string(), - command: INFO_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let info_command = Command { + title: INFO_CODELENS_TITLE.to_string(), + command: INFO_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let info_lens = CodeLens { range, command: Some(info_command), data: None }; + let info_lens = CodeLens { range, command: Some(info_command), data: None }; - lenses.push(info_lens); + lenses.push(info_lens); - let execute_command = Command { - title: EXECUTE_CODELENS_TITLE.to_string(), - command: EXECUTE_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let execute_command = Command { + title: EXECUTE_CODELENS_TITLE.to_string(), + command: EXECUTE_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let execute_lens = CodeLens { range, command: Some(execute_command), data: None }; + let execute_lens = CodeLens { range, command: Some(execute_command), data: None }; - lenses.push(execute_lens); + lenses.push(execute_lens); - let profile_command = Command { - title: PROFILE_CODELENS_TITLE.to_string(), - command: PROFILE_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let profile_command = Command { + title: PROFILE_CODELENS_TITLE.to_string(), + command: PROFILE_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; + let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; - lenses.push(profile_lens); - } + lenses.push(profile_lens); } + } - if package.is_contract() { - // Currently not looking to deduplicate this since we don't have a clear decision on if the Contract stuff is staying - for contract in context.get_all_contracts(&crate_id) { - let location = contract.location; - let file_id = location.file; + if package.is_contract() { + // Currently not looking to deduplicate this since we don't have a clear decision on if the Contract stuff is staying + for contract in context.get_all_contracts(&crate_id) { + let location = contract.location; + let file_id = location.file; - // Ignore diagnostics for any file that wasn't the file we saved - // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != file_path { - continue; - } + // Ignore diagnostics for any file that wasn't the file we saved + // TODO: In the future, we could create "related" diagnostics for these files + if fm.path(file_id) != *file_path { + continue; + } - let range = - byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); + let range = + byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); - let compile_command = Command { - title: with_arrow(COMPILE_CODELENS_TITLE), - command: COMPILE_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let compile_command = Command { + title: with_arrow(COMPILE_CODELENS_TITLE), + command: COMPILE_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; + let compile_lens = CodeLens { range, command: Some(compile_command), data: None }; - lenses.push(compile_lens); + lenses.push(compile_lens); - let info_command = Command { - title: INFO_CODELENS_TITLE.to_string(), - command: INFO_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let info_command = Command { + title: INFO_CODELENS_TITLE.to_string(), + command: INFO_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let info_lens = CodeLens { range, command: Some(info_command), data: None }; + let info_lens = CodeLens { range, command: Some(info_command), data: None }; - lenses.push(info_lens); + lenses.push(info_lens); - let profile_command = Command { - title: PROFILE_CODELENS_TITLE.to_string(), - command: PROFILE_COMMAND.into(), - arguments: Some(package_selection_args(&workspace, package)), - }; + let profile_command = Command { + title: PROFILE_CODELENS_TITLE.to_string(), + command: PROFILE_COMMAND.into(), + arguments: Some(package_selection_args(workspace, package)), + }; - let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; + let profile_lens = CodeLens { range, command: Some(profile_command), data: None }; - lenses.push(profile_lens); - } + lenses.push(profile_lens); } } - if lenses.is_empty() { - Ok(None) - } else { - Ok(Some(lenses)) - } + lenses } diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 95f92262dbd..d3032c06917 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -27,7 +27,8 @@ mod test_run; mod tests; pub(crate) use { - code_lens_request::on_code_lens_request, goto_definition::on_goto_definition_request, profile_run::on_profile_run_request, + code_lens_request::collect_lenses_for_package, code_lens_request::on_code_lens_request, + goto_definition::on_goto_definition_request, profile_run::on_profile_run_request, test_run::on_test_run_request, tests::on_tests_request, }; diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index 755a6405db2..14c0264504d 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -9,10 +9,11 @@ use std::collections::{BTreeMap, HashMap}; // Re-providing lsp_types that we don't need to override pub(crate) use lsp_types::{ - CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams, - DidCloseTextDocumentParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams, - InitializeParams, InitializedParams, LogMessageParams, MessageType, Position, - PublishDiagnosticsParams, Range, ServerInfo, TextDocumentSyncCapability, Url, + CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic, DiagnosticSeverity, + DidChangeConfigurationParams, DidChangeTextDocumentParams, DidCloseTextDocumentParams, + DidOpenTextDocumentParams, DidSaveTextDocumentParams, InitializeParams, InitializedParams, + LogMessageParams, MessageType, Position, PublishDiagnosticsParams, Range, ServerInfo, + TextDocumentSyncCapability, Url, }; pub(crate) mod request { From 318b5d253c0056a26b32bc24cba6f3bd6b83caa3 Mon Sep 17 00:00:00 2001 From: Koby Date: Fri, 15 Dec 2023 17:42:28 +0100 Subject: [PATCH 03/14] feat(lsp): improve LSP code lens performance --- Cargo.lock | 18 ++-- tooling/lsp/Cargo.toml | 1 + tooling/lsp/src/lib.rs | 42 ++++++++- tooling/lsp/src/notifications/mod.rs | 84 ++++++++++------- tooling/lsp/src/requests/code_lens_request.rs | 90 ++++++++----------- tooling/lsp/src/types.rs | 3 +- tooling/nargo/src/lib.rs | 39 +++++++- 7 files changed, 175 insertions(+), 102 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cec7b5f0371..e30c774a106 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2589,6 +2589,7 @@ dependencies = [ "serde", "serde_json", "serde_with", + "thiserror", "tokio", "tower", "wasm-bindgen", @@ -4407,11 +4408,10 @@ checksum = "b6bc1c9ce2b5135ac7f93c72918fc37feb872bdc6a5533a8b85eb4b86bfdae52" [[package]] name = "tracing" -version = "0.1.37" +version = "0.1.40" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ce8c33a8d48bd45d624a6e523445fd21ec13d3653cd51f681abf67418f54eb8" +checksum = "c3523ab5a71916ccf420eebdf5521fcef02141234bbc0b8a49f2fdc4544364ef" dependencies = [ - "cfg-if", "log", "pin-project-lite", "tracing-attributes", @@ -4420,9 +4420,9 @@ dependencies = [ [[package]] name = "tracing-attributes" -version = "0.1.26" +version = "0.1.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f4f31f56159e98206da9efd823404b79b6ef3143b4a7ab76e67b1751b25a4ab" +checksum = "34704c8d6ebcbc939824180af020566b01a7c01f80641264eba0999f6c2b6be7" dependencies = [ "proc-macro2", "quote", @@ -4431,9 +4431,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.31" +version = "0.1.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0955b8137a1df6f1a2e9a37d8a6656291ff0297c1a97c24e0d8425fe2312f79a" +checksum = "c06d3da6113f116aaee68e4d601191614c9053067f9ab7f6edbcb161237daa54" dependencies = [ "once_cell", "valuable", @@ -4451,9 +4451,9 @@ dependencies = [ [[package]] name = "tracing-subscriber" -version = "0.3.17" +version = "0.3.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "30a651bc37f915e81f087d86e62a18eec5f79550c7faff886f7090b4ea757c77" +checksum = "ad0f048c97dbd9faa9b7df56362b8ebcaa52adb06b498c050d2f4e32f90a7a8b" dependencies = [ "sharded-slab", "thread_local", diff --git a/tooling/lsp/Cargo.toml b/tooling/lsp/Cargo.toml index 5f5e701da67..6371bcbac19 100644 --- a/tooling/lsp/Cargo.toml +++ b/tooling/lsp/Cargo.toml @@ -23,6 +23,7 @@ serde_json.workspace = true tower.workspace = true async-lsp = { workspace = true, features = ["omni-trait"] } serde_with = "3.2.0" +thiserror.workspace = true fm.workspace = true [target.'cfg(all(target_arch = "wasm32", not(target_os = "wasi")))'.dependencies] diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 2fdf75ccfd8..8b7a7ca1224 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -7,7 +7,7 @@ use std::{ collections::HashMap, future::Future, ops::{self, ControlFlow}, - path::PathBuf, + path::{Path, PathBuf}, pin::Pin, task::{self, Poll}, }; @@ -19,6 +19,8 @@ use async_lsp::{ }; use fm::codespan_files as files; use lsp_types::CodeLens; +use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection}; +use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; use noirc_frontend::{ graph::{CrateId, CrateName}, hir::{Context, FunctionNameMatch}, @@ -32,6 +34,7 @@ use requests::{ on_profile_run_request, on_shutdown, on_test_run_request, on_tests_request, }; use serde_json::Value as JsonValue; +use thiserror::Error; use tower::Service; mod notifications; @@ -42,13 +45,20 @@ mod types; use solver::WrapperSolver; use types::{notification, request, NargoTest, NargoTestId, Position, Range, Url}; +#[derive(Debug, Error)] +pub enum LspError { + /// Error while Resolving Workspace - {0}. + #[error("Failed to Resolve Workspace - {0}")] + WorkspaceResolutionError(String), +} + // State for the LSP gets implemented on this struct and is internal to the implementation pub struct LspState { root_path: Option, client: ClientSocket, solver: WrapperSolver, input_files: HashMap, - collected_lenses: Option>, + cached_lenses: HashMap>, } impl LspState { @@ -58,7 +68,7 @@ impl LspState { root_path: None, solver: WrapperSolver(Box::new(solver)), input_files: HashMap::new(), - collected_lenses: None, + cached_lenses: HashMap::new(), } } } @@ -179,3 +189,29 @@ fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( None } } + +pub(crate) fn resolve_workspace_for_source_path( + file_path: &Path, +) -> Result { + let package_root = find_file_manifest(file_path); + + let toml_path = match package_root { + Some(toml_path) => toml_path, + None => { + return Err(LspError::WorkspaceResolutionError(format!( + "Nargo.toml not found for file: {:?}", + file_path + ))) + } + }; + + let workspace = match resolve_workspace_from_toml( + &toml_path, + PackageSelection::All, + Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), + ) { + Ok(workspace) => workspace, + Err(err) => return Err(LspError::WorkspaceResolutionError(format!("{err}"))), + }; + Ok(workspace) +} diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 4c5d3cde282..e6465206290 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -2,18 +2,19 @@ use std::ops::ControlFlow; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use nargo::prepare_package; -use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::check_crate; use noirc_errors::{DiagnosticKind, FileDiagnostic}; +use crate::requests::collect_lenses_for_package; use crate::types::{ notification, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, - DidSaveTextDocumentParams, InitializedParams, LogMessageParams, MessageType, NargoPackageTests, - PublishDiagnosticsParams, + DidSaveTextDocumentParams, InitializedParams, NargoPackageTests, PublishDiagnosticsParams, }; -use crate::{byte_span_to_range, get_package_tests_in_crate, LspState}; +use crate::{ + byte_span_to_range, get_package_tests_in_crate, resolve_workspace_for_source_path, LspState, +}; pub(super) fn on_initialized( _state: &mut LspState, @@ -42,8 +43,38 @@ pub(super) fn on_did_change_text_document( params: DidChangeTextDocumentParams, ) -> ControlFlow> { let text = params.content_changes.into_iter().next().unwrap().text; - state.input_files.insert(params.text_document.uri.to_string(), text); - state.collected_lenses = Some(Vec::new()); + state.input_files.insert(params.text_document.uri.to_string(), text.clone()); + + let (mut context, crate_id) = nargo::prepare_source(text); + let _ = check_crate(&mut context, crate_id, false, false); + + let workspace = match resolve_workspace_for_source_path( + params.text_document.uri.to_file_path().unwrap().as_path(), + ) { + Ok(workspace) => workspace, + Err(lsp_error) => { + return ControlFlow::Break(Err(ResponseError::new( + ErrorCode::REQUEST_FAILED, + format!("{}", lsp_error), + ) + .into())) + } + }; + let package = match workspace.members.first() { + Some(package) => package, + None => { + return ControlFlow::Break(Err(ResponseError::new( + ErrorCode::REQUEST_FAILED, + "Selected workspace has no members", + ) + .into())) + } + }; + + let lenses = collect_lenses_for_package(&context, crate_id, &workspace, package, None); + + state.cached_lenses.insert(params.text_document.uri.to_string(), lenses); + ControlFlow::Continue(()) } @@ -52,6 +83,7 @@ pub(super) fn on_did_close_text_document( params: DidCloseTextDocumentParams, ) -> ControlFlow> { state.input_files.remove(¶ms.text_document.uri.to_string()); + state.cached_lenses.remove(¶ms.text_document.uri.to_string()); ControlFlow::Continue(()) } @@ -70,34 +102,14 @@ pub(super) fn on_did_save_text_document( } }; - let package_root = find_file_manifest(file_path.as_path()); - - let toml_path = match package_root { - Some(toml_path) => toml_path, - None => { - // If we cannot find a manifest, we log a warning but return no diagnostics - // We can reconsider this when we can build a file without the need for a Nargo.toml file to resolve deps - let _ = state.client.log_message(LogMessageParams { - typ: MessageType::WARNING, - message: format!("Nargo.toml not found for file: {:}", file_path.display()), - }); - return ControlFlow::Continue(()); - } - }; - - let workspace = match resolve_workspace_from_toml( - &toml_path, - PackageSelection::All, - Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - ) { - Ok(workspace) => workspace, - Err(err) => { - // If we found a manifest, but the workspace is invalid, we raise an error about it + let workspace = match resolve_workspace_for_source_path(&file_path) { + Ok(value) => value, + Err(lsp_error) => { return ControlFlow::Break(Err(ResponseError::new( ErrorCode::REQUEST_FAILED, - format!("{err}"), + format!("{}", lsp_error), ) - .into())); + .into())) } }; @@ -120,9 +132,13 @@ pub(super) fn on_did_save_text_document( } let collected_lenses = crate::requests::collect_lenses_for_package( - &context, crate_id, &file_path, &workspace, package, + &context, + crate_id, + &workspace, + package, + Some(&file_path), ); - state.collected_lenses = Some(collected_lenses); + state.cached_lenses.insert(params.text_document.uri.to_string(), collected_lenses); let fm = &context.file_manager; let files = fm.as_file_map(); diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 04a31288cc0..9f22bed8e5d 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -1,15 +1,14 @@ use std::future::{self, Future}; -use async_lsp::{ErrorCode, LanguageClient, ResponseError}; +use async_lsp::{ErrorCode, ResponseError}; use nargo::{package::Package, workspace::Workspace}; -use nargo_toml::{resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::check_crate; use noirc_frontend::hir::FunctionNameMatch; use crate::{ - byte_span_to_range, - types::{CodeLens, CodeLensParams, CodeLensResult, Command, LogMessageParams, MessageType}, + byte_span_to_range, resolve_workspace_for_source_path, + types::{CodeLens, CodeLensParams, CodeLensResult, Command}, LspState, }; @@ -50,67 +49,46 @@ fn on_code_lens_request_inner( state: &mut LspState, params: CodeLensParams, ) -> Result { - if let Some(collected_lenses) = &state.collected_lenses { - return Ok(Some(collected_lenses.clone())); - } - let file_path = params.text_document.uri.to_file_path().map_err(|_| { ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; - let package_root = nargo_toml::find_file_manifest(file_path.as_path()); - - let toml_path = match package_root { - Some(toml_path) => toml_path, - None => { - // If we cannot find a manifest, we log a warning but return no diagnostics - // We can reconsider this when we can build a file without the need for a Nargo.toml file to resolve deps - let _ = state.client.log_message(LogMessageParams { - typ: MessageType::WARNING, - message: format!("Nargo.toml not found for file: {:}", file_path.display()), - }); - return Ok(None); - } - }; - - let workspace = resolve_workspace_from_toml( - &toml_path, - PackageSelection::All, - Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - ) - .map_err(|err| { - // If we found a manifest, but the workspace is invalid, we raise an error about it - ResponseError::new(ErrorCode::REQUEST_FAILED, err) - })?; + if let Some(collected_lenses) = state.cached_lenses.get(¶ms.text_document.uri.to_string()) { + return Ok(Some(collected_lenses.clone())); + } - let mut lenses: Vec = vec![]; + let source_string = std::fs::read_to_string(&file_path).map_err(|_| { + ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not read file from disk") + })?; - for package in &workspace { - let (mut context, crate_id) = nargo::prepare_package(package); - // We ignore the warnings and errors produced by compilation for producing code lenses - // because we can still get the test functions even if compilation fails - let _ = check_crate(&mut context, crate_id, false, false); + let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); + let package = workspace.members.first().unwrap(); - let collected_lenses = - collect_lenses_for_package(&context, crate_id, &file_path, &workspace, package); + let (mut context, crate_id) = nargo::prepare_source(source_string); + // We ignore the warnings and errors produced by compilation for producing code lenses + // because we can still get the test functions even if compilation fails + let _ = check_crate(&mut context, crate_id, false, false); - lenses.extend(collected_lenses); - } + let collected_lenses = + collect_lenses_for_package(&context, crate_id, &workspace, package, None); - if lenses.is_empty() { + if collected_lenses.is_empty() { + state.cached_lenses.remove(¶ms.text_document.uri.to_string()); Ok(None) } else { - state.collected_lenses = Some(lenses.clone()); - Ok(Some(lenses)) + state + .cached_lenses + .insert(params.text_document.uri.to_string().clone(), collected_lenses.clone()); + Ok(Some(collected_lenses)) } } pub(crate) fn collect_lenses_for_package( context: &noirc_frontend::macros_api::HirContext, crate_id: noirc_frontend::macros_api::CrateId, - file_path: &std::path::PathBuf, workspace: &Workspace, package: &Package, + file_path: Option<&std::path::PathBuf>, ) -> Vec { let mut lenses: Vec = vec![]; let fm = &context.file_manager; @@ -123,8 +101,10 @@ pub(crate) fn collect_lenses_for_package( // Ignore diagnostics for any file that wasn't the file we saved // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != *file_path { - continue; + if let Some(file_path) = file_path { + if fm.path(file_id) != *file_path { + continue; + } } let range = byte_span_to_range(files, file_id, location.span.into()).unwrap_or_default(); @@ -152,8 +132,10 @@ pub(crate) fn collect_lenses_for_package( // Ignore diagnostics for any file that wasn't the file we saved // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != *file_path { - return lenses; + if let Some(file_path) = file_path { + if fm.path(file_id) != *file_path { + return lenses; + } } let range = @@ -209,8 +191,10 @@ pub(crate) fn collect_lenses_for_package( // Ignore diagnostics for any file that wasn't the file we saved // TODO: In the future, we could create "related" diagnostics for these files - if fm.path(file_id) != *file_path { - continue; + if let Some(file_path) = file_path { + if fm.path(file_id) != *file_path { + continue; + } } let range = diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index 14c0264504d..b2960964e7c 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -12,8 +12,7 @@ pub(crate) use lsp_types::{ CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic, DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams, InitializeParams, InitializedParams, - LogMessageParams, MessageType, Position, PublishDiagnosticsParams, Range, ServerInfo, - TextDocumentSyncCapability, Url, + Position, PublishDiagnosticsParams, Range, ServerInfo, TextDocumentSyncCapability, Url, }; pub(crate) mod request { diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 6f3d36febba..08a8ba70411 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -14,7 +14,7 @@ pub mod ops; pub mod package; pub mod workspace; -use std::collections::BTreeMap; +use std::{collections::BTreeMap, path::Path}; use fm::FileManager; use noirc_driver::{add_dep, prepare_crate, prepare_dependency}; @@ -101,6 +101,28 @@ pub fn prepare_package(package: &Package) -> (Context, CrateId) { (context, crate_id) } +/// Prepares a package from a source string +/// This is useful for situations when we don't need dependencies +/// and just need to operate on single file. +/// +/// Use case for this is the LSP server and code lenses +/// which operate on single file and need to understand this file +/// in order to offer code lenses to the user +pub fn prepare_source(source: String) -> (Context, CrateId) { + let root = Path::new(""); + let mut file_manager = FileManager::new(root); + let root_file_id = file_manager.add_file_with_source(Path::new("main.nr"), source).expect( + "Adding source buffer to file manager should never fail when file manager is empty", + ); + + let graph = CrateGraph::default(); + let mut context = Context::new(file_manager, graph); + + let root_crate_id = context.crate_graph.add_crate_root(root_file_id); + + (context, root_crate_id) +} + // Get all paths in the directory and subdirectories. // // Panics: If the path is not a path to a directory. @@ -168,4 +190,19 @@ mod tests { assert!(paths.contains(&path)); } } + #[test] + fn prepare_package_from_source_string() { + let source = r#" + fn main() { + let x = 1; + let y = 2; + let z = x + y; + } + "#; + + let (mut context, crate_id) = crate::prepare_source(source.to_string()); + let _check_result = noirc_driver::check_crate(&mut context, crate_id, false, false); + let main_func_id = context.get_main_function(&crate_id); + assert!(main_func_id.is_some()); + } } From a2943b17b374696faff70fbb8c03b546a36942dc Mon Sep 17 00:00:00 2001 From: Koby Date: Fri, 15 Dec 2023 17:43:20 +0100 Subject: [PATCH 04/14] feat(lsp): accept option to disable code lens --- tooling/lsp/src/requests/mod.rs | 39 ++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index d3032c06917..f4cc3b11b0a 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -4,6 +4,8 @@ use crate::types::{CodeLensOptions, InitializeParams}; use async_lsp::ResponseError; use lsp_types::{Position, TextDocumentSyncCapability, TextDocumentSyncKind}; use nargo_fmt::Config; +use serde::{Deserialize, Serialize}; + use crate::{ types::{InitializeResult, NargoCapability, NargoTestsOptions, ServerCapabilities}, @@ -32,16 +34,47 @@ pub(crate) use { test_run::on_test_run_request, tests::on_tests_request, }; +#[derive(Debug, Deserialize, Serialize)] +struct LspInitializationOptions { + + #[serde(rename = "enableCodeLens", default = "default_enable_code_lens")] + enable_code_lens: bool, +} + +fn default_enable_code_lens() -> bool { + true +} + +impl Default for LspInitializationOptions { + fn default() -> Self { + Self { + enable_code_lens: default_enable_code_lens(), + } + } +} + pub(crate) fn on_initialize( state: &mut LspState, params: InitializeParams, ) -> impl Future> { + state.root_path = params.root_uri.and_then(|root_uri| root_uri.to_file_path().ok()); - async { + let initialization_options = match params.initialization_options { + Some(initialization_options) => serde_json::from_value::(initialization_options).unwrap_or_default(), + None => LspInitializationOptions::default(), + }; + + async move { let text_document_sync = TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL); - let code_lens = CodeLensOptions { resolve_provider: Some(false) }; + + let code_lens = match initialization_options.enable_code_lens { + true => Some(CodeLensOptions { resolve_provider: Some(false) }), + false => None, + }; + + CodeLensOptions { resolve_provider: Some(false) }; let nargo = NargoCapability { tests: Some(NargoTestsOptions { @@ -54,7 +87,7 @@ pub(crate) fn on_initialize( Ok(InitializeResult { capabilities: ServerCapabilities { text_document_sync: Some(text_document_sync), - code_lens_provider: Some(code_lens), + code_lens_provider: code_lens, document_formatting_provider: true, nargo: Some(nargo), definition_provider: Some(lsp_types::OneOf::Left(true)), From 2c06e912fd4aa24680b2962414ee910a144eb097 Mon Sep 17 00:00:00 2001 From: Koby Date: Fri, 15 Dec 2023 17:59:51 +0100 Subject: [PATCH 05/14] chore: document `LspInitializationOptions` --- tooling/lsp/src/requests/mod.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index f4cc3b11b0a..4a5267b1582 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -34,9 +34,13 @@ pub(crate) use { test_run::on_test_run_request, tests::on_tests_request, }; +/// LSP client will send initialization request after the server has started. +/// [InitializeParams].`initialization_options` will contain the options sent from the client. #[derive(Debug, Deserialize, Serialize)] struct LspInitializationOptions { + /// Controls whether code lens is enabled by the server + /// By default this will be set to true (enabled). #[serde(rename = "enableCodeLens", default = "default_enable_code_lens")] enable_code_lens: bool, } From a3fd9a4a5c7d924d31e59852ef48f79362ebb976 Mon Sep 17 00:00:00 2001 From: Koby Date: Fri, 15 Dec 2023 20:34:09 +0100 Subject: [PATCH 06/14] fix: failing test --- tooling/lsp/src/requests/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index b3693e8e26e..a132a61752c 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -147,7 +147,7 @@ pub(crate) fn on_shutdown( #[cfg(test)] mod initialization { use async_lsp::ClientSocket; - use lsp_types::{InitializeParams, TextDocumentSyncCapability, TextDocumentSyncKind}; + use lsp_types::{InitializeParams, TextDocumentSyncCapability, TextDocumentSyncKind, CodeLensOptions}; use tokio::test; use crate::{ From cc1c4848af0dce45c040d2aba2bc39d535cda45b Mon Sep 17 00:00:00 2001 From: Koby Date: Fri, 15 Dec 2023 20:52:08 +0100 Subject: [PATCH 07/14] chore: fmt --- tooling/lsp/src/requests/mod.rs | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index a132a61752c..b475aebe702 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -6,7 +6,6 @@ use lsp_types::{Position, TextDocumentSyncCapability, TextDocumentSyncKind}; use nargo_fmt::Config; use serde::{Deserialize, Serialize}; - use crate::{ types::{InitializeResult, NargoCapability, NargoTestsOptions, ServerCapabilities}, LspState, @@ -38,7 +37,6 @@ pub(crate) use { /// [InitializeParams].`initialization_options` will contain the options sent from the client. #[derive(Debug, Deserialize, Serialize)] struct LspInitializationOptions { - /// Controls whether code lens is enabled by the server /// By default this will be set to true (enabled). #[serde(rename = "enableCodeLens", default = "default_enable_code_lens")] @@ -51,9 +49,7 @@ fn default_enable_code_lens() -> bool { impl Default for LspInitializationOptions { fn default() -> Self { - Self { - enable_code_lens: default_enable_code_lens(), - } + Self { enable_code_lens: default_enable_code_lens() } } } @@ -61,23 +57,24 @@ pub(crate) fn on_initialize( state: &mut LspState, params: InitializeParams, ) -> impl Future> { - state.root_path = params.root_uri.and_then(|root_uri| root_uri.to_file_path().ok()); let initialization_options = match params.initialization_options { - Some(initialization_options) => serde_json::from_value::(initialization_options).unwrap_or_default(), + Some(initialization_options) => { + serde_json::from_value::(initialization_options) + .unwrap_or_default() + } None => LspInitializationOptions::default(), }; - + async move { let text_document_sync = TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL); - let code_lens = match initialization_options.enable_code_lens { true => Some(CodeLensOptions { resolve_provider: Some(false) }), false => None, }; - + CodeLensOptions { resolve_provider: Some(false) }; let nargo = NargoCapability { @@ -147,7 +144,9 @@ pub(crate) fn on_shutdown( #[cfg(test)] mod initialization { use async_lsp::ClientSocket; - use lsp_types::{InitializeParams, TextDocumentSyncCapability, TextDocumentSyncKind, CodeLensOptions}; + use lsp_types::{ + CodeLensOptions, InitializeParams, TextDocumentSyncCapability, TextDocumentSyncKind, + }; use tokio::test; use crate::{ From daf36bc9225653a2a2b47b0e73a77b50bd78afa6 Mon Sep 17 00:00:00 2001 From: Koby Date: Mon, 18 Dec 2023 10:10:50 +0100 Subject: [PATCH 08/14] chore: clippy --- tooling/lsp/src/requests/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index b475aebe702..04be0d3642c 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -75,8 +75,6 @@ pub(crate) fn on_initialize( false => None, }; - CodeLensOptions { resolve_provider: Some(false) }; - let nargo = NargoCapability { tests: Some(NargoTestsOptions { fetch: Some(true), From 308dcef9824a1fba69f63ab669bb930ac89867aa Mon Sep 17 00:00:00 2001 From: Koby Date: Mon, 18 Dec 2023 10:24:08 +0100 Subject: [PATCH 09/14] chore: adjust to newer api --- tooling/nargo/src/lib.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index 2cab131f292..aebf2600400 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -114,8 +114,7 @@ pub fn prepare_source(source: String) -> (Context, CrateId) { "Adding source buffer to file manager should never fail when file manager is empty", ); - let graph = CrateGraph::default(); - let mut context = Context::new(file_manager, graph); + let mut context = Context::new(file_manager); let root_crate_id = context.crate_graph.add_crate_root(root_file_id); From f8f96165d1f1c6efd15b0c525bc59062b59dc654 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 19 Dec 2023 13:48:48 +0100 Subject: [PATCH 10/14] chore: review nits --- tooling/lsp/src/lib.rs | 33 +++++++++++----------------- tooling/lsp/src/notifications/mod.rs | 4 ++-- tooling/lsp/src/requests/mod.rs | 3 +++ 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 8b7a7ca1224..c915e0c1528 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -19,6 +19,7 @@ use async_lsp::{ }; use fm::codespan_files as files; use lsp_types::CodeLens; +use nargo::workspace::Workspace; use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; use noirc_frontend::{ @@ -47,7 +48,7 @@ use types::{notification, request, NargoTest, NargoTestId, Position, Range, Url} #[derive(Debug, Error)] pub enum LspError { - /// Error while Resolving Workspace - {0}. + /// Error while Resolving Workspace. #[error("Failed to Resolve Workspace - {0}")] WorkspaceResolutionError(String), } @@ -192,26 +193,18 @@ fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( pub(crate) fn resolve_workspace_for_source_path( file_path: &Path, -) -> Result { +) -> Result { let package_root = find_file_manifest(file_path); - let toml_path = match package_root { - Some(toml_path) => toml_path, - None => { - return Err(LspError::WorkspaceResolutionError(format!( - "Nargo.toml not found for file: {:?}", - file_path - ))) - } - }; - - let workspace = match resolve_workspace_from_toml( - &toml_path, - PackageSelection::All, - Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - ) { - Ok(workspace) => workspace, - Err(err) => return Err(LspError::WorkspaceResolutionError(format!("{err}"))), - }; + let toml_path = package_root.ok_or_else(|| { + LspError::WorkspaceResolutionError(format!( + "Nargo.toml not found for file: {:?}", + file_path + )) + })?; + + let workspace = resolve_workspace_from_toml(&toml_path, PackageSelection::All, Some(NOIR_ARTIFACT_VERSION_STRING.to_string())) + .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?; + Ok(workspace) } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index e6465206290..c76461c52e8 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -55,7 +55,7 @@ pub(super) fn on_did_change_text_document( Err(lsp_error) => { return ControlFlow::Break(Err(ResponseError::new( ErrorCode::REQUEST_FAILED, - format!("{}", lsp_error), + lsp_error.to_string(), ) .into())) } @@ -107,7 +107,7 @@ pub(super) fn on_did_save_text_document( Err(lsp_error) => { return ControlFlow::Break(Err(ResponseError::new( ErrorCode::REQUEST_FAILED, - format!("{}", lsp_error), + lsp_error.to_string(), ) .into())) } diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 04be0d3642c..e996b129c91 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -59,8 +59,11 @@ pub(crate) fn on_initialize( ) -> impl Future> { state.root_path = params.root_uri.and_then(|root_uri| root_uri.to_file_path().ok()); + let initialization_options = match params.initialization_options { + Some(initialization_options) => { + // We want default value of this setting to be true, so we need to handle the case where the client sends us a empty value serde_json::from_value::(initialization_options) .unwrap_or_default() } From c0683adb5082c001f771a501e71ee910f27cf158 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 19 Dec 2023 14:39:13 +0100 Subject: [PATCH 11/14] chore: review nits - better default --- tooling/lsp/src/lib.rs | 14 ++++++++------ tooling/lsp/src/requests/mod.rs | 21 ++++++++------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index c915e0c1528..3b4fa2d478f 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -191,9 +191,7 @@ fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( } } -pub(crate) fn resolve_workspace_for_source_path( - file_path: &Path, -) -> Result { +pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result { let package_root = find_file_manifest(file_path); let toml_path = package_root.ok_or_else(|| { @@ -203,8 +201,12 @@ pub(crate) fn resolve_workspace_for_source_path( )) })?; - let workspace = resolve_workspace_from_toml(&toml_path, PackageSelection::All, Some(NOIR_ARTIFACT_VERSION_STRING.to_string())) - .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?; - + let workspace = resolve_workspace_from_toml( + &toml_path, + PackageSelection::All, + Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), + ) + .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?; + Ok(workspace) } diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index e996b129c91..2711c597bcf 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -59,23 +59,18 @@ pub(crate) fn on_initialize( ) -> impl Future> { state.root_path = params.root_uri.and_then(|root_uri| root_uri.to_file_path().ok()); - - let initialization_options = match params.initialization_options { - - Some(initialization_options) => { - // We want default value of this setting to be true, so we need to handle the case where the client sends us a empty value - serde_json::from_value::(initialization_options) - .unwrap_or_default() - } - None => LspInitializationOptions::default(), - }; + let initialization_options: LspInitializationOptions = params + .initialization_options + .and_then(|value| serde_json::from_value(value).ok()) + .unwrap_or_default(); async move { let text_document_sync = TextDocumentSyncCapability::Kind(TextDocumentSyncKind::FULL); - let code_lens = match initialization_options.enable_code_lens { - true => Some(CodeLensOptions { resolve_provider: Some(false) }), - false => None, + let code_lens = if initialization_options.enable_code_lens { + Some(CodeLensOptions { resolve_provider: Some(false) }) + } else { + None }; let nargo = NargoCapability { From ca19e4baeb54ffea5c0589559f7f77032e33efa7 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 19 Dec 2023 15:20:21 +0100 Subject: [PATCH 12/14] chore: move prepare_source fn to lsp from nargo --- tooling/lsp/src/lib.rs | 24 +++++++++++++++++++ tooling/lsp/src/notifications/mod.rs | 5 ++-- tooling/lsp/src/requests/code_lens_request.rs | 4 ++-- tooling/nargo/src/lib.rs | 23 +----------------- 4 files changed, 30 insertions(+), 26 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 3b4fa2d478f..d6e9b5b24f4 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -26,6 +26,9 @@ use noirc_frontend::{ graph::{CrateId, CrateName}, hir::{Context, FunctionNameMatch}, }; + +use fm::FileManager; + use notifications::{ on_did_change_configuration, on_did_change_text_document, on_did_close_text_document, on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized, @@ -210,3 +213,24 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result (Context, CrateId) { + let root = Path::new(""); + let mut file_manager = FileManager::new(root); + let root_file_id = file_manager.add_file_with_source(Path::new("main.nr"), source).expect( + "Adding source buffer to file manager should never fail when file manager is empty", + ); + + let mut context = Context::new(file_manager); + + let root_crate_id = context.crate_graph.add_crate_root(root_file_id); + + (context, root_crate_id) +} diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index c76461c52e8..d52d9affdf8 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -13,7 +13,8 @@ use crate::types::{ }; use crate::{ - byte_span_to_range, get_package_tests_in_crate, resolve_workspace_for_source_path, LspState, + byte_span_to_range, get_package_tests_in_crate, prepare_source, + resolve_workspace_for_source_path, LspState, }; pub(super) fn on_initialized( @@ -45,7 +46,7 @@ pub(super) fn on_did_change_text_document( let text = params.content_changes.into_iter().next().unwrap().text; state.input_files.insert(params.text_document.uri.to_string(), text.clone()); - let (mut context, crate_id) = nargo::prepare_source(text); + let (mut context, crate_id) = prepare_source(text); let _ = check_crate(&mut context, crate_id, false, false); let workspace = match resolve_workspace_for_source_path( diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 9f22bed8e5d..4b1d38a137e 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -7,7 +7,7 @@ use noirc_driver::check_crate; use noirc_frontend::hir::FunctionNameMatch; use crate::{ - byte_span_to_range, resolve_workspace_for_source_path, + byte_span_to_range, prepare_source, resolve_workspace_for_source_path, types::{CodeLens, CodeLensParams, CodeLensResult, Command}, LspState, }; @@ -64,7 +64,7 @@ fn on_code_lens_request_inner( let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); let package = workspace.members.first().unwrap(); - let (mut context, crate_id) = nargo::prepare_source(source_string); + let (mut context, crate_id) = prepare_source(source_string); // We ignore the warnings and errors produced by compilation for producing code lenses // because we can still get the test functions even if compilation fails let _ = check_crate(&mut context, crate_id, false, false); diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index aebf2600400..681f277dde4 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -14,7 +14,7 @@ pub mod ops; pub mod package; pub mod workspace; -use std::{collections::BTreeMap, path::Path}; +use std::collections::BTreeMap; use fm::FileManager; use noirc_driver::{add_dep, prepare_crate, prepare_dependency}; @@ -100,27 +100,6 @@ pub fn prepare_package(package: &Package) -> (Context, CrateId) { (context, crate_id) } -/// Prepares a package from a source string -/// This is useful for situations when we don't need dependencies -/// and just need to operate on single file. -/// -/// Use case for this is the LSP server and code lenses -/// which operate on single file and need to understand this file -/// in order to offer code lenses to the user -pub fn prepare_source(source: String) -> (Context, CrateId) { - let root = Path::new(""); - let mut file_manager = FileManager::new(root); - let root_file_id = file_manager.add_file_with_source(Path::new("main.nr"), source).expect( - "Adding source buffer to file manager should never fail when file manager is empty", - ); - - let mut context = Context::new(file_manager); - - let root_crate_id = context.crate_graph.add_crate_root(root_file_id); - - (context, root_crate_id) -} - // Get all paths in the directory and subdirectories. // // Panics: If the path is not a path to a directory. From f0f68c955f2c7a43d6e6f13d13bb5d4229a31030 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 19 Dec 2023 15:29:15 +0100 Subject: [PATCH 13/14] chore: merge fixups --- tooling/lsp/src/lib.rs | 2 +- tooling/lsp/src/notifications/mod.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index d6e9b5b24f4..82adb965b85 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -221,7 +221,7 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result (Context, CrateId) { +fn prepare_source(source: String) -> (Context<'static>, CrateId) { let root = Path::new(""); let mut file_manager = FileManager::new(root); let root_file_id = file_manager.add_file_with_source(Path::new("main.nr"), source).expect( diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 5d44b2f58ab..6cefaa134ce 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -2,8 +2,7 @@ use std::ops::ControlFlow; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package}; -use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::{check_crate, file_manager_with_stdlib}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use crate::requests::collect_lenses_for_package; From 795eb21c97582d629c10091230519bc9dbc97ab2 Mon Sep 17 00:00:00 2001 From: Koby Date: Tue, 19 Dec 2023 15:50:38 +0100 Subject: [PATCH 14/14] chore: move test along prepare_source --- tooling/lsp/src/lib.rs | 16 ++++++++++++++++ tooling/nargo/src/lib.rs | 15 --------------- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index 82adb965b85..9887e5b8e96 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -234,3 +234,19 @@ fn prepare_source(source: String) -> (Context<'static>, CrateId) { (context, root_crate_id) } + +#[test] +fn prepare_package_from_source_string() { + let source = r#" + fn main() { + let x = 1; + let y = 2; + let z = x + y; + } + "#; + + let (mut context, crate_id) = crate::prepare_source(source.to_string()); + let _check_result = noirc_driver::check_crate(&mut context, crate_id, false, false); + let main_func_id = context.get_main_function(&crate_id); + assert!(main_func_id.is_some()); +} diff --git a/tooling/nargo/src/lib.rs b/tooling/nargo/src/lib.rs index e897248cefa..db54fd3d574 100644 --- a/tooling/nargo/src/lib.rs +++ b/tooling/nargo/src/lib.rs @@ -175,19 +175,4 @@ mod tests { assert!(paths.contains(&path)); } } - #[test] - fn prepare_package_from_source_string() { - let source = r#" - fn main() { - let x = 1; - let y = 2; - let z = x + y; - } - "#; - - let (mut context, crate_id) = crate::prepare_source(source.to_string()); - let _check_result = noirc_driver::check_crate(&mut context, crate_id, false, false); - let main_func_id = context.get_main_function(&crate_id); - assert!(main_func_id.is_some()); - } }