Skip to content

Commit

Permalink
feat: Remove the concept of CrateType
Browse files Browse the repository at this point in the history
  • Loading branch information
phated committed Jul 18, 2023
1 parent ea80de5 commit ebd55ba
Show file tree
Hide file tree
Showing 10 changed files with 58 additions and 107 deletions.
6 changes: 3 additions & 3 deletions crates/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();

Expand Down
7 changes: 3 additions & 4 deletions crates/lsp/src/lib_hacky.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};

Expand Down Expand Up @@ -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 {
Expand All @@ -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());
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/nargo_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -101,7 +101,7 @@ mod tests {
/// This is used for tests.
fn file_compiles<P: AsRef<Path>>(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();
Expand Down
7 changes: 1 addition & 6 deletions crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,7 @@ fn run_tests<B: Backend>(
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;
Expand Down
7 changes: 3 additions & 4 deletions crates/nargo_cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -49,7 +48,7 @@ fn find_package_manifest(current_path: &Path) -> Result<PathBuf, InvalidPackageE
.ok_or_else(|| InvalidPackageError::MissingManifestFile(current_path.to_path_buf()))
}

fn lib_or_bin(current_path: impl AsRef<Path>) -> Result<(PathBuf, CrateType), InvalidPackageError> {
fn lib_or_bin(current_path: impl AsRef<Path>) -> Result<PathBuf, InvalidPackageError> {
let current_path = current_path.as_ref();
// A library has a lib.nr and a binary has a main.nr
// You cannot have both.
Expand All @@ -60,8 +59,8 @@ fn lib_or_bin(current_path: impl AsRef<Path>) -> 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),
}
}
Expand Down
26 changes: 11 additions & 15 deletions crates/nargo_cli/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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)?;
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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)
Expand All @@ -246,12 +242,12 @@ fn cache_dep(
dir_path: &Path,
remote: bool,
) -> Result<CachedDep, DependencyResolutionError> {
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 {
Expand Down
36 changes: 9 additions & 27 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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())
}

Expand All @@ -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<P: AsRef<Path>>(
context: &mut Context,
root_file: P,
crate_type: CrateType,
) -> CrateId {
pub fn create_local_crate<P: AsRef<Path>>(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<P: AsRef<Path>>(
context: &mut Context,
root_file: P,
crate_type: CrateType,
) -> CrateId {
pub fn create_non_local_crate<P: AsRef<Path>>(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
Expand All @@ -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")
}

Expand Down Expand Up @@ -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)
Expand Down
43 changes: 15 additions & 28 deletions crates/noirc_frontend/src/graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Dependency>,
}

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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<Item = CrateId> + '_ {
self.arena.keys().copied()
}
Expand Down Expand Up @@ -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<FileId> {
use fm::{FileMap, FILE_EXTENSION};
Expand All @@ -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());
Expand All @@ -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());
}
Expand All @@ -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);
}
}
Loading

0 comments on commit ebd55ba

Please sign in to comment.