diff --git a/Cargo.lock b/Cargo.lock index 3b28ca0d846..8e7094bd48d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2244,6 +2244,7 @@ dependencies = [ "noirc_abi", "noirc_driver", "noirc_errors", + "noirc_evaluator", "noirc_frontend", "noirc_printable_type", "rustc_version", diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 8b4fe6abeed..d797d498572 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -8,6 +8,7 @@ use debug::filter_relevant_files; use fm::FileId; use noirc_abi::{AbiParameter, AbiType, ContractEvent}; use noirc_errors::{CustomDiagnostic, FileDiagnostic}; +use noirc_evaluator::errors::RuntimeError; use noirc_evaluator::{create_circuit, into_abi_params}; use noirc_frontend::graph::{CrateId, CrateName}; use noirc_frontend::hir::def_map::{Contract, CrateDefMap}; @@ -164,7 +165,8 @@ pub fn compile_main( } }; - let compiled_program = compile_no_check(context, options, main, cached_program, force_compile)?; + let compiled_program = compile_no_check(context, options, main, cached_program, force_compile) + .map_err(FileDiagnostic::from)?; if options.print_acir { println!("Compiled ACIR for main (unoptimized):"); @@ -261,7 +263,7 @@ fn compile_contract_inner( let function = match compile_no_check(context, options, function_id, None, true) { Ok(function) => function, Err(new_error) => { - errors.push(new_error); + errors.push(FileDiagnostic::from(new_error)); continue; } }; @@ -316,7 +318,7 @@ pub fn compile_no_check( main_function: FuncId, cached_program: Option, force_compile: bool, -) -> Result { +) -> Result { let program = monomorphize(main_function, &context.def_interner); let hash = fxhash::hash64(&program); diff --git a/compiler/noirc_evaluator/src/lib.rs b/compiler/noirc_evaluator/src/lib.rs index 680fddc44db..b5b697e3b65 100644 --- a/compiler/noirc_evaluator/src/lib.rs +++ b/compiler/noirc_evaluator/src/lib.rs @@ -3,7 +3,7 @@ #![warn(unreachable_pub)] #![warn(clippy::semicolon_if_nothing_returned)] -mod errors; +pub mod errors; // SSA code to create the SSA based IR // for functions and execute different optimizations. diff --git a/tooling/lsp/src/lib.rs b/tooling/lsp/src/lib.rs index f5b2ffcc605..840636fe458 100644 --- a/tooling/lsp/src/lib.rs +++ b/tooling/lsp/src/lib.rs @@ -267,7 +267,7 @@ fn on_test_run_request( result: "pass".to_string(), message: None, }, - TestStatus::Fail { message } => NargoTestRunResult { + TestStatus::Fail { message, .. } => NargoTestRunResult { id: params.id.clone(), result: "fail".to_string(), message: Some(message), diff --git a/tooling/nargo/Cargo.toml b/tooling/nargo/Cargo.toml index c038ab6b1e4..eda15ff2594 100644 --- a/tooling/nargo/Cargo.toml +++ b/tooling/nargo/Cargo.toml @@ -17,6 +17,7 @@ fm.workspace = true noirc_abi.workspace = true noirc_driver.workspace = true noirc_errors.workspace = true +noirc_evaluator.workspace = true noirc_frontend.workspace = true noirc_printable_type.workspace = true iter-extended.workspace = true diff --git a/tooling/nargo/src/errors.rs b/tooling/nargo/src/errors.rs index 466909db24d..ea6e7fa8108 100644 --- a/tooling/nargo/src/errors.rs +++ b/tooling/nargo/src/errors.rs @@ -1,4 +1,8 @@ -use acvm::{acir::circuit::OpcodeLocation, pwg::OpcodeResolutionError}; +use acvm::{ + acir::circuit::OpcodeLocation, + pwg::{ErrorLocation, OpcodeResolutionError}, +}; +use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic, FileDiagnostic, Location}; use noirc_printable_type::ForeignCallError; use thiserror::Error; @@ -57,3 +61,79 @@ pub enum ExecutionError { #[error(transparent)] SolvingError(#[from] OpcodeResolutionError), } + +/// Extracts the opcode locations from a nargo error. +fn extract_locations_from_error( + error: &ExecutionError, + debug: &DebugInfo, +) -> Option> { + let mut opcode_locations = match error { + ExecutionError::SolvingError(OpcodeResolutionError::BrilligFunctionFailed { + call_stack, + .. + }) + | ExecutionError::AssertionFailed(_, call_stack) => Some(call_stack.clone()), + ExecutionError::SolvingError(OpcodeResolutionError::IndexOutOfBounds { + opcode_location: error_location, + .. + }) + | ExecutionError::SolvingError(OpcodeResolutionError::UnsatisfiedConstrain { + opcode_location: error_location, + }) => match error_location { + ErrorLocation::Unresolved => { + unreachable!("Cannot resolve index for unsatisfied constraint") + } + ErrorLocation::Resolved(opcode_location) => Some(vec![*opcode_location]), + }, + _ => None, + }?; + + if let Some(OpcodeLocation::Brillig { acir_index, .. }) = opcode_locations.get(0) { + opcode_locations.insert(0, OpcodeLocation::Acir(*acir_index)); + } + + Some( + opcode_locations + .iter() + .flat_map(|opcode_location| debug.opcode_location(opcode_location).unwrap_or_default()) + .collect(), + ) +} + +/// Tries to generate a runtime diagnostic from a nargo error. It will successfully do so if it's a runtime error with a call stack. +pub fn try_to_diagnose_runtime_error( + nargo_err: &NargoError, + debug: &DebugInfo, +) -> Option { + let execution_error = match nargo_err { + NargoError::ExecutionError(execution_error) => execution_error, + _ => return None, + }; + + let source_locations = extract_locations_from_error(execution_error, debug)?; + + // The location of the error itself will be the location at the top + // of the call stack (the last item in the Vec). + let location = source_locations.last()?; + + let message = match nargo_err { + NargoError::ExecutionError(ExecutionError::AssertionFailed(message, _)) => { + format!("Assertion failed: '{message}'") + } + NargoError::ExecutionError(ExecutionError::SolvingError( + OpcodeResolutionError::IndexOutOfBounds { index, array_size, .. }, + )) => { + format!("Index out of bounds, array has size {array_size:?}, but index was {index:?}") + } + NargoError::ExecutionError(ExecutionError::SolvingError( + OpcodeResolutionError::UnsatisfiedConstrain { .. }, + )) => "Failed constraint".into(), + _ => nargo_err.to_string(), + }; + + Some( + CustomDiagnostic::simple_error(message, String::new(), location.span) + .in_file(location.file) + .with_call_stack(source_locations), + ) +} diff --git a/tooling/nargo/src/ops/test.rs b/tooling/nargo/src/ops/test.rs index 4b113b0e19d..9219807b435 100644 --- a/tooling/nargo/src/ops/test.rs +++ b/tooling/nargo/src/ops/test.rs @@ -1,15 +1,16 @@ use acvm::{acir::native_types::WitnessMap, BlackBoxFunctionSolver}; use noirc_driver::{compile_no_check, CompileOptions}; -use noirc_errors::FileDiagnostic; +use noirc_errors::{debug_info::DebugInfo, FileDiagnostic}; +use noirc_evaluator::errors::RuntimeError; use noirc_frontend::hir::{def_map::TestFunction, Context}; -use crate::NargoError; +use crate::{errors::try_to_diagnose_runtime_error, NargoError}; use super::execute_circuit; pub enum TestStatus { Pass, - Fail { message: String }, + Fail { message: String, error_diagnostic: Option }, CompileError(FileDiagnostic), } @@ -27,9 +28,9 @@ pub fn run_test( // otherwise constraints involving these expressions will not error. let circuit_execution = execute_circuit(blackbox_solver, program.circuit, WitnessMap::new(), show_output); - test_status_program_compile_pass(test_function, circuit_execution) + test_status_program_compile_pass(test_function, program.debug, circuit_execution) } - Err(diag) => test_status_program_compile_fail(diag, test_function), + Err(err) => test_status_program_compile_fail(err, test_function), } } @@ -39,33 +40,19 @@ pub fn run_test( /// that a constraint was never satisfiable. /// An example of this is the program `assert(false)` /// In that case, we check if the test function should fail, and if so, we return `TestStatus::Pass`. -fn test_status_program_compile_fail( - diag: FileDiagnostic, - test_function: TestFunction, -) -> TestStatus { +fn test_status_program_compile_fail(err: RuntimeError, test_function: TestFunction) -> TestStatus { // The test has failed compilation, but it should never fail. Report error. if !test_function.should_fail() { - return TestStatus::CompileError(diag); - } - - // The test has failed compilation, check if it is because the program is never satisfiable. - // If it is never satisfiable, then this is the expected behavior. - let program_is_never_satisfiable = diag.diagnostic.message.contains("Failed constraint"); - if !program_is_never_satisfiable { - // The test has failed compilation, but its a compilation error. Report error - return TestStatus::CompileError(diag); + return TestStatus::CompileError(err.into()); } - // Given "Failed constraint: 'reason'" - // This method will return "reason" - fn extract_constraint_error_message(input: &str) -> Option { - input.split(": '").nth(1)?.split('\'').next().map(str::to_string) + // The test has failed compilation, extract the assertion message if present and check if it's expected. + if let RuntimeError::FailedConstraint { assert_message: Some(assert_message), .. } = &err { + let assert_message = assert_message.clone(); + check_expected_failure_message(test_function, &assert_message, Some(err.into())) + } else { + TestStatus::CompileError(err.into()) } - - check_expected_failure_message( - test_function, - &extract_constraint_error_message(&diag.diagnostic.message).unwrap(), - ) } /// The test function compiled successfully. @@ -74,6 +61,7 @@ fn test_status_program_compile_fail( /// passed/failed to determine the test status. fn test_status_program_compile_pass( test_function: TestFunction, + debug: DebugInfo, circuit_execution: Result, ) -> TestStatus { let circuit_execution_err = match circuit_execution { @@ -83,6 +71,7 @@ fn test_status_program_compile_pass( if test_function.should_fail() { return TestStatus::Fail { message: "error: Test passed when it should have failed".to_string(), + error_diagnostic: None, }; } return TestStatus::Pass; @@ -93,18 +82,26 @@ fn test_status_program_compile_pass( // If we reach here, then the circuit execution failed. // // Check if the function should have passed + let diagnostic = try_to_diagnose_runtime_error(&circuit_execution_err, &debug); let test_should_have_passed = !test_function.should_fail(); if test_should_have_passed { - return TestStatus::Fail { message: circuit_execution_err.to_string() }; + return TestStatus::Fail { + message: circuit_execution_err.to_string(), + error_diagnostic: diagnostic, + }; } - check_expected_failure_message( - test_function, - circuit_execution_err.user_defined_failure_message().unwrap_or_default(), - ) + let assertion_message = + circuit_execution_err.user_defined_failure_message().unwrap_or_default().to_string(); + + check_expected_failure_message(test_function, &assertion_message, diagnostic) } -fn check_expected_failure_message(test_function: TestFunction, got_error: &str) -> TestStatus { +fn check_expected_failure_message( + test_function: TestFunction, + failed_assertion: &str, + error_diagnostic: Option, +) -> TestStatus { // Extract the expected failure message, if there was one // // #[test(should_fail)] will not produce any message @@ -115,7 +112,7 @@ fn check_expected_failure_message(test_function: TestFunction, got_error: &str) None => return TestStatus::Pass, }; - let expected_failure_message_matches = got_error == expected_failure_message; + let expected_failure_message_matches = failed_assertion == expected_failure_message; if expected_failure_message_matches { return TestStatus::Pass; } @@ -125,7 +122,8 @@ fn check_expected_failure_message(test_function: TestFunction, got_error: &str) message: format!( "\nerror: Test failed with the wrong message. \nExpected: {} \nGot: {}", test_function.failure_reason().unwrap_or_default(), - got_error.trim_matches('\'') + failed_assertion.trim_matches('\'') ), + error_diagnostic, } } diff --git a/tooling/nargo_cli/src/cli/execute_cmd.rs b/tooling/nargo_cli/src/cli/execute_cmd.rs index ffc3ae65f4b..4fa828fb747 100644 --- a/tooling/nargo_cli/src/cli/execute_cmd.rs +++ b/tooling/nargo_cli/src/cli/execute_cmd.rs @@ -1,17 +1,14 @@ -use acvm::acir::circuit::OpcodeLocation; use acvm::acir::native_types::WitnessMap; -use acvm::pwg::{ErrorLocation, OpcodeResolutionError}; use clap::Args; use nargo::artifacts::debug::DebugArtifact; use nargo::constants::PROVER_INPUT_FILE; -use nargo::errors::{ExecutionError, NargoError}; +use nargo::errors::try_to_diagnose_runtime_error; use nargo::package::Package; use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection}; use noirc_abi::input_parser::{Format, InputValue}; use noirc_abi::InputMap; use noirc_driver::{CompileOptions, CompiledProgram}; -use noirc_errors::CustomDiagnostic; use noirc_frontend::graph::CrateName; use super::compile_cmd::compile_bin_package; @@ -96,96 +93,6 @@ fn execute_program_and_decode( Ok((return_value, solved_witness)) } -/// There are certain errors that contain an [acvm::pwg::ErrorLocation]. -/// We need to determine whether the error location has been resolving during execution. -/// If the location has been resolved we return the contained [OpcodeLocation]. -fn extract_opcode_error_from_nargo_error( - nargo_err: &NargoError, -) -> Option<(Vec, &ExecutionError)> { - let execution_error = match nargo_err { - NargoError::ExecutionError(err) => err, - _ => return None, - }; - - match execution_error { - ExecutionError::SolvingError(OpcodeResolutionError::BrilligFunctionFailed { - call_stack, - .. - }) - | ExecutionError::AssertionFailed(_, call_stack) => { - Some((call_stack.clone(), execution_error)) - } - ExecutionError::SolvingError(OpcodeResolutionError::IndexOutOfBounds { - opcode_location: error_location, - .. - }) - | ExecutionError::SolvingError(OpcodeResolutionError::UnsatisfiedConstrain { - opcode_location: error_location, - }) => match error_location { - ErrorLocation::Unresolved => { - unreachable!("Cannot resolve index for unsatisfied constraint") - } - ErrorLocation::Resolved(opcode_location) => { - Some((vec![*opcode_location], execution_error)) - } - }, - _ => None, - } -} - -/// Resolve the vector of [OpcodeLocation] that caused an execution error using the debug information -/// generated during compilation to determine the complete call stack for an error. Then report the error using -/// the resolved call stack and any other relevant error information returned from the ACVM. -fn report_error_with_opcode_locations( - opcode_err_info: Option<(Vec, &ExecutionError)>, - debug_artifact: &DebugArtifact, -) { - if let Some((opcode_locations, opcode_err)) = opcode_err_info { - let source_locations: Vec<_> = opcode_locations - .iter() - .flat_map(|opcode_location| { - // This assumes that we're executing the circuit which corresponds to the first `DebugInfo`. - // This holds for all binary crates as there is only one `DebugInfo`. - assert_eq!(debug_artifact.debug_symbols.len(), 1); - let locations = debug_artifact.debug_symbols[0].opcode_location(opcode_location); - locations.unwrap_or_default() - }) - .collect(); - // The location of the error itself will be the location at the top - // of the call stack (the last item in the Vec). - if let Some(location) = source_locations.last() { - let message = match opcode_err { - ExecutionError::AssertionFailed(message, _) => { - format!("Assertion failed: '{message}'") - } - ExecutionError::SolvingError(OpcodeResolutionError::IndexOutOfBounds { - index, - array_size, - .. - }) => { - format!( - "Index out of bounds, array has size {array_size:?}, but index was {index:?}" - ) - } - ExecutionError::SolvingError(OpcodeResolutionError::UnsatisfiedConstrain { - .. - }) => "Failed constraint".into(), - _ => { - // All other errors that do not have corresponding opcode locations - // should not be reported in this method. - // If an error with an opcode location is not handled in this match statement - // the basic message attached to the original error from the ACVM should be reported. - return; - } - }; - CustomDiagnostic::simple_error(message, String::new(), location.span) - .in_file(location.file) - .with_call_stack(source_locations) - .report(debug_artifact, false); - } - } -} - pub(crate) fn execute_program( compiled_program: &CompiledProgram, inputs_map: &InputMap, @@ -209,8 +116,9 @@ pub(crate) fn execute_program( file_map: compiled_program.file_map.clone(), }; - let opcode_err_info = extract_opcode_error_from_nargo_error(&err); - report_error_with_opcode_locations(opcode_err_info, &debug_artifact); + if let Some(diagnostic) = try_to_diagnose_runtime_error(&err, &compiled_program.debug) { + diagnostic.report(&debug_artifact, false); + } Err(crate::errors::CliError::NargoError(err)) } diff --git a/tooling/nargo_cli/src/cli/test_cmd.rs b/tooling/nargo_cli/src/cli/test_cmd.rs index 9d514c8a10e..e281411be8c 100644 --- a/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/tooling/nargo_cli/src/cli/test_cmd.rs @@ -106,7 +106,7 @@ fn run_tests( .expect("Failed to set color"); writeln!(writer, "ok").expect("Failed to write to stdout"); } - TestStatus::Fail { message } => { + TestStatus::Fail { message, error_diagnostic } => { let writer = StandardStream::stderr(ColorChoice::Always); let mut writer = writer.lock(); writer @@ -114,6 +114,13 @@ fn run_tests( .expect("Failed to set color"); writeln!(writer, "{message}").expect("Failed to write to stdout"); writer.reset().expect("Failed to reset writer"); + if let Some(diag) = error_diagnostic { + noirc_errors::reporter::report_all( + context.file_manager.as_file_map(), + &[diag], + compile_options.deny_warnings, + ); + } failing += 1; } TestStatus::CompileError(err) => {