Skip to content

Commit

Permalink
feat(lsp): add goto definition for functions (#3656)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->
feat: goto definition - function #3657

## Summary\*

LSP server now understands goto definition command issued by Client (ie.
vscode). In case of function it will jump to it's definition.

## Additional Context

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.

---------

Co-authored-by: kevaundray <kevtheappdev@gmail.com>
  • Loading branch information
kobyhallx and kevaundray authored Dec 1, 2023
1 parent 6b0bdbc commit 7bb7356
Show file tree
Hide file tree
Showing 11 changed files with 340 additions and 20 deletions.
31 changes: 20 additions & 11 deletions compiler/fm/src/file_map.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
use codespan_reporting::files::{Error, Files, SimpleFile, SimpleFiles};
use serde::{Deserialize, Serialize};
use std::collections::HashMap;
use std::{ops::Range, path::PathBuf};

// XXX: File and FileMap serve as opaque types, so that the rest of the library does not need to import the dependency
// or worry about when we change the dep

#[derive(Clone, Debug)]
#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub struct PathString(PathBuf);

impl std::fmt::Display for PathString {
Expand All @@ -30,7 +31,10 @@ impl From<&PathBuf> for PathString {
}
}
#[derive(Debug)]
pub struct FileMap(SimpleFiles<PathString, String>);
pub struct FileMap {
files: SimpleFiles<PathString, String>,
name_to_id: HashMap<PathString, FileId>,
}

// XXX: Note that we derive Default here due to ModuleOrigin requiring us to set a FileId
#[derive(
Expand Down Expand Up @@ -59,17 +63,22 @@ impl<'input> File<'input> {

impl FileMap {
pub fn add_file(&mut self, file_name: PathString, code: String) -> FileId {
let file_id = self.0.add(file_name, code);
FileId(file_id)
let file_id = FileId(self.files.add(file_name.clone(), code));
self.name_to_id.insert(file_name, file_id);
file_id
}

pub fn get_file(&self, file_id: FileId) -> Option<File> {
self.0.get(file_id.0).map(File).ok()
self.files.get(file_id.0).map(File).ok()
}
}

pub fn get_file_id(&self, file_name: &PathString) -> Option<FileId> {
self.name_to_id.get(file_name).cloned()
}
}
impl Default for FileMap {
fn default() -> Self {
FileMap(SimpleFiles::new())
FileMap { files: SimpleFiles::new(), name_to_id: HashMap::new() }
}
}

Expand All @@ -79,18 +88,18 @@ impl<'a> Files<'a> for FileMap {
type Source = &'a str;

fn name(&self, file_id: Self::FileId) -> Result<Self::Name, Error> {
Ok(self.0.get(file_id.as_usize())?.name().clone())
Ok(self.files.get(file_id.as_usize())?.name().clone())
}

fn source(&'a self, file_id: Self::FileId) -> Result<Self::Source, Error> {
Ok(self.0.get(file_id.as_usize())?.source().as_ref())
Ok(self.files.get(file_id.as_usize())?.source().as_ref())
}

fn line_index(&self, file_id: Self::FileId, byte_index: usize) -> Result<usize, Error> {
self.0.get(file_id.as_usize())?.line_index((), byte_index)
self.files.get(file_id.as_usize())?.line_index((), byte_index)
}

fn line_range(&self, file_id: Self::FileId, line_index: usize) -> Result<Range<usize>, Error> {
self.0.get(file_id.as_usize())?.line_range((), line_index)
self.files.get(file_id.as_usize())?.line_range((), line_index)
}
}
4 changes: 4 additions & 0 deletions compiler/fm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ impl FileManager {

self.add_file(&candidate).ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
}

pub fn name_to_id(&self, file_name: PathBuf) -> Option<FileId> {
self.file_map.get_file_id(&PathString::from_path(file_name))
}
}

/// Returns true if a module's child module's are expected to be in the same directory.
Expand Down
14 changes: 14 additions & 0 deletions compiler/noirc_errors/src/position.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,16 @@ impl Span {
pub fn end(&self) -> u32 {
self.0.end().into()
}

pub fn contains(&self, other: &Span) -> bool {
self.start() <= other.start() && self.end() >= other.end()
}

pub fn is_smaller(&self, other: &Span) -> bool {
let self_distance = self.end() - self.start();
let other_distance = other.end() - other.start();
self_distance < other_distance
}
}

impl From<Span> for Range<usize> {
Expand Down Expand Up @@ -133,4 +143,8 @@ impl Location {
pub fn dummy() -> Self {
Self { span: Span::single_char(0), file: FileId::dummy() }
}

pub fn contains(&self, other: &Location) -> bool {
self.file == other.file && self.span.contains(&other.span)
}
}
1 change: 0 additions & 1 deletion compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ impl CrateDefMap {
})
})
}

/// Go through all modules in this crate, find all `contract ... { ... }` declarations,
/// and collect them all into a Vec.
pub fn get_all_contracts(&self, interner: &NodeInterner) -> Vec<Contract> {
Expand Down
8 changes: 8 additions & 0 deletions compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,14 @@ impl Context {
.collect()
}

/// Returns the [Location] of the definition of the given Ident found at [Span] of the given [FileId].
/// Returns [None] when definition is not found.
pub fn get_definition_location_from(&self, location: Location) -> Option<Location> {
let interner = &self.def_interner;

interner.find_location_index(location).and_then(|index| interner.resolve_location(index))
}

/// Return a Vec of all `contract` declarations in the source code and the functions they contain
pub fn get_all_contracts(&self, crate_id: &CrateId) -> Vec<Contract> {
self.def_map(crate_id)
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ pub struct Trait {
pub self_type_typevar_id: TypeVariableId,
pub self_type_typevar: TypeVariable,
}

#[derive(Debug)]
pub struct TraitImpl {
pub ident: Ident,
pub typ: Type,
Expand Down
62 changes: 60 additions & 2 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::ast::Ident;
use crate::graph::CrateId;
use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias};
use crate::hir::def_map::{LocalModuleId, ModuleId};

use crate::hir_def::stmt::HirLetStatement;
use crate::hir_def::traits::TraitImpl;
use crate::hir_def::traits::{Trait, TraitConstraint};
Expand Down Expand Up @@ -36,6 +37,7 @@ type StructAttributes = Vec<SecondaryAttribute>;
/// each definition or struct, etc. Because it is used on the Hir, the NodeInterner is
/// useful in passes where the Hir is used - name resolution, type checking, and
/// monomorphization - and it is not useful afterward.
#[derive(Debug)]
pub struct NodeInterner {
nodes: Arena<Node>,
func_meta: HashMap<FuncId, FuncMeta>,
Expand Down Expand Up @@ -156,7 +158,7 @@ pub enum TraitImplKind {
///
/// Additionally, types can define specialized impls with methods of the same name
/// as long as these specialized impls do not overlap. E.g. `impl Struct<u32>` and `impl Struct<u64>`
#[derive(Default)]
#[derive(Default, Debug)]
pub struct Methods {
direct: Vec<FuncId>,
trait_impl_methods: Vec<FuncId>,
Expand All @@ -165,6 +167,7 @@ pub struct Methods {
/// All the information from a function that is filled out during definition collection rather than
/// name resolution. As a result, if information about a function is needed during name resolution,
/// this is the only place where it is safe to retrieve it (where all fields are guaranteed to be initialized).
#[derive(Debug, Clone)]
pub struct FunctionModifiers {
pub name: String,

Expand Down Expand Up @@ -451,6 +454,30 @@ impl NodeInterner {
self.id_to_location.insert(expr_id.into(), Location::new(span, file));
}

/// Scans the interner for the item which is located at that [Location]
///
/// The [Location] may not necessarily point to the beginning of the item
/// so we check if the location's span is contained within the start or end
/// of each items [Span]
pub fn find_location_index(&self, location: Location) -> Option<impl Into<Index>> {
let mut location_candidate: Option<(&Index, &Location)> = None;

// Note: we can modify this in the future to not do a linear
// scan by storing a separate map of the spans or by sorting the locations.
for (index, interned_location) in self.id_to_location.iter() {
if interned_location.contains(&location) {
if let Some(current_location) = location_candidate {
if interned_location.span.is_smaller(&current_location.1.span) {
location_candidate = Some((index, interned_location));
}
} else {
location_candidate = Some((index, interned_location));
}
}
}
location_candidate.map(|(index, _location)| *index)
}

/// Interns a HIR Function.
pub fn push_fn(&mut self, func: HirFunction) -> FuncId {
FuncId(self.nodes.insert(Node::Function(func)))
Expand Down Expand Up @@ -1190,6 +1217,37 @@ impl NodeInterner {
pub fn get_selected_impl_for_ident(&self, ident_id: ExprId) -> Option<TraitImplKind> {
self.selected_trait_implementations.get(&ident_id).cloned()
}

/// For a given [Index] we return [Location] to which we resolved to
/// We currently return None for features not yet implemented
/// TODO(#3659): LSP goto def should error when Ident at Location could not resolve
pub(crate) fn resolve_location(&self, index: impl Into<Index>) -> Option<Location> {
let node = self.nodes.get(index.into())?;

match node {
Node::Function(func) => self.resolve_location(func.as_expr()),
Node::Expression(expression) => self.resolve_expression_location(expression),
_ => None,
}
}

/// Resolves the [Location] of the definition for a given [HirExpression]
///
/// Note: current the code returns None because some expressions are not yet implemented.
fn resolve_expression_location(&self, expression: &HirExpression) -> Option<Location> {
match expression {
HirExpression::Ident(ident) => {
let definition_info = self.definition(ident.id);
match definition_info.kind {
DefinitionKind::Function(func_id) => {
Some(self.function_meta(&func_id).location)
}
_ => None,
}
}
_ => None,
}
}
}

impl Methods {
Expand Down Expand Up @@ -1242,7 +1300,7 @@ impl Methods {
}

/// These are the primitive type variants that we support adding methods to
#[derive(Copy, Clone, Hash, PartialEq, Eq)]
#[derive(Copy, Clone, Hash, PartialEq, Eq, Debug)]
enum TypeMethodKey {
/// Fields and integers share methods for ease of use. These methods may still
/// accept only fields or integers, it is just that their names may not clash.
Expand Down
5 changes: 3 additions & 2 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ use notifications::{
on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized,
};
use requests::{
on_code_lens_request, on_formatting, on_initialize, on_profile_run_request, on_shutdown,
on_test_run_request, on_tests_request,
on_code_lens_request, on_formatting, on_goto_definition_request, on_initialize,
on_profile_run_request, on_shutdown, on_test_run_request, on_tests_request,
};
use serde_json::Value as JsonValue;
use tower::Service;
Expand Down Expand Up @@ -76,6 +76,7 @@ impl NargoLspService {
.request::<request::NargoTests, _>(on_tests_request)
.request::<request::NargoTestRun, _>(on_test_run_request)
.request::<request::NargoProfileRun, _>(on_profile_run_request)
.request::<request::GotoDefinition, _>(on_goto_definition_request)
.notification::<notification::Initialized>(on_initialized)
.notification::<notification::DidChangeConfiguration>(on_did_change_configuration)
.notification::<notification::DidOpenTextDocument>(on_did_open_text_document)
Expand Down
Loading

0 comments on commit 7bb7356

Please sign in to comment.