Skip to content

Commit

Permalink
feat: LSP code actions to import or qualify unresolved paths (#5876)
Browse files Browse the repository at this point in the history
# Description

## Problem

Part of #1579

## Summary

There are many code actions we could support, but the most useful ones
are importing unknown identifiers or fully-qualifying them. This is what
this PR is about.


![lsp-code-actions](https://github.com/user-attachments/assets/6107bb2b-ed2d-4430-a213-5a3d4a6e1388)

Also thanks to VS Code that it notices that if you request code action
on an error, it actually requests code actions for that error span... so
it seems like the two are connected, but they are not! And it works just
fine :-)

Also fixes a bug I found in "go to definition" when there was no
definition to resolve to (precisely the case this code action works on).

## Additional Context

Note that in Rust Analyzer it shows as "Qualify [name]" or "Import
[name]" and if there are multiple possibilities, a popup shows up
letting you choose which one. I wanted to do the same, but looking at
the server trace for Rust Analyzer I see each code action has a "group"
property, and that's how they are grouped, but... I couldn't find that
property in the lsp-types crate, or even in the LSP specification 😮 . So
for now each possibility will show up (no intermediate popup). But I
think that's better than not having this functionality at all, and we
could always improve it later if we find out how.

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

---------

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
  • Loading branch information
asterite and TomAFrench authored Sep 2, 2024
1 parent f0c2686 commit 410c1f6
Show file tree
Hide file tree
Showing 9 changed files with 705 additions and 156 deletions.
20 changes: 12 additions & 8 deletions compiler/noirc_frontend/src/resolve_locations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use noirc_errors::Location;
use crate::hir_def::expr::HirExpression;
use crate::hir_def::types::Type;

use crate::node_interner::{DefinitionKind, Node, NodeInterner};
use crate::node_interner::{DefinitionId, DefinitionKind, Node, NodeInterner};

impl NodeInterner {
/// Scans the interner for the item which is located at that [Location]
Expand Down Expand Up @@ -108,14 +108,18 @@ impl NodeInterner {
) -> 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)
if ident.id != DefinitionId::dummy_id() {
let definition_info = self.definition(ident.id);
match definition_info.kind {
DefinitionKind::Function(func_id) => {
Some(self.function_meta(&func_id).location)
}
DefinitionKind::Local(_local_id) => Some(definition_info.location),
DefinitionKind::Global(_global_id) => Some(definition_info.location),
_ => None,
}
DefinitionKind::Local(_local_id) => Some(definition_info.location),
DefinitionKind::Global(_global_id) => Some(definition_info.location),
_ => None,
} else {
None
}
}
HirExpression::Constructor(expr) => {
Expand Down
18 changes: 11 additions & 7 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ use fm::{codespan_files as files, FileManager};
use fxhash::FxHashSet;
use lsp_types::{
request::{
Completion, DocumentSymbolRequest, HoverRequest, InlayHintRequest, PrepareRenameRequest,
References, Rename, SignatureHelpRequest,
CodeActionRequest, Completion, DocumentSymbolRequest, HoverRequest, InlayHintRequest,
PrepareRenameRequest, References, Rename, SignatureHelpRequest,
},
CodeLens,
};
Expand Down Expand Up @@ -51,21 +51,24 @@ use notifications::{
on_did_open_text_document, on_did_save_text_document, on_exit, on_initialized,
};
use requests::{
on_code_lens_request, on_completion_request, on_document_symbol_request, on_formatting,
on_goto_declaration_request, on_goto_definition_request, on_goto_type_definition_request,
on_hover_request, on_initialize, on_inlay_hint_request, on_prepare_rename_request,
on_profile_run_request, on_references_request, on_rename_request, on_shutdown,
on_signature_help_request, on_test_run_request, on_tests_request, LspInitializationOptions,
on_code_action_request, on_code_lens_request, on_completion_request,
on_document_symbol_request, on_formatting, on_goto_declaration_request,
on_goto_definition_request, on_goto_type_definition_request, on_hover_request, on_initialize,
on_inlay_hint_request, on_prepare_rename_request, on_profile_run_request,
on_references_request, on_rename_request, on_shutdown, on_signature_help_request,
on_test_run_request, on_tests_request, LspInitializationOptions,
};
use serde_json::Value as JsonValue;
use thiserror::Error;
use tower::Service;

mod modules;
mod notifications;
mod requests;
mod solver;
mod types;
mod utils;
mod visibility;

#[cfg(test)]
mod test_utils;
Expand Down Expand Up @@ -144,6 +147,7 @@ impl NargoLspService {
.request::<InlayHintRequest, _>(on_inlay_hint_request)
.request::<Completion, _>(on_completion_request)
.request::<SignatureHelpRequest, _>(on_signature_help_request)
.request::<CodeActionRequest, _>(on_code_action_request)
.notification::<notification::Initialized>(on_initialized)
.notification::<notification::DidChangeConfiguration>(on_did_change_configuration)
.notification::<notification::DidOpenTextDocument>(on_did_open_text_document)
Expand Down
131 changes: 131 additions & 0 deletions tooling/lsp/src/modules.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
use std::collections::BTreeMap;

use noirc_frontend::{
ast::ItemVisibility,
graph::CrateId,
hir::def_map::{CrateDefMap, ModuleId},
macros_api::{ModuleDefId, NodeInterner},
node_interner::ReferenceId,
};

use crate::visibility::is_visible;

pub(crate) fn get_parent_module(
interner: &NodeInterner,
module_def_id: ModuleDefId,
) -> Option<ModuleId> {
let reference_id = module_def_id_to_reference_id(module_def_id);
interner.reference_module(reference_id).copied()
}

pub(crate) fn get_parent_module_id(
def_maps: &BTreeMap<CrateId, CrateDefMap>,
module_id: ModuleId,
) -> Option<ModuleId> {
let crate_def_map = &def_maps[&module_id.krate];
let module_data = &crate_def_map.modules()[module_id.local_id.0];
module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent })
}

pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId {
match module_def_id {
ModuleDefId::ModuleId(id) => ReferenceId::Module(id),
ModuleDefId::FunctionId(id) => ReferenceId::Function(id),
ModuleDefId::TypeId(id) => ReferenceId::Struct(id),
ModuleDefId::TypeAliasId(id) => ReferenceId::Alias(id),
ModuleDefId::TraitId(id) => ReferenceId::Trait(id),
ModuleDefId::GlobalId(id) => ReferenceId::Global(id),
}
}

/// Returns the fully-qualified path of the given `ModuleDefId` relative to `current_module_id`:
/// - If `ModuleDefId` is a module, that module's path is returned
/// - Otherwise, that item's parent module's path is returned
pub(crate) fn module_full_path(
module_def_id: ModuleDefId,
visibility: ItemVisibility,
current_module_id: ModuleId,
current_module_parent_id: Option<ModuleId>,
interner: &NodeInterner,
) -> Option<String> {
let full_path;
if let ModuleDefId::ModuleId(module_id) = module_def_id {
if !is_visible(visibility, current_module_id, module_id) {
return None;
}

full_path =
module_id_path(module_id, &current_module_id, current_module_parent_id, interner);
} else {
let Some(parent_module) = get_parent_module(interner, module_def_id) else {
return None;
};

if !is_visible(visibility, current_module_id, parent_module) {
return None;
}

full_path =
module_id_path(parent_module, &current_module_id, current_module_parent_id, interner);
}
Some(full_path)
}

/// Returns the path to reach an item inside `target_module_id` from inside `current_module_id`.
/// Returns a relative path if possible.
pub(crate) fn module_id_path(
target_module_id: ModuleId,
current_module_id: &ModuleId,
current_module_parent_id: Option<ModuleId>,
interner: &NodeInterner,
) -> String {
if Some(target_module_id) == current_module_parent_id {
return "super".to_string();
}

let mut segments: Vec<&str> = Vec::new();
let mut is_relative = false;

if let Some(module_attributes) = interner.try_module_attributes(&target_module_id) {
segments.push(&module_attributes.name);

let mut current_attributes = module_attributes;
loop {
let Some(parent_local_id) = current_attributes.parent else {
break;
};

let parent_module_id =
&ModuleId { krate: target_module_id.krate, local_id: parent_local_id };

if current_module_id == parent_module_id {
is_relative = true;
break;
}

if current_module_parent_id == Some(*parent_module_id) {
segments.push("super");
is_relative = true;
break;
}

let Some(parent_attributes) = interner.try_module_attributes(parent_module_id) else {
break;
};

segments.push(&parent_attributes.name);
current_attributes = parent_attributes;
}
}

if !is_relative {
// We don't record module attributes for the root module,
// so we handle that case separately
if let CrateId::Root(_) = target_module_id.krate {
segments.push("crate");
}
}

segments.reverse();
segments.join("::")
}
Loading

0 comments on commit 410c1f6

Please sign in to comment.