fix: lsp struct rename/reference difference (#5411)

# Description

## Problem

Resolves #5412

## Summary

When a function parameter is `self` and it refers to a struct type, that
variable is linked to that type (through a synthetic "Self" type):

```rust
struct Foo {
}

impl Foo {
  fn foo(self) { // <-- here
  }
}
```

Then, when renaming `Foo`, those `self` occurrences were incorrectly
renamed. To fix that, in this PR we don't track these synthetic `Self`
types in the reference graph.

The second thing here is that when using `Self` to refer to a struct
type, that `Self` was also incorrectly renamed. To fix this we now track
whether a reference to a type came from `Self` or from the actual struct
name. Then we don't rename these `Self` references.

Note that we still keep these `Self` references in order for "find all
references" to return them (this is similar to how rust-analyzer works).


https://github.com/noir-lang/noir/assets/209371/8eeeebd7-816f-4e8f-a6cc-426a5172c1a7

## 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.

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<String>);
 
+impl Ident {
+    pub fn is_self_type_name(&self) -> bool {
+        self.0.contents == SELF_TYPE_NAME
+    }
+}
+
 impl PartialEq<Ident> for Ident {
     fn eq(&self, other: &Ident) -> bool {
         self.0.contents == other.0.contents

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))

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -1449,11 +1449,13 @@ impl<'context> Elaborator<'context> {
             self.generics.clear();
 
             if let Some(trait_id) = trait_id {
+                let trait_name = trait_impl.trait_path.last_segment();
+
                 let referenced = ReferenceId::Trait(trait_id);
-                let reference = ReferenceId::Variable(Location::new(
-                    trait_impl.trait_path.last_segment().span(),
-                    trait_impl.file_id,
-                ));
+                let reference = ReferenceId::Variable(
+                    Location::new(trait_name.span(), trait_impl.file_id),
+                    trait_name.is_self_type_name(),
+                );
                 self.interner.add_reference(referenced, reference);
             }
         }

compiler/noirc_frontend/src/elaborator/patterns.rs

@@ -159,6 +159,7 @@ impl<'context> Elaborator<'context> {
         new_definitions: &mut Vec<HirIdent>,
     ) -> 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);
                     }

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 references))?;
 
             for (referenced, ident) in references.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 {

compiler/noirc_frontend/src/elaborator/types.rs

@@ -58,6 +58,12 @@ impl<'context> Elaborator<'context> {
         use crate::ast::UnresolvedTypeData::*;
 
         let span = typ.span;
+        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,
@@ -155,9 +161,14 @@ 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),
+                        is_self_type_name,
+                    );
+                    self.interner.add_reference(referenced, reference);
+                }
             }
         }
 

compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -343,7 +343,8 @@ impl DefCollector {
                 let file_id = current_def_map.file_id(module_id);
 
                 for (referenced, ident) in references.iter().zip(&collected_import.path.segments) {
-                    let reference = ReferenceId::Variable(Location::new(ident.span(), file_id));
+                    let reference =
+                        ReferenceId::Variable(Location::new(ident.span(), file_id), false);
                     context.def_interner.add_reference(*referenced, reference);
                 }
 
@@ -512,15 +513,15 @@ 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);
         }
         crate::macros_api::ModuleDefId::TraitId(trait_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::Trait(trait_id), variable);
         }
         _ => (),

compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -652,7 +652,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);
+                let reference = ReferenceId::Variable(location, false);
                 context.def_interner.add_reference(referenced, reference);
 
                 errors.extend(collect_defs(

compiler/noirc_frontend/src/locations.rs

@@ -37,7 +37,7 @@ impl NodeInterner {
             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,
+            ReferenceId::Variable(location, _) => location,
         }
     }
 
@@ -83,55 +83,72 @@ 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<ReferenceId> {
+        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])
     }
 
     // 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<Vec<Location>> {
         let node_index = self.location_indices.get_node_from_location(location)?;
 
         let reference_node = self.reference_graph[node_index];
         let found_locations: Vec<Location> = match reference_node {
             ReferenceId::Alias(_) | ReferenceId::Global(_) | ReferenceId::Module(_) => todo!(),
-            ReferenceId::Function(_) | ReferenceId::Struct(_) | ReferenceId::Trait(_) => {
-                self.find_all_references_for_index(node_index, include_reference)
-            }
-
-            ReferenceId::Variable(_) => {
+            ReferenceId::Function(_) | ReferenceId::Struct(_) | ReferenceId::Trait(_) => 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<Location> {
         let id = self.reference_graph[referenced_node_index];
         let mut edit_locations = Vec::new();
-        if include_reference {
+        if include_referenced && (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
     }

compiler/noirc_frontend/src/node_interner.rs

@@ -251,7 +251,13 @@ 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 {
+        matches!(self, Self::Variable(_, true))
+    }
 }
 
 /// A trait implementation is either a normal implementation that is present in the source

tooling/lsp/src/requests/references.rs

@@ -13,7 +13,7 @@ pub(crate) fn on_references_request(
 ) -> impl Future<Output = Result<Option<Vec<Location>>, 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()