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

fix(noirc_driver): Move error printing into nargo #1598

Merged
merged 4 commits into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 1 addition & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/nargo_cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ noir_lsp.workspace = true
noirc_driver.workspace = true
noirc_frontend.workspace = true
noirc_abi.workspace = true
noirc_errors.workspace = true
acvm.workspace = true
toml.workspace = true
serde.workspace = true
Expand Down
11 changes: 10 additions & 1 deletion crates/nargo_cli/src/cli/check_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +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 std::path::{Path, PathBuf};

use super::NargoConfig;
Expand Down Expand Up @@ -34,7 +35,15 @@ fn check_from_path<B: Backend, P: AsRef<Path>>(
) -> Result<(), CliError<B>> {
let mut driver = setup_driver(backend, program_dir.as_ref())?;

driver.check_crate(compile_options).map_err(|_| CliError::CompilationError)?;
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);
}
}

// 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() {
Expand Down
11 changes: 10 additions & 1 deletion crates/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ 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};

Expand Down Expand Up @@ -39,7 +40,15 @@ fn run_tests<B: Backend>(
) -> Result<(), CliError<B>> {
let mut driver = setup_driver(backend, program_dir)?;

driver.check_crate(compile_options).map_err(|_| CliError::CompilationError)?;
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);
}
}

let test_functions = driver.get_all_test_functions_in_crate_matching(test_name);
println!("Running {} test functions...", test_functions.len());
Expand Down
1 change: 0 additions & 1 deletion crates/noirc_driver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ edition.workspace = true

[dependencies]
clap.workspace = true
iter-extended.workspace = true
noirc_errors.workspace = true
noirc_frontend.workspace = true
noirc_evaluator.workspace = true
Expand Down
120 changes: 73 additions & 47 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@
use acvm::acir::circuit::Opcode;
use acvm::Language;
use clap::Args;
use fm::FileType;
use iter_extended::try_vecmap;
use fm::{FileId, FileManager, FileType};
use noirc_abi::FunctionSignature;
use noirc_errors::{reporter, ReportedError};
use noirc_errors::{reporter, 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};
Expand Down Expand Up @@ -71,13 +70,18 @@ impl Driver {
Driver { context: Context::default(), language: language.clone(), is_opcode_supported }
}

// TODO(#1599): Move control of the FileManager into nargo
pub fn file_manager(&self) -> &FileManager {
phated marked this conversation as resolved.
Show resolved Hide resolved
&self.context.file_manager
}

// This is here for backwards compatibility
// with the restricted version which only uses one file
pub fn compile_file(
root_file: PathBuf,
language: &Language,
is_opcode_supported: Box<dyn Fn(&Opcode) -> bool>,
) -> Result<CompiledProgram, ReportedError> {
) -> Result<CompiledProgram, Vec<FileDiagnostic>> {
let mut driver = Driver::new(language, is_opcode_supported);
driver.create_local_crate(root_file, CrateType::Binary);
driver.compile_main(&CompileOptions::default())
Expand Down Expand Up @@ -167,12 +171,14 @@ 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, options: &CompileOptions) -> Result<(), ReportedError> {
pub fn check_crate(&mut self) -> Result<(), Vec<FileDiagnostic>> {
let mut errs = vec![];
CrateDefMap::collect_defs(LOCAL_CRATE, &mut self.context, &mut errs);
let error_count =
reporter::report_all(&self.context.file_manager, &errs, options.deny_warnings);
reporter::finish_report(error_count)
if errs.is_empty() {
Ok(())
} else {
Err(errs)
}
}

pub fn compute_function_signature(&self) -> Option<FunctionSignature> {
Expand All @@ -189,13 +195,16 @@ impl Driver {
pub fn compile_main(
&mut self,
options: &CompileOptions,
) -> Result<CompiledProgram, ReportedError> {
self.check_crate(options)?;
) -> Result<CompiledProgram, Vec<FileDiagnostic>> {
self.check_crate()?;
let main = match self.main_function() {
Ok(m) => m,
Err(e) => {
println!("cannot compile a program with no main function");
return Err(e);
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());
}
};
let compiled_program = self.compile_no_check(options, main)?;
Expand All @@ -210,68 +219,89 @@ impl Driver {
pub fn compile_contracts(
&mut self,
options: &CompileOptions,
) -> Result<Vec<CompiledContract>, ReportedError> {
self.check_crate(options)?;
) -> Result<Vec<CompiledContract>, Vec<FileDiagnostic>> {
self.check_crate()?;
let contracts = self.get_all_contracts();
let compiled_contracts =
try_vecmap(contracts, |contract| self.compile_contract(contract, options))?;
if options.print_acir {
for compiled_contract in &compiled_contracts {
for contract_function in &compiled_contract.functions {
println!(
"Compiled ACIR for {}::{}:",
compiled_contract.name, contract_function.name
);
println!("{}", contract_function.bytecode);
let mut compiled_contracts = vec![];
let mut errs = vec![];
for contract in contracts {
match self.compile_contract(contract, options) {
Ok(contract) => compiled_contracts.push(contract),
Err(err) => errs.push(err),
}
}
if errs.is_empty() {
if options.print_acir {
for compiled_contract in &compiled_contracts {
for contract_function in &compiled_contract.functions {
println!(
"Compiled ACIR for {}::{}:",
compiled_contract.name, contract_function.name
);
println!("{}", contract_function.bytecode);
}
}
}
Ok(compiled_contracts)
} else {
Err(errs.concat())
}
Ok(compiled_contracts)
}

/// Compile all of the functions associated with a Noir contract.
fn compile_contract(
&self,
contract: Contract,
options: &CompileOptions,
) -> Result<CompiledContract, ReportedError> {
let functions = try_vecmap(&contract.functions, |function_id| {
) -> Result<CompiledContract, Vec<FileDiagnostic>> {
let mut functions = vec![];
let mut errs = vec![];
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
for function_id in &contract.functions {
let name = self.function_name(*function_id).to_owned();
let function = self.compile_no_check(options, *function_id)?;
let function = match self.compile_no_check(options, *function_id) {
Ok(function) => function,
Err(err) => {
errs.push(err);
continue;
}
};
let func_meta = self.context.def_interner.function_meta(function_id);
let func_type = func_meta
.contract_function_type
.expect("Expected contract function to have a contract visibility");

let function_type = ContractFunctionType::new(func_type, func_meta.is_unconstrained);

Ok(ContractFunction {
functions.push(ContractFunction {
name,
function_type,
abi: function.abi,
bytecode: function.circuit,
})
})?;
});
}

Ok(CompiledContract { name: contract.name, functions })
if errs.is_empty() {
Ok(CompiledContract { name: contract.name, functions })
} else {
Err(errs)
}
}

/// Returns the FuncId of the 'main' function.
/// - Expects check_crate to be called beforehand
/// - Panics if no main function is found
pub fn main_function(&self) -> Result<FuncId, ReportedError> {
pub fn main_function(&self) -> Option<FuncId> {
// Find the local crate, one should always be present
let local_crate = self.context.def_map(LOCAL_CRATE).unwrap();

// Check the crate type
// We don't panic here to allow users to `evaluate` libraries which will do nothing
if self.context.crate_graph[LOCAL_CRATE].crate_type != CrateType::Binary {
println!("cannot compile crate into a program as the local crate is not a binary. For libraries, please use the check command");
return Err(ReportedError);
};

// All Binaries should have a main function
local_crate.main_function().ok_or(ReportedError)
None
} else {
// All Binaries should have a main function
local_crate.main_function()
}
}

/// Compile the current crate. Assumes self.check_crate is called beforehand!
Expand All @@ -280,7 +310,7 @@ impl Driver {
&self,
options: &CompileOptions,
main_function: FuncId,
) -> Result<CompiledProgram, ReportedError> {
) -> Result<CompiledProgram, FileDiagnostic> {
let program = monomorphize(main_function, &self.context.def_interner);

let np_language = self.language.clone();
Expand Down Expand Up @@ -308,11 +338,7 @@ impl Driver {
Err(err) => {
// The FileId here will be the file id of the file with the main file
// Errors will be shown at the call site without a stacktrace
let file = err.location.map(|loc| loc.file);
let files = &self.context.file_manager;
let error = reporter::report(files, &err.into(), file, options.deny_warnings);
reporter::finish_report(error as u32)?;
Err(ReportedError)
Err(err.into())
}
}
}
Expand Down
1 change: 0 additions & 1 deletion crates/noirc_errors/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,3 @@ codespan-reporting.workspace = true
codespan.workspace = true
fm.workspace = true
chumsky.workspace = true
serde.workspace = true
13 changes: 7 additions & 6 deletions crates/noirc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ mod position;
pub mod reporter;
pub use position::{Location, Position, Span, Spanned};
pub use reporter::{CustomDiagnostic, DiagnosticKind};
use serde::{Deserialize, Serialize};

/// Returned when the Reporter finishes after reporting errors
#[derive(Copy, Clone, Serialize, Deserialize)]
pub struct ReportedError;

#[derive(Debug, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct FileDiagnostic {
pub file_id: fm::FileId,
pub diagnostic: CustomDiagnostic,
}

impl From<FileDiagnostic> for Vec<FileDiagnostic> {
fn from(value: FileDiagnostic) -> Self {
vec![value]
}
}
12 changes: 4 additions & 8 deletions crates/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::{FileDiagnostic, ReportedError, Span};
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;

#[derive(Debug, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct CustomDiagnostic {
message: String,
secondaries: Vec<CustomLabel>,
Expand Down Expand Up @@ -89,7 +89,7 @@ impl std::fmt::Display for CustomDiagnostic {
}
}

#[derive(Debug, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq)]
struct CustomLabel {
message: String,
span: Span,
Expand Down Expand Up @@ -156,17 +156,13 @@ fn convert_diagnostic(
diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(cd.notes.clone())
}

pub fn finish_report(error_count: u32) -> Result<(), ReportedError> {
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();

Err(ReportedError)
} else {
Ok(())
}
}
9 changes: 8 additions & 1 deletion crates/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use noirc_errors::{CustomDiagnostic as Diagnostic, Location};
use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic, Location};
use thiserror::Error;

#[derive(Debug)]
Expand Down Expand Up @@ -38,6 +38,13 @@ impl From<RuntimeErrorKind> for RuntimeError {
}
}

impl From<RuntimeError> for FileDiagnostic {
fn from(err: RuntimeError) -> Self {
let file_id = err.location.map(|loc| loc.file).unwrap();
FileDiagnostic { file_id, diagnostic: err.into() }
}
}

#[derive(Error, Debug)]
pub enum RuntimeErrorKind {
// Array errors
Expand Down
Loading