Skip to content

Commit

Permalink
feat: lsp rename struct (#5380)
Browse files Browse the repository at this point in the history
# Description

## Problem

Resolves #5352

## Summary

Based on top of #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]>
  • Loading branch information
7 people authored Jul 2, 2024
1 parent 32029f9 commit ee8b0cd
Show file tree
Hide file tree
Showing 23 changed files with 212 additions and 70 deletions.
7 changes: 6 additions & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down Expand Up @@ -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))
}

Expand Down
11 changes: 7 additions & 4 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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,
Expand Down
11 changes: 10 additions & 1 deletion compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
14 changes: 11 additions & 3 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
_ => (),
}
}

Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down
5 changes: 5 additions & 0 deletions compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
20 changes: 17 additions & 3 deletions compiler/noirc_frontend/src/locations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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")
};
Expand All @@ -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);
Expand All @@ -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
Expand Down
17 changes: 17 additions & 0 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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, ()>,
Expand Down Expand Up @@ -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, ()>,

Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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]
}
Expand Down
12 changes: 12 additions & 0 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
@@ -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};

Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion tooling/lsp/src/requests/goto_declaration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion tooling/lsp/src/requests/goto_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Loading

0 comments on commit ee8b0cd

Please sign in to comment.