Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: lsp struct rename/reference difference #5411

Merged
merged 10 commits into from
Jul 8, 2024
7 changes: 7 additions & 0 deletions compiler/noirc_frontend/src/ast/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_frontend/src/elaborator/expressions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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))
Expand Down
10 changes: 6 additions & 4 deletions compiler/noirc_frontend/src/elaborator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
8 changes: 5 additions & 3 deletions compiler/noirc_frontend/src/elaborator/patterns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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() {
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down
5 changes: 4 additions & 1 deletion compiler/noirc_frontend/src/elaborator/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
17 changes: 14 additions & 3 deletions compiler/noirc_frontend/src/elaborator/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
}
}

Expand Down
9 changes: 5 additions & 4 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
_ => (),
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,7 @@
}
}
TraitItem::Type { name } => {
// TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id()

Check warning on line 506 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (nickysn)

Check warning on line 506 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (alexvitkov)
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[trait_id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
Expand Down Expand Up @@ -652,7 +652,7 @@
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(
Expand Down Expand Up @@ -689,7 +689,7 @@
// if it's an inline module, or the first char of a the file if it's an external module.
// - `location` will always point to the token "foo" in `mod foo` regardless of whether
// it's inline or external.
// Eventually the location put in `ModuleData` is used for codelenses about `contract`s,

Check warning on line 692 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (codelenses)
// so we keep using `location` so that it continues to work as usual.
let location = Location::new(mod_name.span(), mod_location.file);
let new_module = ModuleData::new(parent, location, is_contract);
Expand Down
49 changes: 33 additions & 16 deletions compiler/noirc_frontend/src/locations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

impl LocationIndices {
pub(crate) fn add_location(&mut self, location: Location, node_index: PetGraphIndex) {
// Some location spans are empty: maybe they are from ficticious nodes?

Check warning on line 16 in compiler/noirc_frontend/src/locations.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (ficticious)
if location.span.start() == location.span.end() {
return;
}
Expand All @@ -37,7 +37,7 @@
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,
}
}

Expand Down Expand Up @@ -83,55 +83,72 @@
.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).

Check warning on line 96 in compiler/noirc_frontend/src/locations.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (referencedd)
// 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
}
Expand Down
8 changes: 7 additions & 1 deletion compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@
/// Stores the [Location] of a [Type] reference
pub(crate) type_ref_locations: Vec<(Type, Location)>,

/// In Noir's metaprogramming, a noir type has the type `Type`. When these are spliced

Check warning on line 191 in compiler/noirc_frontend/src/node_interner.rs

View workflow job for this annotation

GitHub Actions / Code

Unknown word (metaprogramming)
/// into `quoted` expressions, we preserve the original type by assigning it a unique id
/// and creating a `Token::QuotedType(id)` from this id. We cannot create a token holding
/// the actual type since types do not implement Send or Sync.
Expand Down Expand Up @@ -251,7 +251,13 @@
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
Expand Down
2 changes: 1 addition & 1 deletion tooling/lsp/src/requests/references.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading
Loading