From ebd55baca3451eb2e0ac71f63df45e53de9d90f9 Mon Sep 17 00:00:00 2001 From: Blaine Bublitz Date: Wed, 5 Jul 2023 14:01:33 -0700 Subject: [PATCH] feat: Remove the concept of CrateType --- crates/lsp/src/lib.rs | 6 ++-- crates/lsp/src/lib_hacky.rs | 7 ++--- crates/nargo_cli/src/cli/mod.rs | 4 +-- crates/nargo_cli/src/cli/test_cmd.rs | 7 +---- crates/nargo_cli/src/lib.rs | 7 ++--- crates/nargo_cli/src/resolver.rs | 26 +++++++--------- crates/noirc_driver/src/lib.rs | 36 ++++++--------------- crates/noirc_frontend/src/graph/mod.rs | 43 +++++++++----------------- crates/noirc_frontend/src/hir/mod.rs | 20 +++++------- crates/wasm/src/compile.rs | 9 ++---- 10 files changed, 58 insertions(+), 107 deletions(-) diff --git a/crates/lsp/src/lib.rs b/crates/lsp/src/lib.rs index cab74bd5560..851fa6ee1b0 100644 --- a/crates/lsp/src/lib.rs +++ b/crates/lsp/src/lib.rs @@ -22,7 +22,7 @@ use lsp_types::{ }; use noirc_driver::{check_crate, create_local_crate}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; -use noirc_frontend::{graph::CrateType, hir::Context}; +use noirc_frontend::hir::Context; use serde_json::Value as JsonValue; use tower::Service; @@ -167,7 +167,7 @@ fn on_code_lens_request( let mut context = Context::default(); - let crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); + let crate_id = create_local_crate(&mut context, file_path); // We ignore the warnings and errors produced by compilation for producing codelenses // because we can still get the test functions even if compilation fails @@ -254,7 +254,7 @@ fn on_did_save_text_document( let mut context = Context::default(); - let crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); + let crate_id = create_local_crate(&mut context, file_path); let mut diagnostics = Vec::new(); diff --git a/crates/lsp/src/lib_hacky.rs b/crates/lsp/src/lib_hacky.rs index 36feee27348..0ea0971185f 100644 --- a/crates/lsp/src/lib_hacky.rs +++ b/crates/lsp/src/lib_hacky.rs @@ -21,7 +21,7 @@ use lsp_types::{ use noirc_driver::{check_crate, create_local_crate, create_non_local_crate, propagate_dep}; use noirc_errors::{DiagnosticKind, FileDiagnostic}; use noirc_frontend::{ - graph::{CrateId, CrateName, CrateType}, + graph::{CrateId, CrateName}, hir::Context, }; @@ -268,7 +268,7 @@ fn create_context_at_path(actual_path: PathBuf) -> (Context, CrateId) { } let nargo_toml_path = find_nearest_parent_file(&file_path, &["Nargo.toml"]); - let current_crate_id = create_local_crate(&mut context, file_path, CrateType::Binary); + let current_crate_id = create_local_crate(&mut context, file_path); // TODO(AD): undo hacky dependency resolution if let Some(nargo_toml_path) = nargo_toml_path { @@ -279,8 +279,7 @@ fn create_context_at_path(actual_path: PathBuf) -> (Context, CrateId) { .parent() .unwrap() // TODO .join(PathBuf::from(&dependency_path).join("src").join("lib.nr")); - let library_crate = - create_non_local_crate(&mut context, path_to_lib, CrateType::Library); + let library_crate = create_non_local_crate(&mut context, path_to_lib); propagate_dep(&mut context, library_crate, &CrateName::new(crate_name).unwrap()); } } diff --git a/crates/nargo_cli/src/cli/mod.rs b/crates/nargo_cli/src/cli/mod.rs index 2edf36b1166..9afac58aabe 100644 --- a/crates/nargo_cli/src/cli/mod.rs +++ b/crates/nargo_cli/src/cli/mod.rs @@ -90,7 +90,7 @@ pub fn start_cli() -> eyre::Result<()> { mod tests { use noirc_driver::{check_crate, create_local_crate}; use noirc_errors::reporter; - use noirc_frontend::{graph::CrateType, hir::Context}; + use noirc_frontend::hir::Context; use std::path::{Path, PathBuf}; @@ -101,7 +101,7 @@ mod tests { /// This is used for tests. fn file_compiles>(root_file: P) -> bool { let mut context = Context::default(); - let crate_id = create_local_crate(&mut context, &root_file, CrateType::Binary); + let crate_id = create_local_crate(&mut context, &root_file); let result = check_crate(&mut context, crate_id, false, false); let success = result.is_ok(); diff --git a/crates/nargo_cli/src/cli/test_cmd.rs b/crates/nargo_cli/src/cli/test_cmd.rs index afb4481f5c5..4b2a321fbe2 100644 --- a/crates/nargo_cli/src/cli/test_cmd.rs +++ b/crates/nargo_cli/src/cli/test_cmd.rs @@ -48,12 +48,7 @@ fn run_tests( compile_options.experimental_ssa, )?; - let test_functions = match context.crate_graph.crate_type(crate_id) { - noirc_frontend::graph::CrateType::Workspace => { - context.get_all_test_functions_in_workspace_matching(test_name) - } - _ => context.get_all_test_functions_in_crate_matching(&crate_id, test_name), - }; + let test_functions = context.get_all_test_functions_in_crate_matching(&crate_id, test_name); println!("Running {} test functions...", test_functions.len()); let mut failing = 0; diff --git a/crates/nargo_cli/src/lib.rs b/crates/nargo_cli/src/lib.rs index 9426decf194..09a7262489e 100644 --- a/crates/nargo_cli/src/lib.rs +++ b/crates/nargo_cli/src/lib.rs @@ -7,7 +7,6 @@ //! This name was used because it sounds like `cargo` and //! Noir Package Manager abbreviated is npm, which is already taken. -use noirc_frontend::graph::CrateType; use std::{ fs::ReadDir, path::{Path, PathBuf}, @@ -49,7 +48,7 @@ fn find_package_manifest(current_path: &Path) -> Result) -> Result<(PathBuf, CrateType), InvalidPackageError> { +fn lib_or_bin(current_path: impl AsRef) -> Result { let current_path = current_path.as_ref(); // A library has a lib.nr and a binary has a main.nr // You cannot have both. @@ -60,8 +59,8 @@ fn lib_or_bin(current_path: impl AsRef) -> Result<(PathBuf, CrateType), In let bin_nr_path = find_file(&src_path, "main", "nr"); match (lib_nr_path, bin_nr_path) { (Some(_), Some(_)) => Err(InvalidPackageError::ContainsMultipleCrates), - (None, Some(path)) => Ok((path, CrateType::Binary)), - (Some(path), None) => Ok((path, CrateType::Library)), + (None, Some(path)) => Ok(path), + (Some(path), None) => Ok(path), (None, None) => Err(InvalidPackageError::ContainsZeroCrates), } } diff --git a/crates/nargo_cli/src/resolver.rs b/crates/nargo_cli/src/resolver.rs index 9598031b193..2de3aca5bb8 100644 --- a/crates/nargo_cli/src/resolver.rs +++ b/crates/nargo_cli/src/resolver.rs @@ -6,7 +6,7 @@ use std::{ use nargo::manifest::{Dependency, Manifest, PackageManifest, WorkspaceConfig}; use noirc_driver::{add_dep, create_local_crate, create_non_local_crate}; use noirc_frontend::{ - graph::{CrateId, CrateName, CrateType}, + graph::{CrateId, CrateName}, hir::Context, }; use thiserror::Error; @@ -68,7 +68,6 @@ pub(crate) enum DependencyResolutionError { #[derive(Debug, Clone)] struct CachedDep { entry_path: PathBuf, - crate_type: CrateType, manifest: PackageManifest, // Whether the dependency came from // a remote dependency @@ -95,9 +94,9 @@ pub(crate) fn resolve_root_manifest( let crate_id = match manifest { Manifest::Package(package) => { - let (entry_path, crate_type) = super::lib_or_bin(dir_path)?; + let entry_path = super::lib_or_bin(dir_path)?; - let crate_id = create_local_crate(&mut context, entry_path, crate_type); + let crate_id = create_local_crate(&mut context, entry_path); let pkg_root = manifest_path.parent().expect("Every manifest path has a parent."); resolve_package_manifest(&mut context, crate_id, package, pkg_root)?; @@ -134,15 +133,12 @@ fn resolve_package_manifest( for (dep_pkg_name, pkg_src) in manifest.dependencies.iter() { let (dir_path, dep_meta) = cache_dep(pkg_src, pkg_root)?; - let (entry_path, crate_type) = (&dep_meta.entry_path, &dep_meta.crate_type); - - if crate_type == &CrateType::Binary { + let crate_id = create_non_local_crate(context, &dep_meta.entry_path); + if context.is_binary_crate(&crate_id) { return Err(DependencyResolutionError::BinaryDependency { dep_pkg_name: dep_pkg_name.to_string(), }); } - - let crate_id = create_non_local_crate(context, entry_path, *crate_type); add_dep(context, parent_crate, crate_id, dep_pkg_name); cached_packages.insert(dir_path, (crate_id, dep_meta)); @@ -221,12 +217,12 @@ fn resolve_workspace_manifest( .remove(&local_package) .ok_or_else(|| DependencyResolutionError::PackageNotFound(crate_name(local_package)))?; - let (entry_path, _crate_type) = super::lib_or_bin(local_crate)?; - let crate_id = create_local_crate(context, entry_path, CrateType::Workspace); + let entry_path = super::lib_or_bin(local_crate)?; + let crate_id = create_local_crate(context, entry_path); for (_, package_path) in packages.drain() { - let (entry_path, crate_type) = super::lib_or_bin(package_path)?; - create_non_local_crate(context, entry_path, crate_type); + let entry_path = super::lib_or_bin(package_path)?; + create_non_local_crate(context, entry_path); } Ok(crate_id) @@ -246,12 +242,12 @@ fn cache_dep( dir_path: &Path, remote: bool, ) -> Result { - let (entry_path, crate_type) = super::lib_or_bin(dir_path)?; + let entry_path = super::lib_or_bin(dir_path)?; let manifest_path = super::find_package_manifest(dir_path)?; let manifest = super::manifest::parse(manifest_path)? .to_package() .ok_or(DependencyResolutionError::WorkspaceDependency)?; - Ok(CachedDep { entry_path, crate_type, manifest, remote }) + Ok(CachedDep { entry_path, manifest, remote }) } match dep { diff --git a/crates/noirc_driver/src/lib.rs b/crates/noirc_driver/src/lib.rs index 525c15af1e8..2aac9b013b8 100644 --- a/crates/noirc_driver/src/lib.rs +++ b/crates/noirc_driver/src/lib.rs @@ -8,7 +8,7 @@ use fm::FileId; use noirc_abi::FunctionSignature; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; use noirc_evaluator::{create_circuit, ssa_refactor::experimental_create_circuit}; -use noirc_frontend::graph::{CrateId, CrateName, CrateType}; +use noirc_frontend::graph::{CrateId, CrateName}; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; use noirc_frontend::hir::Context; use noirc_frontend::monomorphization::monomorphize; @@ -69,7 +69,7 @@ pub fn compile_file( context: &mut Context, root_file: PathBuf, ) -> Result<(CompiledProgram, Warnings), ErrorsAndWarnings> { - let crate_id = create_local_crate(context, root_file, CrateType::Binary); + let crate_id = create_local_crate(context, root_file); compile_main(context, crate_id, &CompileOptions::default()) } @@ -79,30 +79,22 @@ pub fn compile_file( /// we have multiple binaries in one workspace /// A Fix would be for the driver instance to store the local crate id. // Granted that this is the only place which relies on the local crate being first -pub fn create_local_crate>( - context: &mut Context, - root_file: P, - crate_type: CrateType, -) -> CrateId { +pub fn create_local_crate>(context: &mut Context, root_file: P) -> CrateId { let dir_path = root_file.as_ref().to_path_buf(); let root_file_id = context.file_manager.add_file(&dir_path).unwrap(); - context.crate_graph.add_crate_root(crate_type, root_file_id) + context.crate_graph.add_crate_root(root_file_id) } /// Creates a Non Local Crate. A Non Local Crate is any crate which is the not the crate that /// the compiler is compiling. -pub fn create_non_local_crate>( - context: &mut Context, - root_file: P, - crate_type: CrateType, -) -> CrateId { +pub fn create_non_local_crate>(context: &mut Context, root_file: P) -> CrateId { let dir_path = root_file.as_ref().to_path_buf(); let root_file_id = context.file_manager.add_file(&dir_path).unwrap(); // You can add any crate type to the crate graph // but you cannot depend on Binaries - context.crate_graph.add_crate_root(crate_type, root_file_id) + context.crate_graph.add_crate_root(root_file_id) } /// Adds a edge in the crate graph for two crates @@ -111,7 +103,7 @@ pub fn add_dep(context: &mut Context, this_crate: CrateId, depends_on: CrateId, .expect("crate name contains blacklisted characters, please remove"); // Cannot depend on a binary - if context.crate_graph.crate_type(depends_on) == CrateType::Binary { + if context.is_binary_crate(&depends_on) { panic!("crates cannot depend on binaries. {crate_name:?} is a binary crate") } @@ -156,23 +148,13 @@ pub fn check_crate( let path_to_std_lib_file = PathBuf::from(std_crate_name).join("lib.nr"); let root_file_id = context.file_manager.add_file(&path_to_std_lib_file).unwrap(); - // You can add any crate type to the crate graph - // but you cannot depend on Binaries - let std_crate = context.crate_graph.add_stdlib(CrateType::Library, root_file_id); + let std_crate = context.crate_graph.add_stdlib(root_file_id); propagate_dep(context, std_crate, &CrateName::new(std_crate_name).unwrap()); context.def_interner.enable_slices = enable_slices; let mut errors = vec![]; - match context.crate_graph.crate_type(crate_id) { - CrateType::Workspace => { - let keys: Vec<_> = context.crate_graph.iter_keys().collect(); // avoid borrow checker - for crate_id in keys { - CrateDefMap::collect_defs(crate_id, context, &mut errors); - } - } - _ => CrateDefMap::collect_defs(crate_id, context, &mut errors), - } + CrateDefMap::collect_defs(crate_id, context, &mut errors); if has_errors(&errors, deny_warnings) { Err(errors) diff --git a/crates/noirc_frontend/src/graph/mod.rs b/crates/noirc_frontend/src/graph/mod.rs index 7a95ff324de..3a213202c9d 100644 --- a/crates/noirc_frontend/src/graph/mod.rs +++ b/crates/noirc_frontend/src/graph/mod.rs @@ -66,17 +66,9 @@ impl CrateGraph { /// and we do not want names that differ by a hyphen pub const CHARACTER_BLACK_LIST: [char; 1] = ['-']; -#[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub enum CrateType { - Library, - Binary, - Workspace, -} - #[derive(Debug, Clone, PartialEq, Eq)] pub struct CrateData { pub root_file_id: FileId, - pub crate_type: CrateType, pub dependencies: Vec, } @@ -95,7 +87,7 @@ impl Dependency { } impl CrateGraph { - pub fn add_crate_root(&mut self, crate_type: CrateType, file_id: FileId) -> CrateId { + pub fn add_crate_root(&mut self, file_id: FileId) -> CrateId { let mut roots_with_file_id = self.arena.iter().filter(|(_, crate_data)| crate_data.root_file_id == file_id); @@ -104,14 +96,14 @@ impl CrateGraph { return *file_id.0; } - let data = CrateData { root_file_id: file_id, crate_type, dependencies: Vec::new() }; + let data = CrateData { root_file_id: file_id, dependencies: Vec::new() }; let crate_id = CrateId::Crate(self.arena.len()); let prev = self.arena.insert(crate_id, data); assert!(prev.is_none()); crate_id } - pub fn add_stdlib(&mut self, crate_type: CrateType, file_id: FileId) -> CrateId { + pub fn add_stdlib(&mut self, file_id: FileId) -> CrateId { let mut roots_with_file_id = self.arena.iter().filter(|(_, crate_data)| crate_data.root_file_id == file_id); @@ -120,17 +112,12 @@ impl CrateGraph { return *file_id.0; } - let data = CrateData { root_file_id: file_id, crate_type, dependencies: Vec::new() }; + let data = CrateData { root_file_id: file_id, dependencies: Vec::new() }; let crate_id = CrateId::Stdlib(self.arena.len()); let prev = self.arena.insert(crate_id, data); assert!(prev.is_none()); crate_id } - - pub fn crate_type(&self, crate_id: CrateId) -> CrateType { - self.arena.get(&crate_id).unwrap().crate_type - } - pub fn iter_keys(&self) -> impl Iterator + '_ { self.arena.keys().copied() } @@ -222,7 +209,7 @@ pub struct CyclicDependenciesError { mod tests { use std::path::PathBuf; - use super::{CrateGraph, CrateName, CrateType, FileId}; + use super::{CrateGraph, CrateName, FileId}; fn dummy_file_ids(n: usize) -> Vec { use fm::{FileMap, FILE_EXTENSION}; @@ -244,9 +231,9 @@ mod tests { let file_ids = dummy_file_ids(3); let mut graph = CrateGraph::default(); - let crate1 = graph.add_crate_root(CrateType::Library, file_ids[0]); - let crate2 = graph.add_crate_root(CrateType::Library, file_ids[1]); - let crate3 = graph.add_crate_root(CrateType::Library, file_ids[2]); + let crate1 = graph.add_crate_root(file_ids[0]); + let crate2 = graph.add_crate_root(file_ids[1]); + let crate3 = graph.add_crate_root(file_ids[2]); assert!(graph.add_dep(crate1, CrateName::new("crate2").unwrap(), crate2).is_ok()); assert!(graph.add_dep(crate2, CrateName::new("crate3").unwrap(), crate3).is_ok()); @@ -260,9 +247,9 @@ mod tests { let file_id_1 = file_ids[1]; let file_id_2 = file_ids[2]; let mut graph = CrateGraph::default(); - let crate1 = graph.add_crate_root(CrateType::Library, file_id_0); - let crate2 = graph.add_crate_root(CrateType::Library, file_id_1); - let crate3 = graph.add_crate_root(CrateType::Library, file_id_2); + let crate1 = graph.add_crate_root(file_id_0); + let crate2 = graph.add_crate_root(file_id_1); + let crate3 = graph.add_crate_root(file_id_2); assert!(graph.add_dep(crate1, CrateName::new("crate2").unwrap(), crate2).is_ok()); assert!(graph.add_dep(crate2, CrateName::new("crate3").unwrap(), crate3).is_ok()); } @@ -273,12 +260,12 @@ mod tests { let file_id_1 = file_ids[1]; let file_id_2 = file_ids[2]; let mut graph = CrateGraph::default(); - let _crate1 = graph.add_crate_root(CrateType::Library, file_id_0); - let _crate2 = graph.add_crate_root(CrateType::Library, file_id_1); + let _crate1 = graph.add_crate_root(file_id_0); + let _crate2 = graph.add_crate_root(file_id_1); // Adding the same file, so the crate should be the same. - let crate3 = graph.add_crate_root(CrateType::Library, file_id_2); - let crate3_2 = graph.add_crate_root(CrateType::Library, file_id_2); + let crate3 = graph.add_crate_root(file_id_2); + let crate3_2 = graph.add_crate_root(file_id_2); assert_eq!(crate3, crate3_2); } } diff --git a/crates/noirc_frontend/src/hir/mod.rs b/crates/noirc_frontend/src/hir/mod.rs index cd188f46392..693fa706ced 100644 --- a/crates/noirc_frontend/src/hir/mod.rs +++ b/crates/noirc_frontend/src/hir/mod.rs @@ -4,7 +4,7 @@ pub mod resolution; pub mod scope; pub mod type_check; -use crate::graph::{CrateGraph, CrateId, CrateType}; +use crate::graph::{CrateGraph, CrateId}; use crate::hir_def::function::FuncMeta; use crate::node_interner::{FuncId, NodeInterner}; use def_map::{Contract, CrateDefMap}; @@ -69,17 +69,13 @@ impl Context { // Find the local crate, one should always be present let local_crate = self.def_map(crate_id).unwrap(); - // Check the crate type - // We don't panic here to allow users to `evaluate` libraries which will do nothing - if matches!( - self.crate_graph[*crate_id].crate_type, - CrateType::Binary | CrateType::Workspace - ) { - // All Binaries should have a main function - local_crate.main_function() - } else { - None - } + // All Binaries should have a main function + local_crate.main_function() + } + + pub fn is_binary_crate(&self, crate_id: &CrateId) -> bool { + // Currently, anything with a `main` function is a binary crate + matches!(self.get_main_function(crate_id), Some(_)) } /// Returns a list of all functions in the current crate marked with #[test] diff --git a/crates/wasm/src/compile.rs b/crates/wasm/src/compile.rs index 5496920cf56..f891f1d3b20 100644 --- a/crates/wasm/src/compile.rs +++ b/crates/wasm/src/compile.rs @@ -5,10 +5,7 @@ use noirc_driver::{ check_crate, compile_contracts, compile_no_check, create_local_crate, create_non_local_crate, propagate_dep, CompileOptions, CompiledContract, }; -use noirc_frontend::{ - graph::{CrateName, CrateType}, - hir::Context, -}; +use noirc_frontend::{graph::CrateName, hir::Context}; use serde::{Deserialize, Serialize}; use std::path::PathBuf; use wasm_bindgen::prelude::*; @@ -62,7 +59,7 @@ impl Default for WASMCompileOptions { fn add_noir_lib(context: &mut Context, crate_name: &str) { let path_to_lib = PathBuf::from(&crate_name).join("lib.nr"); - let library_crate = create_non_local_crate(context, path_to_lib, CrateType::Library); + let library_crate = create_non_local_crate(context, path_to_lib); propagate_dep(context, library_crate, &CrateName::new(crate_name).unwrap()); } @@ -83,7 +80,7 @@ pub fn compile(args: JsValue) -> JsValue { let mut context = Context::default(); let path = PathBuf::from(&options.entry_point); - let crate_id = create_local_crate(&mut context, path, CrateType::Binary); + let crate_id = create_local_crate(&mut context, path); for dependency in options.optional_dependencies_set { add_noir_lib(&mut context, dependency.as_str());