From 5617aa051a88c35275f4a105e4103f1212c94232 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 10:46:52 -0300 Subject: [PATCH 01/29] fix: lsp struct reference in return position --- compiler/noirc_frontend/src/elaborator/types.rs | 13 +++++-------- tooling/lsp/src/requests/rename.rs | 2 +- tooling/lsp/src/test_utils.rs | 5 ++--- tooling/lsp/test_programs/rename_struct/src/main.nr | 4 +++- 4 files changed, 11 insertions(+), 13 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index d27e150d649..b334b8fd02f 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -146,13 +146,17 @@ impl<'context> Elaborator<'context> { Resolved(id) => self.interner.get_quoted_type(id).clone(), }; - if let Type::Struct(_, _) = resolved_type { + if let Type::Struct(ref struct_type, _) = resolved_type { if let Some(unresolved_span) = typ.span { // Record the location of the type reference self.interner.push_type_ref_location( resolved_type.clone(), Location::new(unresolved_span, self.file), ); + + let referenced = DependencyId::Struct(struct_type.borrow().id); + let reference = DependencyId::Variable(Location::new(unresolved_span, self.file)); + self.interner.add_reference(referenced, reference); } } @@ -244,8 +248,6 @@ impl<'context> Elaborator<'context> { return Type::Alias(alias, args); } - let last_segment = path.last_segment(); - match self.lookup_struct_or_error(path) { Some(struct_type) => { if self.resolving_ids.contains(&struct_type.borrow().id) { @@ -283,11 +285,6 @@ impl<'context> Elaborator<'context> { self.interner.add_type_dependency(current_item, dependency_id); } - let referenced = DependencyId::Struct(struct_type.borrow().id); - let reference = - DependencyId::Variable(Location::new(last_segment.span(), self.file)); - self.interner.add_reference(referenced, reference); - Type::Struct(struct_type, args) } None => Type::Error, diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 66d5095f797..6dd3cc3fda7 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -102,7 +102,7 @@ mod rename_tests { let changes = response.changes.expect("Expected to find rename changes"); let mut changes: Vec = changes.values().flatten().map(|edit| edit.range).collect(); - changes.sort_by_key(|range| range.start.line); + changes.sort_by_key(|range| (range.start.line, range.start.character)); assert_eq!(changes, ranges); } } diff --git a/tooling/lsp/src/test_utils.rs b/tooling/lsp/src/test_utils.rs index fd1a9090965..c0505107842 100644 --- a/tooling/lsp/src/test_utils.rs +++ b/tooling/lsp/src/test_utils.rs @@ -46,9 +46,8 @@ pub(crate) fn search_in_file(filename: &str, search_string: &str) -> Vec file_lines .iter() .enumerate() - .filter_map(|(line_num, line)| { - // Note: this only finds the first instance of `search_string` on this line. - line.find(search_string).map(|index| { + .flat_map(|(line_num, line)| { + line.match_indices(search_string).map(move |(index, _)| { let start = Position { line: line_num as u32, character: index as u32 }; let end = Position { line: line_num as u32, diff --git a/tooling/lsp/test_programs/rename_struct/src/main.nr b/tooling/lsp/test_programs/rename_struct/src/main.nr index 96cccb4d72a..6b0cddb21b4 100644 --- a/tooling/lsp/test_programs/rename_struct/src/main.nr +++ b/tooling/lsp/test_programs/rename_struct/src/main.nr @@ -15,4 +15,6 @@ fn main(x: Field) -> pub Field { x } -fn foo(foo: Foo) {} +fn foo(foo: Foo) -> Foo { + foo +} From e1ebd0629ee43175255ba2c6a9da30ef6482b0c2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 14:04:41 -0300 Subject: [PATCH 02/29] fix: lsp struct reference in Path --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- .../noirc_frontend/src/elaborator/scope.rs | 18 ++++++- .../src/hir/def_collector/dc_crate.rs | 3 +- .../src/hir/resolution/import.rs | 52 ++++++++++++++++--- .../src/hir/resolution/path_resolver.rs | 10 +++- .../src/hir/resolution/resolver.rs | 4 +- .../src/hir/resolution/traits.rs | 2 +- .../noirc_frontend/src/hir/type_check/mod.rs | 5 +- compiler/noirc_frontend/src/locations.rs | 7 ++- compiler/noirc_frontend/src/node_interner.rs | 8 +++ .../test_programs/rename_struct/src/main.nr | 5 ++ 11 files changed, 97 insertions(+), 19 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index d7087a5ab07..7c780838163 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -534,7 +534,7 @@ impl<'context> Elaborator<'context> { fn resolve_trait_by_path(&mut self, path: Path) -> Option { let path_resolver = StandardPathResolver::new(self.module_id()); - let error = match path_resolver.resolve(self.def_maps, path.clone()) { + let error = match path_resolver.resolve(self.def_maps, path.clone(), &mut None) { Ok(PathResolution { module_def_id: ModuleDefId::TraitId(trait_id), error }) => { if let Some(error) = error { self.push_err(error); diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 9fd3be0a354..b1d707ebecf 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -1,4 +1,4 @@ -use noirc_errors::Spanned; +use noirc_errors::{Location, Spanned}; use crate::ast::ERROR_IDENT; use crate::hir::def_map::{LocalModuleId, ModuleId}; @@ -6,6 +6,7 @@ use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::resolution::resolver::SELF_TYPE_NAME; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::macros_api::Ident; +use crate::node_interner::DependencyId; use crate::{ hir::{ def_map::{ModuleDefId, TryFromModuleDefId}, @@ -44,7 +45,20 @@ impl<'context> Elaborator<'context> { pub(super) fn resolve_path(&mut self, path: Path) -> Result { let resolver = StandardPathResolver::new(self.module_id()); - let path_resolution = resolver.resolve(self.def_maps, path)?; + let path_resolution; + + if self.interner.track_references { + let mut dependencies: Vec = Vec::new(); + path_resolution = + resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut dependencies))?; + + for (referenced, ident) in dependencies.iter().zip(path.segments) { + let reference = DependencyId::Variable(Location::new(ident.span(), self.file)); + self.interner.add_reference(*referenced, reference); + } + } else { + path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; + } if let Some(error) = path_resolution.error { self.push_err(error); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index cd670167d2c..b0d42997104 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -330,7 +330,7 @@ impl DefCollector { // Resolve unresolved imports collected from the crate, one by one. for collected_import in std::mem::take(&mut def_collector.imports) { let module_id = collected_import.module_id; - match resolve_import(crate_id, &collected_import, &context.def_maps) { + match resolve_import(crate_id, &collected_import, &context.def_maps, &mut None) { Ok(resolved_import) => { if let Some(error) = resolved_import.error { errors.push(( @@ -524,6 +524,7 @@ fn inject_prelude( &context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, path, + &mut None, ) { assert!(error.is_none(), "Tried to add private item to prelude"); let module_id = module_def_id.as_module().expect("std::prelude should be a module"); diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index d73130411e4..2a38bee9353 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -3,6 +3,7 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; +use crate::node_interner::DependencyId; use std::collections::BTreeMap; use crate::ast::{Ident, ItemVisibility, Path, PathKind}; @@ -80,13 +81,14 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, + dependencies: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; let NamespaceResolution { module_id: resolved_module, namespace: resolved_namespace, mut error, - } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps)?; + } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps, dependencies)?; let name = resolve_path_name(import_directive); @@ -124,6 +126,7 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, + dependencies: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; let def_map = &def_maps[&crate_id]; @@ -131,7 +134,13 @@ fn resolve_path_to_ns( match import_directive.path.kind { crate::ast::PathKind::Crate => { // Resolve from the root of the crate - resolve_path_from_crate_root(crate_id, importing_crate, import_path, def_maps) + resolve_path_from_crate_root( + crate_id, + importing_crate, + import_path, + def_maps, + dependencies, + ) } crate::ast::PathKind::Plain => { // There is a possibility that the import path is empty @@ -143,6 +152,7 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + dependencies, ); } @@ -151,7 +161,13 @@ fn resolve_path_to_ns( let first_segment = import_path.first().expect("ice: could not fetch first segment"); if current_mod.find_name(first_segment).is_none() { // Resolve externally when first segment is unresolved - return resolve_external_dep(def_map, import_directive, def_maps, importing_crate); + return resolve_external_dep( + def_map, + import_directive, + def_maps, + dependencies, + importing_crate, + ); } resolve_name_in_module( @@ -160,11 +176,12 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, + dependencies, ) } crate::ast::PathKind::Dep => { - resolve_external_dep(def_map, import_directive, def_maps, importing_crate) + resolve_external_dep(def_map, import_directive, def_maps, dependencies, importing_crate) } } } @@ -175,6 +192,7 @@ fn resolve_path_from_crate_root( import_path: &[Ident], def_maps: &BTreeMap, + dependencies: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { resolve_name_in_module( crate_id, @@ -182,6 +200,7 @@ fn resolve_path_from_crate_root( import_path, def_maps[&crate_id].root, def_maps, + dependencies, ) } @@ -191,6 +210,7 @@ fn resolve_name_in_module( import_path: &[Ident], starting_mod: LocalModuleId, def_maps: &BTreeMap, + dependencies: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; @@ -221,12 +241,27 @@ fn resolve_name_in_module( // In the type namespace, only Mod can be used in a path. current_mod_id = match typ { - ModuleDefId::ModuleId(id) => id, + ModuleDefId::ModuleId(id) => { + if let Some(dependencies) = dependencies { + dependencies.push(DependencyId::Module(id)); + } + id + } ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), // TODO: If impls are ever implemented, types can be used in a path - ModuleDefId::TypeId(id) => id.module_id(), + ModuleDefId::TypeId(id) => { + if let Some(dependencies) = dependencies { + dependencies.push(DependencyId::Struct(id)); + } + id.module_id() + } ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), - ModuleDefId::TraitId(id) => id.0, + ModuleDefId::TraitId(id) => { + if let Some(dependencies) = dependencies { + dependencies.push(DependencyId::Trait(id)); + } + id.0 + } ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), }; @@ -270,6 +305,7 @@ fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, def_maps: &BTreeMap, + dependencies: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep @@ -299,7 +335,7 @@ fn resolve_external_dep( is_prelude: false, }; - resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps) + resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, dependencies) } // Issue an error if the given private function is being called from a non-child module, or diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index e423e10b712..f4702d2b1a5 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,5 +1,6 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; use crate::ast::Path; +use crate::node_interner::DependencyId; use std::collections::BTreeMap; use crate::graph::CrateId; @@ -7,10 +8,13 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; pub trait PathResolver { /// Resolve the given path returning the resolved ModuleDefId. + /// If `dependencies` is `Some`, a `DependencyId` for each segment in `path` + /// will be pushed. fn resolve( &self, def_maps: &BTreeMap, path: Path, + dependencies: &mut Option<&mut Vec>, ) -> PathResolutionResult; fn local_module_id(&self) -> LocalModuleId; @@ -34,8 +38,9 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, + dependencies: &mut Option<&mut Vec>, ) -> PathResolutionResult { - resolve_path(def_maps, self.module_id, path) + resolve_path(def_maps, self.module_id, path, dependencies) } fn local_module_id(&self) -> LocalModuleId { @@ -53,11 +58,12 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, + dependencies: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that let import = ImportDirective { module_id: module_id.local_id, path, alias: None, is_prelude: false }; - let resolved_import = resolve_import(module_id.krate, &import, def_maps)?; + let resolved_import = resolve_import(module_id.krate, &import, def_maps, dependencies)?; let namespace = resolved_import.resolved_namespace; let id = diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index c97de6d3e05..364d694462b 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -765,7 +765,7 @@ impl<'a> Resolver<'a> { } // If we cannot find a local generic of the same name, try to look up a global - match self.path_resolver.resolve(self.def_maps, path.clone()) { + match self.path_resolver.resolve(self.def_maps, path.clone(), &mut None) { Ok(PathResolution { module_def_id: ModuleDefId::GlobalId(id), error }) => { if let Some(current_item) = self.current_item { self.interner.add_global_dependency(current_item, id); @@ -2017,7 +2017,7 @@ impl<'a> Resolver<'a> { } fn resolve_path(&mut self, path: Path) -> Result { - let path_resolution = self.path_resolver.resolve(self.def_maps, path)?; + let path_resolution = self.path_resolver.resolve(self.def_maps, path, &mut None)?; if let Some(error) = path_resolution.error { self.push_err(error.into()); diff --git a/compiler/noirc_frontend/src/hir/resolution/traits.rs b/compiler/noirc_frontend/src/hir/resolution/traits.rs index e674a48e779..6781c2833c4 100644 --- a/compiler/noirc_frontend/src/hir/resolution/traits.rs +++ b/compiler/noirc_frontend/src/hir/resolution/traits.rs @@ -388,7 +388,7 @@ pub(crate) fn resolve_trait_by_path( ) -> Result<(TraitId, Option), DefCollectorErrorKind> { let path_resolver = StandardPathResolver::new(module); - match path_resolver.resolve(def_maps, path.clone()) { + match path_resolver.resolve(def_maps, path.clone(), &mut None) { Ok(PathResolution { module_def_id: ModuleDefId::TraitId(trait_id), error }) => { Ok((trait_id, error)) } diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 3f1678f4dba..992d29c3ae1 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -461,7 +461,9 @@ pub mod test { function::{FuncMeta, HirFunction}, stmt::HirStatement, }; - use crate::node_interner::{DefinitionKind, FuncId, NodeInterner, TraitId, TraitMethodId}; + use crate::node_interner::{ + DefinitionKind, DependencyId, FuncId, NodeInterner, TraitId, TraitMethodId, + }; use crate::{ hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleDefId}, @@ -692,6 +694,7 @@ pub mod test { &self, _def_maps: &BTreeMap, path: Path, + _dependencies: &mut Option<&mut Vec>, ) -> PathResolutionResult { // Not here that foo::bar and hello::foo::bar would fetch the same thing let name = path.segments.last().unwrap(); diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index e38e8e2fcc9..3ffdca01cd8 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -31,8 +31,10 @@ impl LocationIndices { impl NodeInterner { pub fn dependency_location(&self, dependency: DependencyId) -> Location { match dependency { + DependencyId::Module(_) => todo!(), DependencyId::Function(id) => self.function_modifiers(&id).name_location, DependencyId::Struct(id) => self.struct_location(&id), + DependencyId::Trait(_) => todo!(), DependencyId::Global(id) => self.get_global(id).location, DependencyId::Alias(id) => self.get_type_alias(id).borrow().location, DependencyId::Variable(location) => location, @@ -99,7 +101,10 @@ impl NodeInterner { let reference_node = self.reference_graph[node_index]; let found_locations: Vec = match reference_node { - DependencyId::Alias(_) | DependencyId::Global(_) => todo!(), + DependencyId::Alias(_) + | DependencyId::Global(_) + | DependencyId::Module(_) + | DependencyId::Trait(_) => todo!(), DependencyId::Function(_) | DependencyId::Struct(_) => { self.find_all_references_for_index(node_index, include_reference) } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 7c30ccf5b8f..dda4750e17f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -228,7 +228,9 @@ pub struct NodeInterner { /// ``` #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub enum DependencyId { + Module(ModuleId), Struct(StructId), + Trait(TraitId), Global(GlobalId), Function(FuncId), Alias(TypeAliasId), @@ -1800,6 +1802,8 @@ impl NodeInterner { DependencyId::Variable(loc) => unreachable!( "Variable used at location {loc:?} caught in a dependency cycle" ), + // These two are never put in the dependency graph + DependencyId::Module(_) | DependencyId::Trait(_) => (), } } } @@ -1824,6 +1828,10 @@ impl NodeInterner { DependencyId::Variable(loc) => { unreachable!("Variable used at location {loc:?} caught in a dependency cycle") } + // These two are never put in the dependency graph + DependencyId::Module(_) | DependencyId::Trait(_) => { + unreachable!("Module or trait dependency shouldn't exist in the dependency graph") + } }; let mut cycle = index_to_string(scc[start_index]).to_string(); diff --git a/tooling/lsp/test_programs/rename_struct/src/main.nr b/tooling/lsp/test_programs/rename_struct/src/main.nr index 6b0cddb21b4..93a0779cf3b 100644 --- a/tooling/lsp/test_programs/rename_struct/src/main.nr +++ b/tooling/lsp/test_programs/rename_struct/src/main.nr @@ -3,6 +3,10 @@ mod foo { struct Foo { field: Field, } + + impl Foo { + fn foo() {} + } } } @@ -12,6 +16,7 @@ fn main(x: Field) -> pub Field { let foo1 = Foo { field: 1 }; let foo2 = Foo { field: 2 }; let Foo { field } = foo1; + Foo::foo(); x } From c3e9c304f63a4c0779e5e10e4a7bd5093bfd8df2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 14:38:57 -0300 Subject: [PATCH 03/29] Introduce `ReferenceId` as an enum exclusive to the reference graph --- .../src/elaborator/expressions.rs | 6 +-- .../noirc_frontend/src/elaborator/patterns.rs | 14 +++--- .../noirc_frontend/src/elaborator/scope.rs | 6 +-- .../noirc_frontend/src/elaborator/types.rs | 7 +-- .../src/hir/def_collector/dc_crate.rs | 10 ++-- .../src/hir/def_collector/dc_mod.rs | 4 +- .../src/hir/resolution/import.rs | 48 ++++++++++--------- .../src/hir/resolution/path_resolver.rs | 16 +++---- .../noirc_frontend/src/hir/type_check/mod.rs | 4 +- compiler/noirc_frontend/src/locations.rs | 48 +++++++++---------- compiler/noirc_frontend/src/node_interner.rs | 23 +++++---- 11 files changed, 98 insertions(+), 88 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 791cb8913d6..e013cf7b4f1 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -27,7 +27,7 @@ use crate::{ HirLiteral, HirStatement, Ident, IndexExpression, Literal, MemberAccessExpression, MethodCallExpression, PrefixExpression, }, - node_interner::{DefinitionKind, DependencyId, ExprId, FuncId}, + node_interner::{DefinitionKind, ExprId, FuncId, ReferenceId}, token::Tokens, Kind, QuotedType, Shared, StructType, Type, }; @@ -432,8 +432,8 @@ impl<'context> Elaborator<'context> { struct_generics, }); - let referenced = DependencyId::Struct(struct_type.borrow().id); - let reference = DependencyId::Variable(Location::new(span, self.file)); + let referenced = ReferenceId::Struct(struct_type.borrow().id); + let reference = ReferenceId::Variable(Location::new(span, self.file)); self.interner.add_reference(referenced, reference); (expr, Type::Struct(struct_type, generics)) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 61d30a915fc..e3c854d615d 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -15,7 +15,7 @@ use crate::{ stmt::HirPattern, }, macros_api::{HirExpression, Ident, Path, Pattern}, - node_interner::{DefinitionId, DefinitionKind, DependencyId, ExprId, GlobalId, TraitImplKind}, + node_interner::{DefinitionId, DefinitionKind, ExprId, GlobalId, ReferenceId, TraitImplKind}, Shared, StructType, Type, TypeBindings, }; @@ -199,8 +199,8 @@ impl<'context> Elaborator<'context> { new_definitions, ); - let referenced = DependencyId::Struct(struct_type.borrow().id); - let reference = DependencyId::Variable(Location::new(name_span, self.file)); + let referenced = ReferenceId::Struct(struct_type.borrow().id); + let reference = ReferenceId::Variable(Location::new(name_span, self.file)); self.interner.add_reference(referenced, reference); HirPattern::Struct(expected_type, fields, location) @@ -446,8 +446,8 @@ impl<'context> Elaborator<'context> { self.interner.add_function_dependency(current_item, func_id); } - let variable = DependencyId::Variable(hir_ident.location); - let function = DependencyId::Function(func_id); + let variable = ReferenceId::Variable(hir_ident.location); + let function = ReferenceId::Function(func_id); self.interner.add_reference(function, variable); } DefinitionKind::Global(global_id) => { @@ -458,8 +458,8 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, global_id); } - let variable = DependencyId::Variable(hir_ident.location); - let global = DependencyId::Global(global_id); + let variable = ReferenceId::Variable(hir_ident.location); + let global = ReferenceId::Global(global_id); self.interner.add_reference(global, variable); } DefinitionKind::GenericType(_) => { diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index b1d707ebecf..c13ea5a2e40 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -6,7 +6,7 @@ use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::resolution::resolver::SELF_TYPE_NAME; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::macros_api::Ident; -use crate::node_interner::DependencyId; +use crate::node_interner::ReferenceId; use crate::{ hir::{ def_map::{ModuleDefId, TryFromModuleDefId}, @@ -48,12 +48,12 @@ impl<'context> Elaborator<'context> { let path_resolution; if self.interner.track_references { - let mut dependencies: Vec = Vec::new(); + let mut dependencies: Vec = Vec::new(); path_resolution = resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut dependencies))?; for (referenced, ident) in dependencies.iter().zip(path.segments) { - let reference = DependencyId::Variable(Location::new(ident.span(), self.file)); + let reference = ReferenceId::Variable(Location::new(ident.span(), self.file)); self.interner.add_reference(*referenced, reference); } } else { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index b334b8fd02f..b6212abb4a2 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -31,7 +31,8 @@ use crate::{ UnaryOp, UnresolvedType, UnresolvedTypeData, }, node_interner::{ - DefinitionKind, DependencyId, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId, + DefinitionKind, DependencyId, ExprId, GlobalId, ReferenceId, TraitId, TraitImplKind, + TraitMethodId, }, Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeVariable, TypeVariableKind, }; @@ -154,8 +155,8 @@ impl<'context> Elaborator<'context> { Location::new(unresolved_span, self.file), ); - let referenced = DependencyId::Struct(struct_type.borrow().id); - let reference = DependencyId::Variable(Location::new(unresolved_span, self.file)); + let referenced = ReferenceId::Struct(struct_type.borrow().id); + let reference = ReferenceId::Variable(Location::new(unresolved_span, self.file)); self.interner.add_reference(referenced, reference); } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index b0d42997104..668dbf33d52 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -20,7 +20,7 @@ use crate::hir::Context; use crate::macros_api::{MacroError, MacroProcessor}; use crate::node_interner::{ - DependencyId, FuncId, GlobalId, NodeInterner, StructId, TraitId, TraitImplId, TypeAliasId, + FuncId, GlobalId, NodeInterner, ReferenceId, StructId, TraitId, TraitImplId, TypeAliasId, }; use crate::ast::{ @@ -491,12 +491,12 @@ fn add_import_reference( match def_id { crate::macros_api::ModuleDefId::FunctionId(func_id) => { - let variable = DependencyId::Variable(Location::new(name.span(), file_id)); - interner.add_reference(DependencyId::Function(func_id), variable); + let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + interner.add_reference(ReferenceId::Function(func_id), variable); } crate::macros_api::ModuleDefId::TypeId(struct_id) => { - let variable = DependencyId::Variable(Location::new(name.span(), file_id)); - interner.add_reference(DependencyId::Struct(struct_id), variable); + let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + interner.add_reference(ReferenceId::Struct(struct_id), variable); } _ => (), } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index e908f5c1545..9077f13abe1 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -14,7 +14,7 @@ use crate::ast::{ TypeImpl, }; use crate::macros_api::NodeInterner; -use crate::node_interner::DependencyId; +use crate::node_interner::ReferenceId; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -314,7 +314,7 @@ impl<'a> ModCollector<'a> { self.def_collector.items.types.insert(id, unresolved); context.def_interner.add_struct_location(id, name_location); - context.def_interner.add_definition_location(DependencyId::Struct(id)); + context.def_interner.add_definition_location(ReferenceId::Struct(id)); } definition_errors } diff --git a/compiler/noirc_frontend/src/hir/resolution/import.rs b/compiler/noirc_frontend/src/hir/resolution/import.rs index 2a38bee9353..710c12a91bf 100644 --- a/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -3,7 +3,7 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; -use crate::node_interner::DependencyId; +use crate::node_interner::ReferenceId; use std::collections::BTreeMap; use crate::ast::{Ident, ItemVisibility, Path, PathKind}; @@ -81,14 +81,14 @@ pub fn resolve_import( crate_id: CrateId, import_directive: &ImportDirective, def_maps: &BTreeMap, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; let NamespaceResolution { module_id: resolved_module, namespace: resolved_namespace, mut error, - } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps, dependencies)?; + } = resolve_path_to_ns(import_directive, crate_id, crate_id, def_maps, path_references)?; let name = resolve_path_name(import_directive); @@ -126,7 +126,7 @@ fn resolve_path_to_ns( crate_id: CrateId, importing_crate: CrateId, def_maps: &BTreeMap, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let import_path = &import_directive.path.segments; let def_map = &def_maps[&crate_id]; @@ -139,7 +139,7 @@ fn resolve_path_to_ns( importing_crate, import_path, def_maps, - dependencies, + path_references, ) } crate::ast::PathKind::Plain => { @@ -152,7 +152,7 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, - dependencies, + path_references, ); } @@ -165,7 +165,7 @@ fn resolve_path_to_ns( def_map, import_directive, def_maps, - dependencies, + path_references, importing_crate, ); } @@ -176,13 +176,17 @@ fn resolve_path_to_ns( import_path, import_directive.module_id, def_maps, - dependencies, + path_references, ) } - crate::ast::PathKind::Dep => { - resolve_external_dep(def_map, import_directive, def_maps, dependencies, importing_crate) - } + crate::ast::PathKind::Dep => resolve_external_dep( + def_map, + import_directive, + def_maps, + path_references, + importing_crate, + ), } } @@ -192,7 +196,7 @@ fn resolve_path_from_crate_root( import_path: &[Ident], def_maps: &BTreeMap, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { resolve_name_in_module( crate_id, @@ -200,7 +204,7 @@ fn resolve_path_from_crate_root( import_path, def_maps[&crate_id].root, def_maps, - dependencies, + path_references, ) } @@ -210,7 +214,7 @@ fn resolve_name_in_module( import_path: &[Ident], starting_mod: LocalModuleId, def_maps: &BTreeMap, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> NamespaceResolutionResult { let def_map = &def_maps[&krate]; let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; @@ -242,23 +246,23 @@ fn resolve_name_in_module( // In the type namespace, only Mod can be used in a path. current_mod_id = match typ { ModuleDefId::ModuleId(id) => { - if let Some(dependencies) = dependencies { - dependencies.push(DependencyId::Module(id)); + if let Some(path_references) = path_references { + path_references.push(ReferenceId::Module(id)); } id } ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), // TODO: If impls are ever implemented, types can be used in a path ModuleDefId::TypeId(id) => { - if let Some(dependencies) = dependencies { - dependencies.push(DependencyId::Struct(id)); + if let Some(path_references) = path_references { + path_references.push(ReferenceId::Struct(id)); } id.module_id() } ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), ModuleDefId::TraitId(id) => { - if let Some(dependencies) = dependencies { - dependencies.push(DependencyId::Trait(id)); + if let Some(path_references) = path_references { + path_references.push(ReferenceId::Trait(id)); } id.0 } @@ -305,7 +309,7 @@ fn resolve_external_dep( current_def_map: &CrateDefMap, directive: &ImportDirective, def_maps: &BTreeMap, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, importing_crate: CrateId, ) -> NamespaceResolutionResult { // Use extern_prelude to get the dep @@ -335,7 +339,7 @@ fn resolve_external_dep( is_prelude: false, }; - resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, dependencies) + resolve_path_to_ns(&dep_directive, dep_module.krate, importing_crate, def_maps, path_references) } // Issue an error if the given private function is being called from a non-child module, or diff --git a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index f4702d2b1a5..c3dc76b635f 100644 --- a/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -1,6 +1,6 @@ use super::import::{resolve_import, ImportDirective, PathResolution, PathResolutionResult}; use crate::ast::Path; -use crate::node_interner::DependencyId; +use crate::node_interner::ReferenceId; use std::collections::BTreeMap; use crate::graph::CrateId; @@ -8,13 +8,13 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; pub trait PathResolver { /// Resolve the given path returning the resolved ModuleDefId. - /// If `dependencies` is `Some`, a `DependencyId` for each segment in `path` - /// will be pushed. + /// If `path_references` is `Some`, a `ReferenceId` for each segment in `path` + /// will be resolved and pushed. fn resolve( &self, def_maps: &BTreeMap, path: Path, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult; fn local_module_id(&self) -> LocalModuleId; @@ -38,9 +38,9 @@ impl PathResolver for StandardPathResolver { &self, def_maps: &BTreeMap, path: Path, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { - resolve_path(def_maps, self.module_id, path, dependencies) + resolve_path(def_maps, self.module_id, path, path_references) } fn local_module_id(&self) -> LocalModuleId { @@ -58,12 +58,12 @@ pub fn resolve_path( def_maps: &BTreeMap, module_id: ModuleId, path: Path, - dependencies: &mut Option<&mut Vec>, + path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // lets package up the path into an ImportDirective and resolve it using that let import = ImportDirective { module_id: module_id.local_id, path, alias: None, is_prelude: false }; - let resolved_import = resolve_import(module_id.krate, &import, def_maps, dependencies)?; + let resolved_import = resolve_import(module_id.krate, &import, def_maps, path_references)?; let namespace = resolved_import.resolved_namespace; let id = diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 992d29c3ae1..a9a51b636a8 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -462,7 +462,7 @@ pub mod test { stmt::HirStatement, }; use crate::node_interner::{ - DefinitionKind, DependencyId, FuncId, NodeInterner, TraitId, TraitMethodId, + DefinitionKind, FuncId, NodeInterner, ReferenceId, TraitId, TraitMethodId, }; use crate::{ hir::{ @@ -694,7 +694,7 @@ pub mod test { &self, _def_maps: &BTreeMap, path: Path, - _dependencies: &mut Option<&mut Vec>, + _path_references: &mut Option<&mut Vec>, ) -> PathResolutionResult { // Not here that foo::bar and hello::foo::bar would fetch the same thing let name = path.segments.last().unwrap(); diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 3ffdca01cd8..79927b03041 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -3,7 +3,7 @@ use noirc_errors::Location; use rangemap::RangeMap; use rustc_hash::FxHashMap; -use crate::{macros_api::NodeInterner, node_interner::DependencyId}; +use crate::{macros_api::NodeInterner, node_interner::ReferenceId}; use petgraph::prelude::NodeIndex as PetGraphIndex; #[derive(Debug, Default)] @@ -29,43 +29,43 @@ impl LocationIndices { } impl NodeInterner { - pub fn dependency_location(&self, dependency: DependencyId) -> Location { - match dependency { - DependencyId::Module(_) => todo!(), - DependencyId::Function(id) => self.function_modifiers(&id).name_location, - DependencyId::Struct(id) => self.struct_location(&id), - DependencyId::Trait(_) => todo!(), - DependencyId::Global(id) => self.get_global(id).location, - DependencyId::Alias(id) => self.get_type_alias(id).borrow().location, - DependencyId::Variable(location) => location, + pub fn reference_location(&self, reference: ReferenceId) -> Location { + match reference { + ReferenceId::Module(_) => todo!(), + ReferenceId::Function(id) => self.function_modifiers(&id).name_location, + ReferenceId::Struct(id) => self.struct_location(&id), + ReferenceId::Trait(_) => todo!(), + ReferenceId::Global(id) => self.get_global(id).location, + ReferenceId::Alias(id) => self.get_type_alias(id).borrow().location, + ReferenceId::Variable(location) => location, } } - pub(crate) fn add_reference(&mut self, referenced: DependencyId, reference: DependencyId) { + pub(crate) fn add_reference(&mut self, referenced: ReferenceId, reference: ReferenceId) { if !self.track_references { return; } let referenced_index = self.get_or_insert_reference(referenced); - let reference_location = self.dependency_location(reference); + let reference_location = self.reference_location(reference); let reference_index = self.reference_graph.add_node(reference); self.reference_graph.add_edge(reference_index, referenced_index, ()); self.location_indices.add_location(reference_location, reference_index); } - pub(crate) fn add_definition_location(&mut self, referenced: DependencyId) { + pub(crate) fn add_definition_location(&mut self, referenced: ReferenceId) { if !self.track_references { return; } let referenced_index = self.get_or_insert_reference(referenced); - let referenced_location = self.dependency_location(referenced); + let referenced_location = self.reference_location(referenced); self.location_indices.add_location(referenced_location, referenced_index); } #[tracing::instrument(skip(self), ret)] - pub(crate) fn get_or_insert_reference(&mut self, id: DependencyId) -> PetGraphIndex { + pub(crate) fn get_or_insert_reference(&mut self, id: ReferenceId) -> PetGraphIndex { if let Some(index) = self.reference_graph_indices.get(&id) { return *index; } @@ -80,7 +80,7 @@ impl NodeInterner { self.location_indices .get_node_from_location(reference_location) .and_then(|node_index| self.referenced_index(node_index)) - .map(|node_index| self.dependency_location(self.reference_graph[node_index])) + .map(|node_index| self.reference_location(self.reference_graph[node_index])) } // Is the given location known to this interner? @@ -101,15 +101,15 @@ impl NodeInterner { let reference_node = self.reference_graph[node_index]; let found_locations: Vec = match reference_node { - DependencyId::Alias(_) - | DependencyId::Global(_) - | DependencyId::Module(_) - | DependencyId::Trait(_) => todo!(), - DependencyId::Function(_) | DependencyId::Struct(_) => { + ReferenceId::Alias(_) + | ReferenceId::Global(_) + | ReferenceId::Module(_) + | ReferenceId::Trait(_) => todo!(), + ReferenceId::Function(_) | ReferenceId::Struct(_) => { self.find_all_references_for_index(node_index, include_reference) } - DependencyId::Variable(_) => { + ReferenceId::Variable(_) => { let referenced_node_index = self.referenced_index(node_index)?; self.find_all_references_for_index(referenced_node_index, include_reference) } @@ -127,14 +127,14 @@ impl NodeInterner { let id = self.reference_graph[referenced_node_index]; let mut edit_locations = Vec::new(); if include_reference { - edit_locations.push(self.dependency_location(id)); + edit_locations.push(self.reference_location(id)); } self.reference_graph .neighbors_directed(referenced_node_index, petgraph::Direction::Incoming) .for_each(|reference_node_index| { let id = self.reference_graph[reference_node_index]; - edit_locations.push(self.dependency_location(id)); + edit_locations.push(self.reference_location(id)); }); edit_locations } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index dda4750e17f..95a6a56ac60 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -207,10 +207,10 @@ pub struct NodeInterner { /// // | | /// // +------+ /// ``` - pub(crate) reference_graph: DiGraph, + pub(crate) reference_graph: DiGraph, /// Tracks the index of the references in the graph - pub(crate) reference_graph_indices: HashMap, + pub(crate) reference_graph_indices: HashMap, /// Store the location of the references in the graph pub(crate) location_indices: LocationIndices, @@ -228,6 +228,17 @@ pub struct NodeInterner { /// ``` #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] pub enum DependencyId { + Struct(StructId), + Global(GlobalId), + Function(FuncId), + Alias(TypeAliasId), + Variable(Location), +} + +/// A reference to a module, struct, trait, etc., mainly used by the LSP code +/// to keep track of how symbols reference each other. +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +pub enum ReferenceId { Module(ModuleId), Struct(StructId), Trait(TraitId), @@ -860,7 +871,7 @@ impl NodeInterner { // This needs to be done after pushing the definition since it will reference the // location that was stored - self.add_definition_location(DependencyId::Function(id)); + self.add_definition_location(ReferenceId::Function(id)); definition_id } @@ -1802,8 +1813,6 @@ impl NodeInterner { DependencyId::Variable(loc) => unreachable!( "Variable used at location {loc:?} caught in a dependency cycle" ), - // These two are never put in the dependency graph - DependencyId::Module(_) | DependencyId::Trait(_) => (), } } } @@ -1828,10 +1837,6 @@ impl NodeInterner { DependencyId::Variable(loc) => { unreachable!("Variable used at location {loc:?} caught in a dependency cycle") } - // These two are never put in the dependency graph - DependencyId::Module(_) | DependencyId::Trait(_) => { - unreachable!("Module or trait dependency shouldn't exist in the dependency graph") - } }; let mut cycle = index_to_string(scc[start_index]).to_string(); From 668bc86500b0685547e8ae841194842a55ee69b2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 15:58:37 -0300 Subject: [PATCH 04/29] feat: lsp "go to definition" for module in call path --- .../src/hir/def_collector/dc_mod.rs | 22 +++++-- compiler/noirc_frontend/src/locations.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 12 ++++ tooling/lsp/src/requests/goto_definition.rs | 61 +++++++++++++------ .../test_programs/go_to_definition/src/bar.nr | 1 + .../go_to_definition/src/main.nr | 3 + 6 files changed, 78 insertions(+), 23 deletions(-) create mode 100644 tooling/lsp/test_programs/go_to_definition/src/bar.nr diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 9077f13abe1..729e5d1b8ac 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -3,7 +3,7 @@ use std::vec; use acvm::{AcirField, FieldElement}; use fm::{FileId, FileManager, FILE_EXTENSION}; -use noirc_errors::Location; +use noirc_errors::{Location, Span}; use num_bigint::BigUint; use num_traits::Num; use rustc_hash::FxHashMap as HashMap; @@ -283,7 +283,7 @@ impl<'a> ModCollector<'a> { ); // Create the corresponding module for the struct namespace - let id = match self.push_child_module(&name, self.file_id, false, false) { + let id = match self.push_child_module(context, &name, self.file_id, false, false) { Ok(local_id) => context.def_interner.new_struct( &unresolved, resolved_generics, @@ -377,7 +377,8 @@ impl<'a> ModCollector<'a> { let name = trait_definition.name.clone(); // Create the corresponding module for the trait namespace - let trait_id = match self.push_child_module(&name, self.file_id, false, false) { + let trait_id = match self.push_child_module(context, &name, self.file_id, false, false) + { Ok(local_id) => TraitId(ModuleId { krate, local_id }), Err(error) => { errors.push((error.into(), self.file_id)); @@ -535,7 +536,13 @@ impl<'a> ModCollector<'a> { ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for submodule in submodules { - match self.push_child_module(&submodule.name, file_id, true, submodule.is_contract) { + match self.push_child_module( + context, + &submodule.name, + file_id, + true, + submodule.is_contract, + ) { Ok(child) => { errors.extend(collect_defs( self.def_collector, @@ -620,7 +627,7 @@ impl<'a> ModCollector<'a> { ); // Add module into def collector and get a ModuleId - match self.push_child_module(&mod_decl.ident, child_file_id, true, false) { + match self.push_child_module(context, &mod_decl.ident, child_file_id, true, false) { Ok(child_mod_id) => { errors.extend(collect_defs( self.def_collector, @@ -643,6 +650,7 @@ impl<'a> ModCollector<'a> { /// On error this returns None and pushes to `errors` fn push_child_module( &mut self, + context: &mut Context, mod_name: &Ident, file_id: FileId, add_to_parent_scope: bool, @@ -681,6 +689,10 @@ impl<'a> ModCollector<'a> { }; return Err(err); } + + context + .def_interner + .add_module_location(mod_id, Location::new(Span::empty(0), file_id)); } Ok(LocalModuleId(module_id)) diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 79927b03041..addc2b0305e 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -31,7 +31,7 @@ impl LocationIndices { impl NodeInterner { pub fn reference_location(&self, reference: ReferenceId) -> Location { match reference { - ReferenceId::Module(_) => todo!(), + ReferenceId::Module(id) => self.module_location(&id), ReferenceId::Function(id) => self.function_modifiers(&id).name_location, ReferenceId::Struct(id) => self.struct_location(&id), ReferenceId::Trait(_) => todo!(), diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 95a6a56ac60..6fcc6a5e3af 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -65,6 +65,9 @@ pub struct NodeInterner { // Contains the source module each function was defined in function_modules: HashMap, + // The location of each module + module_locations: HashMap, + // The location of each struct name struct_name_locations: HashMap, @@ -540,6 +543,7 @@ impl Default for NodeInterner { function_definition_ids: HashMap::new(), function_modifiers: HashMap::new(), function_modules: HashMap::new(), + module_locations: HashMap::new(), struct_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), @@ -980,6 +984,14 @@ impl NodeInterner { self.struct_name_locations[struct_id] } + pub fn add_module_location(&mut self, module_id: ModuleId, location: Location) { + self.module_locations.insert(module_id, location); + } + + pub fn module_location(&self, module_id: &ModuleId) -> Location { + self.module_locations[module_id] + } + pub fn global_attributes(&self, global_id: &GlobalId) -> &[SecondaryAttribute] { &self.global_attributes[global_id] } diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index d52211c08f9..7653476f3a1 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -51,7 +51,7 @@ mod goto_definition_tests { use super::*; - async fn expect_goto(directory: &str, name: &str, definition_index: usize) { + async fn expect_goto_for_all_references(directory: &str, name: &str, definition_index: usize) { let (mut state, noir_text_document) = test_utils::init_lsp_server(directory).await; let ranges = search_in_file(noir_text_document.path(), name); @@ -90,21 +90,20 @@ mod goto_definition_tests { } } - #[test] - async fn goto_from_function_location_to_declaration() { - expect_goto("go_to_definition", "another_function", 0).await; - } - - #[test] - async fn goto_from_use_as() { - let (mut state, noir_text_document) = test_utils::init_lsp_server("go_to_definition").await; + async fn expect_goto( + directory: &str, + position: Position, + expected_file: &str, + expected_range: Range, + ) { + let (mut state, noir_text_document) = test_utils::init_lsp_server(directory).await; let params = GotoDefinitionParams { text_document_position_params: lsp_types::TextDocumentPositionParams { text_document: lsp_types::TextDocumentIdentifier { uri: noir_text_document.clone(), }, - position: Position { line: 7, character: 29 }, // The word after `as` + position: position, }, work_done_progress_params: Default::default(), partial_result_params: Default::default(), @@ -116,15 +115,43 @@ mod goto_definition_tests { .unwrap_or_else(|| panic!("Didn't get a goto definition response")); if let GotoDefinitionResponse::Scalar(location) = response { - assert_eq!( - location.range, - Range { - start: Position { line: 1, character: 11 }, - end: Position { line: 1, character: 27 } - } - ); + assert!(location.uri.to_string().ends_with(expected_file)); + assert_eq!(location.range, expected_range); } else { panic!("Expected a scalar response"); }; } + + #[test] + async fn goto_from_function_location_to_declaration() { + expect_goto_for_all_references("go_to_definition", "another_function", 0).await; + } + + #[test] + async fn goto_from_use_as() { + expect_goto( + "go_to_definition", + Position { line: 7, character: 29 }, // The word after `as`, + "src/main.nr", + Range { + start: Position { line: 1, character: 11 }, + end: Position { line: 1, character: 27 }, + }, + ) + .await + } + + #[test] + async fn goto_module_from_call_path() { + expect_goto( + "go_to_definition", + Position { line: 17, character: 4 }, // "bar" in "bar::baz()" + "src/bar.nr", + Range { + start: Position { line: 0, character: 0 }, + end: Position { line: 0, character: 0 }, + }, + ) + .await + } } diff --git a/tooling/lsp/test_programs/go_to_definition/src/bar.nr b/tooling/lsp/test_programs/go_to_definition/src/bar.nr new file mode 100644 index 00000000000..2c724787193 --- /dev/null +++ b/tooling/lsp/test_programs/go_to_definition/src/bar.nr @@ -0,0 +1 @@ +pub fn baz() {} diff --git a/tooling/lsp/test_programs/go_to_definition/src/main.nr b/tooling/lsp/test_programs/go_to_definition/src/main.nr index c62abb257f2..2a921c53b8f 100644 --- a/tooling/lsp/test_programs/go_to_definition/src/main.nr +++ b/tooling/lsp/test_programs/go_to_definition/src/main.nr @@ -7,10 +7,13 @@ mod foo { use foo::another_function; use foo::another_function as aliased_function; +mod bar; + fn some_function() -> Field { 1 + 2 } fn main() { let _ = another_function(); + bar::baz(); } From 6923fec383afa78c52294b13d593ee2b14e8bddf Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 16:20:44 -0300 Subject: [PATCH 05/29] feat: lsp "go to" inline module from call path --- .../src/hir/def_collector/dc_mod.rs | 35 +++++++++++++------ tooling/lsp/src/requests/goto_definition.rs | 14 ++++++++ .../test_programs/go_to_definition/src/bar.nr | 4 +++ .../go_to_definition/src/main.nr | 1 + 4 files changed, 44 insertions(+), 10 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 729e5d1b8ac..7175c387198 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -283,7 +283,13 @@ impl<'a> ModCollector<'a> { ); // Create the corresponding module for the struct namespace - let id = match self.push_child_module(context, &name, self.file_id, false, false) { + let id = match self.push_child_module( + context, + &name, + Location::new(name.span(), self.file_id), + false, + false, + ) { Ok(local_id) => context.def_interner.new_struct( &unresolved, resolved_generics, @@ -377,8 +383,13 @@ impl<'a> ModCollector<'a> { let name = trait_definition.name.clone(); // Create the corresponding module for the trait namespace - let trait_id = match self.push_child_module(context, &name, self.file_id, false, false) - { + let trait_id = match self.push_child_module( + context, + &name, + Location::new(name.span(), self.file_id), + false, + false, + ) { Ok(local_id) => TraitId(ModuleId { krate, local_id }), Err(error) => { errors.push((error.into(), self.file_id)); @@ -539,7 +550,7 @@ impl<'a> ModCollector<'a> { match self.push_child_module( context, &submodule.name, - file_id, + Location::new(submodule.name.span(), file_id), true, submodule.is_contract, ) { @@ -627,7 +638,13 @@ impl<'a> ModCollector<'a> { ); // Add module into def collector and get a ModuleId - match self.push_child_module(context, &mod_decl.ident, child_file_id, true, false) { + match self.push_child_module( + context, + &mod_decl.ident, + Location::new(Span::empty(0), child_file_id), + true, + false, + ) { Ok(child_mod_id) => { errors.extend(collect_defs( self.def_collector, @@ -652,12 +669,12 @@ impl<'a> ModCollector<'a> { &mut self, context: &mut Context, mod_name: &Ident, - file_id: FileId, + mod_location: Location, add_to_parent_scope: bool, is_contract: bool, ) -> Result { let parent = Some(self.module_id); - let location = Location::new(mod_name.span(), file_id); + let location = Location::new(mod_name.span(), mod_location.file); let new_module = ModuleData::new(parent, location, is_contract); let module_id = self.def_collector.def_map.modules.insert(new_module); @@ -690,9 +707,7 @@ impl<'a> ModCollector<'a> { return Err(err); } - context - .def_interner - .add_module_location(mod_id, Location::new(Span::empty(0), file_id)); + context.def_interner.add_module_location(mod_id, mod_location); } Ok(LocalModuleId(module_id)) diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 7653476f3a1..46df558dc17 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -154,4 +154,18 @@ mod goto_definition_tests { ) .await } + + #[test] + async fn goto_inline_module_from_call_path() { + expect_goto( + "go_to_definition", + Position { line: 18, character: 9 }, // "inline" in "bar::inline::qux()" + "src/bar.nr", + Range { + start: Position { line: 2, character: 4 }, + end: Position { line: 2, character: 10 }, + }, + ) + .await + } } diff --git a/tooling/lsp/test_programs/go_to_definition/src/bar.nr b/tooling/lsp/test_programs/go_to_definition/src/bar.nr index 2c724787193..6792022dc10 100644 --- a/tooling/lsp/test_programs/go_to_definition/src/bar.nr +++ b/tooling/lsp/test_programs/go_to_definition/src/bar.nr @@ -1 +1,5 @@ pub fn baz() {} + +mod inline { + pub fn qux() {} +} diff --git a/tooling/lsp/test_programs/go_to_definition/src/main.nr b/tooling/lsp/test_programs/go_to_definition/src/main.nr index 2a921c53b8f..76a367259b5 100644 --- a/tooling/lsp/test_programs/go_to_definition/src/main.nr +++ b/tooling/lsp/test_programs/go_to_definition/src/main.nr @@ -16,4 +16,5 @@ fn some_function() -> Field { fn main() { let _ = another_function(); bar::baz(); + bar::inline::qux(); } From c6af4c69c3aa055fd88f361d816076d84f9855ba Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 16:43:35 -0300 Subject: [PATCH 06/29] feat: lsp "go to" on `use` module path --- .../noirc_frontend/src/elaborator/scope.rs | 6 ++--- .../src/hir/def_collector/dc_crate.rs | 25 ++++++++++++++++++- tooling/lsp/src/requests/goto_definition.rs | 14 +++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index c13ea5a2e40..b253e272982 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -48,11 +48,11 @@ impl<'context> Elaborator<'context> { let path_resolution; if self.interner.track_references { - let mut dependencies: Vec = Vec::new(); + let mut references: Vec = Vec::new(); path_resolution = - resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut dependencies))?; + resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; - for (referenced, ident) in dependencies.iter().zip(path.segments) { + for (referenced, ident) in references.iter().zip(path.segments) { let reference = ReferenceId::Variable(Location::new(ident.span(), self.file)); self.interner.add_reference(*referenced, reference); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 668dbf33d52..7fe6769464c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -330,7 +330,30 @@ impl DefCollector { // Resolve unresolved imports collected from the crate, one by one. for collected_import in std::mem::take(&mut def_collector.imports) { let module_id = collected_import.module_id; - match resolve_import(crate_id, &collected_import, &context.def_maps, &mut None) { + let resolved_import = if context.def_interner.track_references { + let mut references: Vec = Vec::new(); + let resolved_import = resolve_import( + crate_id, + &collected_import, + &context.def_maps, + &mut Some(&mut references), + ); + + let current_def_map = context.def_maps.get(&crate_id).unwrap(); + let file_id = current_def_map.file_id(module_id); + + for (referenced, ident) in + references.iter().zip(collected_import.clone().path.segments) + { + let reference = ReferenceId::Variable(Location::new(ident.span(), file_id)); + context.def_interner.add_reference(*referenced, reference); + } + + resolved_import + } else { + resolve_import(crate_id, &collected_import, &context.def_maps, &mut None) + }; + match resolved_import { Ok(resolved_import) => { if let Some(error) = resolved_import.error { errors.push(( diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 46df558dc17..d823e6366a9 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -168,4 +168,18 @@ mod goto_definition_tests { ) .await } + + #[test] + async fn goto_module_from_use_path() { + expect_goto( + "go_to_definition", + Position { line: 6, character: 4 }, // "foo" in "use foo::another_function;" + "src/main.nr", + Range { + start: Position { line: 0, character: 4 }, + end: Position { line: 0, character: 7 }, + }, + ) + .await + } } From 5f8f9b4a27227da72fa3c3e9b5d3286d98aa2490 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 17:20:42 -0300 Subject: [PATCH 07/29] feat: lsp "go to" by clicking on module definition ("mod foo") --- .../src/hir/def_collector/dc_mod.rs | 29 +++++++++++-------- tooling/lsp/src/requests/goto_definition.rs | 14 +++++++++ 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 7175c387198..0ff58869fb1 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -290,11 +290,11 @@ impl<'a> ModCollector<'a> { false, false, ) { - Ok(local_id) => context.def_interner.new_struct( + Ok(module_id) => context.def_interner.new_struct( &unresolved, resolved_generics, krate, - local_id, + module_id.local_id, self.file_id, ), Err(error) => { @@ -390,7 +390,7 @@ impl<'a> ModCollector<'a> { false, false, ) { - Ok(local_id) => TraitId(ModuleId { krate, local_id }), + Ok(module_id) => TraitId(ModuleId { krate, local_id: module_id.local_id }), Err(error) => { errors.push((error.into(), self.file_id)); continue; @@ -559,7 +559,7 @@ impl<'a> ModCollector<'a> { self.def_collector, submodule.contents, file_id, - child, + child.local_id, crate_id, context, macro_processors, @@ -646,11 +646,16 @@ impl<'a> ModCollector<'a> { false, ) { Ok(child_mod_id) => { + // Track that the "foo" in `mod foo;` points to the module "foo" + let referenced = ReferenceId::Module(child_mod_id); + let reference = ReferenceId::Variable(location); + context.def_interner.add_reference(referenced, reference); + errors.extend(collect_defs( self.def_collector, ast, child_file_id, - child_mod_id, + child_mod_id.local_id, crate_id, context, macro_processors, @@ -672,7 +677,7 @@ impl<'a> ModCollector<'a> { mod_location: Location, add_to_parent_scope: bool, is_contract: bool, - ) -> Result { + ) -> Result { let parent = Some(self.module_id); let location = Location::new(mod_name.span(), mod_location.file); let new_module = ModuleData::new(parent, location, is_contract); @@ -683,6 +688,11 @@ impl<'a> ModCollector<'a> { // Update the parent module to reference the child modules[self.module_id.0].children.insert(mod_name.clone(), LocalModuleId(module_id)); + let mod_id = ModuleId { + krate: self.def_collector.def_map.krate, + local_id: LocalModuleId(module_id), + }; + // Add this child module into the scope of the parent module as a module definition // module definitions are definitions which can only exist at the module level. // ModuleDefinitionIds can be used across crates since they contain the CrateId @@ -691,11 +701,6 @@ impl<'a> ModCollector<'a> { // to a child module containing its methods) since the module name should not shadow // the struct name. if add_to_parent_scope { - let mod_id = ModuleId { - krate: self.def_collector.def_map.krate, - local_id: LocalModuleId(module_id), - }; - if let Err((first_def, second_def)) = modules[self.module_id.0].declare_child_module(mod_name.to_owned(), mod_id) { @@ -710,7 +715,7 @@ impl<'a> ModCollector<'a> { context.def_interner.add_module_location(mod_id, mod_location); } - Ok(LocalModuleId(module_id)) + Ok(mod_id) } } diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index d823e6366a9..ec641634543 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -182,4 +182,18 @@ mod goto_definition_tests { ) .await } + + #[test] + async fn goto_module_from_mod() { + expect_goto( + "go_to_definition", + Position { line: 9, character: 4 }, // "bar" in "mod bar;" + "src/bar.nr", + Range { + start: Position { line: 0, character: 0 }, + end: Position { line: 0, character: 0 }, + }, + ) + .await + } } From 9930c1199a7729ad567924d9267a108814ce6771 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 17:30:36 -0300 Subject: [PATCH 08/29] clippy --- tooling/lsp/src/requests/goto_definition.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index ec641634543..fe591a433cf 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -103,7 +103,7 @@ mod goto_definition_tests { text_document: lsp_types::TextDocumentIdentifier { uri: noir_text_document.clone(), }, - position: position, + position, }, work_done_progress_params: Default::default(), partial_result_params: Default::default(), @@ -138,7 +138,7 @@ mod goto_definition_tests { end: Position { line: 1, character: 27 }, }, ) - .await + .await; } #[test] @@ -152,7 +152,7 @@ mod goto_definition_tests { end: Position { line: 0, character: 0 }, }, ) - .await + .await; } #[test] @@ -166,7 +166,7 @@ mod goto_definition_tests { end: Position { line: 2, character: 10 }, }, ) - .await + .await; } #[test] @@ -180,7 +180,7 @@ mod goto_definition_tests { end: Position { line: 0, character: 7 }, }, ) - .await + .await; } #[test] @@ -194,6 +194,6 @@ mod goto_definition_tests { end: Position { line: 0, character: 0 }, }, ) - .await + .await; } } From 76995b10d2f0b2647310cfbc13996523b1f229c9 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 08:40:02 -0300 Subject: [PATCH 09/29] feat: lsp rename/find-all-references for traits --- compiler/noirc_frontend/src/elaborator/mod.rs | 15 +++++++++++-- .../src/hir/def_collector/dc_crate.rs | 4 ++++ .../src/hir/def_collector/dc_mod.rs | 4 ++++ compiler/noirc_frontend/src/locations.rs | 9 +++----- compiler/noirc_frontend/src/node_interner.rs | 12 +++++++++++ tooling/lsp/src/requests/rename.rs | 5 +++++ .../lsp/test_programs/rename_trait/Nargo.toml | 6 ++++++ .../test_programs/rename_trait/src/main.nr | 21 +++++++++++++++++++ 8 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 tooling/lsp/test_programs/rename_trait/Nargo.toml create mode 100644 tooling/lsp/test_programs/rename_trait/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 7c780838163..2065657c864 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -30,7 +30,8 @@ use crate::{ SecondaryAttribute, StructId, }, node_interner::{ - DefinitionId, DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, TraitId, TypeAliasId, + DefinitionId, DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ReferenceId, TraitId, + TypeAliasId, }, parser::TopLevelStatement, Shared, Type, TypeBindings, TypeVariable, @@ -1407,7 +1408,8 @@ impl<'context> Elaborator<'context> { self.file = trait_impl.file_id; self.local_module = trait_impl.module_id; - trait_impl.trait_id = self.resolve_trait_by_path(trait_impl.trait_path.clone()); + let trait_id = self.resolve_trait_by_path(trait_impl.trait_path.clone()); + trait_impl.trait_id = trait_id; let unresolved_type = &trait_impl.object_type; self.add_generics(&trait_impl.generics); @@ -1445,6 +1447,15 @@ impl<'context> Elaborator<'context> { trait_impl.resolved_object_type = self.self_type.take(); trait_impl.impl_id = self.current_trait_impl.take(); self.generics.clear(); + + if let Some(trait_id) = trait_id { + let referenced = ReferenceId::Trait(trait_id); + let reference = ReferenceId::Variable(Location::new( + trait_impl.trait_path.last_segment().span(), + trait_impl.file_id, + )); + self.interner.add_reference(referenced, reference); + } } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 7fe6769464c..a386db6b6e6 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -521,6 +521,10 @@ fn add_import_reference( let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); interner.add_reference(ReferenceId::Struct(struct_id), variable); } + crate::macros_api::ModuleDefId::TraitId(trait_id) => { + let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + interner.add_reference(ReferenceId::Trait(trait_id), variable); + } _ => (), } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 0ff58869fb1..f2efaf4c26f 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -381,6 +381,7 @@ impl<'a> ModCollector<'a> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for trait_definition in traits { let name = trait_definition.name.clone(); + let name_location = Location::new(name.span(), self.file_id); // Create the corresponding module for the trait namespace let trait_id = match self.push_child_module( @@ -532,6 +533,9 @@ impl<'a> ModCollector<'a> { }; context.def_interner.push_empty_trait(trait_id, &unresolved, resolved_generics); + context.def_interner.add_trait_location(trait_id, name_location); + context.def_interner.add_definition_location(ReferenceId::Trait(trait_id)); + self.def_collector.items.traits.insert(trait_id, unresolved); } errors diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index addc2b0305e..c142d10319b 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -34,7 +34,7 @@ impl NodeInterner { ReferenceId::Module(id) => self.module_location(&id), ReferenceId::Function(id) => self.function_modifiers(&id).name_location, ReferenceId::Struct(id) => self.struct_location(&id), - ReferenceId::Trait(_) => todo!(), + ReferenceId::Trait(id) => self.trait_location(&id), ReferenceId::Global(id) => self.get_global(id).location, ReferenceId::Alias(id) => self.get_type_alias(id).borrow().location, ReferenceId::Variable(location) => location, @@ -101,11 +101,8 @@ impl NodeInterner { let reference_node = self.reference_graph[node_index]; let found_locations: Vec = match reference_node { - ReferenceId::Alias(_) - | ReferenceId::Global(_) - | ReferenceId::Module(_) - | ReferenceId::Trait(_) => todo!(), - ReferenceId::Function(_) | ReferenceId::Struct(_) => { + ReferenceId::Alias(_) | ReferenceId::Global(_) | ReferenceId::Module(_) => todo!(), + ReferenceId::Function(_) | ReferenceId::Struct(_) | ReferenceId::Trait(_) => { self.find_all_references_for_index(node_index, include_reference) } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 6fcc6a5e3af..76a67e3977c 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -71,6 +71,9 @@ pub struct NodeInterner { // The location of each struct name struct_name_locations: HashMap, + // The location of each trait name + trait_name_locations: HashMap, + /// This graph tracks dependencies between different global definitions. /// This is used to ensure the absence of dependency cycles for globals and types. dependency_graph: DiGraph, @@ -545,6 +548,7 @@ impl Default for NodeInterner { function_modules: HashMap::new(), module_locations: HashMap::new(), struct_name_locations: HashMap::new(), + trait_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), dependency_graph_indices: HashMap::new(), @@ -984,6 +988,14 @@ impl NodeInterner { self.struct_name_locations[struct_id] } + pub fn add_trait_location(&mut self, trait_id: TraitId, location: Location) { + self.trait_name_locations.insert(trait_id, location); + } + + pub fn trait_location(&self, trait_id: &TraitId) -> Location { + self.trait_name_locations[trait_id] + } + pub fn add_module_location(&mut self, module_id: ModuleId, location: Location) { self.module_locations.insert(module_id, location); } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 6dd3cc3fda7..67853c12b81 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -145,4 +145,9 @@ mod rename_tests { async fn test_rename_struct() { check_rename_succeeds("rename_struct", "Foo").await; } + + #[test] + async fn test_rename_trait() { + check_rename_succeeds("rename_trait", "Foo").await; + } } diff --git a/tooling/lsp/test_programs/rename_trait/Nargo.toml b/tooling/lsp/test_programs/rename_trait/Nargo.toml new file mode 100644 index 00000000000..a8cc34898bd --- /dev/null +++ b/tooling/lsp/test_programs/rename_trait/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "rename_trait" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/lsp/test_programs/rename_trait/src/main.nr b/tooling/lsp/test_programs/rename_trait/src/main.nr new file mode 100644 index 00000000000..dd0783985a7 --- /dev/null +++ b/tooling/lsp/test_programs/rename_trait/src/main.nr @@ -0,0 +1,21 @@ +mod foo { + mod bar { + trait Foo { + fn foo() {} + } + } +} + +use foo::bar::Foo; + +struct Bar { + +} + +impl Foo for Bar { + +} + +fn main() { + foo::bar::Foo::foo(); +} From 30d0ee4067eb579cf23fbb1c117d84256331f6c3 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 10:09:05 -0300 Subject: [PATCH 10/29] fix: (lsp) don't rename `self` pointing to struct --- compiler/noirc_frontend/src/elaborator/types.rs | 10 +++++++--- tooling/lsp/test_programs/rename_struct/src/main.nr | 2 ++ 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index b6212abb4a2..22f09460e0a 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -58,6 +58,7 @@ impl<'context> Elaborator<'context> { use crate::ast::UnresolvedTypeData::*; let span = typ.span; + let is_synthetic = if let Named(_, _, synthetic) = typ.typ { synthetic } else { false }; let resolved_type = match typ.typ { FieldElement => Type::FieldElement, @@ -155,9 +156,12 @@ impl<'context> Elaborator<'context> { Location::new(unresolved_span, self.file), ); - let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(unresolved_span, self.file)); - self.interner.add_reference(referenced, reference); + if !is_synthetic { + let referenced = ReferenceId::Struct(struct_type.borrow().id); + let reference = + ReferenceId::Variable(Location::new(unresolved_span, self.file)); + self.interner.add_reference(referenced, reference); + } } } diff --git a/tooling/lsp/test_programs/rename_struct/src/main.nr b/tooling/lsp/test_programs/rename_struct/src/main.nr index 93a0779cf3b..fdbe1ae6020 100644 --- a/tooling/lsp/test_programs/rename_struct/src/main.nr +++ b/tooling/lsp/test_programs/rename_struct/src/main.nr @@ -6,6 +6,8 @@ mod foo { impl Foo { fn foo() {} + + fn bar(self) {} } } } From 3530c4ec7c8b526b6f8762ed9f9187f81bf42fb2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 11:33:08 -0300 Subject: [PATCH 11/29] Make sure `Self` isn't renamed but found as a reference --- compiler/noirc_frontend/src/ast/statement.rs | 7 +++ .../src/elaborator/expressions.rs | 3 +- .../noirc_frontend/src/elaborator/patterns.rs | 8 +-- .../noirc_frontend/src/elaborator/scope.rs | 5 +- .../noirc_frontend/src/elaborator/types.rs | 13 +++-- .../src/hir/def_collector/dc_crate.rs | 4 +- compiler/noirc_frontend/src/locations.rs | 45 +++++++++++----- compiler/noirc_frontend/src/node_interner.rs | 12 ++++- tooling/lsp/src/requests/references.rs | 2 +- tooling/lsp/src/requests/rename.rs | 54 +++++++++++-------- .../test_programs/rename_struct/src/main.nr | 4 ++ 11 files changed, 108 insertions(+), 49 deletions(-) diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 48e34ad7fc9..3e6a140ff93 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -10,6 +10,7 @@ use super::{ BlockExpression, Expression, ExpressionKind, IndexExpression, MemberAccessExpression, MethodCallExpression, UnresolvedType, }; +use crate::hir::resolution::resolver::SELF_TYPE_NAME; use crate::lexer::token::SpannedToken; use crate::macros_api::SecondaryAttribute; use crate::parser::{ParserError, ParserErrorReason}; @@ -165,6 +166,12 @@ impl StatementKind { #[derive(Eq, Debug, Clone)] pub struct Ident(pub Spanned); +impl Ident { + pub fn is_self_type_name(&self) -> bool { + self.0.contents == SELF_TYPE_NAME + } +} + impl PartialEq for Ident { fn eq(&self, other: &Ident) -> bool { self.0.contents == other.0.contents diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index e013cf7b4f1..5c3866816a6 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -403,6 +403,7 @@ impl<'context> Elaborator<'context> { constructor: ConstructorExpression, ) -> (HirExpression, Type) { let span = constructor.type_name.span(); + let is_self_type = constructor.type_name.last_segment().is_self_type_name(); let (r#type, struct_generics) = if let Some(struct_id) = constructor.struct_type { let typ = self.interner.get_struct(struct_id); @@ -433,7 +434,7 @@ impl<'context> Elaborator<'context> { }); let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(span, self.file)); + let reference = ReferenceId::Variable(Location::new(span, self.file), is_self_type); self.interner.add_reference(referenced, reference); (expr, Type::Struct(struct_type, generics)) diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index e3c854d615d..81fffb522bf 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -159,6 +159,7 @@ impl<'context> Elaborator<'context> { new_definitions: &mut Vec, ) -> HirPattern { let name_span = name.last_segment().span(); + let is_self_type = name.last_segment().is_self_type_name(); let error_identifier = |this: &mut Self| { // Must create a name here to return a HirPattern::Identifier. Allowing @@ -200,7 +201,7 @@ impl<'context> Elaborator<'context> { ); let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(name_span, self.file)); + let reference = ReferenceId::Variable(Location::new(name_span, self.file), is_self_type); self.interner.add_reference(referenced, reference); HirPattern::Struct(expected_type, fields, location) @@ -437,6 +438,7 @@ impl<'context> Elaborator<'context> { // Otherwise, then it is referring to an Identifier // This lookup allows support of such statements: let x = foo::bar::SOME_GLOBAL + 10; // If the expression is a singular indent, we search the resolver's current scope as normal. + let is_self_type_name = path.last_segment().is_self_type_name(); let (hir_ident, var_scope_index) = self.get_ident_from_path(path); if hir_ident.id != DefinitionId::dummy_id() { @@ -446,7 +448,7 @@ impl<'context> Elaborator<'context> { self.interner.add_function_dependency(current_item, func_id); } - let variable = ReferenceId::Variable(hir_ident.location); + let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name); let function = ReferenceId::Function(func_id); self.interner.add_reference(function, variable); } @@ -458,7 +460,7 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, global_id); } - let variable = ReferenceId::Variable(hir_ident.location); + let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name); let global = ReferenceId::Global(global_id); self.interner.add_reference(global, variable); } diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index c13ea5a2e40..fdfde36e912 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -53,7 +53,10 @@ impl<'context> Elaborator<'context> { resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut dependencies))?; for (referenced, ident) in dependencies.iter().zip(path.segments) { - let reference = ReferenceId::Variable(Location::new(ident.span(), self.file)); + let reference = ReferenceId::Variable( + Location::new(ident.span(), self.file), + ident.is_self_type_name(), + ); self.interner.add_reference(*referenced, reference); } } else { diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 22f09460e0a..f0625308ec6 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -58,7 +58,12 @@ impl<'context> Elaborator<'context> { use crate::ast::UnresolvedTypeData::*; let span = typ.span; - let is_synthetic = if let Named(_, _, synthetic) = typ.typ { synthetic } else { false }; + let (is_self_type_name, is_synthetic) = if let Named(ref named_path, _, synthetic) = typ.typ + { + (named_path.last_segment().is_self_type_name(), synthetic) + } else { + (false, false) + }; let resolved_type = match typ.typ { FieldElement => Type::FieldElement, @@ -158,8 +163,10 @@ impl<'context> Elaborator<'context> { if !is_synthetic { let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = - ReferenceId::Variable(Location::new(unresolved_span, self.file)); + let reference = ReferenceId::Variable( + Location::new(unresolved_span, self.file), + is_self_type_name, + ); self.interner.add_reference(referenced, reference); } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 668dbf33d52..ae0abd2543c 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -491,11 +491,11 @@ fn add_import_reference( match def_id { crate::macros_api::ModuleDefId::FunctionId(func_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); interner.add_reference(ReferenceId::Function(func_id), variable); } crate::macros_api::ModuleDefId::TypeId(struct_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); interner.add_reference(ReferenceId::Struct(struct_id), variable); } _ => (), diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 79927b03041..8e12134a77a 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -37,7 +37,7 @@ impl NodeInterner { ReferenceId::Trait(_) => todo!(), ReferenceId::Global(id) => self.get_global(id).location, ReferenceId::Alias(id) => self.get_type_alias(id).borrow().location, - ReferenceId::Variable(location) => location, + ReferenceId::Variable(location, _) => location, } } @@ -90,12 +90,15 @@ impl NodeInterner { // Starting at the given location, find the node referenced by it. Then, gather // all locations that reference that node, and return all of them - // (the references and optionally the reference node if `include_reference` is true). + // (the references and optionally the referenced node if `include_referencedd` is true). + // If `include_self_type_name` is true, references where "Self" is written are returned, + // otherwise they are not. // Returns `None` if the location is not known to this interner. pub fn find_all_references( &self, location: Location, - include_reference: bool, + include_referenced: bool, + include_self_type_name: bool, ) -> Option> { let node_index = self.location_indices.get_node_from_location(location)?; @@ -105,36 +108,50 @@ impl NodeInterner { | ReferenceId::Global(_) | ReferenceId::Module(_) | ReferenceId::Trait(_) => todo!(), - ReferenceId::Function(_) | ReferenceId::Struct(_) => { - self.find_all_references_for_index(node_index, include_reference) - } - - ReferenceId::Variable(_) => { + ReferenceId::Function(_) | ReferenceId::Struct(_) => self + .find_all_references_for_index( + node_index, + include_referenced, + include_self_type_name, + ), + + ReferenceId::Variable(_, _) => { let referenced_node_index = self.referenced_index(node_index)?; - self.find_all_references_for_index(referenced_node_index, include_reference) + self.find_all_references_for_index( + referenced_node_index, + include_referenced, + include_self_type_name, + ) } }; Some(found_locations) } // Given a referenced node index, find all references to it and return their locations, optionally together - // with the reference node's location if `include_reference` is true. + // with the reference node's location if `include_referenced` is true. + // If `include_self_type_name` is true, references where "Self" is written are returned, + // otherwise they are not. fn find_all_references_for_index( &self, referenced_node_index: PetGraphIndex, - include_reference: bool, + include_referenced: bool, + include_self_type_name: bool, ) -> Vec { let id = self.reference_graph[referenced_node_index]; let mut edit_locations = Vec::new(); - if include_reference { - edit_locations.push(self.reference_location(id)); + if include_referenced { + if include_self_type_name || !id.is_self_type_name() { + edit_locations.push(self.reference_location(id)); + } } self.reference_graph .neighbors_directed(referenced_node_index, petgraph::Direction::Incoming) .for_each(|reference_node_index| { let id = self.reference_graph[reference_node_index]; - edit_locations.push(self.reference_location(id)); + if include_self_type_name || !id.is_self_type_name() { + edit_locations.push(self.reference_location(id)); + } }); edit_locations } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 95a6a56ac60..d08d3e16b3a 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -245,7 +245,17 @@ pub enum ReferenceId { Global(GlobalId), Function(FuncId), Alias(TypeAliasId), - Variable(Location), + Variable(Location, bool /* is Self */), +} + +impl ReferenceId { + pub fn is_self_type_name(&self) -> bool { + if let Self::Variable(_, true) = self { + true + } else { + false + } + } } /// A trait implementation is either a normal implementation that is present in the source diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index d35ec4a86d8..f8c23632936 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -13,7 +13,7 @@ pub(crate) fn on_references_request( ) -> impl Future>, ResponseError>> { let result = process_request(state, params.text_document_position, |location, interner, files| { - interner.find_all_references(location, params.context.include_declaration).map( + interner.find_all_references(location, params.context.include_declaration, true).map( |locations| { locations .iter() diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 6dd3cc3fda7..ee2ff1a153d 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -29,29 +29,30 @@ pub(crate) fn on_rename_request( ) -> impl Future, ResponseError>> { let result = process_request(state, params.text_document_position, |location, interner, files| { - let rename_changes = interner.find_all_references(location, true).map(|locations| { - let rs = locations.iter().fold( - HashMap::new(), - |mut acc: HashMap>, location| { - let file_id = location.file; - let span = location.span; - - let Some(lsp_location) = to_lsp_location(files, file_id, span) else { - return acc; - }; - - let edit = TextEdit { - range: lsp_location.range, - new_text: params.new_name.clone(), - }; - - acc.entry(lsp_location.uri).or_default().push(edit); - - acc - }, - ); - rs - }); + let rename_changes = + interner.find_all_references(location, true, false).map(|locations| { + let rs = locations.iter().fold( + HashMap::new(), + |mut acc: HashMap>, location| { + let file_id = location.file; + let span = location.span; + + let Some(lsp_location) = to_lsp_location(files, file_id, span) else { + return acc; + }; + + let edit = TextEdit { + range: lsp_location.range, + new_text: params.new_name.clone(), + }; + + acc.entry(lsp_location.uri).or_default().push(edit); + + acc + }, + ); + rs + }); let response = WorkspaceEdit { changes: rename_changes, @@ -103,6 +104,13 @@ mod rename_tests { let mut changes: Vec = changes.values().flatten().map(|edit| edit.range).collect(); changes.sort_by_key(|range| (range.start.line, range.start.character)); + if changes != ranges { + let extra_in_changes: Vec<_> = + changes.iter().filter(|range| !ranges.contains(range)).collect(); + let extra_in_ranges: Vec<_> = + ranges.iter().filter(|range| !changes.contains(range)).collect(); + panic!("Rename locations did not match.\nThese renames were not found: {:?}\nThese renames should not have been found: {:?}", extra_in_ranges, extra_in_changes); + } assert_eq!(changes, ranges); } } diff --git a/tooling/lsp/test_programs/rename_struct/src/main.nr b/tooling/lsp/test_programs/rename_struct/src/main.nr index fdbe1ae6020..d9da3e75763 100644 --- a/tooling/lsp/test_programs/rename_struct/src/main.nr +++ b/tooling/lsp/test_programs/rename_struct/src/main.nr @@ -8,6 +8,10 @@ mod foo { fn foo() {} fn bar(self) {} + + fn baz() -> Self { + Self { field: 1 } + } } } } From 65a5ad045dfd60e15a255cfc1f449e5a188f614d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 11:44:58 -0300 Subject: [PATCH 12/29] Disallow renaming "Self" --- compiler/noirc_frontend/src/locations.rs | 11 ++++++--- tooling/lsp/src/requests/rename.rs | 30 ++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 8e12134a77a..c04f8581f96 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -83,9 +83,14 @@ impl NodeInterner { .map(|node_index| self.reference_location(self.reference_graph[node_index])) } - // Is the given location known to this interner? - pub fn is_location_known(&self, location: Location) -> bool { - self.location_indices.get_node_from_location(location).is_some() + // Returns the `ReferenceId` that exists at a given location, if any. + pub fn reference_at_location(&self, location: Location) -> Option { + if self.location_indices.get_node_from_location(location).is_none() { + return None; + } + + let node_index = self.location_indices.get_node_from_location(location)?; + Some(self.reference_graph[node_index]) } // Starting at the given location, find the node referenced by it. Then, gather diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index ee2ff1a153d..92be8dfa17f 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -7,6 +7,7 @@ use async_lsp::ResponseError; use lsp_types::{ PrepareRenameResponse, RenameParams, TextDocumentPositionParams, TextEdit, Url, WorkspaceEdit, }; +use noirc_frontend::node_interner::ReferenceId; use crate::LspState; @@ -17,7 +18,13 @@ pub(crate) fn on_prepare_rename_request( params: TextDocumentPositionParams, ) -> impl Future, ResponseError>> { let result = process_request(state, params, |location, interner, _| { - let rename_possible = interner.is_location_known(location); + let reference_id = interner.reference_at_location(location); + let rename_possible = match reference_id { + // Rename shouldn't be possible when triggered on top of "Self" + Some(ReferenceId::Variable(_, true /* is self type name */)) => false, + Some(_) => true, + None => false, + }; Some(PrepareRenameResponse::DefaultBehavior { default_behavior: rename_possible }) }); future::ready(result) @@ -116,7 +123,7 @@ mod rename_tests { } #[test] - async fn test_on_prepare_rename_request_cannot_be_applied() { + async fn test_on_prepare_rename_request_cannot_be_applied_if_there_are_no_matches() { let (mut state, noir_text_document) = test_utils::init_lsp_server("rename_function").await; let params = TextDocumentPositionParams { @@ -134,6 +141,25 @@ mod rename_tests { ); } + #[test] + async fn test_on_prepare_rename_request_cannot_be_applied_on_self_type_name() { + let (mut state, noir_text_document) = test_utils::init_lsp_server("rename_struct").await; + + let params = TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri: noir_text_document }, + position: lsp_types::Position { line: 11, character: 24 }, // At "Self" + }; + + let response = on_prepare_rename_request(&mut state, params) + .await + .expect("Could not execute on_prepare_rename_request"); + + assert_eq!( + response, + Some(PrepareRenameResponse::DefaultBehavior { default_behavior: false }) + ); + } + #[test] async fn test_rename_function() { check_rename_succeeds("rename_function", "another_function").await; From afd66265639330e5a25d9deebb92c9ac1d334edd Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 13:09:57 -0300 Subject: [PATCH 13/29] clippy --- compiler/noirc_frontend/src/locations.rs | 10 +++------- compiler/noirc_frontend/src/node_interner.rs | 6 +----- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index c04f8581f96..4357cf9e792 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -85,9 +85,7 @@ impl NodeInterner { // Returns the `ReferenceId` that exists at a given location, if any. pub fn reference_at_location(&self, location: Location) -> Option { - if self.location_indices.get_node_from_location(location).is_none() { - return None; - } + self.location_indices.get_node_from_location(location)?; let node_index = self.location_indices.get_node_from_location(location)?; Some(self.reference_graph[node_index]) @@ -144,10 +142,8 @@ impl NodeInterner { ) -> Vec { let id = self.reference_graph[referenced_node_index]; let mut edit_locations = Vec::new(); - if include_referenced { - if include_self_type_name || !id.is_self_type_name() { - edit_locations.push(self.reference_location(id)); - } + if include_referenced && (include_self_type_name || !id.is_self_type_name()) { + edit_locations.push(self.reference_location(id)); } self.reference_graph diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index d08d3e16b3a..3d40e109628 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -250,11 +250,7 @@ pub enum ReferenceId { impl ReferenceId { pub fn is_self_type_name(&self) -> bool { - if let Self::Variable(_, true) = self { - true - } else { - false - } + matches!(self, Self::Variable(_, true)) } } From e68d25a049942ec73dd6a00c1cad236652d3be4c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 15:09:02 -0300 Subject: [PATCH 14/29] WIP --- .../src/hir/def_collector/dc_crate.rs | 4 ++++ compiler/noirc_frontend/src/locations.rs | 7 +++++-- tooling/lsp/src/requests/rename.rs | 5 +++++ .../test_programs/rename_function/src/main.nr | 6 ++++++ .../test_programs/rename_type_alias/Nargo.toml | 6 ++++++ .../test_programs/rename_type_alias/src/main.nr | 17 +++++++++++++++++ 6 files changed, 43 insertions(+), 2 deletions(-) create mode 100644 tooling/lsp/test_programs/rename_type_alias/Nargo.toml create mode 100644 tooling/lsp/test_programs/rename_type_alias/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index a386db6b6e6..a95ab2dc50b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -525,6 +525,10 @@ fn add_import_reference( let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); interner.add_reference(ReferenceId::Trait(trait_id), variable); } + crate::macros_api::ModuleDefId::TypeAliasId(type_alias_id) => { + let variable = ReferenceId::Variable(Location::new(name.span(), file_id)); + interner.add_reference(ReferenceId::Alias(type_alias_id), variable); + } _ => (), } } diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index c142d10319b..74f3ba566ad 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -101,8 +101,11 @@ impl NodeInterner { let reference_node = self.reference_graph[node_index]; let found_locations: Vec = match reference_node { - ReferenceId::Alias(_) | ReferenceId::Global(_) | ReferenceId::Module(_) => todo!(), - ReferenceId::Function(_) | ReferenceId::Struct(_) | ReferenceId::Trait(_) => { + ReferenceId::Global(_) | ReferenceId::Module(_) => todo!(), + ReferenceId::Function(_) + | ReferenceId::Struct(_) + | ReferenceId::Trait(_) + | ReferenceId::Alias(_) => { self.find_all_references_for_index(node_index, include_reference) } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 67853c12b81..17ab91d68e0 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -150,4 +150,9 @@ mod rename_tests { async fn test_rename_trait() { check_rename_succeeds("rename_trait", "Foo").await; } + + #[test] + async fn test_rename_type_alias() { + check_rename_succeeds("rename_type_alias", "Bar").await; + } } diff --git a/tooling/lsp/test_programs/rename_function/src/main.nr b/tooling/lsp/test_programs/rename_function/src/main.nr index ad526f10966..7a70084276e 100644 --- a/tooling/lsp/test_programs/rename_function/src/main.nr +++ b/tooling/lsp/test_programs/rename_function/src/main.nr @@ -19,3 +19,9 @@ mod foo { crate::another_function() } } + +use foo::some_other_function as bar; + +fn x() { + bar(); +} diff --git a/tooling/lsp/test_programs/rename_type_alias/Nargo.toml b/tooling/lsp/test_programs/rename_type_alias/Nargo.toml new file mode 100644 index 00000000000..1b95727ed8d --- /dev/null +++ b/tooling/lsp/test_programs/rename_type_alias/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "rename_type_alias" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/lsp/test_programs/rename_type_alias/src/main.nr b/tooling/lsp/test_programs/rename_type_alias/src/main.nr new file mode 100644 index 00000000000..4a0c497a21f --- /dev/null +++ b/tooling/lsp/test_programs/rename_type_alias/src/main.nr @@ -0,0 +1,17 @@ +mod foo { + struct Foo { + } + + type Bar = Foo; +} + +use foo::Foo; +use foo::Bar; + +fn main() { + let x: Bar = Foo {}; +} + +fn x(b: Bar) -> Bar { + b +} From 3ff7b2c6ed3ed9d184759d5452f88de9df2594f2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 15:33:38 -0300 Subject: [PATCH 15/29] feat: lsp rename/find-all-references for type aliases --- .../noirc_frontend/src/elaborator/types.rs | 32 ++++++++++++------- .../src/hir/def_collector/dc_mod.rs | 4 +++ compiler/noirc_frontend/src/locations.rs | 2 +- compiler/noirc_frontend/src/node_interner.rs | 12 +++++++ 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index b6212abb4a2..16443a5e62b 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -147,17 +147,27 @@ impl<'context> Elaborator<'context> { Resolved(id) => self.interner.get_quoted_type(id).clone(), }; - if let Type::Struct(ref struct_type, _) = resolved_type { - if let Some(unresolved_span) = typ.span { - // Record the location of the type reference - self.interner.push_type_ref_location( - resolved_type.clone(), - Location::new(unresolved_span, self.file), - ); - - let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(unresolved_span, self.file)); - self.interner.add_reference(referenced, reference); + if let Some(unresolved_span) = typ.span { + match resolved_type { + Type::Struct(ref struct_type, _) => { + // Record the location of the type reference + self.interner.push_type_ref_location( + resolved_type.clone(), + Location::new(unresolved_span, self.file), + ); + + let referenced = ReferenceId::Struct(struct_type.borrow().id); + let reference = + ReferenceId::Variable(Location::new(unresolved_span, self.file)); + self.interner.add_reference(referenced, reference); + } + Type::Alias(ref alias_type, _) => { + let referenced = ReferenceId::Alias(alias_type.borrow().id); + let reference = + ReferenceId::Variable(Location::new(unresolved_span, self.file)); + self.interner.add_reference(referenced, reference); + } + _ => (), } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index f2efaf4c26f..222de08d8bf 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -335,6 +335,7 @@ impl<'a> ModCollector<'a> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for type_alias in type_aliases { let name = type_alias.name.clone(); + let name_location = Location::new(name.span(), self.file_id); // And store the TypeId -> TypeAlias mapping somewhere it is reachable let unresolved = UnresolvedTypeAlias { @@ -366,6 +367,9 @@ impl<'a> ModCollector<'a> { } self.def_collector.items.type_aliases.insert(type_alias_id, unresolved); + + context.def_interner.add_alias_location(type_alias_id, name_location); + context.def_interner.add_definition_location(ReferenceId::Alias(type_alias_id)); } errors } diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 74f3ba566ad..7927c965137 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -36,7 +36,7 @@ impl NodeInterner { ReferenceId::Struct(id) => self.struct_location(&id), ReferenceId::Trait(id) => self.trait_location(&id), ReferenceId::Global(id) => self.get_global(id).location, - ReferenceId::Alias(id) => self.get_type_alias(id).borrow().location, + ReferenceId::Alias(id) => self.alias_location(&id), ReferenceId::Variable(location) => location, } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 76a67e3977c..9b0371fac3f 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -74,6 +74,9 @@ pub struct NodeInterner { // The location of each trait name trait_name_locations: HashMap, + // The location of each type alias name + alias_name_locations: HashMap, + /// This graph tracks dependencies between different global definitions. /// This is used to ensure the absence of dependency cycles for globals and types. dependency_graph: DiGraph, @@ -549,6 +552,7 @@ impl Default for NodeInterner { module_locations: HashMap::new(), struct_name_locations: HashMap::new(), trait_name_locations: HashMap::new(), + alias_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), dependency_graph_indices: HashMap::new(), @@ -988,6 +992,14 @@ impl NodeInterner { self.struct_name_locations[struct_id] } + pub fn add_alias_location(&mut self, alias_id: TypeAliasId, location: Location) { + self.alias_name_locations.insert(alias_id, location); + } + + pub fn alias_location(&self, alias_id: &TypeAliasId) -> Location { + self.alias_name_locations[alias_id] + } + pub fn add_trait_location(&mut self, trait_id: TraitId, location: Location) { self.trait_name_locations.insert(trait_id, location); } From c97464a5aef49bc67bdd5fcf7c5534189dc7d071 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 15:39:54 -0300 Subject: [PATCH 16/29] No need to separately keep track of struct name locations --- .../noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 -- compiler/noirc_frontend/src/locations.rs | 6 +++++- compiler/noirc_frontend/src/node_interner.rs | 12 ------------ 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 222de08d8bf..f6a5e97bf2f 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -268,7 +268,6 @@ impl<'a> ModCollector<'a> { let mut definition_errors = vec![]; for struct_definition in types { let name = struct_definition.name.clone(); - let name_location = Location::new(name.span(), self.file_id); let unresolved = UnresolvedStruct { file_id: self.file_id, @@ -319,7 +318,6 @@ impl<'a> ModCollector<'a> { // And store the TypeId -> StructType mapping somewhere it is reachable self.def_collector.items.types.insert(id, unresolved); - context.def_interner.add_struct_location(id, name_location); context.def_interner.add_definition_location(ReferenceId::Struct(id)); } definition_errors diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 7927c965137..39529d06a40 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -33,7 +33,11 @@ impl NodeInterner { match reference { ReferenceId::Module(id) => self.module_location(&id), ReferenceId::Function(id) => self.function_modifiers(&id).name_location, - ReferenceId::Struct(id) => self.struct_location(&id), + ReferenceId::Struct(id) => { + let struct_type = self.get_struct(id); + let struct_type = struct_type.borrow(); + Location::new(struct_type.name.span(), struct_type.location.file) + } ReferenceId::Trait(id) => self.trait_location(&id), ReferenceId::Global(id) => self.get_global(id).location, ReferenceId::Alias(id) => self.alias_location(&id), diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 9b0371fac3f..9472f3f8018 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -68,9 +68,6 @@ pub struct NodeInterner { // The location of each module module_locations: HashMap, - // The location of each struct name - struct_name_locations: HashMap, - // The location of each trait name trait_name_locations: HashMap, @@ -550,7 +547,6 @@ impl Default for NodeInterner { function_modifiers: HashMap::new(), function_modules: HashMap::new(), module_locations: HashMap::new(), - struct_name_locations: HashMap::new(), trait_name_locations: HashMap::new(), alias_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), @@ -984,14 +980,6 @@ impl NodeInterner { &self.struct_attributes[struct_id] } - pub fn add_struct_location(&mut self, struct_id: StructId, location: Location) { - self.struct_name_locations.insert(struct_id, location); - } - - pub fn struct_location(&self, struct_id: &StructId) -> Location { - self.struct_name_locations[struct_id] - } - pub fn add_alias_location(&mut self, alias_id: TypeAliasId, location: Location) { self.alias_name_locations.insert(alias_id, location); } From 5465f6ab5386956a1028d7b03c5ebbc35d0d9afa Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 15:42:21 -0300 Subject: [PATCH 17/29] No need to separately keep track of alias name locations --- .../noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 -- compiler/noirc_frontend/src/locations.rs | 6 +++++- compiler/noirc_frontend/src/node_interner.rs | 12 ------------ 3 files changed, 5 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index f6a5e97bf2f..394a5090765 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -333,7 +333,6 @@ impl<'a> ModCollector<'a> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for type_alias in type_aliases { let name = type_alias.name.clone(); - let name_location = Location::new(name.span(), self.file_id); // And store the TypeId -> TypeAlias mapping somewhere it is reachable let unresolved = UnresolvedTypeAlias { @@ -366,7 +365,6 @@ impl<'a> ModCollector<'a> { self.def_collector.items.type_aliases.insert(type_alias_id, unresolved); - context.def_interner.add_alias_location(type_alias_id, name_location); context.def_interner.add_definition_location(ReferenceId::Alias(type_alias_id)); } errors diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 39529d06a40..cabe5daa3ff 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -40,7 +40,11 @@ impl NodeInterner { } ReferenceId::Trait(id) => self.trait_location(&id), ReferenceId::Global(id) => self.get_global(id).location, - ReferenceId::Alias(id) => self.alias_location(&id), + ReferenceId::Alias(id) => { + let alias_type = self.get_type_alias(id); + let alias_type = alias_type.borrow(); + Location::new(alias_type.name.span(), alias_type.location.file) + } ReferenceId::Variable(location) => location, } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 9472f3f8018..7e6e431adda 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -71,9 +71,6 @@ pub struct NodeInterner { // The location of each trait name trait_name_locations: HashMap, - // The location of each type alias name - alias_name_locations: HashMap, - /// This graph tracks dependencies between different global definitions. /// This is used to ensure the absence of dependency cycles for globals and types. dependency_graph: DiGraph, @@ -548,7 +545,6 @@ impl Default for NodeInterner { function_modules: HashMap::new(), module_locations: HashMap::new(), trait_name_locations: HashMap::new(), - alias_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), dependency_graph_indices: HashMap::new(), @@ -980,14 +976,6 @@ impl NodeInterner { &self.struct_attributes[struct_id] } - pub fn add_alias_location(&mut self, alias_id: TypeAliasId, location: Location) { - self.alias_name_locations.insert(alias_id, location); - } - - pub fn alias_location(&self, alias_id: &TypeAliasId) -> Location { - self.alias_name_locations[alias_id] - } - pub fn add_trait_location(&mut self, trait_id: TraitId, location: Location) { self.trait_name_locations.insert(trait_id, location); } From d164f52cc00da84fcadef83230cf213dca75101e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 15:45:39 -0300 Subject: [PATCH 18/29] No need to separately keep track of trait locations --- .../noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 -- compiler/noirc_frontend/src/locations.rs | 5 ++++- compiler/noirc_frontend/src/node_interner.rs | 12 ------------ 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 394a5090765..2f324043fd6 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -381,7 +381,6 @@ impl<'a> ModCollector<'a> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; for trait_definition in traits { let name = trait_definition.name.clone(); - let name_location = Location::new(name.span(), self.file_id); // Create the corresponding module for the trait namespace let trait_id = match self.push_child_module( @@ -533,7 +532,6 @@ impl<'a> ModCollector<'a> { }; context.def_interner.push_empty_trait(trait_id, &unresolved, resolved_generics); - context.def_interner.add_trait_location(trait_id, name_location); context.def_interner.add_definition_location(ReferenceId::Trait(trait_id)); self.def_collector.items.traits.insert(trait_id, unresolved); diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index cabe5daa3ff..6164bd1068e 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -38,7 +38,10 @@ impl NodeInterner { let struct_type = struct_type.borrow(); Location::new(struct_type.name.span(), struct_type.location.file) } - ReferenceId::Trait(id) => self.trait_location(&id), + ReferenceId::Trait(id) => { + let trait_type = self.get_trait(id); + Location::new(trait_type.name.span(), trait_type.location.file) + } ReferenceId::Global(id) => self.get_global(id).location, ReferenceId::Alias(id) => { let alias_type = self.get_type_alias(id); diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 7e6e431adda..eb5acbb14e5 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -68,9 +68,6 @@ pub struct NodeInterner { // The location of each module module_locations: HashMap, - // The location of each trait name - trait_name_locations: HashMap, - /// This graph tracks dependencies between different global definitions. /// This is used to ensure the absence of dependency cycles for globals and types. dependency_graph: DiGraph, @@ -544,7 +541,6 @@ impl Default for NodeInterner { function_modifiers: HashMap::new(), function_modules: HashMap::new(), module_locations: HashMap::new(), - trait_name_locations: HashMap::new(), func_id_to_trait: HashMap::new(), dependency_graph: petgraph::graph::DiGraph::new(), dependency_graph_indices: HashMap::new(), @@ -976,14 +972,6 @@ impl NodeInterner { &self.struct_attributes[struct_id] } - pub fn add_trait_location(&mut self, trait_id: TraitId, location: Location) { - self.trait_name_locations.insert(trait_id, location); - } - - pub fn trait_location(&self, trait_id: &TraitId) -> Location { - self.trait_name_locations[trait_id] - } - pub fn add_module_location(&mut self, module_id: ModuleId, location: Location) { self.module_locations.insert(module_id, location); } From 9e097aa354076b4810814c8dd4358e0e90b32231 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 4 Jul 2024 16:49:59 -0300 Subject: [PATCH 19/29] feat: lsp rename/find-all-references for globals --- compiler/noirc_frontend/src/elaborator/mod.rs | 2 ++ .../src/hir/def_collector/dc_crate.rs | 27 +++++++---------- compiler/noirc_frontend/src/locations.rs | 30 +++++++------------ tooling/lsp/src/requests/rename.rs | 5 ++++ .../test_programs/rename_global/Nargo.toml | 6 ++++ .../test_programs/rename_global/src/main.nr | 10 +++++++ .../rename_type_alias/src/main.nr | 7 +++++ 7 files changed, 51 insertions(+), 36 deletions(-) create mode 100644 tooling/lsp/test_programs/rename_global/Nargo.toml create mode 100644 tooling/lsp/test_programs/rename_global/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 7c829b5df47..e4d339bf442 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1350,6 +1350,8 @@ impl<'context> Elaborator<'context> { self.elaborate_comptime_global(global_id); } + self.interner.add_definition_location(ReferenceId::Global(global_id)); + self.local_module = old_module; self.file = old_file; self.current_item = old_item; diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index ee9981863cb..390f6528592 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -511,25 +511,18 @@ fn add_import_reference( return; } - match def_id { - crate::macros_api::ModuleDefId::FunctionId(func_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); - interner.add_reference(ReferenceId::Function(func_id), variable); - } - crate::macros_api::ModuleDefId::TypeId(struct_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); - interner.add_reference(ReferenceId::Struct(struct_id), variable); - } - crate::macros_api::ModuleDefId::TraitId(trait_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); - interner.add_reference(ReferenceId::Trait(trait_id), variable); - } + let referenced = match def_id { + crate::macros_api::ModuleDefId::ModuleId(module_id) => ReferenceId::Module(module_id), + crate::macros_api::ModuleDefId::FunctionId(func_id) => ReferenceId::Function(func_id), + crate::macros_api::ModuleDefId::TypeId(struct_id) => ReferenceId::Struct(struct_id), + crate::macros_api::ModuleDefId::TraitId(trait_id) => ReferenceId::Trait(trait_id), crate::macros_api::ModuleDefId::TypeAliasId(type_alias_id) => { - let variable = ReferenceId::Variable(Location::new(name.span(), file_id), false); - interner.add_reference(ReferenceId::Alias(type_alias_id), variable); + ReferenceId::Alias(type_alias_id) } - _ => (), - } + crate::macros_api::ModuleDefId::GlobalId(global_id) => ReferenceId::Global(global_id), + }; + let reference = ReferenceId::Variable(Location::new(name.span(), file_id), false); + interner.add_reference(referenced, reference); } fn inject_prelude( diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 30095fbb8c7..dae7edb21e6 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -117,26 +117,18 @@ impl NodeInterner { let node_index = self.location_indices.get_node_from_location(location)?; let reference_node = self.reference_graph[node_index]; - let found_locations: Vec = match reference_node { - ReferenceId::Global(_) | ReferenceId::Module(_) => todo!(), - ReferenceId::Function(_) - | ReferenceId::Struct(_) - | ReferenceId::Trait(_) - | ReferenceId::Alias(_) => self.find_all_references_for_index( - node_index, - include_referenced, - include_self_type_name, - ), - - ReferenceId::Variable(_, _) => { - let referenced_node_index = self.referenced_index(node_index)?; - self.find_all_references_for_index( - referenced_node_index, - include_referenced, - include_self_type_name, - ) - } + let referenced_node_index = if let ReferenceId::Variable(_, _) = reference_node { + self.referenced_index(node_index)? + } else { + node_index }; + + let found_locations = self.find_all_references_for_index( + referenced_node_index, + include_referenced, + include_self_type_name, + ); + Some(found_locations) } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 54c3297afb9..24d15f91c79 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -189,4 +189,9 @@ mod rename_tests { async fn test_rename_type_alias() { check_rename_succeeds("rename_type_alias", "Bar").await; } + + #[test] + async fn test_rename_global() { + check_rename_succeeds("rename_global", "FOO").await; + } } diff --git a/tooling/lsp/test_programs/rename_global/Nargo.toml b/tooling/lsp/test_programs/rename_global/Nargo.toml new file mode 100644 index 00000000000..350c6fe5506 --- /dev/null +++ b/tooling/lsp/test_programs/rename_global/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "rename_global" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/lsp/test_programs/rename_global/src/main.nr b/tooling/lsp/test_programs/rename_global/src/main.nr new file mode 100644 index 00000000000..ed5f1508e93 --- /dev/null +++ b/tooling/lsp/test_programs/rename_global/src/main.nr @@ -0,0 +1,10 @@ +mod foo { + global FOO = 1; +} + +use foo::FOO; + +fn main() { + let _ = foo::FOO; + let _ = FOO; +} diff --git a/tooling/lsp/test_programs/rename_type_alias/src/main.nr b/tooling/lsp/test_programs/rename_type_alias/src/main.nr index 4a0c497a21f..2072d9cae87 100644 --- a/tooling/lsp/test_programs/rename_type_alias/src/main.nr +++ b/tooling/lsp/test_programs/rename_type_alias/src/main.nr @@ -3,10 +3,17 @@ mod foo { } type Bar = Foo; + + mod bar { + struct Baz { + + } + } } use foo::Foo; use foo::Bar; +use foo::bar; fn main() { let x: Bar = Foo {}; From 4968178fb74623bde6c4b65129c517da1db9648d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 8 Jul 2024 11:41:23 -0300 Subject: [PATCH 20/29] Rename ReferenceId::Variable to ReferenceId::Reference --- compiler/noirc_frontend/src/elaborator/expressions.rs | 2 +- compiler/noirc_frontend/src/elaborator/mod.rs | 2 +- compiler/noirc_frontend/src/elaborator/patterns.rs | 8 +++++--- compiler/noirc_frontend/src/elaborator/scope.rs | 2 +- compiler/noirc_frontend/src/elaborator/types.rs | 4 ++-- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 4 ++-- compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs | 2 +- compiler/noirc_frontend/src/locations.rs | 4 ++-- compiler/noirc_frontend/src/node_interner.rs | 4 ++-- tooling/lsp/src/requests/rename.rs | 2 +- 10 files changed, 18 insertions(+), 16 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 5c3866816a6..b35b36d4270 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -434,7 +434,7 @@ impl<'context> Elaborator<'context> { }); let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(span, self.file), is_self_type); + let reference = ReferenceId::Reference(Location::new(span, self.file), is_self_type); self.interner.add_reference(referenced, reference); (expr, Type::Struct(struct_type, generics)) diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index e4d339bf442..8e6ddcc72fb 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1454,7 +1454,7 @@ impl<'context> Elaborator<'context> { let trait_name = trait_impl.trait_path.last_segment(); let referenced = ReferenceId::Trait(trait_id); - let reference = ReferenceId::Variable( + let reference = ReferenceId::Reference( Location::new(trait_name.span(), trait_impl.file_id), trait_name.is_self_type_name(), ); diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 81fffb522bf..65627741fb7 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -201,7 +201,7 @@ impl<'context> Elaborator<'context> { ); let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable(Location::new(name_span, self.file), is_self_type); + let reference = ReferenceId::Reference(Location::new(name_span, self.file), is_self_type); self.interner.add_reference(referenced, reference); HirPattern::Struct(expected_type, fields, location) @@ -448,7 +448,8 @@ impl<'context> Elaborator<'context> { self.interner.add_function_dependency(current_item, func_id); } - let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name); + let variable = + ReferenceId::Reference(hir_ident.location, is_self_type_name); let function = ReferenceId::Function(func_id); self.interner.add_reference(function, variable); } @@ -460,7 +461,8 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, global_id); } - let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name); + let variable = + ReferenceId::Reference(hir_ident.location, is_self_type_name); let global = ReferenceId::Global(global_id); self.interner.add_reference(global, variable); } diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index 6c556e000d5..b7016280453 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -53,7 +53,7 @@ impl<'context> Elaborator<'context> { resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; for (referenced, ident) in references.iter().zip(path.segments) { - let reference = ReferenceId::Variable( + let reference = ReferenceId::Reference( Location::new(ident.span(), self.file), ident.is_self_type_name(), ); diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 6c4eac287ef..06049be32c8 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -164,7 +164,7 @@ impl<'context> Elaborator<'context> { if !is_synthetic { let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Variable( + let reference = ReferenceId::Reference( Location::new(unresolved_span, self.file), is_self_type_name, ); @@ -173,7 +173,7 @@ impl<'context> Elaborator<'context> { } Type::Alias(ref alias_type, _) => { let referenced = ReferenceId::Alias(alias_type.borrow().id); - let reference = ReferenceId::Variable( + let reference = ReferenceId::Reference( Location::new(unresolved_span, self.file), is_self_type_name, ); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 390f6528592..43ab6224ea7 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -344,7 +344,7 @@ impl DefCollector { for (referenced, ident) in references.iter().zip(&collected_import.path.segments) { let reference = - ReferenceId::Variable(Location::new(ident.span(), file_id), false); + ReferenceId::Reference(Location::new(ident.span(), file_id), false); context.def_interner.add_reference(*referenced, reference); } @@ -521,7 +521,7 @@ fn add_import_reference( } crate::macros_api::ModuleDefId::GlobalId(global_id) => ReferenceId::Global(global_id), }; - let reference = ReferenceId::Variable(Location::new(name.span(), file_id), false); + let reference = ReferenceId::Reference(Location::new(name.span(), file_id), false); interner.add_reference(referenced, reference); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 483b061998e..48985116f4f 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -650,7 +650,7 @@ impl<'a> ModCollector<'a> { Ok(child_mod_id) => { // Track that the "foo" in `mod foo;` points to the module "foo" let referenced = ReferenceId::Module(child_mod_id); - let reference = ReferenceId::Variable(location, false); + let reference = ReferenceId::Reference(location, false); context.def_interner.add_reference(referenced, reference); errors.extend(collect_defs( diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index dae7edb21e6..aac1fa2f9f6 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -48,7 +48,7 @@ impl NodeInterner { let alias_type = alias_type.borrow(); Location::new(alias_type.name.span(), alias_type.location.file) } - ReferenceId::Variable(location, _) => location, + ReferenceId::Reference(location, _) => location, } } @@ -117,7 +117,7 @@ impl NodeInterner { let node_index = self.location_indices.get_node_from_location(location)?; let reference_node = self.reference_graph[node_index]; - let referenced_node_index = if let ReferenceId::Variable(_, _) = reference_node { + let referenced_node_index = if let ReferenceId::Reference(_, _) = reference_node { self.referenced_index(node_index)? } else { node_index diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 1618058a9f0..5793922b9b7 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -245,12 +245,12 @@ pub enum ReferenceId { Global(GlobalId), Function(FuncId), Alias(TypeAliasId), - Variable(Location, bool /* is Self */), + Reference(Location, bool /* is Self */), } impl ReferenceId { pub fn is_self_type_name(&self) -> bool { - matches!(self, Self::Variable(_, true)) + matches!(self, Self::Reference(_, true)) } } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 24d15f91c79..db3d6091cae 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -21,7 +21,7 @@ pub(crate) fn on_prepare_rename_request( let reference_id = interner.reference_at_location(location); let rename_possible = match reference_id { // Rename shouldn't be possible when triggered on top of "Self" - Some(ReferenceId::Variable(_, true /* is self type name */)) => false, + Some(ReferenceId::Reference(_, true /* is self type name */)) => false, Some(_) => true, None => false, }; From 56b7ea641349f00e1f6b2dc10774bc891fd2a191 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 8 Jul 2024 12:36:35 -0300 Subject: [PATCH 21/29] lsp rename/find-all-references for local variables --- compiler/noirc_frontend/src/elaborator/patterns.rs | 6 ++++++ compiler/noirc_frontend/src/elaborator/statements.rs | 6 +++++- compiler/noirc_frontend/src/locations.rs | 1 + compiler/noirc_frontend/src/node_interner.rs | 8 ++++++++ tooling/lsp/src/requests/goto_definition.rs | 5 +++++ tooling/lsp/src/requests/rename.rs | 5 +++++ tooling/lsp/test_programs/local_variable/Nargo.toml | 6 ++++++ tooling/lsp/test_programs/local_variable/src/main.nr | 5 +++++ 8 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 tooling/lsp/test_programs/local_variable/Nargo.toml create mode 100644 tooling/lsp/test_programs/local_variable/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 65627741fb7..2abb453778d 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -438,6 +438,7 @@ impl<'context> Elaborator<'context> { // Otherwise, then it is referring to an Identifier // This lookup allows support of such statements: let x = foo::bar::SOME_GLOBAL + 10; // If the expression is a singular indent, we search the resolver's current scope as normal. + let span = path.span(); let is_self_type_name = path.last_segment().is_self_type_name(); let (hir_ident, var_scope_index) = self.get_ident_from_path(path); @@ -479,6 +480,11 @@ impl<'context> Elaborator<'context> { DefinitionKind::Local(_) => { // only local variables can be captured by closures. self.resolve_local_variable(hir_ident.clone(), var_scope_index); + + let referenced = ReferenceId::Local(hir_ident.id); + let reference = + ReferenceId::Reference(Location::new(span, self.file), false); + self.interner.add_reference(referenced, reference); } } } diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 8d97bd1a25d..3af73b5d52e 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -16,7 +16,7 @@ use crate::{ macros_api::{ ForLoopStatement, ForRange, HirStatement, LetStatement, Path, Statement, StatementKind, }, - node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId}, + node_interner::{DefinitionId, DefinitionKind, GlobalId, ReferenceId, StmtId}, Type, }; @@ -256,6 +256,10 @@ impl<'context> Elaborator<'context> { typ.follow_bindings() }; + let referenced = ReferenceId::Local(ident.id); + let reference = ReferenceId::Reference(Location::new(span, self.file), false); + self.interner.add_reference(referenced, reference); + (HirLValue::Ident(ident.clone(), typ.clone()), typ, mutable) } LValue::MemberAccess { object, field_name, span } => { diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index aac1fa2f9f6..fcaef0a8dd6 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -48,6 +48,7 @@ impl NodeInterner { let alias_type = alias_type.borrow(); Location::new(alias_type.name.span(), alias_type.location.file) } + ReferenceId::Local(id) => self.definition(id).location, ReferenceId::Reference(location, _) => location, } } diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 5793922b9b7..588b56afa1a 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -245,6 +245,7 @@ pub enum ReferenceId { Global(GlobalId), Function(FuncId), Alias(TypeAliasId), + Local(DefinitionId), Reference(Location, bool /* is Self */), } @@ -836,12 +837,19 @@ impl NodeInterner { location: Location, ) -> DefinitionId { let id = DefinitionId(self.definitions.len()); + let is_local = matches!(definition, DefinitionKind::Local(_)); + if let DefinitionKind::Function(func_id) = definition { self.function_definition_ids.insert(func_id, id); } let kind = definition; self.definitions.push(DefinitionInfo { name, mutable, comptime, kind, location }); + + if is_local { + self.add_definition_location(ReferenceId::Local(id)); + } + id } diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index fe591a433cf..3713e8b646a 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -196,4 +196,9 @@ mod goto_definition_tests { ) .await; } + + #[test] + async fn goto_for_local_variable() { + expect_goto_for_all_references("local_variable", "some_var", 0).await; + } } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index db3d6091cae..ac6c6792e15 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -194,4 +194,9 @@ mod rename_tests { async fn test_rename_global() { check_rename_succeeds("rename_global", "FOO").await; } + + #[test] + async fn test_rename_local_variable() { + check_rename_succeeds("local_variable", "some_var").await; + } } diff --git a/tooling/lsp/test_programs/local_variable/Nargo.toml b/tooling/lsp/test_programs/local_variable/Nargo.toml new file mode 100644 index 00000000000..df881fb5f4d --- /dev/null +++ b/tooling/lsp/test_programs/local_variable/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "local_variable" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/lsp/test_programs/local_variable/src/main.nr b/tooling/lsp/test_programs/local_variable/src/main.nr new file mode 100644 index 00000000000..e41cbed085f --- /dev/null +++ b/tooling/lsp/test_programs/local_variable/src/main.nr @@ -0,0 +1,5 @@ +fn main() { + let mut some_var = 1; + some_var = 2; + let _ = some_var; +} From 37b763ead8a9811cec7adca64d67b3e4d97eaace Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 8 Jul 2024 15:36:39 -0300 Subject: [PATCH 22/29] feat: lsp rename/find-all-references for struct members --- .../src/elaborator/expressions.rs | 20 ++++++++++++++++--- compiler/noirc_frontend/src/elaborator/mod.rs | 6 ++++++ .../noirc_frontend/src/elaborator/patterns.rs | 10 +++++++++- .../src/elaborator/statements.rs | 4 ++++ compiler/noirc_frontend/src/hir_def/types.rs | 5 +++++ compiler/noirc_frontend/src/locations.rs | 5 +++++ compiler/noirc_frontend/src/node_interner.rs | 1 + tooling/lsp/src/requests/rename.rs | 5 +++++ .../test_programs/struct_member/Nargo.toml | 6 ++++++ .../test_programs/struct_member/src/main.nr | 12 +++++++++++ 10 files changed, 70 insertions(+), 4 deletions(-) create mode 100644 tooling/lsp/test_programs/struct_member/Nargo.toml create mode 100644 tooling/lsp/test_programs/struct_member/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index b35b36d4270..026e8e0d323 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -452,11 +452,17 @@ impl<'context> Elaborator<'context> { ) -> Vec<(Ident, ExprId)> { let mut ret = Vec::with_capacity(fields.len()); let mut seen_fields = HashSet::default(); + let struct_id = struct_type.borrow().id; let mut unseen_fields = struct_type.borrow().field_names(); for (field_name, field) in fields { - let expected_type = field_types.iter().find(|(name, _)| name == &field_name.0.contents); - let expected_type = expected_type.map(|(_, typ)| typ).unwrap_or(&Type::Error); + let expected_field_with_index = field_types + .iter() + .enumerate() + .find(|(_, (name, _))| name == &field_name.0.contents); + let expected_index = expected_field_with_index.map(|(index, _)| index); + let expected_type = + expected_field_with_index.map(|(_, (_, typ))| typ).unwrap_or(&Type::Error); let field_span = field.span; let (resolved, field_type) = self.elaborate_expression(field); @@ -483,6 +489,13 @@ impl<'context> Elaborator<'context> { }); } + if let Some(expected_index) = expected_index { + let referenced = ReferenceId::StructMember(struct_id, expected_index); + let reference = + ReferenceId::Reference(Location::new(field_name.span(), self.file), false); + self.interner.add_reference(referenced, reference); + } + ret.push((field_name, resolved)); } @@ -504,10 +517,11 @@ impl<'context> Elaborator<'context> { ) -> (ExprId, Type) { let (lhs, lhs_type) = self.elaborate_expression(access.lhs); let rhs = access.rhs; + let rhs_span = rhs.span(); // `is_offset` is only used when lhs is a reference and we want to return a reference to rhs let access = HirMemberAccess { lhs, rhs, is_offset: false }; let expr_id = self.intern_expr(HirExpression::MemberAccess(access.clone()), span); - let typ = self.type_check_member_access(access, expr_id, lhs_type, span); + let typ = self.type_check_member_access(access, expr_id, lhs_type, rhs_span); self.interner.push_expr_type(expr_id, typ.clone()); (expr_id, typ) } diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 8e6ddcc72fb..7f8bc6b1d93 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1192,6 +1192,7 @@ impl<'context> Elaborator<'context> { let span = typ.struct_def.span; let fields = self.resolve_struct_fields(typ.struct_def, type_id); + let fields_len = fields.len(); self.interner.update_struct(type_id, |struct_def| { struct_def.set_fields(fields); @@ -1218,6 +1219,11 @@ impl<'context> Elaborator<'context> { } }); + for field_index in 0..fields_len { + self.interner + .add_definition_location(ReferenceId::StructMember(type_id, field_index)); + } + self.run_comptime_attributes_on_struct(attributes, type_id, span, &mut generated_items); } diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 2abb453778d..0e35d5c08f5 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -200,10 +200,18 @@ impl<'context> Elaborator<'context> { new_definitions, ); - let referenced = ReferenceId::Struct(struct_type.borrow().id); + let struct_id = struct_type.borrow().id; + + let referenced = ReferenceId::Struct(struct_id); let reference = ReferenceId::Reference(Location::new(name_span, self.file), is_self_type); self.interner.add_reference(referenced, reference); + for (field_index, field) in fields.iter().enumerate() { + let referenced = ReferenceId::StructMember(struct_id, field_index); + let reference = ReferenceId::Reference(Location::new(field.0.span(), self.file), false); + self.interner.add_reference(referenced, reference); + } + HirPattern::Struct(expected_type, fields, location) } diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 3af73b5d52e..febd1968578 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -381,6 +381,10 @@ impl<'context> Elaborator<'context> { Type::Struct(s, args) => { let s = s.borrow(); if let Some((field, index)) = s.get_field(field_name, args) { + let referenced = ReferenceId::StructMember(s.id, index); + let reference = ReferenceId::Reference(Location::new(span, self.file), false); + self.interner.add_reference(referenced, reference); + return Some((field, index)); } } diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index b529ca17887..163339fd61d 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -342,6 +342,11 @@ impl StructType { vecmap(&self.fields, |(name, typ)| (name.0.contents.clone(), typ.clone())) } + /// Returns the field at the given index. Panics if no field exists at the given index. + pub fn field_at(&self, index: usize) -> &(Ident, Type) { + &self.fields[index] + } + pub fn field_names(&self) -> BTreeSet { self.fields.iter().map(|(name, _)| name.clone()).collect() } diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index fcaef0a8dd6..0efe385aa0a 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -38,6 +38,11 @@ impl NodeInterner { let struct_type = struct_type.borrow(); Location::new(struct_type.name.span(), struct_type.location.file) } + ReferenceId::StructMember(id, field_index) => { + let struct_type = self.get_struct(id); + let struct_type = struct_type.borrow(); + Location::new(struct_type.field_at(field_index).0.span(), struct_type.location.file) + } ReferenceId::Trait(id) => { let trait_type = self.get_trait(id); Location::new(trait_type.name.span(), trait_type.location.file) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 588b56afa1a..2de9b04f591 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -241,6 +241,7 @@ pub enum DependencyId { pub enum ReferenceId { Module(ModuleId), Struct(StructId), + StructMember(StructId, usize), Trait(TraitId), Global(GlobalId), Function(FuncId), diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index ac6c6792e15..906a5cbcaab 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -199,4 +199,9 @@ mod rename_tests { async fn test_rename_local_variable() { check_rename_succeeds("local_variable", "some_var").await; } + + #[test] + async fn test_rename_struct_member() { + check_rename_succeeds("struct_member", "some_member").await; + } } diff --git a/tooling/lsp/test_programs/struct_member/Nargo.toml b/tooling/lsp/test_programs/struct_member/Nargo.toml new file mode 100644 index 00000000000..5272b9abb68 --- /dev/null +++ b/tooling/lsp/test_programs/struct_member/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "struct_member" +type = "bin" +authors = [""] + +[dependencies] diff --git a/tooling/lsp/test_programs/struct_member/src/main.nr b/tooling/lsp/test_programs/struct_member/src/main.nr new file mode 100644 index 00000000000..3f1bac9df66 --- /dev/null +++ b/tooling/lsp/test_programs/struct_member/src/main.nr @@ -0,0 +1,12 @@ +struct Foo { + some_member: Field +} + +fn main() { + let mut foo = Foo { some_member: 1 }; + foo.some_member = 2; + let _ = foo.some_member; + + let Foo { some_member } = foo; + let Foo { some_member: some_var } = foo; +} From 5e543859afa7d012e4068c99fd83c26071366292 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 10 Jul 2024 11:18:05 -0300 Subject: [PATCH 23/29] Move struct_id definition closer to its usage --- compiler/noirc_frontend/src/elaborator/expressions.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index 33255cb5c7b..d2e3e153518 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -437,7 +437,6 @@ impl<'context> Elaborator<'context> { ) -> Vec<(Ident, ExprId)> { let mut ret = Vec::with_capacity(fields.len()); let mut seen_fields = HashSet::default(); - let struct_id = struct_type.borrow().id; let mut unseen_fields = struct_type.borrow().field_names(); for (field_name, field) in fields { @@ -475,6 +474,7 @@ impl<'context> Elaborator<'context> { } if let Some(expected_index) = expected_index { + let struct_id = struct_type.borrow().id; let referenced = ReferenceId::StructMember(struct_id, expected_index); let reference = ReferenceId::Reference(Location::new(field_name.span(), self.file), false); From 2d4d013e26474e13b133071f88bfc08011a5430b Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 9 Jul 2024 08:54:37 -0300 Subject: [PATCH 24/29] Easier way to track references --- .../src/elaborator/expressions.rs | 18 +++--- compiler/noirc_frontend/src/elaborator/mod.rs | 6 +- .../noirc_frontend/src/elaborator/patterns.rs | 31 +++------- .../noirc_frontend/src/elaborator/scope.rs | 4 +- .../src/elaborator/statements.rs | 12 ++-- .../noirc_frontend/src/elaborator/types.rs | 30 +++------ .../src/hir/def_collector/dc_crate.rs | 36 +++++++---- .../src/hir/def_collector/dc_mod.rs | 4 +- compiler/noirc_frontend/src/locations.rs | 62 ++++++++++++++++++- 9 files changed, 122 insertions(+), 81 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs index d2e3e153518..9a018cfd04f 100644 --- a/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -27,7 +27,7 @@ use crate::{ HirLiteral, HirStatement, Ident, IndexExpression, Literal, MemberAccessExpression, MethodCallExpression, PrefixExpression, }, - node_interner::{DefinitionKind, ExprId, FuncId, ReferenceId}, + node_interner::{DefinitionKind, ExprId, FuncId}, token::Tokens, QuotedType, Shared, StructType, Type, }; @@ -418,9 +418,9 @@ impl<'context> Elaborator<'context> { struct_generics, }); - let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Reference(Location::new(span, self.file), is_self_type); - self.interner.add_reference(referenced, reference); + let struct_id = struct_type.borrow().id; + let reference_location = Location::new(span, self.file); + self.interner.add_struct_reference(struct_id, reference_location, is_self_type); (expr, Type::Struct(struct_type, generics)) } @@ -474,11 +474,11 @@ impl<'context> Elaborator<'context> { } if let Some(expected_index) = expected_index { - let struct_id = struct_type.borrow().id; - let referenced = ReferenceId::StructMember(struct_id, expected_index); - let reference = - ReferenceId::Reference(Location::new(field_name.span(), self.file), false); - self.interner.add_reference(referenced, reference); + self.interner.add_struct_member_reference( + struct_type.borrow().id, + expected_index, + Location::new(field_name.span(), self.file), + ); } ret.push((field_name, resolved)); diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index f9ae2570b93..34c14b3cc48 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -1457,13 +1457,11 @@ impl<'context> Elaborator<'context> { if let Some(trait_id) = trait_id { let trait_name = trait_impl.trait_path.last_segment(); - - let referenced = ReferenceId::Trait(trait_id); - let reference = ReferenceId::Reference( + self.interner.add_trait_reference( + trait_id, Location::new(trait_name.span(), trait_impl.file_id), trait_name.is_self_type_name(), ); - self.interner.add_reference(referenced, reference); } } } diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs index 0e4822d76e3..98b369ed7a8 100644 --- a/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -15,9 +15,7 @@ use crate::{ stmt::HirPattern, }, macros_api::{HirExpression, Ident, Path, Pattern}, - node_interner::{ - DefinitionId, DefinitionKind, ExprId, FuncId, GlobalId, ReferenceId, TraitImplKind, - }, + node_interner::{DefinitionId, DefinitionKind, ExprId, FuncId, GlobalId, TraitImplKind}, Shared, StructType, Type, TypeBindings, }; @@ -204,14 +202,12 @@ impl<'context> Elaborator<'context> { let struct_id = struct_type.borrow().id; - let referenced = ReferenceId::Struct(struct_id); - let reference = ReferenceId::Reference(Location::new(name_span, self.file), is_self_type); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(name_span, self.file); + self.interner.add_struct_reference(struct_id, reference_location, is_self_type); for (field_index, field) in fields.iter().enumerate() { - let referenced = ReferenceId::StructMember(struct_id, field_index); - let reference = ReferenceId::Reference(Location::new(field.0.span(), self.file), false); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(field.0.span(), self.file); + self.interner.add_struct_member_reference(struct_id, field_index, reference_location); } HirPattern::Struct(expected_type, fields, location) @@ -487,7 +483,6 @@ impl<'context> Elaborator<'context> { // This lookup allows support of such statements: let x = foo::bar::SOME_GLOBAL + 10; // If the expression is a singular indent, we search the resolver's current scope as normal. let span = path.span(); - let is_self_type_name = path.last_segment().is_self_type_name(); let (hir_ident, var_scope_index) = self.get_ident_from_path(path); if hir_ident.id != DefinitionId::dummy_id() { @@ -497,10 +492,7 @@ impl<'context> Elaborator<'context> { self.interner.add_function_dependency(current_item, func_id); } - let variable = - ReferenceId::Reference(hir_ident.location, is_self_type_name); - let function = ReferenceId::Function(func_id); - self.interner.add_reference(function, variable); + self.interner.add_function_reference(func_id, hir_ident.location); } DefinitionKind::Global(global_id) => { if let Some(global) = self.unresolved_globals.remove(&global_id) { @@ -510,10 +502,7 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, global_id); } - let variable = - ReferenceId::Reference(hir_ident.location, is_self_type_name); - let global = ReferenceId::Global(global_id); - self.interner.add_reference(global, variable); + self.interner.add_global_reference(global_id, hir_ident.location); } DefinitionKind::GenericType(_) => { // Initialize numeric generics to a polymorphic integer type in case @@ -529,10 +518,8 @@ impl<'context> Elaborator<'context> { // only local variables can be captured by closures. self.resolve_local_variable(hir_ident.clone(), var_scope_index); - let referenced = ReferenceId::Local(hir_ident.id); - let reference = - ReferenceId::Reference(Location::new(span, self.file), false); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(span, self.file); + self.interner.add_local_reference(hir_ident.id, reference_location); } } } diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs index b7016280453..af6fc0e5d5e 100644 --- a/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/compiler/noirc_frontend/src/elaborator/scope.rs @@ -53,11 +53,11 @@ impl<'context> Elaborator<'context> { resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?; for (referenced, ident) in references.iter().zip(path.segments) { - let reference = ReferenceId::Reference( + self.interner.add_reference( + *referenced, Location::new(ident.span(), self.file), ident.is_self_type_name(), ); - self.interner.add_reference(*referenced, reference); } } else { path_resolution = resolver.resolve(self.def_maps, path, &mut None)?; diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs index 9410324496d..f76bd34e586 100644 --- a/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/compiler/noirc_frontend/src/elaborator/statements.rs @@ -16,7 +16,7 @@ use crate::{ macros_api::{ ForLoopStatement, ForRange, HirStatement, LetStatement, Path, Statement, StatementKind, }, - node_interner::{DefinitionId, DefinitionKind, GlobalId, ReferenceId, StmtId}, + node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId}, Type, }; @@ -256,9 +256,8 @@ impl<'context> Elaborator<'context> { typ.follow_bindings() }; - let referenced = ReferenceId::Local(ident.id); - let reference = ReferenceId::Reference(Location::new(span, self.file), false); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(span, self.file); + self.interner.add_local_reference(ident.id, reference_location); (HirLValue::Ident(ident.clone(), typ.clone()), typ, mutable) } @@ -381,9 +380,8 @@ impl<'context> Elaborator<'context> { Type::Struct(s, args) => { let s = s.borrow(); if let Some((field, index)) = s.get_field(field_name, args) { - let referenced = ReferenceId::StructMember(s.id, index); - let reference = ReferenceId::Reference(Location::new(span, self.file), false); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(span, self.file); + self.interner.add_struct_member_reference(s.id, index, reference_location); return Some((field, index)); } diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 698114cfb5e..e7030eb8229 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -31,8 +31,7 @@ use crate::{ UnaryOp, UnresolvedType, UnresolvedTypeData, }, node_interner::{ - DefinitionKind, DependencyId, ExprId, GlobalId, ReferenceId, TraitId, TraitImplKind, - TraitMethodId, + DefinitionKind, DependencyId, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId, }, Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeVariable, TypeVariableKind, }; @@ -154,30 +153,23 @@ impl<'context> Elaborator<'context> { }; if let Some(unresolved_span) = typ.span { + let location = Location::new(unresolved_span, self.file); + match resolved_type { Type::Struct(ref struct_type, _) => { // Record the location of the type reference - self.interner.push_type_ref_location( - resolved_type.clone(), - Location::new(unresolved_span, self.file), - ); + self.interner.push_type_ref_location(resolved_type.clone(), location); if !is_synthetic { - let referenced = ReferenceId::Struct(struct_type.borrow().id); - let reference = ReferenceId::Reference( - Location::new(unresolved_span, self.file), + self.interner.add_struct_reference( + struct_type.borrow().id, + location, is_self_type_name, ); - self.interner.add_reference(referenced, reference); } } Type::Alias(ref alias_type, _) => { - let referenced = ReferenceId::Alias(alias_type.borrow().id); - let reference = ReferenceId::Reference( - Location::new(unresolved_span, self.file), - is_self_type_name, - ); - self.interner.add_reference(referenced, reference); + self.interner.add_alias_reference(alias_type.borrow().id, location); } _ => (), } @@ -369,10 +361,8 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, id); } - let referenced = ReferenceId::Global(id); - let reference = - ReferenceId::Reference(Location::new(path.span(), self.file), false); - self.interner.add_reference(referenced, reference); + let reference_location = Location::new(path.span(), self.file); + self.interner.add_global_reference(id, reference_location); Some(Type::Constant(self.eval_global_as_array_length(id, path))) } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 43ab6224ea7..f04f5d635cf 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -343,9 +343,11 @@ impl DefCollector { let file_id = current_def_map.file_id(module_id); for (referenced, ident) in references.iter().zip(&collected_import.path.segments) { - let reference = - ReferenceId::Reference(Location::new(ident.span(), file_id), false); - context.def_interner.add_reference(*referenced, reference); + context.def_interner.add_reference( + *referenced, + Location::new(ident.span(), file_id), + false, + ); } resolved_import @@ -511,18 +513,28 @@ fn add_import_reference( return; } - let referenced = match def_id { - crate::macros_api::ModuleDefId::ModuleId(module_id) => ReferenceId::Module(module_id), - crate::macros_api::ModuleDefId::FunctionId(func_id) => ReferenceId::Function(func_id), - crate::macros_api::ModuleDefId::TypeId(struct_id) => ReferenceId::Struct(struct_id), - crate::macros_api::ModuleDefId::TraitId(trait_id) => ReferenceId::Trait(trait_id), + let location = Location::new(name.span(), file_id); + + match def_id { + crate::macros_api::ModuleDefId::ModuleId(module_id) => { + interner.add_module_location(module_id, location) + } + crate::macros_api::ModuleDefId::FunctionId(func_id) => { + interner.add_function_reference(func_id, location) + } + crate::macros_api::ModuleDefId::TypeId(struct_id) => { + interner.add_struct_reference(struct_id, location, false) + } + crate::macros_api::ModuleDefId::TraitId(trait_id) => { + interner.add_trait_reference(trait_id, location, false) + } crate::macros_api::ModuleDefId::TypeAliasId(type_alias_id) => { - ReferenceId::Alias(type_alias_id) + interner.add_alias_reference(type_alias_id, location) + } + crate::macros_api::ModuleDefId::GlobalId(global_id) => { + interner.add_global_reference(global_id, location) } - crate::macros_api::ModuleDefId::GlobalId(global_id) => ReferenceId::Global(global_id), }; - let reference = ReferenceId::Reference(Location::new(name.span(), file_id), false); - interner.add_reference(referenced, reference); } fn inject_prelude( diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 48985116f4f..bcd24ca8ed3 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -649,9 +649,7 @@ impl<'a> ModCollector<'a> { ) { Ok(child_mod_id) => { // Track that the "foo" in `mod foo;` points to the module "foo" - let referenced = ReferenceId::Module(child_mod_id); - let reference = ReferenceId::Reference(location, false); - context.def_interner.add_reference(referenced, reference); + context.def_interner.add_module_reference(child_mod_id, location); errors.extend(collect_defs( self.def_collector, diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 0efe385aa0a..425d2947062 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -3,7 +3,11 @@ use noirc_errors::Location; use rangemap::RangeMap; use rustc_hash::FxHashMap; -use crate::{macros_api::NodeInterner, node_interner::ReferenceId}; +use crate::{ + hir::def_map::ModuleId, + macros_api::{NodeInterner, StructId}, + node_interner::{DefinitionId, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId}, +}; use petgraph::prelude::NodeIndex as PetGraphIndex; #[derive(Debug, Default)] @@ -58,11 +62,65 @@ impl NodeInterner { } } - pub(crate) fn add_reference(&mut self, referenced: ReferenceId, reference: ReferenceId) { + pub(crate) fn add_module_reference(&mut self, id: ModuleId, location: Location) { + self.add_reference(ReferenceId::Module(id), location, false); + } + + pub(crate) fn add_struct_reference( + &mut self, + id: StructId, + location: Location, + is_self_type: bool, + ) { + self.add_reference(ReferenceId::Struct(id), location, is_self_type); + } + + pub(crate) fn add_struct_member_reference( + &mut self, + id: StructId, + member_index: usize, + location: Location, + ) { + self.add_reference(ReferenceId::StructMember(id, member_index), location, false); + } + + pub(crate) fn add_trait_reference( + &mut self, + id: TraitId, + location: Location, + is_self_type: bool, + ) { + self.add_reference(ReferenceId::Trait(id), location, is_self_type); + } + + pub(crate) fn add_alias_reference(&mut self, id: TypeAliasId, location: Location) { + self.add_reference(ReferenceId::Alias(id), location, false); + } + + pub(crate) fn add_function_reference(&mut self, id: FuncId, location: Location) { + self.add_reference(ReferenceId::Function(id), location, false); + } + + pub(crate) fn add_global_reference(&mut self, id: GlobalId, location: Location) { + self.add_reference(ReferenceId::Global(id), location, false); + } + + pub(crate) fn add_local_reference(&mut self, id: DefinitionId, location: Location) { + self.add_reference(ReferenceId::Local(id), location, false); + } + + pub(crate) fn add_reference( + &mut self, + referenced: ReferenceId, + location: Location, + is_self_type: bool, + ) { if !self.track_references { return; } + let reference = ReferenceId::Reference(location, is_self_type); + let referenced_index = self.get_or_insert_reference(referenced); let reference_location = self.reference_location(reference); let reference_index = self.reference_graph.add_node(reference); From e348d4e65649bf1553ea5f5560395ec3b9811b90 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 9 Jul 2024 17:54:02 -0300 Subject: [PATCH 25/29] fix: let LSP work well in Noir workspaces --- compiler/noirc_frontend/src/locations.rs | 23 ++-- tooling/lsp/src/lib.rs | 103 ++++++++++++------ tooling/lsp/src/notifications/mod.rs | 15 ++- tooling/lsp/src/requests/code_lens_request.rs | 8 +- tooling/lsp/src/requests/goto_declaration.rs | 2 +- tooling/lsp/src/requests/goto_definition.rs | 2 +- tooling/lsp/src/requests/mod.rs | 87 ++++++++++++++- tooling/lsp/src/requests/references.rs | 26 +++-- tooling/lsp/src/requests/rename.rs | 57 +++++----- 9 files changed, 225 insertions(+), 98 deletions(-) diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs index 425d2947062..5ca8c1493eb 100644 --- a/compiler/noirc_frontend/src/locations.rs +++ b/compiler/noirc_frontend/src/locations.rs @@ -178,14 +178,8 @@ impl NodeInterner { include_referenced: bool, include_self_type_name: bool, ) -> Option> { - let node_index = self.location_indices.get_node_from_location(location)?; - - let reference_node = self.reference_graph[node_index]; - let referenced_node_index = if let ReferenceId::Reference(_, _) = reference_node { - self.referenced_index(node_index)? - } else { - node_index - }; + let referenced_node = self.find_referenced(location)?; + let referenced_node_index = self.reference_graph_indices[&referenced_node]; let found_locations = self.find_all_references_for_index( referenced_node_index, @@ -196,6 +190,19 @@ impl NodeInterner { Some(found_locations) } + // Returns the `ReferenceId` that is referenced by the given location, if any. + pub fn find_referenced(&self, location: Location) -> Option { + let node_index = self.location_indices.get_node_from_location(location)?; + + let reference_node = self.reference_graph[node_index]; + if let ReferenceId::Reference(_, _) = reference_node { + let node_index = self.referenced_index(node_index)?; + Some(self.reference_graph[node_index]) + } else { + Some(reference_node) + } + } + // Given a referenced node index, find all references to it and return their locations, optionally together // with the reference node's location if `include_referenced` is true. // If `include_self_type_name` is true, references where "Self" is written are returned, diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index e47dfff714a..bd149635865 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -229,43 +229,74 @@ fn byte_span_to_range<'a, F: files::Files<'a> + ?Sized>( } } -pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result { - if let Some(toml_path) = find_file_manifest(file_path) { - resolve_workspace_from_toml( - &toml_path, - PackageSelection::All, - Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - ) - .map_err(|err| LspError::WorkspaceResolutionError(err.to_string())) - } else { - let Some(parent_folder) = file_path - .parent() - .and_then(|f| f.file_name()) - .and_then(|file_name_os_str| file_name_os_str.to_str()) - else { - return Err(LspError::WorkspaceResolutionError(format!( - "Could not resolve parent folder for file: {:?}", - file_path - ))); - }; - let assumed_package = Package { - version: None, - compiler_required_version: Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), - root_dir: PathBuf::from(parent_folder), - package_type: PackageType::Binary, - entry_path: PathBuf::from(file_path), - name: CrateName::from_str(parent_folder) - .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?, - dependencies: BTreeMap::new(), - }; - let workspace = Workspace { - root_dir: PathBuf::from(parent_folder), - members: vec![assumed_package], - selected_package_index: Some(0), - is_assumed: true, - }; - Ok(workspace) +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())); + } } + + let Some(parent_folder) = file_path + .parent() + .and_then(|f| f.file_name()) + .and_then(|file_name_os_str| file_name_os_str.to_str()) + else { + return Err(LspError::WorkspaceResolutionError(format!( + "Could not resolve parent folder for file: {:?}", + file_path + ))); + }; + let assumed_package = Package { + version: None, + compiler_required_version: Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), + root_dir: PathBuf::from(parent_folder), + package_type: PackageType::Binary, + entry_path: PathBuf::from(file_path), + name: CrateName::from_str(parent_folder) + .map_err(|err| LspError::WorkspaceResolutionError(err.to_string()))?, + dependencies: BTreeMap::new(), + }; + let workspace = Workspace { + root_dir: PathBuf::from(parent_folder), + members: vec![assumed_package], + selected_package_index: Some(0), + is_assumed: true, + }; + Ok(workspace) +} + +pub(crate) fn workspace_package_for_file<'a>( + workspace: &'a Workspace, + file_path: &Path, +) -> Option<&'a Package> { + workspace.members.iter().filter(|package| file_path.starts_with(&package.root_dir)).next() } pub(crate) fn prepare_package<'file_manager, 'parsed_files>( diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index ed0ac13fae5..94b9c67d814 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -14,7 +14,7 @@ use crate::types::{ use crate::{ byte_span_to_range, get_package_tests_in_crate, parse_diff, prepare_source, - resolve_workspace_for_source_path, LspState, + resolve_workspace_for_source_path, workspace_package_for_file, LspState, }; pub(super) fn on_initialized( @@ -55,11 +55,14 @@ 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 file_path = params.text_document.uri.to_file_path().unwrap(); + let (mut context, crate_id) = prepare_source(text, state); let _ = check_crate(&mut context, crate_id, false, false, false); let workspace = match resolve_workspace_for_source_path( params.text_document.uri.to_file_path().unwrap().as_path(), + &state.root_path, ) { Ok(workspace) => workspace, Err(lsp_error) => { @@ -70,7 +73,8 @@ pub(super) fn on_did_change_text_document( .into())) } }; - let package = match workspace.members.first() { + + let package = match workspace_package_for_file(&workspace, &file_path) { Some(package) => package, None => { return ControlFlow::Break(Err(ResponseError::new( @@ -124,9 +128,10 @@ fn process_noir_document( ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path") })?; - let workspace = resolve_workspace_for_source_path(&file_path).map_err(|lsp_error| { - ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) - })?; + 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 mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); diff --git a/tooling/lsp/src/requests/code_lens_request.rs b/tooling/lsp/src/requests/code_lens_request.rs index 744bddedd9d..9888d3ec1fe 100644 --- a/tooling/lsp/src/requests/code_lens_request.rs +++ b/tooling/lsp/src/requests/code_lens_request.rs @@ -61,8 +61,12 @@ 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()).unwrap(); - let package = workspace.members.first().unwrap(); + let workspace = + resolve_workspace_for_source_path(file_path.as_path(), &state.root_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") + })?; let (mut context, crate_id) = prepare_source(source_string, state); // We ignore the warnings and errors produced by compilation for producing code lenses diff --git a/tooling/lsp/src/requests/goto_declaration.rs b/tooling/lsp/src/requests/goto_declaration.rs index 627cd8d203e..1f87f2c87d4 100644 --- a/tooling/lsp/src/requests/goto_declaration.rs +++ b/tooling/lsp/src/requests/goto_declaration.rs @@ -20,7 +20,7 @@ fn on_goto_definition_inner( state: &mut LspState, params: GotoDeclarationParams, ) -> Result { - process_request(state, params.text_document_position_params, |location, interner, files| { + process_request(state, params.text_document_position_params, |location, interner, files, _| { interner.get_declaration_location_from(location).and_then(|found_location| { let file_id = found_location.file; let definition_position = to_lsp_location(files, file_id, found_location.span)?; diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs index 3713e8b646a..27d3503a2fd 100644 --- a/tooling/lsp/src/requests/goto_definition.rs +++ b/tooling/lsp/src/requests/goto_definition.rs @@ -29,7 +29,7 @@ fn on_goto_definition_inner( params: GotoDefinitionParams, return_type_location_instead: bool, ) -> Result { - process_request(state, params.text_document_position_params, |location, interner, files| { + process_request(state, params.text_document_position_params, |location, interner, files, _| { interner.get_definition_location_from(location, return_type_location_instead).and_then( |found_location| { let file_id = found_location.file; diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 266b5b124ac..827f358c39e 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -1,4 +1,4 @@ -use std::future::Future; +use std::{collections::HashMap, future::Future}; use crate::{ parse_diff, resolve_workspace_for_source_path, @@ -270,15 +270,18 @@ pub(crate) fn process_request( callback: F, ) -> Result where - F: FnOnce(noirc_errors::Location, &NodeInterner, &FileMap) -> T, + F: FnOnce(noirc_errors::Location, &NodeInterner, &FileMap, &HashMap) -> T, { let file_path = 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 workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap(); - let package = workspace.members.first().unwrap(); + let workspace = + resolve_workspace_for_source_path(file_path.as_path(), &state.root_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") + })?; let package_root_path: String = package.root_dir.as_os_str().to_string_lossy().into(); @@ -306,7 +309,81 @@ where &text_document_position_params.position, )?; - Ok(callback(location, interner, files)) + Ok(callback(location, interner, files, &state.cached_definitions)) +} +pub(crate) fn find_all_references_in_workspace( + location: noirc_errors::Location, + interner: &NodeInterner, + cached_interners: &HashMap, + files: &FileMap, + include_declaration: bool, + include_self_type_name: bool, +) -> Option> { + // First find the node that's referenced by the given location, if any + let referenced = interner.find_referenced(location); + + if let Some(referenced) = referenced { + // If we found the referenced node, find its location + let referenced_location = interner.reference_location(referenced); + + // Now we find all references that point to this location, in all interners + // (there's one interner per package, and all interners in a workspace rely on the + // same FileManager so a Location/FileId in one package is the same as in another package) + let mut locations = find_all_references( + referenced_location, + interner, + files, + include_declaration, + include_self_type_name, + ); + for interner in cached_interners.values() { + locations.extend(find_all_references( + referenced_location, + interner, + files, + include_declaration, + include_self_type_name, + )); + } + + // The LSP client usually removes duplicate loctions, but we do it here just in case they don't + locations.sort_by_key(|location| { + ( + location.uri.to_string(), + location.range.start.line, + location.range.start.character, + location.range.end.line, + location.range.end.character, + ) + }); + locations.dedup(); + + if locations.is_empty() { + None + } else { + Some(locations) + } + } else { + None + } +} + +pub(crate) fn find_all_references( + referenced_location: noirc_errors::Location, + interner: &NodeInterner, + files: &FileMap, + include_declaration: bool, + include_self_type_name: bool, +) -> Vec { + interner + .find_all_references(referenced_location, include_declaration, include_self_type_name) + .map(|locations| { + locations + .iter() + .filter_map(|location| to_lsp_location(files, location.file, location.span)) + .collect() + }) + .unwrap_or(vec![]) } #[cfg(test)] diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index f8c23632936..9de918651a9 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -5,23 +5,27 @@ use lsp_types::{Location, ReferenceParams}; use crate::LspState; -use super::{process_request, to_lsp_location}; +use super::{find_all_references_in_workspace, process_request}; pub(crate) fn on_references_request( state: &mut LspState, params: ReferenceParams, ) -> impl Future>, ResponseError>> { - let result = - process_request(state, params.text_document_position, |location, interner, files| { - interner.find_all_references(location, params.context.include_declaration, true).map( - |locations| { - locations - .iter() - .filter_map(|location| to_lsp_location(files, location.file, location.span)) - .collect() - }, + let include_declaration = params.context.include_declaration; + let result = process_request( + state, + params.text_document_position, + |location, interner, files, cached_interners| { + find_all_references_in_workspace( + location, + interner, + cached_interners, + files, + include_declaration, + true, ) - }); + }, + ); future::ready(result) } diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs index 906a5cbcaab..245c2ed0882 100644 --- a/tooling/lsp/src/requests/rename.rs +++ b/tooling/lsp/src/requests/rename.rs @@ -11,13 +11,13 @@ use noirc_frontend::node_interner::ReferenceId; use crate::LspState; -use super::{process_request, to_lsp_location}; +use super::{find_all_references_in_workspace, process_request}; pub(crate) fn on_prepare_rename_request( state: &mut LspState, params: TextDocumentPositionParams, ) -> impl Future, ResponseError>> { - let result = process_request(state, params, |location, interner, _| { + let result = process_request(state, params, |location, interner, _, _| { let reference_id = interner.reference_at_location(location); let rename_possible = match reference_id { // Rename shouldn't be possible when triggered on top of "Self" @@ -34,32 +34,30 @@ pub(crate) fn on_rename_request( state: &mut LspState, params: RenameParams, ) -> impl Future, ResponseError>> { - let result = - process_request(state, params.text_document_position, |location, interner, files| { - let rename_changes = - interner.find_all_references(location, true, false).map(|locations| { - let rs = locations.iter().fold( - HashMap::new(), - |mut acc: HashMap>, location| { - let file_id = location.file; - let span = location.span; - - let Some(lsp_location) = to_lsp_location(files, file_id, span) else { - return acc; - }; - - let edit = TextEdit { - range: lsp_location.range, - new_text: params.new_name.clone(), - }; - - acc.entry(lsp_location.uri).or_default().push(edit); - - acc - }, - ); - rs - }); + let result = process_request( + state, + params.text_document_position, + |location, interner, files, cached_interners| { + let rename_changes = find_all_references_in_workspace( + location, + interner, + cached_interners, + files, + true, + false, + ) + .map(|locations| { + let rs = locations.iter().fold( + HashMap::new(), + |mut acc: HashMap>, location| { + let edit = + TextEdit { range: location.range, new_text: params.new_name.clone() }; + acc.entry(location.uri.clone()).or_default().push(edit); + acc + }, + ); + rs + }); let response = WorkspaceEdit { changes: rename_changes, @@ -68,7 +66,8 @@ pub(crate) fn on_rename_request( }; Some(response) - }); + }, + ); future::ready(result) } From 5ded06e726761b804992f6601d239cd24177e3b2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 9 Jul 2024 18:12:22 -0300 Subject: [PATCH 26/29] clippy --- .../noirc_frontend/src/hir/def_collector/dc_crate.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index f04f5d635cf..e1ee00d4f64 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -517,22 +517,22 @@ fn add_import_reference( match def_id { crate::macros_api::ModuleDefId::ModuleId(module_id) => { - interner.add_module_location(module_id, location) + interner.add_module_location(module_id, location); } crate::macros_api::ModuleDefId::FunctionId(func_id) => { - interner.add_function_reference(func_id, location) + interner.add_function_reference(func_id, location); } crate::macros_api::ModuleDefId::TypeId(struct_id) => { - interner.add_struct_reference(struct_id, location, false) + interner.add_struct_reference(struct_id, location, false); } crate::macros_api::ModuleDefId::TraitId(trait_id) => { - interner.add_trait_reference(trait_id, location, false) + interner.add_trait_reference(trait_id, location, false); } crate::macros_api::ModuleDefId::TypeAliasId(type_alias_id) => { - interner.add_alias_reference(type_alias_id, location) + interner.add_alias_reference(type_alias_id, location); } crate::macros_api::ModuleDefId::GlobalId(global_id) => { - interner.add_global_reference(global_id, location) + interner.add_global_reference(global_id, location); } }; } From a6460d1d4f0bad012922c7ea4f6793784ff8884f Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Tue, 9 Jul 2024 18:15:56 -0300 Subject: [PATCH 27/29] more clippy --- tooling/lsp/src/lib.rs | 2 +- tooling/lsp/src/requests/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index bd149635865..a193944acec 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -296,7 +296,7 @@ pub(crate) fn workspace_package_for_file<'a>( workspace: &'a Workspace, file_path: &Path, ) -> Option<&'a Package> { - workspace.members.iter().filter(|package| file_path.starts_with(&package.root_dir)).next() + workspace.members.iter().find(|package| file_path.starts_with(&package.root_dir)) } pub(crate) fn prepare_package<'file_manager, 'parsed_files>( diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index 827f358c39e..16605250882 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -383,7 +383,7 @@ pub(crate) fn find_all_references( .filter_map(|location| to_lsp_location(files, location.file, location.span)) .collect() }) - .unwrap_or(vec![]) + .unwrap_or_default() } #[cfg(test)] From 024a322b40e8708e711b4b59bbfc743b68eba314 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 10 Jul 2024 10:07:03 -0300 Subject: [PATCH 28/29] Add a test for finding references across a workspace --- tooling/lsp/src/notifications/mod.rs | 9 ++- tooling/lsp/src/requests/references.rs | 70 ++++++++++++++++++- .../lsp/test_programs/workspace/Nargo.toml | 2 + .../test_programs/workspace/one/Nargo.toml | 4 ++ .../test_programs/workspace/one/src/lib.nr | 1 + .../test_programs/workspace/two/Nargo.toml | 7 ++ .../test_programs/workspace/two/src/lib.nr | 5 ++ 7 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 tooling/lsp/test_programs/workspace/Nargo.toml create mode 100644 tooling/lsp/test_programs/workspace/one/Nargo.toml create mode 100644 tooling/lsp/test_programs/workspace/one/src/lib.nr create mode 100644 tooling/lsp/test_programs/workspace/two/Nargo.toml create mode 100644 tooling/lsp/test_programs/workspace/two/src/lib.nr diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 94b9c67d814..6f66571e709 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -39,7 +39,7 @@ pub(super) fn on_did_open_text_document( let document_uri = params.text_document.uri; - match process_noir_document(document_uri, state) { + match process_workspace_for_noir_document(document_uri, state) { Ok(_) => { state.open_documents_count += 1; ControlFlow::Continue(()) @@ -114,13 +114,16 @@ pub(super) fn on_did_save_text_document( ) -> ControlFlow> { let document_uri = params.text_document.uri; - match process_noir_document(document_uri, state) { + match process_workspace_for_noir_document(document_uri, state) { Ok(_) => ControlFlow::Continue(()), Err(err) => ControlFlow::Break(Err(err)), } } -fn process_noir_document( +// Given a Noir document, find the workspace it's contained in (an assumed workspace is created if +// it's only contained in a package), then type-checks the workspace's packages, +// caching code lenses and type definitions, and notifying about compilation errors. +pub(crate) fn process_workspace_for_noir_document( document_uri: lsp_types::Url, state: &mut LspState, ) -> Result<(), async_lsp::Error> { diff --git a/tooling/lsp/src/requests/references.rs b/tooling/lsp/src/requests/references.rs index 9de918651a9..6c872656c28 100644 --- a/tooling/lsp/src/requests/references.rs +++ b/tooling/lsp/src/requests/references.rs @@ -32,9 +32,11 @@ pub(crate) fn on_references_request( #[cfg(test)] mod references_tests { use super::*; + use crate::notifications; use crate::test_utils::{self, search_in_file}; use lsp_types::{ - PartialResultParams, ReferenceContext, TextDocumentPositionParams, WorkDoneProgressParams, + PartialResultParams, Position, Range, ReferenceContext, TextDocumentPositionParams, Url, + WorkDoneProgressParams, }; use tokio::test; @@ -95,4 +97,70 @@ mod references_tests { async fn test_on_references_request_without_including_declaration() { check_references_succeeds("rename_function", "another_function", 0, false).await; } + + #[test] + async fn test_on_references_request_works_accross_workspace_packages() { + let (mut state, noir_text_document) = test_utils::init_lsp_server("workspace").await; + + // noir_text_document is always `src/main.nr` in the workspace directory, so let's go to the workspace dir + let noir_text_document = noir_text_document.to_file_path().unwrap(); + let workspace_dir = noir_text_document.parent().unwrap().parent().unwrap(); + + // Let's check that we can find references to `function_one` by doing that in the package "one" + // and getting results in the package "two" too. + let one_lib = Url::from_file_path(workspace_dir.join("one/src/lib.nr")).unwrap(); + 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 + notifications::process_workspace_for_noir_document(one_lib.clone(), &mut state).unwrap(); + + let params = ReferenceParams { + text_document_position: TextDocumentPositionParams { + text_document: lsp_types::TextDocumentIdentifier { uri: one_lib.clone() }, + position: Position { line: 0, character: 7 }, + }, + work_done_progress_params: WorkDoneProgressParams { work_done_token: None }, + partial_result_params: PartialResultParams { partial_result_token: None }, + context: ReferenceContext { include_declaration: true }, + }; + + let mut locations = on_references_request(&mut state, params) + .await + .expect("Could not execute on_references_request") + .unwrap(); + + // The definition, a use in "two", and a call in "two" + assert_eq!(locations.len(), 3); + + locations.sort_by_cached_key(|location| { + (location.uri.to_file_path().unwrap(), location.range.start.line) + }); + + assert_eq!(locations[0].uri, one_lib); + assert_eq!( + locations[0].range, + Range { + start: Position { line: 0, character: 7 }, + end: Position { line: 0, character: 19 }, + } + ); + + assert_eq!(locations[1].uri, two_lib); + assert_eq!( + locations[1].range, + Range { + start: Position { line: 0, character: 9 }, + end: Position { line: 0, character: 21 }, + } + ); + + assert_eq!(locations[2].uri, two_lib); + assert_eq!( + locations[2].range, + Range { + start: Position { line: 3, character: 4 }, + end: Position { line: 3, character: 16 }, + } + ); + } } diff --git a/tooling/lsp/test_programs/workspace/Nargo.toml b/tooling/lsp/test_programs/workspace/Nargo.toml new file mode 100644 index 00000000000..d0a0badc295 --- /dev/null +++ b/tooling/lsp/test_programs/workspace/Nargo.toml @@ -0,0 +1,2 @@ +[workspace] +members = ["one", "two"] diff --git a/tooling/lsp/test_programs/workspace/one/Nargo.toml b/tooling/lsp/test_programs/workspace/one/Nargo.toml new file mode 100644 index 00000000000..39838d73362 --- /dev/null +++ b/tooling/lsp/test_programs/workspace/one/Nargo.toml @@ -0,0 +1,4 @@ +[package] +name = "one" +authors = [] +type = "lib" diff --git a/tooling/lsp/test_programs/workspace/one/src/lib.nr b/tooling/lsp/test_programs/workspace/one/src/lib.nr new file mode 100644 index 00000000000..d4f660e35fb --- /dev/null +++ b/tooling/lsp/test_programs/workspace/one/src/lib.nr @@ -0,0 +1 @@ +pub fn function_one() {} diff --git a/tooling/lsp/test_programs/workspace/two/Nargo.toml b/tooling/lsp/test_programs/workspace/two/Nargo.toml new file mode 100644 index 00000000000..26d99b65df1 --- /dev/null +++ b/tooling/lsp/test_programs/workspace/two/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "two" +authors = [] +type = "lib" + +[dependencies] +one = { path = "../one" } \ No newline at end of file diff --git a/tooling/lsp/test_programs/workspace/two/src/lib.nr b/tooling/lsp/test_programs/workspace/two/src/lib.nr new file mode 100644 index 00000000000..adf7079b4c9 --- /dev/null +++ b/tooling/lsp/test_programs/workspace/two/src/lib.nr @@ -0,0 +1,5 @@ +use one::function_one; + +pub fn function_two() { + function_one() +} From 518570df463b25586026e702398b6894239f3a26 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 10 Jul 2024 16:47:40 -0300 Subject: [PATCH 29/29] Fix: add_module_reference, not add_module_location --- compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index e580ada46fe..d8a701f266b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -551,7 +551,7 @@ fn add_import_reference( match def_id { crate::macros_api::ModuleDefId::ModuleId(module_id) => { - interner.add_module_location(module_id, location); + interner.add_module_reference(module_id, location); } crate::macros_api::ModuleDefId::FunctionId(func_id) => { interner.add_function_reference(func_id, location);