feat: lsp rename/find-all-references for local variables (noir-lang#5439

) # Description ## Problem Resolves noir-lang#5458 ## Summary This wasn't working precisely for local variables. In particular, if you had code like this: ```rust let mut mutable_var = 1; mutable_var = 2; <-- here ... ``` "go to definition" didn't work in the pointed line (assignments) and previously "find all references" or "rename" didn't work at all. https://github.com/noir-lang/noir/assets/209371/630d8440-3a2c-4621-9788-c8769f9afde9 ## 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. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings.
blocksense-network · Jul 9, 2024 · bb6913a · bb6913a
1 parent 82a67a0
commit bb6913a
Show file tree

Hide file tree

Showing 14 changed files with 61 additions and 18 deletions.
diff --git a/compiler/noirc_frontend/src/elaborator/expressions.rs b/compiler/noirc_frontend/src/elaborator/expressions.rs
@@ -419,7 +419,7 @@ impl<'context> Elaborator<'context> {
         });
 
         let referenced = ReferenceId::Struct(struct_type.borrow().id);
-        let reference = ReferenceId::Variable(Location::new(span, self.file), is_self_type);
+        let reference = ReferenceId::Reference(Location::new(span, self.file), is_self_type);
         self.interner.add_reference(referenced, reference);
 
         (expr, Type::Struct(struct_type, generics))

diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs
@@ -1453,7 +1453,7 @@ impl<'context> Elaborator<'context> {
                 let trait_name = trait_impl.trait_path.last_segment();
 
                 let referenced = ReferenceId::Trait(trait_id);
-                let reference = ReferenceId::Variable(
+                let reference = ReferenceId::Reference(
                     Location::new(trait_name.span(), trait_impl.file_id),
                     trait_name.is_self_type_name(),
                 );

diff --git a/compiler/noirc_frontend/src/elaborator/patterns.rs b/compiler/noirc_frontend/src/elaborator/patterns.rs
@@ -203,7 +203,7 @@ impl<'context> Elaborator<'context> {
         );
 
         let referenced = ReferenceId::Struct(struct_type.borrow().id);
-        let reference = ReferenceId::Variable(Location::new(name_span, self.file), is_self_type);
+        let reference = ReferenceId::Reference(Location::new(name_span, self.file), is_self_type);
         self.interner.add_reference(referenced, reference);
 
         HirPattern::Struct(expected_type, fields, location)
@@ -478,6 +478,7 @@ impl<'context> Elaborator<'context> {
             // Otherwise, then it is referring to an Identifier
             // This lookup allows support of such statements: let x = foo::bar::SOME_GLOBAL + 10;
             // If the expression is a singular indent, we search the resolver's current scope as normal.
+            let span = path.span();
             let is_self_type_name = path.last_segment().is_self_type_name();
             let (hir_ident, var_scope_index) = self.get_ident_from_path(path);
 
@@ -488,7 +489,8 @@ impl<'context> Elaborator<'context> {
                             self.interner.add_function_dependency(current_item, func_id);
                         }
 
-                        let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name);
+                        let variable =
+                            ReferenceId::Reference(hir_ident.location, is_self_type_name);
                         let function = ReferenceId::Function(func_id);
                         self.interner.add_reference(function, variable);
                     }
@@ -500,7 +502,8 @@ impl<'context> Elaborator<'context> {
                             self.interner.add_global_dependency(current_item, global_id);
                         }
 
-                        let variable = ReferenceId::Variable(hir_ident.location, is_self_type_name);
+                        let variable =
+                            ReferenceId::Reference(hir_ident.location, is_self_type_name);
                         let global = ReferenceId::Global(global_id);
                         self.interner.add_reference(global, variable);
                     }
@@ -517,6 +520,11 @@ impl<'context> Elaborator<'context> {
                     DefinitionKind::Local(_) => {
                         // only local variables can be captured by closures.
                         self.resolve_local_variable(hir_ident.clone(), var_scope_index);
+
+                        let referenced = ReferenceId::Local(hir_ident.id);
+                        let reference =
+                            ReferenceId::Reference(Location::new(span, self.file), false);
+                        self.interner.add_reference(referenced, reference);
                     }
                 }
             }

diff --git a/compiler/noirc_frontend/src/elaborator/scope.rs b/compiler/noirc_frontend/src/elaborator/scope.rs
@@ -53,7 +53,7 @@ impl<'context> Elaborator<'context> {
                 resolver.resolve(self.def_maps, path.clone(), &mut Some(&mut references))?;
 
             for (referenced, ident) in references.iter().zip(path.segments) {
-                let reference = ReferenceId::Variable(
+                let reference = ReferenceId::Reference(
                     Location::new(ident.span(), self.file),
                     ident.is_self_type_name(),
                 );

diff --git a/compiler/noirc_frontend/src/elaborator/statements.rs b/compiler/noirc_frontend/src/elaborator/statements.rs
@@ -16,7 +16,7 @@ use crate::{
     macros_api::{
         ForLoopStatement, ForRange, HirStatement, LetStatement, Path, Statement, StatementKind,
     },
-    node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId},
+    node_interner::{DefinitionId, DefinitionKind, GlobalId, ReferenceId, StmtId},
     Type,
 };
 
@@ -256,6 +256,10 @@ impl<'context> Elaborator<'context> {
                     typ.follow_bindings()
                 };
 
+                let referenced = ReferenceId::Local(ident.id);
+                let reference = ReferenceId::Reference(Location::new(span, self.file), false);
+                self.interner.add_reference(referenced, reference);
+
                 (HirLValue::Ident(ident.clone(), typ.clone()), typ, mutable)
             }
             LValue::MemberAccess { object, field_name, span } => {

diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs
@@ -164,7 +164,7 @@ impl<'context> Elaborator<'context> {
 
                     if !is_synthetic {
                         let referenced = ReferenceId::Struct(struct_type.borrow().id);
-                        let reference = ReferenceId::Variable(
+                        let reference = ReferenceId::Reference(
                             Location::new(unresolved_span, self.file),
                             is_self_type_name,
                         );
@@ -173,7 +173,7 @@ impl<'context> Elaborator<'context> {
                 }
                 Type::Alias(ref alias_type, _) => {
                     let referenced = ReferenceId::Alias(alias_type.borrow().id);
-                    let reference = ReferenceId::Variable(
+                    let reference = ReferenceId::Reference(
                         Location::new(unresolved_span, self.file),
                         is_self_type_name,
                     );
@@ -370,7 +370,8 @@ impl<'context> Elaborator<'context> {
                 }
 
                 let referenced = ReferenceId::Global(id);
-                let reference = ReferenceId::Variable(Location::new(path.span(), self.file), false);
+                let reference =
+                    ReferenceId::Reference(Location::new(path.span(), self.file), false);
                 self.interner.add_reference(referenced, reference);
 
                 Some(Type::Constant(self.eval_global_as_array_length(id, path)))

diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
@@ -344,7 +344,7 @@ impl DefCollector {
 
                 for (referenced, ident) in references.iter().zip(&collected_import.path.segments) {
                     let reference =
-                        ReferenceId::Variable(Location::new(ident.span(), file_id), false);
+                        ReferenceId::Reference(Location::new(ident.span(), file_id), false);
                     context.def_interner.add_reference(*referenced, reference);
                 }
 
@@ -521,7 +521,7 @@ fn add_import_reference(
         }
         crate::macros_api::ModuleDefId::GlobalId(global_id) => ReferenceId::Global(global_id),
     };
-    let reference = ReferenceId::Variable(Location::new(name.span(), file_id), false);
+    let reference = ReferenceId::Reference(Location::new(name.span(), file_id), false);
     interner.add_reference(referenced, reference);
 }
 

diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
@@ -650,7 +650,7 @@ impl<'a> ModCollector<'a> {
             Ok(child_mod_id) => {
                 // Track that the "foo" in `mod foo;` points to the module "foo"
                 let referenced = ReferenceId::Module(child_mod_id);
-                let reference = ReferenceId::Variable(location, false);
+                let reference = ReferenceId::Reference(location, false);
                 context.def_interner.add_reference(referenced, reference);
 
                 errors.extend(collect_defs(

diff --git a/compiler/noirc_frontend/src/locations.rs b/compiler/noirc_frontend/src/locations.rs
@@ -48,7 +48,8 @@ impl NodeInterner {
                 let alias_type = alias_type.borrow();
                 Location::new(alias_type.name.span(), alias_type.location.file)
             }
-            ReferenceId::Variable(location, _) => location,
+            ReferenceId::Local(id) => self.definition(id).location,
+            ReferenceId::Reference(location, _) => location,
         }
     }
 
@@ -117,7 +118,7 @@ impl NodeInterner {
         let node_index = self.location_indices.get_node_from_location(location)?;
 
         let reference_node = self.reference_graph[node_index];
-        let referenced_node_index = if let ReferenceId::Variable(_, _) = reference_node {
+        let referenced_node_index = if let ReferenceId::Reference(_, _) = reference_node {
             self.referenced_index(node_index)?
         } else {
             node_index

diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs
@@ -245,12 +245,13 @@ pub enum ReferenceId {
     Global(GlobalId),
     Function(FuncId),
     Alias(TypeAliasId),
-    Variable(Location, bool /* is Self */),
+    Local(DefinitionId),
+    Reference(Location, bool /* is Self */),
 }
 
 impl ReferenceId {
     pub fn is_self_type_name(&self) -> bool {
-        matches!(self, Self::Variable(_, true))
+        matches!(self, Self::Reference(_, true))
     }
 }
 
@@ -836,12 +837,19 @@ impl NodeInterner {
         location: Location,
     ) -> DefinitionId {
         let id = DefinitionId(self.definitions.len());
+        let is_local = matches!(definition, DefinitionKind::Local(_));
+
         if let DefinitionKind::Function(func_id) = definition {
             self.function_definition_ids.insert(func_id, id);
         }
 
         let kind = definition;
         self.definitions.push(DefinitionInfo { name, mutable, comptime, kind, location });
+
+        if is_local {
+            self.add_definition_location(ReferenceId::Local(id));
+        }
+
         id
     }
 

diff --git a/tooling/lsp/src/requests/goto_definition.rs b/tooling/lsp/src/requests/goto_definition.rs
@@ -196,4 +196,9 @@ mod goto_definition_tests {
         )
         .await;
     }
+
+    #[test]
+    async fn goto_for_local_variable() {
+        expect_goto_for_all_references("local_variable", "some_var", 0).await;
+    }
 }
diff --git a/tooling/lsp/src/requests/rename.rs b/tooling/lsp/src/requests/rename.rs
@@ -21,7 +21,7 @@ pub(crate) fn on_prepare_rename_request(
         let reference_id = interner.reference_at_location(location);
         let rename_possible = match reference_id {
             // Rename shouldn't be possible when triggered on top of "Self"
-            Some(ReferenceId::Variable(_, true /* is self type name */)) => false,
+            Some(ReferenceId::Reference(_, true /* is self type name */)) => false,
             Some(_) => true,
             None => false,
         };
@@ -194,4 +194,9 @@ mod rename_tests {
     async fn test_rename_global() {
         check_rename_succeeds("rename_global", "FOO").await;
     }
+
+    #[test]
+    async fn test_rename_local_variable() {
+        check_rename_succeeds("local_variable", "some_var").await;
+    }
 }
diff --git a/tooling/lsp/test_programs/local_variable/Nargo.toml b/tooling/lsp/test_programs/local_variable/Nargo.toml
@@ -0,0 +1,6 @@
+[package]
+name = "local_variable"
+type = "bin"
+authors = [""]
+
+[dependencies]
diff --git a/tooling/lsp/test_programs/local_variable/src/main.nr b/tooling/lsp/test_programs/local_variable/src/main.nr
@@ -0,0 +1,5 @@
+fn main() {
+    let mut some_var = 1;
+    some_var = 2;
+    let _ = some_var;
+}
-Original file line number
+Diff line change
@@ Expand Up / @@ -196,4 +196,9 @@ mod goto_definition_tests { @@
             )
             .await;
         }
+        #[test]
+        async fn goto_for_local_variable() {
+            expect_goto_for_all_references("local_variable", "some_var", 0).await;
+        }
     }