From 1f0a3479cb345482185a1a7c48dcb8059c389488 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Sun, 8 Sep 2024 19:06:38 -0300 Subject: [PATCH 1/3] feat: let `nargo` and LSP work well in the stdlib --- compiler/noirc_driver/src/lib.rs | 16 +++++----- compiler/noirc_frontend/src/graph/mod.rs | 38 ++++++++++++++++++++++-- tooling/lsp/src/modules.rs | 2 +- tooling/lsp/src/notifications/mod.rs | 5 ++-- tooling/lsp/src/requests/hover.rs | 3 +- tooling/lsp/src/requests/mod.rs | 4 +-- tooling/lsp/src/requests/profile_run.rs | 6 ++-- tooling/lsp/src/requests/test_run.rs | 6 ++-- tooling/lsp/src/requests/tests.rs | 4 +-- tooling/nargo/src/workspace.rs | 18 +++++++++++ tooling/nargo_cli/src/cli/check_cmd.rs | 5 ++-- tooling/nargo_cli/src/cli/compile_cmd.rs | 4 +-- tooling/nargo_cli/src/cli/export_cmd.rs | 5 ++-- tooling/nargo_cli/src/cli/fmt_cmd.rs | 4 +-- tooling/nargo_cli/src/cli/test_cmd.rs | 6 ++-- 15 files changed, 85 insertions(+), 41 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 31c279bc0f3..344428f23a4 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -212,18 +212,20 @@ fn add_debug_source_to_file_manager(file_manager: &mut FileManager) { /// Adds the file from the file system at `Path` to the crate graph as a root file /// -/// Note: This methods adds the stdlib as a dependency to the crate. -/// This assumes that the stdlib has already been added to the file manager. +/// Note: If the stdlib dependency has not been added yet, it's added. Otherwise +/// this method assumes the root crate is the stdlib (useful for running tests +/// in the stdlib, getting LSP stuff for the stdlib, etc.). pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId { let path_to_std_lib_file = Path::new(STD_CRATE_NAME).join("lib.nr"); - let std_file_id = context - .file_manager - .name_to_id(path_to_std_lib_file) - .expect("stdlib file id is expected to be present"); - let std_crate_id = context.crate_graph.add_stdlib(std_file_id); + let std_file_id = context.file_manager.name_to_id(path_to_std_lib_file); + let std_crate_id = std_file_id.map(|std_file_id| context.crate_graph.add_stdlib(std_file_id)); let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}")); + let Some(std_crate_id) = std_crate_id else { + return context.crate_graph.add_crate_root_and_stdlib(root_file_id); + }; + let root_crate_id = context.crate_graph.add_crate_root(root_file_id); add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap()); diff --git a/compiler/noirc_frontend/src/graph/mod.rs b/compiler/noirc_frontend/src/graph/mod.rs index 452aef74b36..094594a50ac 100644 --- a/compiler/noirc_frontend/src/graph/mod.rs +++ b/compiler/noirc_frontend/src/graph/mod.rs @@ -16,6 +16,10 @@ pub enum CrateId { Root(usize), Crate(usize), Stdlib(usize), + /// The special case of running the compiler against the stdlib. + /// In that case there's only one crate, and it's both the root + /// crate and the stdlib crate. + RootAndStdlib(usize), Dummy, } @@ -25,11 +29,17 @@ impl CrateId { } pub fn is_stdlib(&self) -> bool { - matches!(self, CrateId::Stdlib(_)) + match self { + CrateId::Stdlib(_) | CrateId::RootAndStdlib(_) => true, + CrateId::Root(_) | CrateId::Crate(_) | CrateId::Dummy => false, + } } pub fn is_root(&self) -> bool { - matches!(self, CrateId::Root(_)) + match self { + CrateId::Root(_) | CrateId::RootAndStdlib(_) => true, + CrateId::Stdlib(_) | CrateId::Crate(_) | CrateId::Dummy => false, + } } } @@ -178,7 +188,7 @@ impl CrateGraph { Some((CrateId::Root(_), _)) => { panic!("ICE: Tried to re-add the root crate as a regular crate") } - Some((CrateId::Stdlib(_), _)) => { + Some((CrateId::Stdlib(_), _)) | Some((CrateId::RootAndStdlib(_), _)) => { panic!("ICE: Tried to re-add the stdlib crate as a regular crate") } Some((CrateId::Dummy, _)) => { @@ -212,6 +222,28 @@ impl CrateGraph { crate_id } + pub fn add_crate_root_and_stdlib(&mut self, file_id: FileId) -> CrateId { + for (crate_id, crate_data) in self.arena.iter() { + if crate_id.is_root() { + panic!("ICE: Cannot add two crate roots to a graph - use `add_crate` instead"); + } + + if crate_id.is_stdlib() { + panic!("ICE: Cannot add two stdlib crates to a graph - use `add_crate` instead"); + } + + if crate_data.root_file_id == file_id { + panic!("ICE: This FileId was already added to the CrateGraph") + } + } + + let data = CrateData { root_file_id: file_id, dependencies: Vec::new() }; + let crate_id = CrateId::RootAndStdlib(self.arena.len()); + let prev = self.arena.insert(crate_id, data); + assert!(prev.is_none()); + crate_id + } + pub fn iter_keys(&self) -> impl Iterator + '_ { self.arena.keys().copied() } diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index d78da15a8ff..cce7f324a2e 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -122,7 +122,7 @@ pub(crate) fn module_id_path( 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 { + if target_module_id.krate.is_root() { segments.push("crate"); } } diff --git a/tooling/lsp/src/notifications/mod.rs b/tooling/lsp/src/notifications/mod.rs index 87e7bea8c3b..96e339ee212 100644 --- a/tooling/lsp/src/notifications/mod.rs +++ b/tooling/lsp/src/notifications/mod.rs @@ -7,7 +7,7 @@ use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use fm::{FileManager, FileMap}; use fxhash::FxHashMap as HashMap; use lsp_types::{DiagnosticTag, Url}; -use noirc_driver::{check_crate, file_manager_with_stdlib}; +use noirc_driver::check_crate; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use crate::types::{ @@ -120,7 +120,8 @@ pub(crate) fn process_workspace_for_noir_document( ResponseError::new(ErrorCode::REQUEST_FAILED, lsp_error.to_string()) })?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); + insert_all_files_for_workspace_into_file_manager( state, &workspace, diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index ae1e57f5ecc..8b07d6f4f0a 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -5,7 +5,6 @@ use fm::FileMap; use lsp_types::{Hover, HoverContents, HoverParams, MarkupContent, MarkupKind}; use noirc_frontend::{ ast::Visibility, - graph::CrateId, hir::def_map::ModuleId, hir_def::{stmt::HirPattern, traits::Trait}, macros_api::{NodeInterner, StructId}, @@ -353,7 +352,7 @@ fn format_parent_module_from_module_id( // We don't record module attriubtes for the root module, // so we handle that case separately - if let CrateId::Root(_) = module.krate { + if module.krate.is_root() { segments.push(&args.crate_name); }; diff --git a/tooling/lsp/src/requests/mod.rs b/tooling/lsp/src/requests/mod.rs index af58396550d..fea758e0e52 100644 --- a/tooling/lsp/src/requests/mod.rs +++ b/tooling/lsp/src/requests/mod.rs @@ -15,7 +15,7 @@ use lsp_types::{ WorkDoneProgressOptions, }; use nargo_fmt::Config; -use noirc_driver::file_manager_with_stdlib; + use noirc_frontend::graph::CrateId; use noirc_frontend::hir::def_map::CrateDefMap; use noirc_frontend::{graph::Dependency, macros_api::NodeInterner}; @@ -432,7 +432,7 @@ where ResponseError::new(ErrorCode::REQUEST_FAILED, "Could not find package for file") })?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( state, &workspace, diff --git a/tooling/lsp/src/requests/profile_run.rs b/tooling/lsp/src/requests/profile_run.rs index d3b7743557a..a7362300adc 100644 --- a/tooling/lsp/src/requests/profile_run.rs +++ b/tooling/lsp/src/requests/profile_run.rs @@ -9,9 +9,7 @@ use async_lsp::{ErrorCode, ResponseError}; use nargo::ops::report_errors; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_artifacts::debug::DebugArtifact; -use noirc_driver::{ - file_manager_with_stdlib, CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING, -}; +use noirc_driver::{CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING}; use noirc_errors::{debug_info::OpCodesCount, Location}; use crate::{ @@ -53,7 +51,7 @@ fn on_profile_run_request_inner( ResponseError::new(ErrorCode::REQUEST_FAILED, err) })?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( state, &workspace, diff --git a/tooling/lsp/src/requests/test_run.rs b/tooling/lsp/src/requests/test_run.rs index 5f4f9cd98d0..081eb815c8b 100644 --- a/tooling/lsp/src/requests/test_run.rs +++ b/tooling/lsp/src/requests/test_run.rs @@ -4,9 +4,7 @@ use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, ResponseError}; use nargo::ops::{run_test, TestStatus}; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{ - check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, -}; +use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; use noirc_frontend::hir::FunctionNameMatch; use crate::{ @@ -48,7 +46,7 @@ fn on_test_run_request_inner( ResponseError::new(ErrorCode::REQUEST_FAILED, err) })?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( state, &workspace, diff --git a/tooling/lsp/src/requests/tests.rs b/tooling/lsp/src/requests/tests.rs index 7203aca7f09..81910bebedb 100644 --- a/tooling/lsp/src/requests/tests.rs +++ b/tooling/lsp/src/requests/tests.rs @@ -4,7 +4,7 @@ use crate::insert_all_files_for_workspace_into_file_manager; use async_lsp::{ErrorCode, LanguageClient, ResponseError}; use lsp_types::{LogMessageParams, MessageType}; use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{check_crate, file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING}; use crate::{ get_package_tests_in_crate, parse_diff, @@ -50,7 +50,7 @@ fn on_tests_request_inner( ResponseError::new(ErrorCode::REQUEST_FAILED, err) })?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager( state, &workspace, diff --git a/tooling/nargo/src/workspace.rs b/tooling/nargo/src/workspace.rs index 0795ffd9304..810a9edb7e1 100644 --- a/tooling/nargo/src/workspace.rs +++ b/tooling/nargo/src/workspace.rs @@ -9,6 +9,9 @@ use std::{ slice, }; +use fm::FileManager; +use noirc_driver::file_manager_with_stdlib; + use crate::{ constants::{CONTRACT_DIR, EXPORT_DIR, PROOFS_DIR, TARGET_DIR}, package::Package, @@ -46,6 +49,21 @@ impl Workspace { pub fn export_directory_path(&self) -> PathBuf { self.root_dir.join(EXPORT_DIR) } + + /// Returns a new `FileManager` for the root directory of this workspace. + /// If the root directory is not the standard library, the standard library + /// is added to the returned `FileManager`. + pub fn new_file_manager(&self) -> FileManager { + if self.is_stdlib() { + FileManager::new(&self.root_dir) + } else { + file_manager_with_stdlib(&self.root_dir) + } + } + + fn is_stdlib(&self) -> bool { + self.members.len() == 1 && self.members[0].name.to_string() == "std" + } } pub enum IntoIter<'a, T> { diff --git a/tooling/nargo_cli/src/cli/check_cmd.rs b/tooling/nargo_cli/src/cli/check_cmd.rs index 5239070b4d2..65a09dcdd11 100644 --- a/tooling/nargo_cli/src/cli/check_cmd.rs +++ b/tooling/nargo_cli/src/cli/check_cmd.rs @@ -10,8 +10,7 @@ use nargo::{ 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_abi, file_manager_with_stdlib, CompileOptions, - NOIR_ARTIFACT_VERSION_STRING, + check_crate, compute_function_abi, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::{ graph::{CrateId, CrateName}, @@ -52,7 +51,7 @@ pub(crate) fn run(args: CheckCommand, config: NargoConfig) -> Result<(), CliErro Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), )?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let parsed_files = parse_all(&workspace_file_manager); diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 85faf574a0a..ae96f6436e2 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -9,8 +9,8 @@ use nargo::package::Package; use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; +use noirc_driver::DEFAULT_EXPRESSION_WIDTH; use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; -use noirc_driver::{file_manager_with_stdlib, DEFAULT_EXPRESSION_WIDTH}; use noirc_driver::{CompilationResult, CompileOptions, CompiledContract}; use noirc_frontend::graph::CrateName; @@ -114,7 +114,7 @@ pub(super) fn compile_workspace_full( workspace: &Workspace, compile_options: &CompileOptions, ) -> Result<(), CliError> { - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager(workspace, &mut workspace_file_manager); let parsed_files = parse_all(&workspace_file_manager); diff --git a/tooling/nargo_cli/src/cli/export_cmd.rs b/tooling/nargo_cli/src/cli/export_cmd.rs index 19add7f30dc..c3752db7fbd 100644 --- a/tooling/nargo_cli/src/cli/export_cmd.rs +++ b/tooling/nargo_cli/src/cli/export_cmd.rs @@ -12,8 +12,7 @@ use nargo::workspace::Workspace; use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_driver::{ - compile_no_check, file_manager_with_stdlib, CompileOptions, CompiledProgram, - NOIR_ARTIFACT_VERSION_STRING, + compile_no_check, CompileOptions, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING, }; use noirc_frontend::graph::CrateName; @@ -54,7 +53,7 @@ pub(crate) fn run(args: ExportCommand, config: NargoConfig) -> Result<(), CliErr Some(NOIR_ARTIFACT_VERSION_STRING.to_owned()), )?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let parsed_files = parse_all(&workspace_file_manager); diff --git a/tooling/nargo_cli/src/cli/fmt_cmd.rs b/tooling/nargo_cli/src/cli/fmt_cmd.rs index 8f66a0a328f..66496db517c 100644 --- a/tooling/nargo_cli/src/cli/fmt_cmd.rs +++ b/tooling/nargo_cli/src/cli/fmt_cmd.rs @@ -3,7 +3,7 @@ use std::{fs::DirEntry, path::Path}; use clap::Args; use nargo::{insert_all_files_for_workspace_into_file_manager, ops::report_errors}; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING}; +use noirc_driver::NOIR_ARTIFACT_VERSION_STRING; use noirc_errors::CustomDiagnostic; use noirc_frontend::{hir::def_map::parse_file, parser::ParserError}; @@ -29,7 +29,7 @@ pub(crate) fn run(args: FormatCommand, config: NargoConfig) -> Result<(), CliErr Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), )?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let config = nargo_fmt::Config::read(&config.program_dir) diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 2f9390d72e0..8b3c0e29c7d 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -9,9 +9,7 @@ use nargo::{ prepare_package, }; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; -use noirc_driver::{ - check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING, -}; +use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING}; use noirc_frontend::{ graph::CrateName, hir::{FunctionNameMatch, ParsedFiles}, @@ -65,7 +63,7 @@ pub(crate) fn run(args: TestCommand, config: NargoConfig) -> Result<(), CliError Some(NOIR_ARTIFACT_VERSION_STRING.to_string()), )?; - let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir); + let mut workspace_file_manager = workspace.new_file_manager(); insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager); let parsed_files = parse_all(&workspace_file_manager); From b23a728316dc93935eed61f2a6f35c79a170ec92 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 9 Sep 2024 10:59:11 -0300 Subject: [PATCH 2/3] Fix typo --- tooling/lsp/src/requests/hover.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tooling/lsp/src/requests/hover.rs b/tooling/lsp/src/requests/hover.rs index 8b07d6f4f0a..8e16035a5eb 100644 --- a/tooling/lsp/src/requests/hover.rs +++ b/tooling/lsp/src/requests/hover.rs @@ -350,7 +350,7 @@ fn format_parent_module_from_module_id( } } - // We don't record module attriubtes for the root module, + // We don't record module attributes for the root module, // so we handle that case separately if module.krate.is_root() { segments.push(&args.crate_name); From ad323a89fc0ed49bb5df7e3c29c16ac569395907 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Mon, 9 Sep 2024 11:00:07 -0300 Subject: [PATCH 3/3] Avoid early return --- compiler/noirc_driver/src/lib.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 344428f23a4..18a13517b75 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -222,15 +222,15 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId { let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}")); - let Some(std_crate_id) = std_crate_id else { - return context.crate_graph.add_crate_root_and_stdlib(root_file_id); - }; - - let root_crate_id = context.crate_graph.add_crate_root(root_file_id); + if let Some(std_crate_id) = std_crate_id { + let root_crate_id = context.crate_graph.add_crate_root(root_file_id); - add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap()); + add_dep(context, root_crate_id, std_crate_id, STD_CRATE_NAME.parse().unwrap()); - root_crate_id + root_crate_id + } else { + context.crate_graph.add_crate_root_and_stdlib(root_file_id) + } } pub fn link_to_debug_crate(context: &mut Context, root_crate_id: CrateId) {