feat: lsp rename struct (noir-lang#5380)

# Description ## Problem Resolves noir-lang#5352 ## Summary Based on top of noir-lang#4294 Renames all references to structs, either in constructors, patterns, function parameters or its definition. Let me know if there's another place where struct names can show up! https://github.com/noir-lang/noir/assets/209371/ff12f9df-0110-4a52-8b0d-c9a5fc735d81 ## Additional Context None. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Koby <[email protected]> Co-authored-by: Koby Hall <[email protected]> Co-authored-by: Jake Fecher <[email protected]> Co-authored-by: jfecher <[email protected]> Co-authored-by: Tom French <[email protected]> Co-authored-by: Tom French <[email protected]>
manastech · Jul 2, 2024 · ee8b0cd · ee8b0cd
1 parent 32029f9
commit ee8b0cd
Show file tree

Hide file tree

Showing 23 changed files with 212 additions and 70 deletions.
diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs
@@ -27,7 +27,7 @@ use crate::{
         HirLiteral, HirStatement, Ident, IndexExpression, Literal, MemberAccessExpression,
         MethodCallExpression, PrefixExpression,
     },
-    node_interner::{DefinitionKind, ExprId, FuncId},
+    node_interner::{DefinitionKind, DependencyId, ExprId, FuncId},
     token::Tokens,
     Kind, QuotedType, Shared, StructType, Type,
 };
@@ -431,6 +431,11 @@ impl<'context> Elaborator<'context> {
             r#type,
             struct_generics,
         });
+
+        let referenced = DependencyId::Struct(struct_type.borrow().id);
+        let reference = DependencyId::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
@@ -157,6 +157,8 @@ impl<'context> Elaborator<'context> {
         mutable: Option<Span>,
         new_definitions: &mut Vec<HirIdent>,
     ) -> HirPattern {
+        let name_span = name.last_segment().span();
+
         let error_identifier = |this: &mut Self| {
             // Must create a name here to return a HirPattern::Identifier. Allowing
             // shadowing here lets us avoid further errors if we define ERROR_IDENT
@@ -196,6 +198,10 @@ impl<'context> Elaborator<'context> {
             new_definitions,
         );
 
+        let referenced = DependencyId::Struct(struct_type.borrow().id);
+        let reference = DependencyId::Variable(Location::new(name_span, self.file));
+        self.interner.add_reference(referenced, reference);
+
         HirPattern::Struct(expected_type, fields, location)
     }
 
@@ -584,10 +590,7 @@ impl<'context> Elaborator<'context> {
     }
 
     pub fn get_ident_from_path(&mut self, path: Path) -> (HirIdent, usize) {
-        let location = Location::new(
-            path.segments.last().expect("ice: path without segments").span(),
-            self.file,
-        );
+        let location = Location::new(path.last_segment().span(), self.file);
 
         let error = match path.as_ident().map(|ident| self.use_variable(ident)) {
             Some(Ok(found)) => return found,

diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs
@@ -30,7 +30,9 @@ use crate::{
         HirExpression, HirLiteral, HirStatement, Path, PathKind, SecondaryAttribute, Signedness,
         UnaryOp, UnresolvedType, UnresolvedTypeData,
     },
-    node_interner::{DefinitionKind, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId},
+    node_interner::{
+        DefinitionKind, DependencyId, ExprId, GlobalId, TraitId, TraitImplKind, TraitMethodId,
+    },
     Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeVariable, TypeVariableKind,
 };
 
@@ -242,6 +244,8 @@ 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) {
@@ -279,6 +283,11 @@ 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/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
@@ -483,9 +483,17 @@ fn add_import_reference(
         // We ignore empty spans at 0 location, this must be Stdlib
         return;
     }
-    if let crate::macros_api::ModuleDefId::FunctionId(func_id) = def_id {
-        let variable = DependencyId::Variable(Location::new(name.span(), file_id));
-        interner.add_reference_for(DependencyId::Function(func_id), variable);
+
+    match def_id {
+        crate::macros_api::ModuleDefId::FunctionId(func_id) => {
+            let variable = DependencyId::Variable(Location::new(name.span(), file_id));
+            interner.add_reference_for(DependencyId::Function(func_id), variable);
+        }
+        crate::macros_api::ModuleDefId::TypeId(struct_id) => {
+            let variable = DependencyId::Variable(Location::new(name.span(), file_id));
+            interner.add_reference_for(DependencyId::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
@@ -14,6 +14,7 @@ use crate::ast::{
     TypeImpl,
 };
 use crate::macros_api::NodeInterner;
+use crate::node_interner::DependencyId;
 use crate::{
     graph::CrateId,
     hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait},
@@ -267,6 +268,7 @@ 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,
@@ -310,6 +312,9 @@ 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(DependencyId::Struct(id));
         }
         definition_errors
     }

diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs
@@ -290,4 +290,9 @@ impl Context<'_, '_> {
             ResolvedGeneric { name, type_var, kind, span }
         })
     }
+
+    // Enables reference tracking (useful for tools like LSP).
+    pub fn track_references(&mut self) {
+        self.def_interner.track_references = true;
+    }
 }
diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs
@@ -32,14 +32,18 @@ impl NodeInterner {
     pub fn dependency_location(&self, dependency: DependencyId) -> Location {
         match dependency {
             DependencyId::Function(id) => self.function_modifiers(&id).name_location,
-            DependencyId::Struct(id) => self.get_struct(id).borrow().location,
+            DependencyId::Struct(id) => self.struct_location(&id),
             DependencyId::Global(id) => self.get_global(id).location,
             DependencyId::Alias(id) => self.get_type_alias(id).borrow().location,
             DependencyId::Variable(location) => location,
         }
     }
 
     pub(crate) fn add_reference(&mut self, referenced: DependencyId, reference: DependencyId) {
+        if !self.track_references {
+            return;
+        }
+
         let referenced_index = self.get_or_insert_reference(referenced);
         let reference_index = self.reference_graph.add_node(reference);
 
@@ -56,6 +60,10 @@ impl NodeInterner {
         referenced_id: DependencyId,
         reference: DependencyId,
     ) {
+        if !self.track_references {
+            return;
+        }
+
         let Some(referenced_index) = self.reference_graph_indices.get(&referenced_id) else {
             panic!("Compiler Error: Referenced index not found")
         };
@@ -67,6 +75,10 @@ impl NodeInterner {
     }
 
     pub(crate) fn add_definition_location(&mut self, referenced: DependencyId) {
+        if !self.track_references {
+            return;
+        }
+
         let referenced_index = self.get_or_insert_reference(referenced);
         let referenced_location = self.dependency_location(referenced);
         self.location_indices.add_location(referenced_location, referenced_index);
@@ -92,8 +104,10 @@ impl NodeInterner {
 
         let reference_node = self.reference_graph[node_index];
         let found_locations: Vec<Location> = match reference_node {
-            DependencyId::Alias(_) | DependencyId::Struct(_) | DependencyId::Global(_) => todo!(),
-            DependencyId::Function(_) => self.get_edit_locations(node_index),
+            DependencyId::Alias(_) | DependencyId::Global(_) => todo!(),
+            DependencyId::Function(_) | DependencyId::Struct(_) => {
+                self.get_edit_locations(node_index)
+            }
 
             DependencyId::Variable(_) => {
                 let referenced_node_index = self

diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs
@@ -1,6 +1,7 @@
 use std::borrow::Cow;
 use std::collections::HashMap;
 use std::fmt;
+use std::hash::Hash;
 use std::ops::Deref;
 
 use fm::FileId;
@@ -64,6 +65,9 @@ pub struct NodeInterner {
     // Contains the source module each function was defined in
     function_modules: HashMap<FuncId, ModuleId>,
 
+    // The location of each struct name
+    struct_name_locations: HashMap<StructId, Location>,
+
     /// 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<DependencyId, ()>,
@@ -184,6 +188,9 @@ pub struct NodeInterner {
     /// the actual type since types do not implement Send or Sync.
     quoted_types: noirc_arena::Arena<Type>,
 
+    /// Whether to track references. In regular compilations this is false, but tools set it to true.
+    pub(crate) track_references: bool,
+
     /// Store the location of the references in the graph
     pub(crate) reference_graph: DiGraph<DependencyId, ()>,
 
@@ -504,6 +511,7 @@ impl Default for NodeInterner {
             function_definition_ids: HashMap::new(),
             function_modifiers: HashMap::new(),
             function_modules: HashMap::new(),
+            struct_name_locations: HashMap::new(),
             func_id_to_trait: HashMap::new(),
             dependency_graph: petgraph::graph::DiGraph::new(),
             dependency_graph_indices: HashMap::new(),
@@ -531,6 +539,7 @@ impl Default for NodeInterner {
             type_alias_ref: Vec::new(),
             type_ref_locations: Vec::new(),
             quoted_types: Default::default(),
+            track_references: false,
             location_indices: LocationIndices::default(),
             reference_graph: petgraph::graph::DiGraph::new(),
             reference_graph_indices: HashMap::new(),
@@ -928,6 +937,14 @@ 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 global_attributes(&self, global_id: &GlobalId) -> &[SecondaryAttribute] {
         &self.global_attributes[global_id]
     }

diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs
@@ -266,6 +266,16 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result<Work
     }
 }
 
+pub(crate) fn prepare_package<'file_manager, 'parsed_files>(
+    file_manager: &'file_manager FileManager,
+    parsed_files: &'parsed_files ParsedFiles,
+    package: &Package,
+) -> (Context<'file_manager, 'parsed_files>, CrateId) {
+    let (mut context, crate_id) = nargo::prepare_package(file_manager, parsed_files, package);
+    context.track_references();
+    (context, crate_id)
+}
+
 /// Prepares a package from a source string
 /// This is useful for situations when we don't need dependencies
 /// and just need to operate on single file.
@@ -283,6 +293,8 @@ fn prepare_source(source: String, state: &mut LspState) -> (Context<'static, 'st
     let parsed_files = parse_diff(&file_manager, state);
 
     let mut context = Context::new(file_manager, parsed_files);
+    context.track_references();
+
     let root_crate_id = prepare_crate(&mut context, file_name);
 
     (context, root_crate_id)

diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs
@@ -1,7 +1,7 @@
 use std::ops::ControlFlow;
 
 use async_lsp::{ErrorCode, LanguageClient, ResponseError};
-use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package};
+use nargo::insert_all_files_for_workspace_into_file_manager;
 use noirc_driver::{check_crate, file_manager_with_stdlib};
 use noirc_errors::{DiagnosticKind, FileDiagnostic};
 
@@ -137,7 +137,7 @@ fn process_noir_document(
         .into_iter()
         .flat_map(|package| -> Vec<Diagnostic> {
             let (mut context, crate_id) =
-                prepare_package(&workspace_file_manager, &parsed_files, package);
+                crate::prepare_package(&workspace_file_manager, &parsed_files, package);
 
             let file_diagnostics = match check_crate(&mut context, crate_id, false, false, false) {
                 Ok(((), warnings)) => warnings,

diff --git a/tooling/lsp/src/requests/goto_declaration.rs b/tooling/lsp/src/requests/goto_declaration.rs
@@ -38,7 +38,7 @@ fn on_goto_definition_inner(
     let parsed_files = parse_diff(&workspace_file_manager, state);
 
     let (mut context, crate_id) =
-        nargo::prepare_package(&workspace_file_manager, &parsed_files, package);
+        crate::prepare_package(&workspace_file_manager, &parsed_files, package);
 
     let package_root_path = package.root_dir.as_os_str().to_string_lossy().into_owned();
     let interner = if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {

diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs
@@ -46,7 +46,7 @@ fn on_goto_definition_inner(
     let parsed_files = parse_diff(&workspace_file_manager, state);
 
     let (mut context, crate_id) =
-        nargo::prepare_package(&workspace_file_manager, &parsed_files, package);
+        crate::prepare_package(&workspace_file_manager, &parsed_files, package);
 
     let package_root_path = package.root_dir.as_os_str().to_string_lossy().into_owned();
     let interner = if let Some(def_interner) = state.cached_definitions.get(&package_root_path) {