From 7bb735662c86e6533ac58f448ad748e34f02edb7 Mon Sep 17 00:00:00 2001 From: Koby Hall <102518238+kobyhallx@users.noreply.github.com> Date: Fri, 1 Dec 2023 22:56:00 +0100 Subject: [PATCH] feat(lsp): add goto definition for functions (#3656) # Description ## Problem\* Resolves feat: goto definition - function #3657 ## Summary\* LSP server now understands goto definition command issued by Client (ie. vscode). In case of function it will jump to it's definition. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: kevaundray --- compiler/fm/src/file_map.rs | 31 ++- compiler/fm/src/lib.rs | 4 + compiler/noirc_errors/src/position.rs | 14 ++ .../noirc_frontend/src/hir/def_map/mod.rs | 1 - compiler/noirc_frontend/src/hir/mod.rs | 8 + compiler/noirc_frontend/src/hir_def/traits.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 62 ++++- tooling/lsp/src/lib.rs | 5 +- tooling/lsp/src/requests/goto_definition.rs | 217 ++++++++++++++++++ tooling/lsp/src/requests/mod.rs | 6 +- tooling/lsp/src/types.rs | 10 +- 11 files changed, 340 insertions(+), 20 deletions(-) create mode 100644 tooling/lsp/src/requests/goto_definition.rs diff --git a/compiler/fm/src/file_map.rs b/compiler/fm/src/file_map.rs index 199723a28b5..0cbdc535e40 100644 --- a/compiler/fm/src/file_map.rs +++ b/compiler/fm/src/file_map.rs @@ -1,11 +1,12 @@ use codespan_reporting::files::{Error, Files, SimpleFile, SimpleFiles}; use serde::{Deserialize, Serialize}; +use std::collections::HashMap; use std::{ops::Range, path::PathBuf}; // XXX: File and FileMap serve as opaque types, so that the rest of the library does not need to import the dependency // or worry about when we change the dep -#[derive(Clone, Debug)] +#[derive(Clone, Debug, Eq, Hash, PartialEq)] pub struct PathString(PathBuf); impl std::fmt::Display for PathString { @@ -30,7 +31,10 @@ impl From<&PathBuf> for PathString { } } #[derive(Debug)] -pub struct FileMap(SimpleFiles); +pub struct FileMap { + files: SimpleFiles, + name_to_id: HashMap, +} // XXX: Note that we derive Default here due to ModuleOrigin requiring us to set a FileId #[derive( @@ -59,17 +63,22 @@ impl<'input> File<'input> { impl FileMap { pub fn add_file(&mut self, file_name: PathString, code: String) -> FileId { - let file_id = self.0.add(file_name, code); - FileId(file_id) + let file_id = FileId(self.files.add(file_name.clone(), code)); + self.name_to_id.insert(file_name, file_id); + file_id } + pub fn get_file(&self, file_id: FileId) -> Option { - self.0.get(file_id.0).map(File).ok() + self.files.get(file_id.0).map(File).ok() } -} + pub fn get_file_id(&self, file_name: &PathString) -> Option { + self.name_to_id.get(file_name).cloned() + } +} impl Default for FileMap { fn default() -> Self { - FileMap(SimpleFiles::new()) + FileMap { files: SimpleFiles::new(), name_to_id: HashMap::new() } } } @@ -79,18 +88,18 @@ impl<'a> Files<'a> for FileMap { type Source = &'a str; fn name(&self, file_id: Self::FileId) -> Result { - Ok(self.0.get(file_id.as_usize())?.name().clone()) + Ok(self.files.get(file_id.as_usize())?.name().clone()) } fn source(&'a self, file_id: Self::FileId) -> Result { - Ok(self.0.get(file_id.as_usize())?.source().as_ref()) + Ok(self.files.get(file_id.as_usize())?.source().as_ref()) } fn line_index(&self, file_id: Self::FileId, byte_index: usize) -> Result { - self.0.get(file_id.as_usize())?.line_index((), byte_index) + self.files.get(file_id.as_usize())?.line_index((), byte_index) } fn line_range(&self, file_id: Self::FileId, line_index: usize) -> Result, Error> { - self.0.get(file_id.as_usize())?.line_range((), line_index) + self.files.get(file_id.as_usize())?.line_range((), line_index) } } diff --git a/compiler/fm/src/lib.rs b/compiler/fm/src/lib.rs index 6e11489f7f2..ea964055759 100644 --- a/compiler/fm/src/lib.rs +++ b/compiler/fm/src/lib.rs @@ -109,6 +109,10 @@ impl FileManager { self.add_file(&candidate).ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string()) } + + pub fn name_to_id(&self, file_name: PathBuf) -> Option { + self.file_map.get_file_id(&PathString::from_path(file_name)) + } } /// Returns true if a module's child module's are expected to be in the same directory. diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs index 24b5c4d5ff0..007ec58ca27 100644 --- a/compiler/noirc_errors/src/position.rs +++ b/compiler/noirc_errors/src/position.rs @@ -85,6 +85,16 @@ impl Span { pub fn end(&self) -> u32 { self.0.end().into() } + + pub fn contains(&self, other: &Span) -> bool { + self.start() <= other.start() && self.end() >= other.end() + } + + pub fn is_smaller(&self, other: &Span) -> bool { + let self_distance = self.end() - self.start(); + let other_distance = other.end() - other.start(); + self_distance < other_distance + } } impl From for Range { @@ -133,4 +143,8 @@ impl Location { pub fn dummy() -> Self { Self { span: Span::single_char(0), file: FileId::dummy() } } + + pub fn contains(&self, other: &Location) -> bool { + self.file == other.file && self.span.contains(&other.span) + } } diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 5f38c80a5fe..026f407981d 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -173,7 +173,6 @@ impl CrateDefMap { }) }) } - /// Go through all modules in this crate, find all `contract ... { ... }` declarations, /// and collect them all into a Vec. pub fn get_all_contracts(&self, interner: &NodeInterner) -> Vec { diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index 154b695d552..3a435f302af 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -180,6 +180,14 @@ impl Context { .collect() } + /// Returns the [Location] of the definition of the given Ident found at [Span] of the given [FileId]. + /// Returns [None] when definition is not found. + pub fn get_definition_location_from(&self, location: Location) -> Option { + let interner = &self.def_interner; + + interner.find_location_index(location).and_then(|index| interner.resolve_location(index)) + } + /// Return a Vec of all `contract` declarations in the source code and the functions they contain pub fn get_all_contracts(&self, crate_id: &CrateId) -> Vec { self.def_map(crate_id) diff --git a/compiler/noirc_frontend/src/hir_def/traits.rs b/compiler/noirc_frontend/src/hir_def/traits.rs index 062f505c179..5f0bf49ca0f 100644 --- a/compiler/noirc_frontend/src/hir_def/traits.rs +++ b/compiler/noirc_frontend/src/hir_def/traits.rs @@ -60,7 +60,7 @@ pub struct Trait { pub self_type_typevar_id: TypeVariableId, pub self_type_typevar: TypeVariable, } - +#[derive(Debug)] pub struct TraitImpl { pub ident: Ident, pub typ: Type, diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index e66a6d57605..72bed209715 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -9,6 +9,7 @@ use crate::ast::Ident; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias}; use crate::hir::def_map::{LocalModuleId, ModuleId}; + use crate::hir_def::stmt::HirLetStatement; use crate::hir_def::traits::TraitImpl; use crate::hir_def::traits::{Trait, TraitConstraint}; @@ -36,6 +37,7 @@ type StructAttributes = Vec; /// each definition or struct, etc. Because it is used on the Hir, the NodeInterner is /// useful in passes where the Hir is used - name resolution, type checking, and /// monomorphization - and it is not useful afterward. +#[derive(Debug)] pub struct NodeInterner { nodes: Arena, func_meta: HashMap, @@ -156,7 +158,7 @@ pub enum TraitImplKind { /// /// Additionally, types can define specialized impls with methods of the same name /// as long as these specialized impls do not overlap. E.g. `impl Struct` and `impl Struct` -#[derive(Default)] +#[derive(Default, Debug)] pub struct Methods { direct: Vec, trait_impl_methods: Vec, @@ -165,6 +167,7 @@ pub struct Methods { /// All the information from a function that is filled out during definition collection rather than /// name resolution. As a result, if information about a function is needed during name resolution, /// this is the only place where it is safe to retrieve it (where all fields are guaranteed to be initialized). +#[derive(Debug, Clone)] pub struct FunctionModifiers { pub name: String, @@ -451,6 +454,30 @@ impl NodeInterner { self.id_to_location.insert(expr_id.into(), Location::new(span, file)); } + /// Scans the interner for the item which is located at that [Location] + /// + /// The [Location] may not necessarily point to the beginning of the item + /// so we check if the location's span is contained within the start or end + /// of each items [Span] + pub fn find_location_index(&self, location: Location) -> Option> { + let mut location_candidate: Option<(&Index, &Location)> = None; + + // Note: we can modify this in the future to not do a linear + // scan by storing a separate map of the spans or by sorting the locations. + for (index, interned_location) in self.id_to_location.iter() { + if interned_location.contains(&location) { + if let Some(current_location) = location_candidate { + if interned_location.span.is_smaller(¤t_location.1.span) { + location_candidate = Some((index, interned_location)); + } + } else { + location_candidate = Some((index, interned_location)); + } + } + } + location_candidate.map(|(index, _location)| *index) + } + /// Interns a HIR Function. pub fn push_fn(&mut self, func: HirFunction) -> FuncId { FuncId(self.nodes.insert(Node::Function(func))) @@ -1190,6 +1217,37 @@ impl NodeInterner { pub fn get_selected_impl_for_ident(&self, ident_id: ExprId) -> Option { self.selected_trait_implementations.get(&ident_id).cloned() } + + /// For a given [Index] we return [Location] to which we resolved to + /// We currently return None for features not yet implemented + /// TODO(#3659): LSP goto def should error when Ident at Location could not resolve + pub(crate) fn resolve_location(&self, index: impl Into) -> Option { + let node = self.nodes.get(index.into())?; + + match node { + Node::Function(func) => self.resolve_location(func.as_expr()), + Node::Expression(expression) => self.resolve_expression_location(expression), + _ => None, + } + } + + /// Resolves the [Location] of the definition for a given [HirExpression] + /// + /// Note: current the code returns None because some expressions are not yet implemented. + fn resolve_expression_location(&self, expression: &HirExpression) -> 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) + } + _ => None, + } + } + _ => None, + } + } } impl Methods { @@ -1242,7 +1300,7 @@ impl Methods { } /// These are the primitive type variants that we support adding methods to -#[derive(Copy, Clone, Hash, PartialEq, Eq)] +#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)] enum TypeMethodKey { /// Fields and integers share methods for ease of use. These methods may still /// accept only fields or integers, it is just that their names may not clash. diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index d8a992155dd..1474085a330 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -27,8 +27,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_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; @@ -76,6 +76,7 @@ impl NargoLspService { .request::(on_tests_request) .request::(on_test_run_request) .request::(on_profile_run_request) + .request::(on_goto_definition_request) .notification::(on_initialized) .notification::(on_did_change_configuration) .notification::(on_did_open_text_document) diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs new file mode 100644 index 00000000000..706c95d44a3 --- /dev/null +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -0,0 +1,217 @@ +use std::future::{self, Future}; + +use crate::{types::GotoDefinitionResult, LspState}; +use async_lsp::{ErrorCode, LanguageClient, ResponseError}; +use codespan_reporting::files::Error; +use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse, Location}; +use lsp_types::{Position, Url}; +use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; +use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; + +pub(crate) fn on_goto_definition_request( + state: &mut LspState, + params: GotoDefinitionParams, +) -> impl Future> { + let result = on_goto_definition_inner(state, params); + future::ready(result) +} + +fn on_goto_definition_inner( + state: &mut LspState, + params: GotoDefinitionParams, +) -> Result { + let root_path = state.root_path.as_deref().ok_or_else(|| { + ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find project root") + })?; + + let file_path = + params.text_document_position_params.text_document.uri.to_file_path().map_err(|_| { + ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") + })?; + + let toml_path = match find_package_manifest(root_path, &file_path) { + Ok(toml_path) => toml_path, + Err(err) => { + let _ = state.client.log_message(lsp_types::LogMessageParams { + typ: lsp_types::MessageType::WARNING, + message: err.to_string(), + }); + 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 definition_position = None; + + for package in &workspace { + let (mut context, crate_id) = + nargo::prepare_package(package, Box::new(crate::get_non_stdlib_asset)); + + // We ignore the warnings and errors produced by compilation while resolving the definition + let _ = noirc_driver::check_crate(&mut context, crate_id, false); + + let files = context.file_manager.as_file_map(); + let file_id = context.file_manager.name_to_id(file_path.clone()); + + if let Some(file_id) = file_id { + let byte_index = position_to_byte_index( + files, + file_id, + ¶ms.text_document_position_params.position, + ); + + if let Ok(byte_index) = byte_index { + let search_for_location = noirc_errors::Location { + file: file_id, + span: noirc_errors::Span::single_char(byte_index as u32), + }; + let found_location = context.get_definition_location_from(search_for_location); + + if let Some(found_location) = found_location { + let file_id = found_location.file; + definition_position = to_lsp_location(files, file_id, found_location.span); + } + } + } + } + + if let Some(definition_position) = definition_position { + let response: GotoDefinitionResponse = + GotoDefinitionResponse::from(definition_position).to_owned(); + Ok(Some(response)) + } else { + Ok(None) + } +} + +fn to_lsp_location<'a, F>( + files: &'a F, + file_id: F::FileId, + definition_span: noirc_errors::Span, +) -> Option +where + F: codespan_reporting::files::Files<'a> + ?Sized, +{ + let range = crate::byte_span_to_range(files, file_id, definition_span.into())?; + let file_name = files.name(file_id).ok()?; + + let path = file_name.to_string(); + let uri = Url::from_file_path(path).ok()?; + + Some(Location { uri, range }) +} + +pub(crate) fn position_to_byte_index<'a, F>( + files: &'a F, + file_id: F::FileId, + position: &Position, +) -> Result +where + F: codespan_reporting::files::Files<'a> + ?Sized, +{ + let source = files.source(file_id)?; + let source = source.as_ref(); + + let line_span = files.line_range(file_id, position.line as usize)?; + + let line_str = source.get(line_span.clone()); + + if let Some(line_str) = line_str { + let byte_offset = character_to_line_offset(line_str, position.character)?; + Ok(line_span.start + byte_offset) + } else { + Err(Error::InvalidCharBoundary { given: position.line as usize }) + } +} + +fn character_to_line_offset(line: &str, character: u32) -> Result { + let line_len = line.len(); + let mut character_offset = 0; + + let mut chars = line.chars(); + while let Some(ch) = chars.next() { + if character_offset == character { + let chars_off = chars.as_str().len(); + let ch_off = ch.len_utf8(); + + return Ok(line_len - chars_off - ch_off); + } + + character_offset += ch.len_utf16() as u32; + } + + // Handle positions after the last character on the line + if character_offset == character { + Ok(line_len) + } else { + Err(Error::ColumnTooLarge { given: character_offset as usize, max: line.len() }) + } +} + +#[cfg(test)] +mod goto_definition_tests { + + use async_lsp::ClientSocket; + use tokio::test; + + use crate::solver::MockBackend; + + use super::*; + + #[test] + async fn test_on_goto_definition() { + let client = ClientSocket::new_closed(); + let solver = MockBackend; + let mut state = LspState::new(&client, solver); + + let root_path = std::env::current_dir() + .unwrap() + .join("../../test_programs/execution_success/7_function") + .canonicalize() + .expect("Could not resolve root path"); + let noir_text_document = Url::from_file_path(root_path.join("src/main.nr").as_path()) + .expect("Could not convert text document path to URI"); + let root_uri = Some( + Url::from_file_path(root_path.as_path()).expect("Could not convert root path to URI"), + ); + + #[allow(deprecated)] + let initialize_params = lsp_types::InitializeParams { + process_id: Default::default(), + root_path: None, + root_uri, + initialization_options: None, + capabilities: Default::default(), + trace: Some(lsp_types::TraceValue::Verbose), + workspace_folders: None, + client_info: None, + locale: None, + }; + let _initialize_response = crate::requests::on_initialize(&mut state, initialize_params) + .await + .expect("Could not initialize LSP server"); + + let params = GotoDefinitionParams { + text_document_position_params: lsp_types::TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri: noir_text_document }, + position: Position { line: 95, character: 5 }, + }, + work_done_progress_params: Default::default(), + partial_result_params: Default::default(), + }; + + let response = on_goto_definition_request(&mut state, params) + .await + .expect("Could execute on_goto_definition_request"); + + assert!(&response.is_some()); + } +} diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index a319f2593a4..840678ddfc1 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -21,13 +21,14 @@ use crate::{ // and params passed in. mod code_lens_request; +mod goto_definition; mod profile_run; mod test_run; mod tests; pub(crate) use { - code_lens_request::on_code_lens_request, profile_run::on_profile_run_request, - test_run::on_test_run_request, tests::on_tests_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, }; pub(crate) fn on_initialize( @@ -55,6 +56,7 @@ pub(crate) fn on_initialize( code_lens_provider: Some(code_lens), document_formatting_provider: true, nargo: Some(nargo), + definition_provider: Some(lsp_types::OneOf::Left(true)), }, server_info: None, }) diff --git a/tooling/lsp/src/types.rs b/tooling/lsp/src/types.rs index ba964cba0c1..14c0264504d 100644 --- a/tooling/lsp/src/types.rs +++ b/tooling/lsp/src/types.rs @@ -1,4 +1,5 @@ use fm::FileId; +use lsp_types::{DefinitionOptions, OneOf}; use noirc_driver::DebugFile; use noirc_errors::{debug_info::OpCodesCount, Location}; use noirc_frontend::graph::CrateName; @@ -24,7 +25,9 @@ pub(crate) mod request { }; // Re-providing lsp_types that we don't need to override - pub(crate) use lsp_types::request::{CodeLensRequest as CodeLens, Formatting, Shutdown}; + pub(crate) use lsp_types::request::{ + CodeLensRequest as CodeLens, Formatting, GotoDefinition, Shutdown, + }; #[derive(Debug)] pub(crate) struct Initialize; @@ -108,6 +111,10 @@ pub(crate) struct ServerCapabilities { #[serde(skip_serializing_if = "Option::is_none")] pub(crate) text_document_sync: Option, + /// The server provides goto definition support. + #[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, @@ -215,3 +222,4 @@ pub(crate) struct NargoProfileRunResult { } pub(crate) type CodeLensResult = Option>; +pub(crate) type GotoDefinitionResult = Option;