Skip to content
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.

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

chore: Remove the concept of CrateType #1868

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
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