fix: speed up LSP

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<PathBuf>,
-) -> Result<Workspace, LspError> {
-    // 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<Workspace, LspError> {
+    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

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<Result<(), async_lsp::Error>> {
     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<Diagnostic> {
             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);
 

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")

tooling/lsp/src/requests/hover.rs

@@ -23,39 +23,45 @@
     params: HoverParams,
 ) -> impl Future<Output = Result<Option<Hover>, 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<String> {
     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<String> {
+    // 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 @@
     string.push_str("    ");
     string.push_str("mod ");
     string.push_str(&module_attributes.name);
-    string
+    Some(string)
 }
 
 fn format_struct(id: StructId, args: &ProcessRequestCallbackArgs) -> String {
@@ -402,7 +408,7 @@
             "two/src/lib.nr",
             Position { line: 6, character: 9 },
             r#"    one
     mod subone"#,
         )
         .await;
     }
@@ -413,7 +419,7 @@
             "workspace",
             "two/src/lib.nr",
             Position { line: 9, character: 20 },
             r#"    one::subone
     struct SubOneStruct {
         some_field: i32,
         some_other_field: Field,
@@ -428,7 +434,7 @@
             "workspace",
             "two/src/lib.nr",
             Position { line: 46, character: 17 },
             r#"    one::subone
     struct GenericStruct<A, B> {
     }"#,
         )
@@ -441,7 +447,7 @@
             "workspace",
             "two/src/lib.nr",
             Position { line: 9, character: 35 },
             r#"    one::subone::SubOneStruct
     some_field: i32"#,
         )
         .await;
@@ -453,7 +459,7 @@
             "workspace",
             "two/src/lib.nr",
             Position { line: 12, character: 17 },
             r#"    one::subone
     trait SomeTrait"#,
         )
         .await;

tooling/lsp/src/requests/inlay_hint.rs

@@ -41,14 +41,14 @@
         args.files.get_file_id(&path).map(|file_id| {
             let file = args.files.get_file(file_id).unwrap();
             let source = file.source();
             let (parsed_moduled, _errors) = noirc_frontend::parse_program(source);
 
             let span = range_to_byte_span(args.files, file_id, &params.range)
                 .map(|range| Span::from(range.start as u32..range.end as u32));
 
             let mut collector =
                 InlayHintCollector::new(args.files, file_id, args.interner, span, options);
             collector.collect_in_parsed_module(&parsed_moduled);
             collector.inlay_hints
         })
     });
@@ -104,7 +104,11 @@
             }
             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(_) => (),

tooling/lsp/src/requests/mod.rs

@@ -346,7 +346,7 @@
     location: noirc_errors::Location,
     files: &'a FileMap,
     interner: &'a NodeInterner,
     interners: &'a HashMap<String, NodeInterner>,
     root_crate_name: String,
     root_crate_dependencies: &'a Vec<Dependency>,
 }
@@ -364,8 +364,7 @@
             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")
     })?;
@@ -404,7 +403,7 @@
         location,
         files,
         interner,
         interners: &state.cached_definitions,
         root_crate_name: package.name.to_string(),
         root_crate_dependencies: &context.crate_graph[context.root_crate_id()].dependencies,
     }))
@@ -412,7 +411,7 @@
 pub(crate) fn find_all_references_in_workspace(
     location: noirc_errors::Location,
     interner: &NodeInterner,
     cached_interners: &HashMap<String, NodeInterner>,
     files: &FileMap,
     include_declaration: bool,
     include_self_type_name: bool,

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();