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

feat(lsp): cache definitions for goto requests #3930

Merged
merged 7 commits into from
Jan 4, 2024
Merged
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 @@ -35,7 +35,7 @@ pub struct TraitType {
/// Represents a trait in the type system. Each instance of this struct
/// will be shared across all Type::Trait variants that represent
/// the same trait.
#[derive(Debug, Eq)]
#[derive(Clone, Debug, Eq)]
kobyhallx marked this conversation as resolved.
Show resolved Hide resolved
pub struct Trait {
/// A unique id representing this trait type. Used to check if two
/// struct traits are equal.
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +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)]
#[derive(Clone, Debug)]
pub struct NodeInterner {
nodes: Arena<Node>,
func_meta: HashMap<FuncId, FuncMeta>,
Expand Down Expand Up @@ -164,7 +164,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, Debug)]
#[derive(Clone, Default, Debug)]
pub struct Methods {
direct: Vec<FuncId>,
trait_impl_methods: Vec<FuncId>,
Expand Down
5 changes: 5 additions & 0 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ use noirc_driver::{file_manager_with_stdlib, prepare_crate, NOIR_ARTIFACT_VERSIO
use noirc_frontend::{
graph::{CrateId, CrateName},
hir::{Context, FunctionNameMatch},
node_interner::NodeInterner,
};

use notifications::{
Expand Down Expand Up @@ -59,8 +60,10 @@ pub struct LspState {
root_path: Option<PathBuf>,
client: ClientSocket,
solver: WrapperSolver,
open_documents_count: usize,
input_files: HashMap<String, String>,
cached_lenses: HashMap<String, Vec<CodeLens>>,
cached_definitions: HashMap<String, NodeInterner>,
}

impl LspState {
Expand All @@ -71,6 +74,8 @@ impl LspState {
solver: WrapperSolver(Box::new(solver)),
input_files: HashMap::new(),
cached_lenses: HashMap::new(),
cached_definitions: HashMap::new(),
open_documents_count: 0,
}
}
}
Expand Down
67 changes: 42 additions & 25 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,16 @@ pub(super) fn on_did_open_text_document(
params: DidOpenTextDocumentParams,
) -> ControlFlow<Result<(), async_lsp::Error>> {
state.input_files.insert(params.text_document.uri.to_string(), params.text_document.text);
ControlFlow::Continue(())

let document_uri = params.text_document.uri;

match process_noir_document(document_uri, state) {
Ok(_) => {
state.open_documents_count += 1;
ControlFlow::Continue(())
}
Err(err) => ControlFlow::Break(Err(err)),
}
}

pub(super) fn on_did_change_text_document(
Expand Down Expand Up @@ -85,34 +94,39 @@ pub(super) fn on_did_close_text_document(
) -> ControlFlow<Result<(), async_lsp::Error>> {
state.input_files.remove(&params.text_document.uri.to_string());
state.cached_lenses.remove(&params.text_document.uri.to_string());

state.open_documents_count -= 1;

if state.open_documents_count == 0 {
state.cached_definitions.clear();
}

ControlFlow::Continue(())
}

pub(super) fn on_did_save_text_document(
state: &mut LspState,
params: DidSaveTextDocumentParams,
) -> ControlFlow<Result<(), async_lsp::Error>> {
let file_path = match params.text_document.uri.to_file_path() {
Ok(file_path) => file_path,
Err(()) => {
return ControlFlow::Break(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
"URI is not a valid file path",
)
.into()))
}
};
let document_uri = params.text_document.uri;

let workspace = match resolve_workspace_for_source_path(&file_path) {
Ok(value) => value,
Err(lsp_error) => {
return ControlFlow::Break(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
lsp_error.to_string(),
)
.into()))
}
};
match process_noir_document(document_uri, state) {
Ok(_) => ControlFlow::Continue(()),
Err(err) => ControlFlow::Break(Err(err)),
}
}

fn process_noir_document(
document_uri: lsp_types::Url,
state: &mut LspState,
) -> Result<(), async_lsp::Error> {
let file_path = document_uri.to_file_path().map_err(|_| {
ResponseError::new(ErrorCode::REQUEST_FAILED, "URI is not a valid file path")
})?;

let workspace = resolve_workspace_for_source_path(&file_path).map_err(|lsp_error| {
ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string())
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
Expand All @@ -127,6 +141,10 @@ pub(super) fn on_did_save_text_document(
Err(errors_and_warnings) => errors_and_warnings,
};

let package_root_dir: String = package.root_dir.as_os_str().to_string_lossy().into();

state.cached_definitions.insert(package_root_dir, context.def_interner.clone());
kobyhallx marked this conversation as resolved.
Show resolved Hide resolved

// We don't add test headings for a package if it contains no `#[test]` functions
if let Some(tests) = get_package_tests_in_crate(&context, &crate_id, &package.name) {
let _ = state.client.notify::<notification::NargoUpdateTests>(NargoPackageTests {
Expand All @@ -142,7 +160,7 @@ pub(super) fn on_did_save_text_document(
package,
Some(&file_path),
);
state.cached_lenses.insert(params.text_document.uri.to_string(), collected_lenses);
state.cached_lenses.insert(document_uri.to_string(), collected_lenses);

let fm = &context.file_manager;
let files = fm.as_file_map();
Expand Down Expand Up @@ -178,14 +196,13 @@ pub(super) fn on_did_save_text_document(
.collect()
})
.collect();

let _ = state.client.publish_diagnostics(PublishDiagnosticsParams {
uri: params.text_document.uri,
uri: document_uri,
version: None,
diagnostics,
});

ControlFlow::Continue(())
Ok(())
}

pub(super) fn on_exit(
Expand Down
10 changes: 8 additions & 2 deletions tooling/lsp/src/requests/goto_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,19 @@ fn on_goto_definition_inner(
let workspace = resolve_workspace_for_source_path(file_path.as_path()).unwrap();
let package = workspace.members.first().unwrap();

let package_root_path: String = package.root_dir.as_os_str().to_string_lossy().into();

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);

let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);

// We ignore the warnings and errors produced by compilation while resolving the definition
let _ = noirc_driver::check_crate(&mut context, crate_id, false, false);
if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) {
context.def_interner = def_interner.clone();
} else {
// We ignore the warnings and errors produced by compilation while resolving the definition
let _ = noirc_driver::check_crate(&mut context, crate_id, false, false);
}

let files = context.file_manager.as_file_map();
let file_id = context.file_manager.name_to_id(file_path.clone()).ok_or(ResponseError::new(
Expand Down
Loading