Skip to content

fix(lsp): Improve dependency resolution in context of Nargo.toml #2226

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.

Sign up for GitHub

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

Merged
TomAFrench merged 2 commits into from
Aug 10, 2023
Merged

fix(lsp): Improve dependency resolution in context of Nargo.toml #2226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d702d68
fix(lsp): Improve dependency resolution in context of `Nargo.toml`
phated Aug 8, 2023
b02f794
better error handling instead of unwraps
phated Aug 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions crates/lsp/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ codespan-lsp.workspace = true
codespan-reporting.workspace = true
fm.workspace = true
lsp-types.workspace = true
nargo.workspace = true
nargo_toml.workspace = true
noirc_driver.workspace = true
noirc_errors.workspace = true
noirc_frontend.workspace = true
Expand Down
254 changes: 132 additions & 122 deletions crates/lsp/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
mod lib_hacky;
use std::env;

use std::{
future::{self, Future},
ops::{self, ControlFlow},
path::PathBuf,
pin::Pin,
task::{self, Poll},
};
Expand All @@ -14,17 +10,18 @@ use async_lsp::{
LanguageClient, LspService, ResponseError,
};
use codespan_reporting::files;
use fm::FileManager;
use fm::FILE_EXTENSION;
use lsp_types::{
notification, request, CodeLens, CodeLensOptions, CodeLensParams, Command, Diagnostic,
DiagnosticSeverity, DidChangeConfigurationParams, DidChangeTextDocumentParams,
DidCloseTextDocumentParams, DidOpenTextDocumentParams, DidSaveTextDocumentParams,
InitializeParams, InitializeResult, InitializedParams, Position, PublishDiagnosticsParams,
Range, ServerCapabilities, TextDocumentSyncOptions,
InitializeParams, InitializeResult, InitializedParams, LogMessageParams, MessageType, Position,
PublishDiagnosticsParams, Range, ServerCapabilities, TextDocumentSyncOptions,
};
use noirc_driver::{check_crate, prepare_crate};
use nargo::prepare_package;
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml};
use noirc_driver::check_crate;
use noirc_errors::{DiagnosticKind, FileDiagnostic};
use noirc_frontend::{graph::CrateGraph, hir::Context};
use serde_json::Value as JsonValue;
use tower::Service;

Expand All @@ -33,13 +30,12 @@ const TEST_CODELENS_TITLE: &str = "▶\u{fe0e} Run Test";

// State for the LSP gets implemented on this struct and is internal to the implementation
pub struct LspState {
root_path: Option<PathBuf>,
client: ClientSocket,
}

impl LspState {
fn new(client: &ClientSocket) -> Self {
Self { client: client.clone(), root_path: None }
Self { client: client.clone() }
}
}

Expand All @@ -49,35 +45,6 @@ pub struct NargoLspService {

impl NargoLspService {
pub fn new(client: &ClientSocket) -> Self {
// Using conditional running with lib_hacky to prevent non-hacky code from being identified as dead code
// Secondarily, provides a runtime way to stress the non-hacky code.
if env::var("NOIR_LSP_NO_HACK").is_err() {
let state = LspState::new(client);
let mut router = Router::new(state);
router
.request::<request::Initialize, _>(lib_hacky::on_initialize)
.request::<request::Shutdown, _>(lib_hacky::on_shutdown)
.request::<request::CodeLensRequest, _>(lib_hacky::on_code_lens_request)
.notification::<notification::Initialized>(lib_hacky::on_initialized)
.notification::<notification::DidChangeConfiguration>(
lib_hacky::on_did_change_configuration,
)
.notification::<notification::DidOpenTextDocument>(
lib_hacky::on_did_open_text_document,
)
.notification::<notification::DidChangeTextDocument>(
lib_hacky::on_did_change_text_document,
)
.notification::<notification::DidCloseTextDocument>(
lib_hacky::on_did_close_text_document,
)
.notification::<notification::DidSaveTextDocument>(
lib_hacky::on_did_save_text_document,
)
.notification::<notification::Exit>(lib_hacky::on_exit);
return Self { router };
}

let state = LspState::new(client);
let mut router = Router::new(state);
router
Expand Down Expand Up @@ -134,13 +101,9 @@ impl LspService for NargoLspService {
// and params passed in.

fn on_initialize(
state: &mut LspState,
params: InitializeParams,
_state: &mut LspState,
_params: InitializeParams,
) -> impl Future<Output = Result<InitializeResult, ResponseError>> {
if let Some(root_uri) = params.root_uri {
state.root_path = root_uri.to_file_path().ok();
}

async {
let text_document_sync =
TextDocumentSyncOptions { save: Some(true.into()), ..Default::default() };
Expand Down Expand Up @@ -170,54 +133,76 @@ fn on_code_lens_request(
state: &mut LspState,
params: CodeLensParams,
) -> impl Future<Output = Result<Option<Vec<CodeLens>>, ResponseError>> {
let file_path = &params.text_document.uri.to_file_path().unwrap();
let file_path = match params.text_document.uri.to_file_path() {
Ok(file_path) => file_path,
Err(()) => {
return future::ready(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
"URI is not a valid file path",
)))
}
};

let mut context = match &state.root_path {
Some(root_path) => {
let fm = FileManager::new(root_path);
let graph = CrateGraph::default();
Context::new(fm, graph)
let toml_path = match find_package_manifest(&file_path) {
Ok(toml_path) => toml_path,
Err(err) => {
// If we cannot find a manifest, we log a warning but return no code lenses
// We can reconsider this when we can build a file without the need for a Nargo.toml file to resolve deps
let _ = state.client.log_message(LogMessageParams {
typ: MessageType::WARNING,
message: format!("{}", err),
});
return future::ready(Ok(None));
}
None => {
let err = ResponseError::new(
};
let workspace = match resolve_workspace_from_toml(&toml_path, None) {
Ok(workspace) => workspace,
Err(err) => {
// If we found a manifest, but the workspace is invalid, we raise an error about it
return future::ready(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
"Unable to determine the project root path",
);
return future::ready(Err(err));
format!("{}", err),
)));
}
};

let crate_id = prepare_crate(&mut context, file_path);
let mut lenses: Vec<CodeLens> = vec![];

// We ignore the warnings and errors produced by compilation for producing codelenses
// because we can still get the test functions even if compilation fails
let _ = check_crate(&mut context, crate_id, false);
for package in &workspace {
let (mut context, crate_id) = prepare_package(package);
// We ignore the warnings and errors produced by compilation for producing codelenses
// because we can still get the test functions even if compilation fails
let _ = check_crate(&mut context, crate_id, false);

let fm = &context.file_manager;
let files = fm.as_simple_files();
let tests = context.get_all_test_functions_in_crate_matching(&crate_id, "");
let fm = &context.file_manager;
let files = fm.as_simple_files();
let tests = context.get_all_test_functions_in_crate_matching(&crate_id, "");

let mut lenses: Vec<CodeLens> = vec![];
for (func_name, func_id) in tests {
let location = context.function_meta(&func_id).name.location;
let file_id = location.file;
// TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file
if file_id.as_usize() != 0 {
continue;
}
for (func_name, func_id) in tests {
let location = context.function_meta(&func_id).name.location;
let file_id = location.file;

// Ignore diagnostics for any file that wasn't the file we saved
// TODO: In the future, we could create "related" diagnostics for these files
// TODO: This currently just appends the `.nr` file extension that we store as a constant,
// but that won't work if we accept other extensions
if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
continue;
}

let range =
byte_span_to_range(files, file_id.as_usize(), location.span.into()).unwrap_or_default();
let range = byte_span_to_range(files, file_id.as_usize(), location.span.into())
.unwrap_or_default();

let command = Command {
title: TEST_CODELENS_TITLE.into(),
command: TEST_COMMAND.into(),
arguments: Some(vec![func_name.into()]),
};
let command = Command {
title: TEST_CODELENS_TITLE.into(),
command: TEST_COMMAND.into(),
arguments: Some(vec![func_name.into()]),
};

let lens = CodeLens { range, command: command.into(), data: None };
let lens = CodeLens { range, command: command.into(), data: None };

lenses.push(lens);
lenses.push(lens);
}
}

let res = if lenses.is_empty() { Ok(None) } else { Ok(Some(lenses)) };
Expand Down Expand Up @@ -264,60 +249,85 @@ fn on_did_save_text_document(
state: &mut LspState,
params: DidSaveTextDocumentParams,
) -> ControlFlow<Result<(), async_lsp::Error>> {
let file_path = &params.text_document.uri.to_file_path().unwrap();
let mut context = match &state.root_path {
Some(root_path) => {
let fm = FileManager::new(root_path);
let graph = CrateGraph::default();
Context::new(fm, graph)
}
None => {
let err = ResponseError::new(
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,
"Unable to determine the project root path",
);
return ControlFlow::Break(Err(err.into()));
"URI is not a valid file path",
)
.into()))
}
};

let crate_id = prepare_crate(&mut context, file_path);
let toml_path = match find_package_manifest(&file_path) {
Ok(toml_path) => toml_path,
Err(err) => {
// If we cannot find a manifest, we log a warning but return no diagnostics
// We can reconsider this when we can build a file without the need for a Nargo.toml file to resolve deps
let _ = state.client.log_message(LogMessageParams {
typ: MessageType::WARNING,
message: format!("{}", err),
});
return ControlFlow::Continue(());
}
};
let workspace = match resolve_workspace_from_toml(&toml_path, None) {
Ok(workspace) => workspace,
Err(err) => {
// If we found a manifest, but the workspace is invalid, we raise an error about it
return ControlFlow::Break(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
format!("{}", err),
)
.into()));
}
};

let mut diagnostics = Vec::new();

let file_diagnostics = match check_crate(&mut context, crate_id, false) {
Ok(warnings) => warnings,
Err(errors_and_warnings) => errors_and_warnings,
};
for package in &workspace {
let (mut context, crate_id) = prepare_package(package);

if !file_diagnostics.is_empty() {
let fm = &context.file_manager;
let files = fm.as_simple_files();
let file_diagnostics = match check_crate(&mut context, crate_id, false) {
Ok(warnings) => warnings,
Err(errors_and_warnings) => errors_and_warnings,
};

for FileDiagnostic { file_id, diagnostic } in file_diagnostics {
// TODO(#1681): This file_id never be 0 because the "path" where it maps is the directory, not a file
if file_id.as_usize() != 0 {
continue;
}
if !file_diagnostics.is_empty() {
let fm = &context.file_manager;
let files = fm.as_simple_files();

for FileDiagnostic { file_id, diagnostic } in file_diagnostics {
// Ignore diagnostics for any file that wasn't the file we saved
// TODO: In the future, we could create "related" diagnostics for these files
// TODO: This currently just appends the `.nr` file extension that we store as a constant,
// but that won't work if we accept other extensions
if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
continue;
}

let mut range = Range::default();
let mut range = Range::default();

// TODO: Should this be the first item in secondaries? Should we bail when we find a range?
for sec in diagnostic.secondaries {
// Not using `unwrap_or_default` here because we don't want to overwrite a valid range with a default range
if let Some(r) = byte_span_to_range(files, file_id.as_usize(), sec.span.into()) {
range = r
// TODO: Should this be the first item in secondaries? Should we bail when we find a range?
for sec in diagnostic.secondaries {
// Not using `unwrap_or_default` here because we don't want to overwrite a valid range with a default range
if let Some(r) = byte_span_to_range(files, file_id.as_usize(), sec.span.into())
{
range = r
}
}
let severity = match diagnostic.kind {
DiagnosticKind::Error => Some(DiagnosticSeverity::ERROR),
DiagnosticKind::Warning => Some(DiagnosticSeverity::WARNING),
};
diagnostics.push(Diagnostic {
range,
severity,
message: diagnostic.message,
..Diagnostic::default()
})
}
let severity = match diagnostic.kind {
DiagnosticKind::Error => Some(DiagnosticSeverity::ERROR),
DiagnosticKind::Warning => Some(DiagnosticSeverity::WARNING),
};
diagnostics.push(Diagnostic {
range,
severity,
message: diagnostic.message,
..Diagnostic::default()
})
}
}

Expand Down
Loading