From 5cdade3415abd0c3eaeeab7ca511242fab308aec Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 18:32:43 -0300 Subject: [PATCH 1/6] feat: LSP code actions to import or qualified unresolved paths --- tooling/lsp/src/lib.rs | 18 +- tooling/lsp/src/modules.rs | 131 ++++++++ tooling/lsp/src/requests/code_action.rs | 305 ++++++++++++++++++ .../src/requests/completion/auto_import.rs | 141 +------- tooling/lsp/src/requests/mod.rs | 27 +- tooling/lsp/src/types.rs | 10 +- tooling/lsp/src/visibility.rs | 13 + 7 files changed, 497 insertions(+), 148 deletions(-) create mode 100644 tooling/lsp/src/modules.rs create mode 100644 tooling/lsp/src/requests/code_action.rs create mode 100644 tooling/lsp/src/visibility.rs diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index c2885543844..4a764f4268b 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -22,8 +22,8 @@ use fm::{codespan_files as files, FileManager}; use fxhash::FxHashSet; use lsp_types::{ request::{ - Completion, DocumentSymbolRequest, HoverRequest, InlayHintRequest, PrepareRenameRequest, - References, Rename, SignatureHelpRequest, + CodeActionRequest, Completion, DocumentSymbolRequest, HoverRequest, InlayHintRequest, + PrepareRenameRequest, References, Rename, SignatureHelpRequest, }, CodeLens, }; @@ -51,21 +51,24 @@ use notifications::{ on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized, }; use requests::{ - on_code_lens_request, on_completion_request, on_document_symbol_request, on_formatting, - on_goto_declaration_request, on_goto_definition_request, on_goto_type_definition_request, - on_hover_request, on_initialize, on_inlay_hint_request, on_prepare_rename_request, - on_profile_run_request, on_references_request, on_rename_request, on_shutdown, - on_signature_help_request, on_test_run_request, on_tests_request, LspInitializationOptions, + on_code_action_request, on_code_lens_request, on_completion_request, + on_document_symbol_request, on_formatting, on_goto_declaration_request, + on_goto_definition_request, on_goto_type_definition_request, on_hover_request, on_initialize, + on_inlay_hint_request, on_prepare_rename_request, on_profile_run_request, + on_references_request, on_rename_request, on_shutdown, on_signature_help_request, + on_test_run_request, on_tests_request, LspInitializationOptions, }; use serde_json::Value as JsonValue; use thiserror::Error; use tower::Service; +mod modules; mod notifications; mod requests; mod solver; mod types; mod utils; +mod visibility; #[cfg(test)] mod test_utils; @@ -144,6 +147,7 @@ impl NargoLspService { .request::(on_inlay_hint_request) .request::(on_completion_request) .request::(on_signature_help_request) + .request::(on_code_action_request) .notification::(on_initialized) .notification::(on_did_change_configuration) .notification::(on_did_open_text_document) diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs new file mode 100644 index 00000000000..53ee2c82f86 --- /dev/null +++ b/tooling/lsp/src/modules.rs @@ -0,0 +1,131 @@ +use std::collections::BTreeMap; + +use noirc_frontend::{ + ast::ItemVisibility, + graph::CrateId, + hir::def_map::{CrateDefMap, ModuleId}, + macros_api::{ModuleDefId, NodeInterner}, + node_interner::ReferenceId, +}; + +use crate::visibility::is_visible; + +pub(crate) fn get_parent_module( + interner: &NodeInterner, + module_def_id: ModuleDefId, +) -> Option { + let reference_id = module_def_id_to_reference_id(module_def_id); + interner.reference_module(reference_id).copied() +} + +pub(crate) fn get_parent_module_id( + def_maps: &BTreeMap, + module_id: ModuleId, +) -> Option { + let crate_def_map = &def_maps[&module_id.krate]; + let module_data = &crate_def_map.modules()[module_id.local_id.0]; + module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent }) +} + +pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { + match module_def_id { + ModuleDefId::ModuleId(id) => ReferenceId::Module(id), + ModuleDefId::FunctionId(id) => ReferenceId::Function(id), + ModuleDefId::TypeId(id) => ReferenceId::Struct(id), + ModuleDefId::TypeAliasId(id) => ReferenceId::Alias(id), + ModuleDefId::TraitId(id) => ReferenceId::Trait(id), + ModuleDefId::GlobalId(id) => ReferenceId::Global(id), + } +} + +/// Returns the fully-qualified path of the given `ModuleDefId` relative to `current_module_id`: +/// - If `ModuleDefId` is a module, that module's path is returned +/// - Otherwise, that item's parent module's path is returned +pub(crate) fn module_full_path( + module_def_id: ModuleDefId, + visibility: ItemVisibility, + current_module_id: ModuleId, + current_module_parent_id: Option, + interner: &NodeInterner, +) -> Option { + let full_path; + if let ModuleDefId::ModuleId(module_id) = module_def_id { + if !is_visible(visibility, current_module_id, module_id) { + return None; + } + + full_path = + module_id_path(module_id, ¤t_module_id, current_module_parent_id, interner); + } else { + let Some(parent_module) = get_parent_module(interner, module_def_id) else { + return None; + }; + + if !is_visible(visibility, current_module_id, parent_module) { + return None; + } + + full_path = + module_id_path(parent_module, ¤t_module_id, current_module_parent_id, interner); + } + Some(full_path) +} + +/// Returns the path to reach an item inside `target_module_id` from inside `current_module_id`. +/// Returns a relative path if possible. +pub(crate) fn module_id_path( + target_module_id: ModuleId, + current_module_id: &ModuleId, + current_module_parent_id: Option, + interner: &NodeInterner, +) -> String { + if Some(target_module_id) == current_module_parent_id { + return "super".to_string(); + } + + let mut segments: Vec<&str> = Vec::new(); + let mut is_relative = false; + + if let Some(module_attributes) = interner.try_module_attributes(&target_module_id) { + segments.push(&module_attributes.name); + + let mut current_attributes = module_attributes; + loop { + let Some(parent_local_id) = current_attributes.parent else { + break; + }; + + let parent_module_id = + &ModuleId { krate: target_module_id.krate, local_id: parent_local_id }; + + if current_module_id == parent_module_id { + is_relative = true; + break; + } + + if current_module_parent_id == Some(*parent_module_id) { + segments.push("super"); + is_relative = true; + break; + } + + let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else { + break; + }; + + segments.push(&parent_attributes.name); + current_attributes = parent_attributes; + } + } + + if !is_relative { + // We don't record module attriubtes for the root module, + // so we handle that case separately + if let CrateId::Root(_) = target_module_id.krate { + segments.push("crate"); + } + } + + segments.reverse(); + segments.join("::") +} diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs new file mode 100644 index 00000000000..5a01ccc6c36 --- /dev/null +++ b/tooling/lsp/src/requests/code_action.rs @@ -0,0 +1,305 @@ +use std::{ + collections::{BTreeMap, HashMap}, + future::{self, Future}, +}; + +use async_lsp::ResponseError; +use fm::{FileId, FileMap, PathString}; +use lsp_types::{ + CodeAction, CodeActionKind, CodeActionOrCommand, CodeActionParams, CodeActionResponse, + Position, Range, TextDocumentPositionParams, TextEdit, Url, WorkspaceEdit, +}; +use noirc_errors::{Location, Span}; +use noirc_frontend::{ + ast::{Ident, Path, Visitor}, + graph::CrateId, + hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}, + macros_api::{ModuleDefId, NodeInterner}, + parser::{Item, ItemKind, ParsedSubModule}, + ParsedModule, +}; + +use crate::{ + byte_span_to_range, + modules::{get_parent_module_id, module_full_path}, + utils, LspState, +}; + +use super::{process_request, to_lsp_location}; + +pub(crate) fn on_code_action_request( + state: &mut LspState, + params: CodeActionParams, +) -> impl Future, ResponseError>> { + let uri = params.text_document.clone().uri; + let position = params.range.start; + let text_document_position_params = + TextDocumentPositionParams { text_document: params.text_document, position }; + + let result = process_request(state, text_document_position_params, |args| { + let path = PathString::from_path(uri.to_file_path().unwrap()); + args.files.get_file_id(&path).and_then(|file_id| { + utils::position_to_byte_index(args.files, file_id, &position).and_then(|byte_index| { + let file = args.files.get_file(file_id).unwrap(); + let source = file.source(); + let (parsed_module, _errors) = noirc_frontend::parse_program(source); + + let mut finder = CodeActionFinder::new( + uri, + args.files, + file_id, + source, + byte_index, + args.crate_id, + args.def_maps, + args.interner, + ); + finder.find(&parsed_module) + }) + }) + }); + future::ready(result) +} + +struct CodeActionFinder<'a> { + uri: Url, + files: &'a FileMap, + file: FileId, + lines: Vec<&'a str>, + byte_index: usize, + /// The module ID in scope. This might change as we traverse the AST + /// if we are analyzing something inside an inline module declaration. + module_id: ModuleId, + def_maps: &'a BTreeMap, + interner: &'a NodeInterner, + /// How many nested `mod` we are in deep + nesting: usize, + /// The line where an auto_import must be inserted + auto_import_line: usize, + code_actions: Vec, +} + +impl<'a> CodeActionFinder<'a> { + #[allow(clippy::too_many_arguments)] + fn new( + uri: Url, + files: &'a FileMap, + file: FileId, + source: &'a str, + byte_index: usize, + krate: CrateId, + def_maps: &'a BTreeMap, + interner: &'a NodeInterner, + ) -> Self { + // Find the module the current file belongs to + let def_map = &def_maps[&krate]; + let local_id = if let Some((module_index, _)) = + def_map.modules().iter().find(|(_, module_data)| module_data.location.file == file) + { + LocalModuleId(module_index) + } else { + def_map.root() + }; + let module_id = ModuleId { krate, local_id }; + Self { + uri, + files, + file, + lines: source.lines().collect(), + byte_index, + module_id, + def_maps, + interner, + nesting: 0, + auto_import_line: 0, + code_actions: vec![], + } + } + + fn find(&mut self, parsed_module: &ParsedModule) -> Option { + parsed_module.accept(self); + + if self.code_actions.is_empty() { + return None; + } + + let mut code_actions = std::mem::take(&mut self.code_actions); + code_actions.sort_by_key(|code_action| { + let CodeActionOrCommand::CodeAction(code_action) = code_action else { + panic!("We only gather code actions, never commands"); + }; + code_action.title.clone() + }); + + Some(code_actions) + } + + fn push_import_code_action(&mut self, full_path: &str) { + let line = self.auto_import_line as u32; + let character = (self.nesting * 4) as u32; + let indent = " ".repeat(self.nesting * 4); + let mut newlines = "\n"; + + // If the line we are inserting into is not an empty line, insert an extra line to make some room + if let Some(line_text) = self.lines.get(line as usize) { + if !line_text.trim().is_empty() { + newlines = "\n\n"; + } + } + + let text_edit = TextEdit { + range: Range { start: Position { line, character }, end: Position { line, character } }, + new_text: format!("use {};{}{}", full_path, newlines, indent), + }; + + let mut changes = HashMap::new(); + changes.insert(self.uri.clone(), vec![text_edit]); + + let workspace_edit = WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }; + + let title = format!("Import {}", full_path); + let code_action = new_quick_fix(title, workspace_edit); + self.code_actions.push(CodeActionOrCommand::CodeAction(code_action)); + } + + fn push_qualify_code_action(&mut self, ident: &Ident, prefix: &str, full_path: &str) { + let Some(range) = byte_span_to_range( + self.files, + self.file, + ident.span().start() as usize..ident.span().start() as usize, + ) else { + return; + }; + + let text_edit = TextEdit { range, new_text: format!("{}::", prefix) }; + let mut changes = HashMap::new(); + changes.insert(self.uri.clone(), vec![text_edit]); + + let workspace_edit = WorkspaceEdit { + changes: Some(changes), + document_changes: None, + change_annotations: None, + }; + + let title = format!("Qualify as {}", full_path); + let code_action = new_quick_fix(title, workspace_edit); + self.code_actions.push(CodeActionOrCommand::CodeAction(code_action)); + } + + fn includes_span(&self, span: Span) -> bool { + span.start() as usize <= self.byte_index && self.byte_index <= span.end() as usize + } +} + +impl<'a> Visitor for CodeActionFinder<'a> { + fn visit_item(&mut self, item: &Item) -> bool { + if let ItemKind::Import(..) = &item.kind { + if let Some(lsp_location) = to_lsp_location(self.files, self.file, item.span) { + self.auto_import_line = (lsp_location.range.end.line + 1) as usize; + } + } + + self.includes_span(item.span) + } + + fn visit_parsed_submodule(&mut self, parsed_sub_module: &ParsedSubModule, span: Span) -> bool { + // Switch `self.module_id` to the submodule + let previous_module_id = self.module_id; + + let def_map = &self.def_maps[&self.module_id.krate]; + let Some(module_data) = def_map.modules().get(self.module_id.local_id.0) else { + return false; + }; + if let Some(child_module) = module_data.children.get(&parsed_sub_module.name) { + self.module_id = ModuleId { krate: self.module_id.krate, local_id: *child_module }; + } + + let old_auto_import_line = self.auto_import_line; + self.nesting += 1; + + if let Some(lsp_location) = to_lsp_location(self.files, self.file, span) { + self.auto_import_line = (lsp_location.range.start.line + 1) as usize; + } + + parsed_sub_module.contents.accept(self); + + // Restore the old module before continuing + self.module_id = previous_module_id; + self.nesting -= 1; + self.auto_import_line = old_auto_import_line; + + false + } + + fn visit_path(&mut self, path: &Path) { + if path.segments.len() != 1 { + return; + } + + let ident = &path.segments[0].ident; + if !self.includes_span(ident.span()) { + return; + } + + let location = Location::new(ident.span(), self.file); + if self.interner.find_referenced(location).is_some() { + return; + } + + let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); + + // The Path doesn't resolve to anything so it means it's an error and maybe we + // can suggest an import or to fully-qualify the path. + for (name, entries) in self.interner.get_auto_import_names() { + if name != &ident.0.contents { + continue; + } + + for (module_def_id, visibility) in entries { + let Some(module_full_path) = module_full_path( + *module_def_id, + *visibility, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + continue; + }; + + let full_path = if let ModuleDefId::ModuleId(..) = module_def_id { + module_full_path.clone() + } else { + format!("{}::{}", module_full_path, name) + }; + + let qualify_prefix = if let ModuleDefId::ModuleId(..) = module_def_id { + let mut segments: Vec<_> = module_full_path.split("::").collect(); + segments.pop(); + segments.join("::") + } else { + module_full_path + }; + + self.push_import_code_action(&full_path); + self.push_qualify_code_action(ident, &qualify_prefix, &full_path); + } + } + } +} + +fn new_quick_fix(title: String, workspace_edit: WorkspaceEdit) -> CodeAction { + CodeAction { + title, + kind: Some(CodeActionKind::QUICKFIX), + diagnostics: None, + edit: Some(workspace_edit), + command: None, + is_preferred: None, + disabled: None, + data: None, + } +} diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index 32f777dec34..bf3ff7f0291 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,13 +1,7 @@ -use std::collections::BTreeMap; - use lsp_types::{Position, Range, TextEdit}; -use noirc_frontend::{ - ast::ItemVisibility, - graph::CrateId, - hir::def_map::{CrateDefMap, ModuleId}, - macros_api::{ModuleDefId, NodeInterner}, - node_interner::ReferenceId, -}; +use noirc_frontend::macros_api::ModuleDefId; + +use crate::modules::{get_parent_module_id, module_full_path}; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, @@ -45,41 +39,15 @@ impl<'a> NodeFinder<'a> { continue; }; - let module_full_path; - if let ModuleDefId::ModuleId(module_id) = module_def_id { - module_full_path = module_id_path( - *module_id, - &self.module_id, - current_module_parent_id, - self.interner, - ); - } else { - let Some(parent_module) = get_parent_module(self.interner, *module_def_id) - else { - continue; - }; - - match *visibility { - ItemVisibility::Public => (), - ItemVisibility::Private => { - // Technically this can't be reached because we don't record private items for auto-import, - // but this is here for completeness. - continue; - } - ItemVisibility::PublicCrate => { - if self.module_id.krate != parent_module.krate { - continue; - } - } - } - - module_full_path = module_id_path( - parent_module, - &self.module_id, - current_module_parent_id, - self.interner, - ); - } + let Some(module_full_path) = module_full_path( + *module_def_id, + *visibility, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + continue; + }; let full_path = if let ModuleDefId::ModuleId(..) = module_def_id { module_full_path @@ -119,88 +87,3 @@ impl<'a> NodeFinder<'a> { } } } - -fn get_parent_module(interner: &NodeInterner, module_def_id: ModuleDefId) -> Option { - let reference_id = module_def_id_to_reference_id(module_def_id); - interner.reference_module(reference_id).copied() -} - -fn get_parent_module_id( - def_maps: &BTreeMap, - module_id: ModuleId, -) -> Option { - let crate_def_map = &def_maps[&module_id.krate]; - let module_data = &crate_def_map.modules()[module_id.local_id.0]; - module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent }) -} - -fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { - match module_def_id { - ModuleDefId::ModuleId(id) => ReferenceId::Module(id), - ModuleDefId::FunctionId(id) => ReferenceId::Function(id), - ModuleDefId::TypeId(id) => ReferenceId::Struct(id), - ModuleDefId::TypeAliasId(id) => ReferenceId::Alias(id), - ModuleDefId::TraitId(id) => ReferenceId::Trait(id), - ModuleDefId::GlobalId(id) => ReferenceId::Global(id), - } -} - -/// Returns the path to reach an item inside `target_module_id` from inside `current_module_id`. -/// Returns a relative path if possible. -fn module_id_path( - target_module_id: ModuleId, - current_module_id: &ModuleId, - current_module_parent_id: Option, - interner: &NodeInterner, -) -> String { - if Some(target_module_id) == current_module_parent_id { - return "super".to_string(); - } - - let mut segments: Vec<&str> = Vec::new(); - let mut is_relative = false; - - if let Some(module_attributes) = interner.try_module_attributes(&target_module_id) { - segments.push(&module_attributes.name); - - let mut current_attributes = module_attributes; - - loop { - let Some(parent_local_id) = current_attributes.parent else { - break; - }; - - let parent_module_id = - &ModuleId { krate: target_module_id.krate, local_id: parent_local_id }; - - if current_module_id == parent_module_id { - is_relative = true; - break; - } - - if current_module_parent_id == Some(*parent_module_id) { - segments.push("super"); - is_relative = true; - break; - } - - let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else { - break; - }; - - segments.push(&parent_attributes.name); - current_attributes = parent_attributes; - } - } - - if !is_relative { - // We don't record module attributes for the root module, - // so we handle that case separately - if let CrateId::Root(_) = target_module_id.krate { - segments.push("crate"); - } - } - - segments.reverse(); - segments.join("::") -} diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index e88c7f11465..5bd9959fd63 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -10,7 +10,7 @@ use crate::{ use async_lsp::{ErrorCode, ResponseError}; use fm::{codespan_files::Error, FileMap, PathString}; use lsp_types::{ - DeclarationCapability, Location, Position, TextDocumentPositionParams, + CodeActionKind, DeclarationCapability, Location, Position, TextDocumentPositionParams, TextDocumentSyncCapability, TextDocumentSyncKind, TypeDefinitionProviderCapability, Url, WorkDoneProgressOptions, }; @@ -36,6 +36,7 @@ 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_action; mod code_lens_request; mod completion; mod document_symbol; @@ -51,14 +52,15 @@ mod test_run; mod tests; pub(crate) use { - code_lens_request::collect_lenses_for_package, code_lens_request::on_code_lens_request, - completion::on_completion_request, document_symbol::on_document_symbol_request, - goto_declaration::on_goto_declaration_request, goto_definition::on_goto_definition_request, - goto_definition::on_goto_type_definition_request, hover::on_hover_request, - inlay_hint::on_inlay_hint_request, profile_run::on_profile_run_request, - references::on_references_request, rename::on_prepare_rename_request, - rename::on_rename_request, signature_help::on_signature_help_request, - test_run::on_test_run_request, tests::on_tests_request, + code_action::on_code_action_request, code_lens_request::collect_lenses_for_package, + code_lens_request::on_code_lens_request, completion::on_completion_request, + document_symbol::on_document_symbol_request, goto_declaration::on_goto_declaration_request, + goto_definition::on_goto_definition_request, goto_definition::on_goto_type_definition_request, + hover::on_hover_request, inlay_hint::on_inlay_hint_request, + profile_run::on_profile_run_request, references::on_references_request, + rename::on_prepare_rename_request, rename::on_rename_request, + signature_help::on_signature_help_request, test_run::on_test_run_request, + tests::on_tests_request, }; /// LSP client will send initialization request after the server has started. @@ -252,6 +254,13 @@ pub(crate) fn on_initialize( }, }, )), + code_action_provider: Some(lsp_types::OneOf::Right(lsp_types::CodeActionOptions { + code_action_kinds: Some(vec![CodeActionKind::QUICKFIX]), + work_done_progress_options: WorkDoneProgressOptions { + work_done_progress: None, + }, + resolve_provider: None, + })), }, server_info: None, }) diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index 3ac1f35e18e..043c50a87fd 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -1,8 +1,8 @@ use fm::FileId; use lsp_types::{ - CompletionOptions, DeclarationCapability, DefinitionOptions, DocumentSymbolOptions, - HoverOptions, InlayHintOptions, OneOf, ReferencesOptions, RenameOptions, SignatureHelpOptions, - TypeDefinitionProviderCapability, + CodeActionOptions, CompletionOptions, DeclarationCapability, DefinitionOptions, + DocumentSymbolOptions, HoverOptions, InlayHintOptions, OneOf, ReferencesOptions, RenameOptions, + SignatureHelpOptions, TypeDefinitionProviderCapability, }; use noirc_driver::DebugFile; use noirc_errors::{debug_info::OpCodesCount, Location}; @@ -165,6 +165,10 @@ pub(crate) struct ServerCapabilities { /// The server provides signature help support. #[serde(skip_serializing_if = "Option::is_none")] pub(crate) signature_help_provider: Option>, + + /// The server provides code action support. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) code_action_provider: Option>, } #[derive(Debug, PartialEq, Clone, Default, Deserialize, Serialize)] diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs new file mode 100644 index 00000000000..aad8b47fbbe --- /dev/null +++ b/tooling/lsp/src/visibility.rs @@ -0,0 +1,13 @@ +use noirc_frontend::{ast::ItemVisibility, hir::def_map::ModuleId}; + +pub(super) fn is_visible( + visibility: ItemVisibility, + current_module: ModuleId, + target_module: ModuleId, +) -> bool { + match visibility { + ItemVisibility::Public => true, + ItemVisibility::Private => false, + ItemVisibility::PublicCrate => current_module.krate == target_module.krate, + } +} From de9e2270bb97269a7040c196bf7e1ef3957cfcc3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 30 Aug 2024 17:37:28 -0300 Subject: [PATCH 2/6] Don't crash on "go to definition" when id is dummy --- .../noirc_frontend/src/resolve_locations.rs | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/compiler/noirc_frontend/src/resolve_locations.rs b/compiler/noirc_frontend/src/resolve_locations.rs index 61ff6baf919..b9e86bf0ef7 100644 --- a/compiler/noirc_frontend/src/resolve_locations.rs +++ b/compiler/noirc_frontend/src/resolve_locations.rs @@ -4,7 +4,7 @@ use noirc_errors::Location; use crate::hir_def::expr::HirExpression; use crate::hir_def::types::Type; -use crate::node_interner::{DefinitionKind, Node, NodeInterner}; +use crate::node_interner::{DefinitionId, DefinitionKind, Node, NodeInterner}; impl NodeInterner { /// Scans the interner for the item which is located at that [Location] @@ -108,14 +108,18 @@ impl NodeInterner { ) -> Option { match expression { HirExpression::Ident(ident, _) => { - let definition_info = self.definition(ident.id); - match definition_info.kind { - DefinitionKind::Function(func_id) => { - Some(self.function_meta(&func_id).location) + if ident.id != DefinitionId::dummy_id() { + let definition_info = self.definition(ident.id); + match definition_info.kind { + DefinitionKind::Function(func_id) => { + Some(self.function_meta(&func_id).location) + } + DefinitionKind::Local(_local_id) => Some(definition_info.location), + DefinitionKind::Global(_global_id) => Some(definition_info.location), + _ => None, } - DefinitionKind::Local(_local_id) => Some(definition_info.location), - DefinitionKind::Global(_global_id) => Some(definition_info.location), - _ => None, + } else { + None } } HirExpression::Constructor(expr) => { From c5eaacff4338f8853c6aff0b81a95a1cbd84cffa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 30 Aug 2024 17:53:40 -0300 Subject: [PATCH 3/6] Add tests --- tooling/lsp/src/requests/code_action.rs | 3 + tooling/lsp/src/requests/code_action/tests.rs | 158 ++++++++++++++++++ 2 files changed, 161 insertions(+) create mode 100644 tooling/lsp/src/requests/code_action/tests.rs diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index 5a01ccc6c36..dd48aba6447 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -27,6 +27,9 @@ use crate::{ use super::{process_request, to_lsp_location}; +#[cfg(test)] +mod tests; + pub(crate) fn on_code_action_request( state: &mut LspState, params: CodeActionParams, diff --git a/tooling/lsp/src/requests/code_action/tests.rs b/tooling/lsp/src/requests/code_action/tests.rs new file mode 100644 index 00000000000..91d211fc922 --- /dev/null +++ b/tooling/lsp/src/requests/code_action/tests.rs @@ -0,0 +1,158 @@ +use crate::{notifications::on_did_open_text_document, test_utils}; + +use lsp_types::{ + CodeActionContext, CodeActionOrCommand, CodeActionParams, CodeActionResponse, + DidOpenTextDocumentParams, PartialResultParams, Position, Range, TextDocumentIdentifier, + TextDocumentItem, WorkDoneProgressParams, +}; +use tokio::test; + +use super::on_code_action_request; + +async fn get_code_action(src: &str) -> CodeActionResponse { + let (mut state, noir_text_document) = test_utils::init_lsp_server("document_symbol").await; + + let (line, column) = src + .lines() + .enumerate() + .find_map(|(line_index, line)| line.find(">|<").map(|char_index| (line_index, char_index))) + .expect("Expected to find one >|< in the source code"); + + let src = src.replace(">|<", ""); + + on_did_open_text_document( + &mut state, + DidOpenTextDocumentParams { + text_document: TextDocumentItem { + uri: noir_text_document.clone(), + language_id: "noir".to_string(), + version: 0, + text: src.to_string(), + }, + }, + ); + + let position = Position { line: line as u32, character: column as u32 }; + + on_code_action_request( + &mut state, + CodeActionParams { + text_document: TextDocumentIdentifier { uri: noir_text_document }, + range: Range { start: position, end: position }, + context: CodeActionContext { diagnostics: Vec::new(), only: None, trigger_kind: None }, + work_done_progress_params: WorkDoneProgressParams { work_done_token: None }, + partial_result_params: PartialResultParams { partial_result_token: None }, + }, + ) + .await + .expect("Could not execute on_code_action_request") + .unwrap() +} + +#[test] +async fn test_code_action_for_unresolved_path_for_struct() { + let src = r#" + mod foo { + mod bar { + struct SomeTypeInBar {} + } + } + + fn foo(x: SomeType>|| Date: Sun, 1 Sep 2024 10:29:52 -0300 Subject: [PATCH 4/6] Simplify --- tooling/lsp/src/requests/code_action.rs | 46 ++++++++++--------------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/tooling/lsp/src/requests/code_action.rs b/tooling/lsp/src/requests/code_action.rs index dd48aba6447..fd8c42a3b87 100644 --- a/tooling/lsp/src/requests/code_action.rs +++ b/tooling/lsp/src/requests/code_action.rs @@ -150,22 +150,13 @@ impl<'a> CodeActionFinder<'a> { } } + let title = format!("Import {}", full_path); let text_edit = TextEdit { range: Range { start: Position { line, character }, end: Position { line, character } }, new_text: format!("use {};{}{}", full_path, newlines, indent), }; - let mut changes = HashMap::new(); - changes.insert(self.uri.clone(), vec![text_edit]); - - let workspace_edit = WorkspaceEdit { - changes: Some(changes), - document_changes: None, - change_annotations: None, - }; - - let title = format!("Import {}", full_path); - let code_action = new_quick_fix(title, workspace_edit); + let code_action = self.new_quick_fix(title, text_edit); self.code_actions.push(CodeActionOrCommand::CodeAction(code_action)); } @@ -178,7 +169,14 @@ impl<'a> CodeActionFinder<'a> { return; }; + let title = format!("Qualify as {}", full_path); let text_edit = TextEdit { range, new_text: format!("{}::", prefix) }; + + let code_action = self.new_quick_fix(title, text_edit); + self.code_actions.push(CodeActionOrCommand::CodeAction(code_action)); + } + + fn new_quick_fix(&self, title: String, text_edit: TextEdit) -> CodeAction { let mut changes = HashMap::new(); changes.insert(self.uri.clone(), vec![text_edit]); @@ -188,9 +186,16 @@ impl<'a> CodeActionFinder<'a> { change_annotations: None, }; - let title = format!("Qualify as {}", full_path); - let code_action = new_quick_fix(title, workspace_edit); - self.code_actions.push(CodeActionOrCommand::CodeAction(code_action)); + CodeAction { + title, + kind: Some(CodeActionKind::QUICKFIX), + diagnostics: None, + edit: Some(workspace_edit), + command: None, + is_preferred: None, + disabled: None, + data: None, + } } fn includes_span(&self, span: Span) -> bool { @@ -293,16 +298,3 @@ impl<'a> Visitor for CodeActionFinder<'a> { } } } - -fn new_quick_fix(title: String, workspace_edit: WorkspaceEdit) -> CodeAction { - CodeAction { - title, - kind: Some(CodeActionKind::QUICKFIX), - diagnostics: None, - edit: Some(workspace_edit), - command: None, - is_preferred: None, - disabled: None, - data: None, - } -} From 23d0cca034c9ee9813c0408081b976a65e8acb88 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 1 Sep 2024 16:12:42 -0300 Subject: [PATCH 5/6] Better tests --- tooling/lsp/src/requests/code_action/tests.rs | 169 +++++++++++------- 1 file changed, 106 insertions(+), 63 deletions(-) diff --git a/tooling/lsp/src/requests/code_action/tests.rs b/tooling/lsp/src/requests/code_action/tests.rs index 91d211fc922..21b5fab96c6 100644 --- a/tooling/lsp/src/requests/code_action/tests.rs +++ b/tooling/lsp/src/requests/code_action/tests.rs @@ -3,7 +3,7 @@ use crate::{notifications::on_did_open_text_document, test_utils}; use lsp_types::{ CodeActionContext, CodeActionOrCommand, CodeActionParams, CodeActionResponse, DidOpenTextDocumentParams, PartialResultParams, Position, Range, TextDocumentIdentifier, - TextDocumentItem, WorkDoneProgressParams, + TextDocumentItem, TextEdit, WorkDoneProgressParams, }; use tokio::test; @@ -49,8 +49,49 @@ async fn get_code_action(src: &str) -> CodeActionResponse { .unwrap() } +async fn assert_code_action(title: &str, src: &str, expected: &str) { + let actions = get_code_action(src).await; + let action = actions + .iter() + .filter_map(|action| { + if let CodeActionOrCommand::CodeAction(action) = action { + if action.title == title { + Some(action) + } else { + None + } + } else { + None + } + }) + .next() + .expect("Couldn't find an action with the given title"); + + let workspace_edit = action.edit.as_ref().unwrap(); + let text_edits = workspace_edit.changes.as_ref().unwrap().iter().next().unwrap().1; + assert_eq!(text_edits.len(), 1); + + let result = apply_text_edit(&src.replace(">|<", ""), &text_edits[0]); + assert_eq!(result, expected); +} + +fn apply_text_edit(src: &str, text_edit: &TextEdit) -> String { + let mut lines: Vec<_> = src.lines().collect(); + assert_eq!(text_edit.range.start.line, text_edit.range.end.line); + + let mut line = lines[text_edit.range.start.line as usize].to_string(); + line.replace_range( + text_edit.range.start.character as usize..text_edit.range.end.character as usize, + &text_edit.new_text, + ); + lines[text_edit.range.start.line as usize] = &line; + lines.join("\n") +} + #[test] -async fn test_code_action_for_unresolved_path_for_struct() { +async fn test_qualify_code_action_for_struct() { + let title = "Qualify as foo::bar::SomeTypeInBar"; + let src = r#" mod foo { mod bar { @@ -61,49 +102,48 @@ async fn test_code_action_for_unresolved_path_for_struct() { fn foo(x: SomeType>||| Date: Mon, 2 Sep 2024 08:39:12 -0300 Subject: [PATCH 6/6] Update tooling/lsp/src/modules.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- tooling/lsp/src/modules.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index 53ee2c82f86..54074dbd94c 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -119,7 +119,7 @@ pub(crate) fn module_id_path( } if !is_relative { - // We don't record module attriubtes for the root module, + // We don't record module attributes for the root module, // so we handle that case separately if let CrateId::Root(_) = target_module_id.krate { segments.push("crate");