From 5617aa051a88c35275f4a105e4103f1212c94232 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 3 Jul 2024 10:46:52 -0300 Subject: [PATCH 01/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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/20] 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 701606fcb893ba0c7478df57a4fa4f46167f591e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 8 Jul 2024 17:18:51 -0300 Subject: [PATCH 20/20] More places where globals can show up --- compiler/noirc_frontend/src/elaborator/types.rs | 4 ++++ tooling/lsp/test_programs/rename_global/src/main.nr | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index 6c4eac287ef..83810b141fb 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -369,6 +369,10 @@ impl<'context> Elaborator<'context> { self.interner.add_global_dependency(current_item, id); } + let referenced = ReferenceId::Global(id); + let reference = ReferenceId::Variable(Location::new(path.span(), self.file), false); + self.interner.add_reference(referenced, reference); + Some(Type::Constant(self.eval_global_as_array_length(id, path))) } _ => None, diff --git a/tooling/lsp/test_programs/rename_global/src/main.nr b/tooling/lsp/test_programs/rename_global/src/main.nr index ed5f1508e93..3ae8cf4bfc3 100644 --- a/tooling/lsp/test_programs/rename_global/src/main.nr +++ b/tooling/lsp/test_programs/rename_global/src/main.nr @@ -7,4 +7,16 @@ use foo::FOO; fn main() { let _ = foo::FOO; let _ = FOO; + let _: [Field; FOO] = [1]; +} + +trait WithNumber { + +} + +struct Some { +} + +impl WithNumber for Some { + }