diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index c7b70339e1d..2268fbccb46 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -236,38 +236,14 @@ fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( } } -pub(crate) fn resolve_workspace_for_source_path( - file_path: &Path, - root_path: &Option, -) -> Result { - // If there's a LSP root path, starting from file_path go up the directory tree - // searching for Nargo.toml files. The last one we find is the one we'll use - // (we'll assume Noir workspaces aren't nested) - if let Some(root_path) = root_path { - let mut current_path = file_path; - let mut current_toml_path = None; - while current_path.starts_with(root_path) { - if let Some(toml_path) = find_file_manifest(current_path) { - current_toml_path = Some(toml_path); - - if let Some(next_path) = current_path.parent() { - current_path = next_path; - } else { - break; - } - } else { - break; - } - } - - if let Some(toml_path) = current_toml_path { - return resolve_workspace_from_toml( - &toml_path, - PackageSelection::All, - Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - ) - .map_err(|err| LspError::WorkspaceResolutionError(err.to_string())); - } +pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result { + if let Some(toml_path) = find_file_manifest(file_path) { + return resolve_workspace_from_toml( + &toml_path, + PackageSelection::All, + Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), + ) + .map_err(|err| LspError::WorkspaceResolutionError(err.to_string())); } let Some(parent_folder) = file_path diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 24409e85db8..b8ff4fb371f 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -37,15 +37,9 @@ pub(super) fn on_did_open_text_document( state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text); let document_uri = params.text_document.uri; - let only_process_document_uri_package = false; let output_diagnostics = true; - match process_workspace_for_noir_document( - state, - document_uri, - only_process_document_uri_package, - output_diagnostics, - ) { + match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { Ok(_) => { state.open_documents_count += 1; ControlFlow::Continue(()) @@ -62,15 +56,9 @@ pub(super) fn on_did_change_text_document( state.input_files.insert(params.text_document.uri.to_string(), text.clone()); let document_uri = params.text_document.uri; - let only_process_document_uri_package = true; - let output_diagnotics = false; + let output_diagnostics = false; - match process_workspace_for_noir_document( - state, - document_uri, - only_process_document_uri_package, - output_diagnotics, - ) { + match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -90,15 +78,9 @@ pub(super) fn on_did_close_text_document( } let document_uri = params.text_document.uri; - let only_process_document_uri_package = true; - let output_diagnotics = false; + let output_diagnostics = false; - match process_workspace_for_noir_document( - state, - document_uri, - only_process_document_uri_package, - output_diagnotics, - ) { + match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -109,15 +91,9 @@ pub(super) fn on_did_save_text_document( params: DidSaveTextDocumentParams, ) -> ControlFlow> { let document_uri = params.text_document.uri; - let only_process_document_uri_package = false; - let output_diagnotics = true; + let output_diagnostics = true; - match process_workspace_for_noir_document( - state, - document_uri, - only_process_document_uri_package, - output_diagnotics, - ) { + match process_workspace_for_noir_document(state, document_uri, output_diagnostics) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } @@ -129,17 +105,15 @@ pub(super) fn on_did_save_text_document( pub(crate) fn process_workspace_for_noir_document( state: &mut LspState, document_uri: lsp_types::Url, - only_process_document_uri_package: bool, output_diagnostics: bool, ) -> Result<(), async_lsp::Error> { let file_path = document_uri.to_file_path().map_err(|_| { ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; - let workspace = - resolve_workspace_for_source_path(&file_path, &state.root_path).map_err(|lsp_error| { - ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) - })?; + let workspace = resolve_workspace_for_source_path(&file_path).map_err(|lsp_error| { + ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) + })?; let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager( @@ -155,10 +129,6 @@ pub(crate) fn process_workspace_for_noir_document( .flat_map(|package| -> Vec { let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into(); - if only_process_document_uri_package && !file_path.starts_with(&package.root_dir) { - return vec![]; - } - let (mut context, crate_id) = crate::prepare_package(&workspace_file_manager, &parsed_files, package); diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 51336a324da..325392e150c 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -63,8 +63,7 @@ fn on_code_lens_request_inner( ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not read file from disk") })?; - let workspace = - resolve_workspace_for_source_path(file_path.as_path(), &state.root_path).unwrap(); + let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); let package = crate::workspace_package_for_file(&workspace, &file_path).ok_or_else(|| { ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file") diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 161fd20f555..40d4bb92448 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -23,39 +23,45 @@ pub(crate) fn on_hover_request( params: HoverParams, ) -> impl Future, ResponseError>> { let result = process_request(state, params.text_document_position_params, |args| { - args.interner.reference_at_location(args.location).map(|reference| { + args.interner.reference_at_location(args.location).and_then(|reference| { let location = args.interner.reference_location(reference); let lsp_location = to_lsp_location(args.files, location.file, location.span); - Hover { + format_reference(reference, &args).map(|formatted| Hover { range: lsp_location.map(|location| location.range), contents: HoverContents::Markup(MarkupContent { kind: MarkupKind::Markdown, - value: format_reference(reference, &args), + value: formatted, }), - } + }) }) }); future::ready(result) } -fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) -> String { +fn format_reference(reference: ReferenceId, args: &ProcessRequestCallbackArgs) -> Option { match reference { ReferenceId::Module(id) => format_module(id, args), - ReferenceId::Struct(id) => format_struct(id, args), - ReferenceId::StructMember(id, field_index) => format_struct_member(id, field_index, args), - ReferenceId::Trait(id) => format_trait(id, args), - ReferenceId::Global(id) => format_global(id, args), - ReferenceId::Function(id) => format_function(id, args), - ReferenceId::Alias(id) => format_alias(id, args), - ReferenceId::Local(id) => format_local(id, args), + ReferenceId::Struct(id) => Some(format_struct(id, args)), + ReferenceId::StructMember(id, field_index) => { + Some(format_struct_member(id, field_index, args)) + } + ReferenceId::Trait(id) => Some(format_trait(id, args)), + ReferenceId::Global(id) => Some(format_global(id, args)), + ReferenceId::Function(id) => Some(format_function(id, args)), + ReferenceId::Alias(id) => Some(format_alias(id, args)), + ReferenceId::Local(id) => Some(format_local(id, args)), ReferenceId::Reference(location, _) => { format_reference(args.interner.find_referenced(location).unwrap(), args) } } } -fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> String { - let module_attributes = args.interner.module_attributes(&id); +fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> Option { + // Note: it's not clear why `try_module_attributes` might return None here, but it happens. + // This is a workaround to avoid panicking in that case (which brings the LSP server down). + // Cases where this happens are related to generated code, so once that stops happening + // this won't be an issue anymore. + let module_attributes = args.interner.try_module_attributes(&id)?; let mut string = String::new(); if format_parent_module_from_module_id( @@ -68,7 +74,7 @@ fn format_module(id: ModuleId, args: &ProcessRequestCallbackArgs) -> String { string.push_str(" "); string.push_str("mod "); string.push_str(&module_attributes.name); - string + Some(string) } fn format_struct(id: StructId, args: &ProcessRequestCallbackArgs) -> String { diff --git a/tooling/lsp/src/requests/inlay_hint.rs b/tooling/lsp/src/requests/inlay_hint.rs index 35ee36e11fa..a2ce8b46c8b 100644 --- a/tooling/lsp/src/requests/inlay_hint.rs +++ b/tooling/lsp/src/requests/inlay_hint.rs @@ -104,7 +104,11 @@ impl<'a> InlayHintCollector<'a> { } ItemKind::Global(let_statement) => self.collect_in_let_statement(let_statement), ItemKind::Submodules(parsed_submodule) => { - self.collect_in_parsed_module(&parsed_submodule.contents); + // Inlay hints inside a contract might show up incorrectly because contracts can + // have generated code whose location overlaps with real code. + if !parsed_submodule.is_contract { + self.collect_in_parsed_module(&parsed_submodule.contents); + } } ItemKind::ModuleDecl(_) => (), ItemKind::Import(_) => (), diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 54f571af5a5..59ce91ea681 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -364,8 +364,7 @@ where ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; - let workspace = - resolve_workspace_for_source_path(file_path.as_path(), &state.root_path).unwrap(); + let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); let package = crate::workspace_package_for_file(&workspace, &file_path).ok_or_else(|| { ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file") })?; diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index 375e0b69aed..c720156659d 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -94,6 +94,10 @@ mod references_tests { check_references_succeeds("rename_function", "another_function", 0, false).await; } + // Ignored because making this work slows down everything, so for now things will not work + // as ideally, but they'll be fast. + // See https://github.com/noir-lang/noir/issues/5460 + #[ignore] #[test] async fn test_on_references_request_works_accross_workspace_packages() { let (mut state, noir_text_document) = test_utils::init_lsp_server("workspace").await; @@ -108,13 +112,11 @@ mod references_tests { let two_lib = Url::from_file_path(workspace_dir.join("two/src/lib.nr")).unwrap(); // We call this to open the document, so that the entire workspace is analyzed - let only_process_document_uri_package = false; let output_diagnostics = true; notifications::process_workspace_for_noir_document( &mut state, one_lib.clone(), - only_process_document_uri_package, output_diagnostics, ) .unwrap();