fix: Fix nargo not showing compiler errors or warnings

crates/nargo_cli/src/cli/check_cmd.rs

@@ -4,7 +4,6 @@ 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 std::path::{Path, PathBuf};
 
 use super::NargoConfig;
@@ -34,16 +33,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);
-        }
-    }
+    driver.check_crate_and_report_errors(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() {

crates/nargo_cli/src/cli/compile_cmd.rs

@@ -49,9 +49,10 @@ 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 compiled_contracts =
+            driver.report_errors(result, 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)
@@ -118,5 +119,6 @@ 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);
+    driver.report_errors(result, compile_options.deny_warnings).map_err(Into::into)
 }

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,7 +4,6 @@ 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};
 
@@ -40,15 +39,7 @@ fn run_tests<B: Backend>(
 ) -> 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);
-        }
-    }
+    driver.check_crate_and_report_errors(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,8 @@ use acvm::Language;
 use clap::Args;
 use fm::{FileId, FileManager, FileType};
 use noirc_abi::FunctionSignature;
-use noirc_errors::{reporter, CustomDiagnostic, FileDiagnostic};
+use noirc_errors::reporter::ReportedErrors;
+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 +66,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 +88,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,18 +166,46 @@ 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)
         }
     }
 
+    /// Run the lexing, parsing, name resolution, and type checking passes and report any warnings
+    /// and errors found.
+    pub fn check_crate_and_report_errors(
+        &mut self,
+        deny_warnings: bool,
+    ) -> Result<(), ReportedErrors> {
+        let result = self.check_crate(deny_warnings).map(|warnings| ((), warnings));
+        self.report_errors(result, deny_warnings)
+    }
+
+    /// 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 fn report_errors<T>(
+        &self,
+        result: Result<(T, Warnings), ErrorsAndWarnings>,
+        deny_warnings: bool,
+    ) -> Result<T, ReportedErrors> {
+        let (t, warnings) = result.map_err(|errors| {
+            noirc_errors::reporter::report_all(self.file_manager(), &errors, deny_warnings)
+        })?;
+
+        noirc_errors::reporter::report_all(self.file_manager(), &warnings, deny_warnings);
+        Ok(t)
+    }
+
     pub fn compute_function_signature(&self) -> Option<FunctionSignature> {
         let local_crate = self.context.def_map(LOCAL_CRATE).unwrap();
 
@@ -192,45 +217,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 +279,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 +350,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,

crates/noirc_errors/src/reporter.rs

@@ -1,10 +1,7 @@
 use crate::{FileDiagnostic, Span};
 use codespan_reporting::diagnostic::{Diagnostic, Label};
 use codespan_reporting::term;
-use codespan_reporting::term::termcolor::{
-    Color, ColorChoice, ColorSpec, StandardStream, WriteColor,
-};
-use std::io::Write;
+use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
 
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct CustomDiagnostic {
@@ -20,6 +17,12 @@ pub enum DiagnosticKind {
     Warning,
 }
 
+/// A count of errors that have been already reported to stderr
+#[derive(Debug, Copy, Clone)]
+pub struct ReportedErrors {
+    pub error_count: u32,
+}
+
 impl CustomDiagnostic {
     pub fn from_message(msg: &str) -> CustomDiagnostic {
         Self {
@@ -68,7 +71,7 @@ impl CustomDiagnostic {
         self.secondaries.push(CustomLabel::new(message, span));
     }
 
-    fn is_error(&self) -> bool {
+    pub fn is_error(&self) -> bool {
         matches!(self.kind, DiagnosticKind::Error)
     }
 }
@@ -107,11 +110,13 @@ pub fn report_all(
     files: &fm::FileManager,
     diagnostics: &[FileDiagnostic],
     deny_warnings: bool,
-) -> u32 {
-    diagnostics
+) -> ReportedErrors {
+    let error_count = diagnostics
         .iter()
         .map(|error| report(files, &error.diagnostic, Some(error.file_id), deny_warnings) as u32)
-        .sum()
+        .sum();
+
+    ReportedErrors { error_count }
 }
 
 /// Report the given diagnostic, and return true if it was an error
@@ -155,14 +160,3 @@ fn convert_diagnostic(
 
     diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(cd.notes.clone())
 }
-
-pub fn finish_report(error_count: u32) {
-    if error_count != 0 {
-        let writer = StandardStream::stderr(ColorChoice::Always);
-        let mut writer = writer.lock();
-
-        writer.set_color(ColorSpec::new().set_fg(Some(Color::Red))).unwrap();
-        writeln!(&mut writer, "error: aborting due to {error_count} previous errors").unwrap();
-        writer.reset().ok();
-    }
-}