Skip to content

Commit

Permalink
fix(lsp): Ensure lsp does not crawl past the root specified (#2322)
Browse files Browse the repository at this point in the history
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <tom@tomfren.ch>
  • Loading branch information
3 people authored Aug 15, 2023
1 parent 9eb45da commit d69e372
Show file tree
Hide file tree
Showing 11 changed files with 101 additions and 51 deletions.
35 changes: 30 additions & 5 deletions crates/lsp/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::{
future::{self, Future},
ops::{self, ControlFlow},
path::PathBuf,
pin::Pin,
task::{self, Poll},
};
Expand Down Expand Up @@ -31,12 +32,13 @@ 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() }
Self { client: client.clone(), root_path: None }
}
}

Expand Down Expand Up @@ -102,9 +104,11 @@ 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>> {
state.root_path = params.root_uri.and_then(|root_uri| root_uri.to_file_path().ok());

async {
let text_document_sync =
TextDocumentSyncOptions { save: Some(true.into()), ..Default::default() };
Expand Down Expand Up @@ -144,7 +148,17 @@ fn on_code_lens_request(
}
};

let toml_path = match find_package_manifest(&file_path) {
let root_path = match &state.root_path {
Some(root) => root,
None => {
return future::ready(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
"Could not find project root",
)))
}
};

let toml_path = match find_package_manifest(root_path, &file_path) {
Ok(toml_path) => toml_path,
Err(err) => {
// If we cannot find a manifest, we log a warning but return no code lenses
Expand Down Expand Up @@ -269,7 +283,18 @@ fn on_did_save_text_document(
}
};

let toml_path = match find_package_manifest(&file_path) {
let root_path = match &state.root_path {
Some(root) => root,
None => {
return ControlFlow::Break(Err(ResponseError::new(
ErrorCode::REQUEST_FAILED,
"Could not find project root",
)
.into()));
}
};

let toml_path = match find_package_manifest(root_path, &file_path) {
Ok(toml_path) => toml_path,
Err(err) => {
// If we cannot find a manifest, we log a warning but return no diagnostics
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use acvm::Backend;
use clap::Args;
use iter_extended::btree_map;
use nargo::{package::Package, prepare_package};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME};
use noirc_driver::{check_crate, compute_function_signature, CompileOptions};
use noirc_frontend::{
Expand Down Expand Up @@ -34,7 +34,7 @@ pub(crate) fn run<B: Backend>(
args: CheckCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/codegen_verifier_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use nargo::{
ops::{codegen_verifier, preprocess_program},
package::Package,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::CompileOptions;
use noirc_frontend::graph::CrateName;

Expand All @@ -44,7 +44,7 @@ pub(crate) fn run<B: Backend>(
args: CodegenVerifierCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use nargo::artifacts::debug::DebugArtifact;
use nargo::package::Package;
use nargo::prepare_package;
use nargo::{artifacts::contract::PreprocessedContract, NargoError};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
compile_contracts, compile_main, CompileOptions, CompiledContract, CompiledProgram,
ErrorsAndWarnings, Warnings,
Expand Down Expand Up @@ -62,7 +62,7 @@ pub(crate) fn run<B: Backend>(
args: CompileCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use clap::Args;
use nargo::constants::PROVER_INPUT_FILE;
use nargo::package::Package;
use nargo::NargoError;
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_abi::input_parser::{Format, InputValue};
use noirc_abi::{Abi, InputMap};
use noirc_driver::{CompileOptions, CompiledProgram};
Expand Down Expand Up @@ -45,7 +45,7 @@ pub(crate) fn run<B: Backend>(
args: ExecuteCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/info_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use acvm::Backend;
use clap::Args;
use iter_extended::try_vecmap;
use nargo::{package::Package, prepare_package};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{compile_contracts, CompileOptions};
use noirc_frontend::graph::CrateName;
use prettytable::{row, Table};
Expand Down Expand Up @@ -38,7 +38,7 @@ pub(crate) fn run<B: Backend>(
args: InfoCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/prove_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use nargo::artifacts::program::PreprocessedProgram;
use nargo::constants::{PROVER_INPUT_FILE, VERIFIER_INPUT_FILE};
use nargo::ops::{preprocess_program, prove_execution, verify_proof};
use nargo::package::Package;
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_abi::input_parser::Format;
use noirc_driver::CompileOptions;
use noirc_frontend::graph::CrateName;
Expand Down Expand Up @@ -56,7 +56,7 @@ pub(crate) fn run<B: Backend>(
args: ProveCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::io::Write;
use acvm::{acir::native_types::WitnessMap, Backend};
use clap::Args;
use nargo::{ops::execute_circuit, package::Package, prepare_package};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{compile_no_check, CompileOptions};
use noirc_frontend::{
graph::CrateName,
Expand Down Expand Up @@ -47,7 +47,7 @@ pub(crate) fn run<B: Backend>(
args: TestCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/verify_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use clap::Args;
use nargo::constants::{PROOF_EXT, VERIFIER_INPUT_FILE};
use nargo::ops::{preprocess_program, verify_proof};
use nargo::{artifacts::program::PreprocessedProgram, package::Package};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_abi::input_parser::Format;
use noirc_driver::CompileOptions;
use noirc_frontend::graph::CrateName;
Expand Down Expand Up @@ -48,7 +48,7 @@ pub(crate) fn run<B: Backend>(
args: VerifyCommand,
config: NargoConfig,
) -> Result<(), CliError<B>> {
let toml_path = find_package_manifest(&config.program_dir)?;
let toml_path = get_package_manifest(&config.program_dir)?;
let default_selection =
if args.workspace { PackageSelection::All } else { PackageSelection::DefaultOrAll };
let selection = args.package.map_or(default_selection, PackageSelection::Selected);
Expand Down
3 changes: 3 additions & 0 deletions crates/nargo_toml/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,7 @@ pub enum ManifestError {

#[error("Missing `name` field in {toml}")]
MissingNameField { toml: PathBuf },

#[error("No common ancestor between {root} and {current}")]
NoCommonAncestor { root: PathBuf, current: PathBuf },
}
82 changes: 52 additions & 30 deletions crates/nargo_toml/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::{
collections::BTreeMap,
fs::ReadDir,
path::{Path, PathBuf},
path::{Component, Path, PathBuf},
};

use fm::{NormalizePath, FILE_EXTENSION};
Expand All @@ -18,48 +17,71 @@ mod git;
pub use errors::ManifestError;
use git::clone_git_repo;

/// Returns the path of the root directory of the package containing `current_path`.
/// Returns the [PathBuf] of the directory containing the `Nargo.toml` by searching from `current_path` to the root of its [Path].
///
/// Returns a `CliError` if no parent directories of `current_path` contain a manifest file.
/// Returns a [ManifestError] if no parent directories of `current_path` contain a manifest file.
pub fn find_package_root(current_path: &Path) -> Result<PathBuf, ManifestError> {
let manifest_path = find_package_manifest(current_path)?;
let root = path_root(current_path);
let manifest_path = find_package_manifest(&root, current_path)?;

let package_root =
manifest_path.parent().expect("infallible: manifest file path can't be root directory");

Ok(package_root.to_path_buf())
}

/// Returns the path of the manifest file (`Nargo.toml`) of the package containing `current_path`.
///
/// Returns a `CliError` if no parent directories of `current_path` contain a manifest file.
pub fn find_package_manifest(current_path: &Path) -> Result<PathBuf, ManifestError> {
current_path
.ancestors()
.find_map(|dir| find_file(dir, "Nargo", "toml"))
.ok_or_else(|| ManifestError::MissingFile(current_path.to_path_buf()))
}
// TODO(#2323): We are probably going to need a "filepath utils" crate soon
fn path_root(path: &Path) -> PathBuf {
let mut components = path.components();

// Looks for file named `file_name` in path
fn find_file<P: AsRef<Path>>(path: P, file_name: &str, extension: &str) -> Option<PathBuf> {
let entries = list_files_and_folders_in(path)?;
let file_name = format!("{file_name}.{extension}");

find_artifact(entries, &file_name)
match (components.next(), components.next()) {
// Preserve prefix if one exists
(Some(prefix @ Component::Prefix(_)), Some(root @ Component::RootDir)) => {
PathBuf::from(prefix.as_os_str()).join(root.as_os_str())
}
(Some(root @ Component::RootDir), _) => PathBuf::from(root.as_os_str()),
_ => PathBuf::new(),
}
}

// There is no distinction between files and folders
fn find_artifact(entries: ReadDir, artifact_name: &str) -> Option<PathBuf> {
let entry = entries
.into_iter()
.flatten()
.find(|entry| entry.file_name().to_str() == Some(artifact_name))?;
/// Returns the [PathBuf] of the `Nargo.toml` file by searching from `current_path` and stopping at `root_path`.
///
/// Returns a [ManifestError] if no parent directories of `current_path` contain a manifest file.
pub fn find_package_manifest(
root_path: &Path,
current_path: &Path,
) -> Result<PathBuf, ManifestError> {
if current_path.starts_with(root_path) {
let mut found_toml_paths = Vec::new();
for path in current_path.ancestors() {
if let Ok(toml_path) = get_package_manifest(path) {
found_toml_paths.push(toml_path);
}
// While traversing, break once we process the root specified
if path == root_path {
break;
}
}

Some(entry.path())
// Return the shallowest Nargo.toml, which will be the last in the list
found_toml_paths.pop().ok_or_else(|| ManifestError::MissingFile(current_path.to_path_buf()))
} else {
Err(ManifestError::NoCommonAncestor {
root: root_path.to_path_buf(),
current: current_path.to_path_buf(),
})
}
}

fn list_files_and_folders_in<P: AsRef<Path>>(path: P) -> Option<ReadDir> {
std::fs::read_dir(path).ok()
/// Returns the [PathBuf] of the `Nargo.toml` file in the `current_path` directory.
///
/// Returns a [ManifestError] if `current_path` does not contain a manifest file.
pub fn get_package_manifest(current_path: &Path) -> Result<PathBuf, ManifestError> {
let toml_path = current_path.join("Nargo.toml");
if toml_path.exists() {
Ok(toml_path)
} else {
Err(ManifestError::MissingFile(current_path.to_path_buf()))
}
}

#[derive(Debug, Deserialize, Clone)]
Expand Down

0 comments on commit d69e372

Please sign in to comment.