fix: Fix nargo not showing compiler errors or warnings (#1694)

* Fix #1645

* Do not fail when only warnings are given

* Fix test

* Move methods to free functions

* Update wasm crate

crates/nargo_cli/src/cli/check_cmd.rs

@@ -4,7 +4,7 @@ use clap::Args;
 use iter_extended::btree_map;
 use noirc_abi::{AbiParameter, AbiType, MAIN_RETURN_NAME};
 use noirc_driver::CompileOptions;
-use noirc_errors::reporter;
+use noirc_errors::reporter::ReportedErrors;
 use std::path::{Path, PathBuf};
 
 use super::NargoConfig;
@@ -34,16 +34,7 @@ fn check_from_path<B: Backend, P: AsRef<Path>>(
     compile_options: &CompileOptions,
 ) -> Result<(), CliError<B>> {
     let mut driver = setup_driver(backend, program_dir.as_ref())?;
-
-    let result = driver.check_crate();
-    if let Err(errs) = result {
-        let file_manager = driver.file_manager();
-        let error_count = reporter::report_all(file_manager, &errs, compile_options.deny_warnings);
-        if error_count != 0 {
-            reporter::finish_report(error_count);
-            return Err(CliError::CompilationError);
-        }
-    }
+    check_crate_and_report_errors(&mut driver, compile_options.deny_warnings)?;
 
     // XXX: We can have a --overwrite flag to determine if you want to overwrite the Prover/Verifier.toml files
     if let Some((parameters, return_type)) = driver.compute_function_signature() {
@@ -214,3 +205,13 @@ d2 = ["", "", ""]
         }
     }
 }
+
+/// Run the lexing, parsing, name resolution, and type checking passes and report any warnings
+/// and errors found.
+pub(crate) fn check_crate_and_report_errors(
+    driver: &mut noirc_driver::Driver,
+    deny_warnings: bool,
+) -> Result<(), ReportedErrors> {
+    let result = driver.check_crate(deny_warnings).map(|warnings| ((), warnings));
+    super::compile_cmd::report_errors(result, driver, deny_warnings)
+}

crates/nargo_cli/src/cli/compile_cmd.rs

@@ -1,7 +1,8 @@
 use acvm::Backend;
 use iter_extended::try_vecmap;
 use nargo::artifacts::contract::PreprocessedContract;
-use noirc_driver::{CompileOptions, CompiledProgram, Driver};
+use noirc_driver::{CompileOptions, CompiledProgram, Driver, ErrorsAndWarnings, Warnings};
+use noirc_errors::reporter::ReportedErrors;
 use std::path::Path;
 
 use clap::Args;
@@ -49,16 +50,16 @@ pub(crate) fn run<B: Backend>(
     // If contracts is set we're compiling every function in a 'contract' rather than just 'main'.
     if args.contracts {
         let mut driver = setup_driver(backend, &config.program_dir)?;
-        let compiled_contracts = driver
-            .compile_contracts(&args.compile_options)
-            .map_err(|_| CliError::CompilationError)?;
+
+        let result = driver.compile_contracts(&args.compile_options);
+        let contracts = report_errors(result, &driver, args.compile_options.deny_warnings)?;
 
         // TODO(#1389): I wonder if it is incorrect for nargo-core to know anything about contracts.
         // As can be seen here, It seems like a leaky abstraction where ContractFunctions (essentially CompiledPrograms)
         // are compiled via nargo-core and then the PreprocessedContract is constructed here.
         // This is due to EACH function needing it's own CRS, PKey, and VKey from the backend.
         let preprocessed_contracts: Result<Vec<PreprocessedContract>, CliError<B>> =
-            try_vecmap(compiled_contracts, |contract| {
+            try_vecmap(contracts, |contract| {
                 let preprocessed_contract_functions = try_vecmap(contract.functions, |func| {
                     common_reference_string = update_common_reference_string(
                         backend,
@@ -118,5 +119,21 @@ pub(crate) fn compile_circuit<B: Backend>(
     compile_options: &CompileOptions,
 ) -> Result<CompiledProgram, CliError<B>> {
     let mut driver = setup_driver(backend, program_dir)?;
-    driver.compile_main(compile_options).map_err(|_| CliError::CompilationError)
+    let result = driver.compile_main(compile_options);
+    report_errors(result, &driver, compile_options.deny_warnings).map_err(Into::into)
+}
+
+/// Helper function for reporting any errors in a Result<(T, Warnings), ErrorsAndWarnings>
+/// structure that is commonly used as a return result in this file.
+pub(crate) fn report_errors<T>(
+    result: Result<(T, Warnings), ErrorsAndWarnings>,
+    driver: &Driver,
+    deny_warnings: bool,
+) -> Result<T, ReportedErrors> {
+    let (t, warnings) = result.map_err(|errors| {
+        noirc_errors::reporter::report_all(driver.file_manager(), &errors, deny_warnings)
+    })?;
+
+    noirc_errors::reporter::report_all(driver.file_manager(), &warnings, deny_warnings);
+    Ok(t)
 }

crates/nargo_cli/src/cli/mod.rs

@@ -167,6 +167,7 @@ pub fn prove_and_verify(program_dir: &Path, experimental_ssa: bool) -> bool {
 #[cfg(test)]
 mod tests {
     use noirc_driver::Driver;
+    use noirc_errors::reporter;
     use noirc_frontend::graph::CrateType;
 
     use std::path::{Path, PathBuf};
@@ -184,7 +185,17 @@ mod tests {
         );
         driver.create_local_crate(&root_file, CrateType::Binary);
         crate::resolver::add_std_lib(&mut driver);
-        driver.file_compiles()
+
+        let result = driver.check_crate(false);
+        let success = result.is_ok();
+
+        let errors = match result {
+            Ok(warnings) => warnings,
+            Err(errors) => errors,
+        };
+
+        reporter::report_all(driver.file_manager(), &errors, false);
+        success
     }
 
     #[test]

crates/nargo_cli/src/cli/test_cmd.rs

@@ -4,11 +4,13 @@ use acvm::{acir::native_types::WitnessMap, Backend};
 use clap::Args;
 use nargo::ops::execute_circuit;
 use noirc_driver::{CompileOptions, Driver};
-use noirc_errors::reporter;
 use noirc_frontend::node_interner::FuncId;
 use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};
 
-use crate::{cli::compile_cmd::setup_driver, errors::CliError};
+use crate::{
+    cli::{check_cmd::check_crate_and_report_errors, compile_cmd::setup_driver},
+    errors::CliError,
+};
 
 use super::NargoConfig;
 
@@ -39,16 +41,7 @@ fn run_tests<B: Backend>(
     compile_options: &CompileOptions,
 ) -> Result<(), CliError<B>> {
     let mut driver = setup_driver(backend, program_dir)?;
-
-    let result = driver.check_crate();
-    if let Err(errs) = result {
-        let file_manager = driver.file_manager();
-        let error_count = reporter::report_all(file_manager, &errs, compile_options.deny_warnings);
-        if error_count != 0 {
-            reporter::finish_report(error_count);
-            return Err(CliError::CompilationError);
-        }
-    }
+    check_crate_and_report_errors(&mut driver, compile_options.deny_warnings)?;
 
     let test_functions = driver.get_all_test_functions_in_crate_matching(test_name);
     println!("Running {} test functions...", test_functions.len());

crates/nargo_cli/src/errors.rs

@@ -5,6 +5,7 @@ use acvm::{
 use hex::FromHexError;
 use nargo::NargoError;
 use noirc_abi::errors::{AbiError, InputParserError};
+use noirc_errors::reporter::ReportedErrors;
 use std::path::PathBuf;
 use thiserror::Error;
 
@@ -43,9 +44,10 @@ pub(crate) enum CliError<B: Backend> {
     #[error(transparent)]
     ResolutionError(#[from] DependencyResolutionError),
 
-    /// Error while compiling Noir into ACIR.
-    #[error("Failed to compile circuit")]
-    CompilationError,
+    /// Errors encountered while compiling the noir program.
+    /// These errors are already written to stderr.
+    #[error("Aborting due to {} previous error{}", .0.error_count, if .0.error_count == 1 { "" } else { "s" })]
+    ReportedErrors(ReportedErrors),
 
     /// ABI encoding/decoding error
     #[error(transparent)]
@@ -74,3 +76,9 @@ pub(crate) enum CliError<B: Backend> {
     #[error(transparent)]
     CommonReferenceStringError(<B as CommonReferenceString>::Error), // Unfortunately, Rust won't let us `impl From` over an Associated Type on a generic
 }
+
+impl<B: Backend> From<ReportedErrors> for CliError<B> {
+    fn from(errors: ReportedErrors) -> Self {
+        Self::ReportedErrors(errors)
+    }
+}

crates/noirc_driver/src/lib.rs

@@ -8,7 +8,7 @@ use acvm::Language;
 use clap::Args;
 use fm::{FileId, FileManager, FileType};
 use noirc_abi::FunctionSignature;
-use noirc_errors::{reporter, CustomDiagnostic, FileDiagnostic};
+use noirc_errors::{CustomDiagnostic, FileDiagnostic};
 use noirc_evaluator::{create_circuit, ssa_refactor::experimental_create_circuit};
 use noirc_frontend::graph::{CrateId, CrateName, CrateType, LOCAL_CRATE};
 use noirc_frontend::hir::def_map::{Contract, CrateDefMap};
@@ -65,6 +65,12 @@ impl Default for CompileOptions {
     }
 }
 
+/// Helper type used to signify where only warnings are expected in file diagnostics
+pub type Warnings = Vec<FileDiagnostic>;
+
+/// Helper type used to signify where errors or warnings are expected in file diagnostics
+pub type ErrorsAndWarnings = Vec<FileDiagnostic>;
+
 impl Driver {
     pub fn new(language: &Language, is_opcode_supported: Box<dyn Fn(&Opcode) -> bool>) -> Self {
         Driver { context: Context::default(), language: language.clone(), is_opcode_supported }
@@ -81,22 +87,12 @@ impl Driver {
         root_file: PathBuf,
         language: &Language,
         is_opcode_supported: Box<dyn Fn(&Opcode) -> bool>,
-    ) -> Result<CompiledProgram, Vec<FileDiagnostic>> {
+    ) -> Result<(CompiledProgram, Warnings), ErrorsAndWarnings> {
         let mut driver = Driver::new(language, is_opcode_supported);
         driver.create_local_crate(root_file, CrateType::Binary);
         driver.compile_main(&CompileOptions::default())
     }
 
-    /// Compiles a file and returns true if compilation was successful
-    ///
-    /// This is used for tests.
-    pub fn file_compiles(&mut self) -> bool {
-        let mut errs = vec![];
-        CrateDefMap::collect_defs(LOCAL_CRATE, &mut self.context, &mut errs);
-        reporter::report_all(&self.context.file_manager, &errs, false);
-        errs.is_empty()
-    }
-
     /// Adds the File with the local crate root to the file system
     /// and adds the local crate to the graph
     /// XXX: This may pose a problem with workspaces, where you can change the local crate and where
@@ -169,15 +165,18 @@ impl Driver {
         }
     }
 
-    /// Run the lexing, parsing, name resolution, and type checking passes,
-    /// returning Err(FrontendError) and printing any errors that were found.
-    pub fn check_crate(&mut self) -> Result<(), Vec<FileDiagnostic>> {
-        let mut errs = vec![];
-        CrateDefMap::collect_defs(LOCAL_CRATE, &mut self.context, &mut errs);
-        if errs.is_empty() {
-            Ok(())
+    /// Run the lexing, parsing, name resolution, and type checking passes.
+    ///
+    /// This returns a (possibly empty) vector of any warnings found on success.
+    /// On error, this returns a non-empty vector of warnings and error messages, with at least one error.
+    pub fn check_crate(&mut self, deny_warnings: bool) -> Result<Warnings, ErrorsAndWarnings> {
+        let mut errors = vec![];
+        CrateDefMap::collect_defs(LOCAL_CRATE, &mut self.context, &mut errors);
+
+        if Self::has_errors(&errors, deny_warnings) {
+            Err(errors)
         } else {
-            Err(errs)
+            Ok(errors)
         }
     }
 
@@ -192,45 +191,57 @@ impl Driver {
     }
 
     /// Run the frontend to check the crate for errors then compile the main function if there were none
+    ///
+    /// On success this returns the compiled program alongside any warnings that were found.
+    /// On error this returns the non-empty list of warnings and errors.
     pub fn compile_main(
         &mut self,
         options: &CompileOptions,
-    ) -> Result<CompiledProgram, Vec<FileDiagnostic>> {
-        self.check_crate()?;
+    ) -> Result<(CompiledProgram, Warnings), ErrorsAndWarnings> {
+        let warnings = self.check_crate(options.deny_warnings)?;
+
         let main = match self.main_function() {
             Some(m) => m,
             None => {
                 let err = FileDiagnostic {
                     file_id: FileId::default(),
                     diagnostic: CustomDiagnostic::from_message("cannot compile crate into a program as the local crate is not a binary. For libraries, please use the check command")
                 };
-                return Err(err.into());
+                return Err(vec![err]);
             }
         };
+
         let compiled_program = self.compile_no_check(options, main)?;
+
         if options.print_acir {
             println!("Compiled ACIR for main:");
             println!("{}", compiled_program.circuit);
         }
-        Ok(compiled_program)
+
+        Ok((compiled_program, warnings))
     }
 
     /// Run the frontend to check the crate for errors then compile all contracts if there were none
     pub fn compile_contracts(
         &mut self,
         options: &CompileOptions,
-    ) -> Result<Vec<CompiledContract>, Vec<FileDiagnostic>> {
-        self.check_crate()?;
+    ) -> Result<(Vec<CompiledContract>, Warnings), ErrorsAndWarnings> {
+        let warnings = self.check_crate(options.deny_warnings)?;
+
         let contracts = self.get_all_contracts();
         let mut compiled_contracts = vec![];
-        let mut errs = vec![];
+        let mut errors = warnings;
+
         for contract in contracts {
             match self.compile_contract(contract, options) {
                 Ok(contract) => compiled_contracts.push(contract),
-                Err(err) => errs.push(err),
+                Err(mut more_errors) => errors.append(&mut more_errors),
             }
         }
-        if errs.is_empty() {
+
+        if Self::has_errors(&errors, options.deny_warnings) {
+            Err(errors)
+        } else {
             if options.print_acir {
                 for compiled_contract in &compiled_contracts {
                     for contract_function in &compiled_contract.functions {
@@ -242,9 +253,17 @@ impl Driver {
                     }
                 }
             }
-            Ok(compiled_contracts)
+            // errors here is either empty or contains only warnings
+            Ok((compiled_contracts, errors))
+        }
+    }
+
+    /// True if there are (non-warning) errors present and we should halt compilation
+    fn has_errors(errors: &[FileDiagnostic], deny_warnings: bool) -> bool {
+        if deny_warnings {
+            !errors.is_empty()
         } else {
-            Err(errs.concat())
+            errors.iter().filter(|error| error.diagnostic.is_error()).count() != 0
         }
     }
 
@@ -305,6 +324,9 @@ impl Driver {
     }
 
     /// Compile the current crate. Assumes self.check_crate is called beforehand!
+    ///
+    /// This function also assumes all errors in experimental_create_circuit and create_circuit
+    /// are not warnings.
     #[allow(deprecated)]
     pub fn compile_no_check(
         &self,