Merge branch 'master' into remove-cached-packages-from-resolver-struct

* master:
  feat(nargo): add `InvalidPackageError` and `DependencyResolutionError` error types. (#1007)

crates/nargo/src/cli/compile_cmd.rs

@@ -7,6 +7,7 @@ use std::path::Path;
 
 use clap::Args;
 
+use crate::resolver::DependencyResolutionError;
 use crate::{constants::TARGET_DIR, errors::CliError, resolver::Resolver};
 
 use super::fs::program::save_program_to_file;
@@ -66,7 +67,7 @@ pub(crate) fn run(args: CompileCommand, config: NargoConfig) -> Result<(), CliEr
     }
 }
 
-fn setup_driver(program_dir: &Path) -> Result<Driver, CliError> {
+fn setup_driver(program_dir: &Path) -> Result<Driver, DependencyResolutionError> {
     let backend = crate::backends::ConcreteBackend;
     Resolver::resolve_root_manifest(program_dir, backend.np_language())
 }

crates/nargo/src/errors.rs

@@ -4,6 +4,8 @@ use noirc_abi::errors::{AbiError, InputParserError};
 use std::path::PathBuf;
 use thiserror::Error;
 
+use crate::resolver::DependencyResolutionError;
+
 #[derive(Debug, Error)]
 pub(crate) enum CliError {
     #[error("{0}")]
@@ -23,6 +25,9 @@ pub(crate) enum CliError {
     #[error("Failed to verify proof {}", .0.display())]
     InvalidProof(PathBuf),
 
+    #[error(transparent)]
+    ResolutionError(#[from] DependencyResolutionError),
+
     /// Error while compiling Noir into ACIR.
     #[error("Failed to compile circuit")]
     CompilationError,

crates/nargo/src/lib.rs

@@ -2,24 +2,16 @@
 #![warn(unused_crate_dependencies, unused_extern_crates)]
 #![warn(unreachable_pub)]
 
-// Necessary as we use `color_eyre` in `main.rs`.
-use color_eyre as _;
+//! Nargo is the package manager for Noir
+//! 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},
 };
 
-use crate::errors::CliError;
-// Nargo is the package manager for Noir
-// This name was used because it sounds like `cargo` and
-// Noir Package Manager abbreviated is npm, which is already taken.
-
-fn nargo_crates() -> PathBuf {
-    dirs::home_dir().unwrap().join("nargo")
-}
-
 mod backends;
 pub mod cli;
 mod constants;
@@ -28,45 +20,36 @@ mod git;
 mod manifest;
 mod resolver;
 
+use manifest::InvalidPackageError;
+
+fn nargo_crates() -> PathBuf {
+    dirs::home_dir().unwrap().join("nargo")
+}
+
 /// Searches for the Nargo.toml file
 ///
 /// XXX: In the end, this should find the root of the project and check
 /// for the Nargo.toml file there
 /// However, it should only do this after checking the current path
 /// This allows the use of workspace settings in the future.
-fn find_package_manifest(current_path: &Path) -> Result<PathBuf, CliError> {
-    match find_file(current_path, "Nargo", "toml") {
-        Some(p) => Ok(p),
-        None => Err(CliError::Generic(format!(
-            "cannot find a Nargo.toml in {}",
-            current_path.display()
-        ))),
-    }
+fn find_package_manifest(current_path: &Path) -> Result<PathBuf, InvalidPackageError> {
+    find_file(current_path, "Nargo", "toml")
+        .ok_or_else(|| InvalidPackageError::MissingManifestFile(current_path.to_path_buf()))
 }
 
-fn lib_or_bin(current_path: &Path) -> Result<(PathBuf, CrateType), CliError> {
+fn lib_or_bin(current_path: &Path) -> Result<(PathBuf, CrateType), InvalidPackageError> {
     // A library has a lib.nr and a binary has a main.nr
     // You cannot have both.
-    let src_path = match find_dir(current_path, "src") {
-        Some(path) => path,
-        None => {
-            return Err(CliError::Generic(format!(
-                "cannot find src file in path {}",
-                current_path.display()
-            )))
-        }
-    };
+    let src_path = find_dir(current_path, "src")
+        .ok_or_else(|| InvalidPackageError::NoSourceDir(current_path.to_path_buf()))?;
+
     let lib_nr_path = find_file(&src_path, "lib", "nr");
     let bin_nr_path = find_file(&src_path, "main", "nr");
     match (lib_nr_path, bin_nr_path) {
-        (Some(_), Some(_)) => Err(CliError::Generic(
-            "package cannot contain both a `lib.nr` and a `main.nr`".to_owned(),
-        )),
+        (Some(_), Some(_)) => Err(InvalidPackageError::ContainsMultipleCrates),
         (None, Some(path)) => Ok((path, CrateType::Binary)),
         (Some(path), None) => Ok((path, CrateType::Library)),
-        (None, None) => Err(CliError::Generic(
-            "package must contain either a `lib.nr`(Library) or a `main.nr`(Binary).".to_owned(),
-        )),
+        (None, None) => Err(InvalidPackageError::ContainsZeroCrates),
     }
 }
 

crates/nargo/src/manifest.rs

@@ -1,8 +1,31 @@
 use serde::Deserialize;
 use std::collections::BTreeMap;
-use std::path::Path;
+use std::path::{Path, PathBuf};
+use thiserror::Error;
 
-use crate::errors::CliError;
+/// Errors covering situations where a package is either missing or malformed.
+#[derive(Debug, Error)]
+pub(crate) enum InvalidPackageError {
+    /// Package doesn't have a manifest file
+    #[error("cannot find a Nargo.toml in {}", .0.display())]
+    MissingManifestFile(PathBuf),
+
+    /// Package manifest is unreadable.
+    #[error("Nargo.toml is badly formed, could not parse.\n\n {0}")]
+    MalformedManifestFile(toml::de::Error),
+
+    /// Package does not contain Noir source files.
+    #[error("cannot find src directory in path {}", .0.display())]
+    NoSourceDir(PathBuf),
+
+    /// Package has neither of `main.nr` and `lib.nr`.
+    #[error("package must contain either a `lib.nr`(Library) or a `main.nr`(Binary).")]
+    ContainsZeroCrates,
+
+    /// Package has both a `main.nr` (for binaries) and `lib.nr` (for libraries)
+    #[error("package cannot contain both a `lib.nr` and a `main.nr`")]
+    ContainsMultipleCrates,
+}
 
 #[derive(Debug, Deserialize, Clone)]
 pub(crate) struct PackageManifest {
@@ -53,32 +76,18 @@ pub(crate) enum Dependency {
 /// Parses a Nargo.toml file from it's path
 /// The path to the toml file must be present.
 /// Calling this function without this guarantee is an ICE.
-pub(crate) fn parse<P: AsRef<Path>>(path_to_toml: P) -> Result<PackageManifest, CliError> {
+pub(crate) fn parse<P: AsRef<Path>>(
+    path_to_toml: P,
+) -> Result<PackageManifest, InvalidPackageError> {
     let toml_as_string =
         std::fs::read_to_string(&path_to_toml).expect("ice: path given for toml file is invalid");
 
-    match parse_toml_str(&toml_as_string) {
-        Ok(cfg) => Ok(cfg),
-        Err(msg) => {
-            let path = path_to_toml.as_ref();
-            Err(CliError::Generic(format!("{}\n Location: {}", msg, path.display())))
-        }
-    }
+    parse_toml_str(&toml_as_string)
 }
 
-fn parse_toml_str(toml_as_string: &str) -> Result<PackageManifest, String> {
-    match toml::from_str::<PackageManifest>(toml_as_string) {
-        Ok(cfg) => Ok(cfg),
-        Err(err) => {
-            let mut message = "input.toml file is badly formed, could not parse\n\n".to_owned();
-            // XXX: This error is not always that helpful, but it gives the line number
-            // and the entry, in those cases
-            // which is useful for telling the user where the error occurred
-            // XXX: maybe there is a way to extract ErrorInner from toml
-            message.push_str(&err.to_string());
-            Err(message)
-        }
-    }
+fn parse_toml_str(toml_as_string: &str) -> Result<PackageManifest, InvalidPackageError> {
+    toml::from_str::<PackageManifest>(toml_as_string)
+        .map_err(InvalidPackageError::MalformedManifestFile)
 }
 
 #[test]

crates/nargo/src/resolver.rs

@@ -6,10 +6,12 @@ use std::{
 use acvm::Language;
 use noirc_driver::Driver;
 use noirc_frontend::graph::{CrateId, CrateName, CrateType};
+use thiserror::Error;
 
 use crate::{
-    errors::CliError,
+    git::clone_git_repo,
     manifest::{Dependency, PackageManifest},
+    InvalidPackageError,
 };
 
 /// Creates a unique folder name for a GitHub repo
@@ -21,6 +23,27 @@ pub(crate) fn resolve_folder_name(base: &url::Url, tag: &str) -> String {
     folder_name
 }
 
+/// Errors covering situations where a crate's dependency tree cannot be resolved.
+#[derive(Debug, Error)]
+pub(crate) enum DependencyResolutionError {
+    /// Encountered error while downloading git repository.
+    #[error("{0}")]
+    GitError(String),
+
+    /// Attempted to depend on a binary crate.
+    #[error("dependency {dep_pkg_name} is a binary package and so it cannot be depended upon.")]
+    BinaryDependency { dep_pkg_name: String },
+
+    /// Attempted to depend on remote crate which has a local dependency.
+    /// We have no guarantees that this local dependency will be available so must error.
+    #[error("remote(git) dependency has a local dependency.\ndependency located at {}", dependency_path.display())]
+    RemoteDepWithLocalDep { dependency_path: PathBuf },
+
+    /// Dependency is not a valid crate
+    #[error(transparent)]
+    MalformedDependency(#[from] InvalidPackageError),
+}
+
 #[derive(Debug, Clone)]
 struct CachedDep {
     entry_path: PathBuf,
@@ -51,7 +74,7 @@ impl<'a> Resolver<'a> {
     pub(crate) fn resolve_root_manifest(
         dir_path: &std::path::Path,
         np_language: Language,
-    ) -> Result<Driver, CliError> {
+    ) -> Result<Driver, DependencyResolutionError> {
         let mut driver = Driver::new(&np_language);
         let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;
 
@@ -77,7 +100,7 @@ impl<'a> Resolver<'a> {
         &mut self,
         parent_crate: CrateId,
         manifest: PackageManifest,
-    ) -> Result<(), CliError> {
+    ) -> Result<(), DependencyResolutionError> {
         let mut cached_packages: HashMap<PathBuf, (CrateId, CachedDep)> = HashMap::new();
 
         // First download and add these top level dependencies crates to the Driver
@@ -87,9 +110,9 @@ impl<'a> Resolver<'a> {
             let (entry_path, crate_type) = (&dep_meta.entry_path, &dep_meta.crate_type);
 
             if crate_type == &CrateType::Binary {
-                return Err(CliError::Generic(format!(
-                    "{dep_pkg_name} is a binary package and so it cannot be depended upon. src : {pkg_src:?}"
-                )));
+                return Err(DependencyResolutionError::BinaryDependency {
+                    dep_pkg_name: dep_pkg_name.to_string(),
+                });
             }
 
             let crate_id = self.driver.create_non_local_crate(entry_path, *crate_type);
@@ -99,12 +122,9 @@ impl<'a> Resolver<'a> {
         }
 
         // Resolve all transitive dependencies
-        for (dir_path, (crate_id, dep_meta)) in cached_packages.into_iter() {
+        for (dependency_path, (crate_id, dep_meta)) in cached_packages.into_iter() {
             if dep_meta.remote && manifest.has_local_path() {
-                return Err(CliError::Generic(format!(
-                    "remote(git) dependency depends on a local path. \ndependency located at {}",
-                    dir_path.display()
-                )));
+                return Err(DependencyResolutionError::RemoteDepWithLocalDep { dependency_path });
             }
             let mut new_res = Resolver::with_driver(self.driver);
             new_res.resolve_manifest(crate_id, dep_meta.manifest)?;
@@ -118,8 +138,11 @@ impl<'a> Resolver<'a> {
     ///
     /// If it's a local path, the same applies, however it will not
     /// be downloaded
-    fn cache_dep(dep: &Dependency) -> Result<(PathBuf, CachedDep), CliError> {
-        fn retrieve_meta(dir_path: &Path, remote: bool) -> Result<CachedDep, CliError> {
+    fn cache_dep(dep: &Dependency) -> Result<(PathBuf, CachedDep), DependencyResolutionError> {
+        fn retrieve_meta(
+            dir_path: &Path,
+            remote: bool,
+        ) -> Result<CachedDep, DependencyResolutionError> {
             let (entry_path, crate_type) = super::lib_or_bin(dir_path)?;
             let manifest_path = super::find_package_manifest(dir_path)?;
             let manifest = super::manifest::parse(manifest_path)?;
@@ -128,7 +151,8 @@ impl<'a> Resolver<'a> {
 
         match dep {
             Dependency::Github { git, tag } => {
-                let dir_path = Resolver::resolve_git_dep(git, tag)?;
+                let dir_path =
+                    clone_git_repo(git, tag).map_err(DependencyResolutionError::GitError)?;
                 let meta = retrieve_meta(&dir_path, true)?;
                 Ok((dir_path, meta))
             }
@@ -139,13 +163,6 @@ impl<'a> Resolver<'a> {
             }
         }
     }
-
-    fn resolve_git_dep(url: &str, tag: &str) -> Result<PathBuf, CliError> {
-        match super::git::clone_git_repo(url, tag) {
-            Ok(path) => Ok(path),
-            Err(msg) => Err(CliError::Generic(msg)),
-        }
-    }
 }
 
 // This needs to be public to support the tests in `cli/mod.rs`.