From 91bf6a8260b39a51867d826d15b6cd665dd445af Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 11 Sep 2023 20:07:49 +0100 Subject: [PATCH 1/3] chore: use `DebugArtifact`s instead of `FileManager` to report errors --- Cargo.lock | 1 + crates/fm/src/lib.rs | 2 +- crates/nargo/Cargo.toml | 1 + crates/nargo/src/artifacts/debug.rs | 32 +++++++- .../nargo_cli/src/cli/codegen_verifier_cmd.rs | 2 +- crates/nargo_cli/src/cli/compile_cmd.rs | 76 ++++++++++--------- crates/nargo_cli/src/cli/execute_cmd.rs | 26 ++++--- crates/nargo_cli/src/cli/info_cmd.rs | 7 +- crates/nargo_cli/src/cli/prove_cmd.rs | 4 +- crates/nargo_cli/src/cli/verify_cmd.rs | 2 +- 10 files changed, 96 insertions(+), 57 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 872fdf6c79f..0bc0aa05c1c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2173,6 +2173,7 @@ version = "0.11.1" dependencies = [ "acvm", "base64", + "codespan-reporting", "fm", "iter-extended", "noirc_abi", diff --git a/crates/fm/src/lib.rs b/crates/fm/src/lib.rs index 41a9d9ed0a8..f615555601c 100644 --- a/crates/fm/src/lib.rs +++ b/crates/fm/src/lib.rs @@ -6,7 +6,7 @@ mod file_map; mod file_reader; -pub use file_map::{File, FileId, FileMap}; +pub use file_map::{File, FileId, FileMap, PathString}; use file_reader::is_stdlib_asset; use std::{ diff --git a/crates/nargo/Cargo.toml b/crates/nargo/Cargo.toml index cefc7ffb154..9783fe5d8ff 100644 --- a/crates/nargo/Cargo.toml +++ b/crates/nargo/Cargo.toml @@ -22,3 +22,4 @@ iter-extended.workspace = true serde.workspace = true thiserror.workspace = true base64.workspace = true +codespan-reporting.workspace = true diff --git a/crates/nargo/src/artifacts/debug.rs b/crates/nargo/src/artifacts/debug.rs index c60b0752e50..2a201a82c48 100644 --- a/crates/nargo/src/artifacts/debug.rs +++ b/crates/nargo/src/artifacts/debug.rs @@ -1,11 +1,13 @@ +use codespan_reporting::files::{Error, Files, SimpleFile}; use noirc_errors::debug_info::DebugInfo; use serde::{Deserialize, Serialize}; use std::{ collections::{BTreeMap, BTreeSet}, + ops::Range, path::PathBuf, }; -use fm::{FileId, FileManager}; +use fm::{FileId, FileManager, PathString}; /// For a given file, we store the source code and the path to the file /// so consumers of the debug artifact can reconstruct the original source code structure. @@ -52,3 +54,31 @@ impl DebugArtifact { Self { debug_symbols, file_map } } } + +impl<'a> Files<'a> for DebugArtifact { + type FileId = FileId; + type Name = PathString; + type Source = &'a str; + + fn name(&self, file_id: Self::FileId) -> Result { + self.file_map.get(&file_id).ok_or(Error::FileMissing).map(|file| file.path.clone().into()) + } + + fn source(&'a self, file_id: Self::FileId) -> Result { + self.file_map.get(&file_id).ok_or(Error::FileMissing).map(|file| file.source.as_ref()) + } + + fn line_index(&self, file_id: Self::FileId, byte_index: usize) -> Result { + self.file_map.get(&file_id).ok_or(Error::FileMissing).and_then(|file| { + SimpleFile::new(PathString::from(file.path.clone()), file.source.clone()) + .line_index((), byte_index) + }) + } + + fn line_range(&self, file_id: Self::FileId, line_index: usize) -> Result, Error> { + self.file_map.get(&file_id).ok_or(Error::FileMissing).and_then(|file| { + SimpleFile::new(PathString::from(file.path.clone()), file.source.clone()) + .line_range((), line_index) + }) + } +} diff --git a/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs b/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs index 36e660e02e0..4589c0f7cfb 100644 --- a/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs +++ b/crates/nargo_cli/src/cli/codegen_verifier_cmd.rs @@ -81,7 +81,7 @@ fn smart_contract_for_package( let preprocessed_program = if circuit_build_path.exists() { read_program_from_file(circuit_build_path)? } else { - let (_, program) = + let (program, _) = compile_package(package, compile_options, np_language, &is_opcode_supported)?; PreprocessedProgram { diff --git a/crates/nargo_cli/src/cli/compile_cmd.rs b/crates/nargo_cli/src/cli/compile_cmd.rs index 67cf5b49788..202ef6f4e3d 100644 --- a/crates/nargo_cli/src/cli/compile_cmd.rs +++ b/crates/nargo_cli/src/cli/compile_cmd.rs @@ -14,7 +14,6 @@ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelec use noirc_driver::{ compile_main, CompileOptions, CompiledContract, CompiledProgram, ErrorsAndWarnings, Warnings, }; -use noirc_errors::debug_info::DebugInfo; use noirc_frontend::graph::CrateName; use clap::Args; @@ -69,17 +68,22 @@ pub(crate) fn run( for package in &workspace { // If `contract` package type, we're compiling every function in a 'contract' rather than just 'main'. if package.is_contract() { - let (file_manager, contracts) = compile_contracts( + let contracts_with_debug_artifacts = compile_contracts( package, &args.compile_options, np_language, &is_opcode_supported, )?; - save_contracts(&file_manager, contracts, package, &circuit_dir, args.output_debug); + save_contracts( + contracts_with_debug_artifacts, + package, + &circuit_dir, + args.output_debug, + ); } else { - let (file_manager, program) = + let (program, debug_artifact) = compile_package(package, &args.compile_options, np_language, &is_opcode_supported)?; - save_program(&file_manager, program, package, &circuit_dir, args.output_debug); + save_program(debug_artifact, program, package, &circuit_dir, args.output_debug); } } @@ -91,7 +95,7 @@ pub(crate) fn compile_package( compile_options: &CompileOptions, np_language: Language, is_opcode_supported: &impl Fn(&Opcode) -> bool, -) -> Result<(FileManager, CompiledProgram), CliError> { +) -> Result<(CompiledProgram, DebugArtifact), CliError> { if package.is_library() { return Err(CompileError::LibraryCrate(package.name.clone()).into()); } @@ -105,7 +109,10 @@ pub(crate) fn compile_package( nargo::ops::optimize_program(program, np_language, &is_opcode_supported) .expect("Backend does not support an opcode that is in the IR"); - Ok((context.file_manager, optimized_program)) + let debug_artifact = + DebugArtifact::new(vec![optimized_program.debug.clone()], &context.file_manager); + + Ok((optimized_program, debug_artifact)) } pub(crate) fn compile_contracts( @@ -113,7 +120,7 @@ pub(crate) fn compile_contracts( compile_options: &CompileOptions, np_language: Language, is_opcode_supported: &impl Fn(&Opcode) -> bool, -) -> Result<(FileManager, Vec), CliError> { +) -> Result, CliError> { let (mut context, crate_id) = prepare_package(package); let result = noirc_driver::compile_contracts(&mut context, crate_id, compile_options); let contracts = report_errors(result, &context.file_manager, compile_options.deny_warnings)?; @@ -121,11 +128,19 @@ pub(crate) fn compile_contracts( let optimized_contracts = try_vecmap(contracts, |contract| { nargo::ops::optimize_contract(contract, np_language, &is_opcode_supported) })?; - Ok((context.file_manager, optimized_contracts)) + + let contracts_with_debug_artifacts = vecmap(optimized_contracts, |contract| { + let debug_infos = vecmap(&contract.functions, |func| func.debug.clone()); + let debug_artifact = DebugArtifact::new(debug_infos, &context.file_manager); + + (contract, debug_artifact) + }); + + Ok(contracts_with_debug_artifacts) } fn save_program( - file_manager: &FileManager, + debug_artifact: DebugArtifact, program: CompiledProgram, package: &Package, circuit_dir: &Path, @@ -140,15 +155,13 @@ fn save_program( save_program_to_file(&preprocessed_program, &package.name, circuit_dir); if output_debug { - let debug_artifact = DebugArtifact::new(vec![program.debug], file_manager); let circuit_name: String = (&package.name).into(); save_debug_artifact_to_file(&debug_artifact, &circuit_name, circuit_dir); } } fn save_contracts( - file_manager: &FileManager, - contracts: Vec, + contracts: Vec<(CompiledContract, DebugArtifact)>, package: &Package, circuit_dir: &Path, output_debug: bool, @@ -157,35 +170,29 @@ fn save_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: Vec<(PreprocessedContract, Vec)> = - vecmap(contracts, |contract| { - let preprocess_result = vecmap(contract.functions, |func| { - ( - PreprocessedContractFunction { - name: func.name, - function_type: func.function_type, - is_internal: func.is_internal, - abi: func.abi, - - bytecode: func.bytecode, - }, - func.debug, - ) - }); - - let (preprocessed_contract_functions, debug_infos): (Vec<_>, Vec<_>) = - preprocess_result.into_iter().unzip(); + let preprocessed_contracts: Vec<(PreprocessedContract, DebugArtifact)> = + vecmap(contracts, |(contract, debug_artifact)| { + let preprocessed_functions = + vecmap(contract.functions, |func| PreprocessedContractFunction { + name: func.name, + function_type: func.function_type, + is_internal: func.is_internal, + abi: func.abi, + + bytecode: func.bytecode, + }); ( PreprocessedContract { name: contract.name, backend: String::from(BACKEND_IDENTIFIER), - functions: preprocessed_contract_functions, + functions: preprocessed_functions, }, - debug_infos, + debug_artifact, ) }); - for (contract, debug_infos) in preprocessed_contracts { + + for (contract, debug_artifact) in preprocessed_contracts { save_contract_to_file( &contract, &format!("{}-{}", package.name, contract.name), @@ -193,7 +200,6 @@ fn save_contracts( ); if output_debug { - let debug_artifact = DebugArtifact::new(debug_infos, file_manager); save_debug_artifact_to_file( &debug_artifact, &format!("{}-{}", package.name, contract.name), diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index ccbc5d03776..c8fae842c91 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -3,7 +3,8 @@ use acvm::acir::{circuit::Circuit, native_types::WitnessMap}; use acvm::pwg::{ErrorLocation, OpcodeResolutionError}; use acvm::Language; use clap::Args; -use fm::FileManager; + +use nargo::artifacts::debug::DebugArtifact; use nargo::constants::PROVER_INPUT_FILE; use nargo::errors::{ExecutionError, NargoError}; use nargo::package::Package; @@ -11,7 +12,7 @@ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelec use noirc_abi::input_parser::{Format, InputValue}; use noirc_abi::{Abi, InputMap}; use noirc_driver::{CompileOptions, CompiledProgram}; -use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic}; +use noirc_errors::CustomDiagnostic; use noirc_frontend::graph::CrateName; use super::compile_cmd::compile_package; @@ -86,15 +87,15 @@ fn execute_package( np_language: Language, is_opcode_supported: &impl Fn(&Opcode) -> bool, ) -> Result<(Option, WitnessMap), CliError> { - let (context, compiled_program) = + let (compiled_program, debug_artifact) = compile_package(package, compile_options, np_language, &is_opcode_supported)?; - let CompiledProgram { abi, circuit, debug } = compiled_program; + let CompiledProgram { abi, circuit, .. } = compiled_program; // Parse the initial witness values from Prover.toml let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &abi)?; let solved_witness = - execute_program(backend, circuit, &abi, &inputs_map, Some((debug, context)))?; + execute_program(backend, circuit, &abi, &inputs_map, Some(debug_artifact))?; let public_abi = abi.public_abi(); let (_, return_value) = public_abi.decode(&solved_witness)?; @@ -143,14 +144,15 @@ fn extract_opcode_error_from_nargo_error( /// 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: &DebugInfo, - file_manager: &FileManager, + debug_artifact: &DebugArtifact, ) { if let Some((opcode_locations, opcode_err)) = opcode_err_info { let source_locations: Vec<_> = opcode_locations .iter() .flat_map(|opcode_location| { - let locations = debug.opcode_location(opcode_location); + // This assumes that the we're executing the circuit which corresponds to the first `DebugInfo`. + // This holds for all binary crates. + let locations = debug_artifact.debug_symbols[0].opcode_location(opcode_location); locations.unwrap_or_default() }) .collect(); @@ -184,7 +186,7 @@ fn report_error_with_opcode_locations( CustomDiagnostic::simple_error(message, String::new(), location.span) .in_file(location.file) .with_call_stack(source_locations) - .report(file_manager.as_file_map(), false); + .report(debug_artifact, false); } } } @@ -194,7 +196,7 @@ pub(crate) fn execute_program( circuit: Circuit, abi: &Abi, inputs_map: &InputMap, - debug_data: Option<(DebugInfo, FileManager)>, + debug_data: Option, ) -> Result { #[allow(deprecated)] let blackbox_solver = acvm::blackbox_solver::BarretenbergSolver::new(); @@ -206,9 +208,9 @@ pub(crate) fn execute_program( match solved_witness_err { Ok(solved_witness) => Ok(solved_witness), Err(err) => { - if let Some((debug, file_manager)) = debug_data { + if let Some(debug_data) = debug_data { let opcode_err_info = extract_opcode_error_from_nargo_error(&err); - report_error_with_opcode_locations(opcode_err_info, &debug, &file_manager); + report_error_with_opcode_locations(opcode_err_info, &debug_data); } Err(crate::errors::CliError::NargoError(err)) diff --git a/crates/nargo_cli/src/cli/info_cmd.rs b/crates/nargo_cli/src/cli/info_cmd.rs index 83e54fc109c..b030c21f312 100644 --- a/crates/nargo_cli/src/cli/info_cmd.rs +++ b/crates/nargo_cli/src/cli/info_cmd.rs @@ -171,7 +171,7 @@ fn count_opcodes_and_gates_in_program( np_language: Language, is_opcode_supported: &impl Fn(&Opcode) -> bool, ) -> Result { - let (_, compiled_program) = + let (compiled_program, _) = compile_package(package, compile_options, np_language, &is_opcode_supported)?; let (language, _) = backend.get_backend_info()?; @@ -190,11 +190,10 @@ fn count_opcodes_and_gates_in_contracts( np_language: Language, is_opcode_supported: &impl Fn(&Opcode) -> bool, ) -> Result, CliError> { - let (_, contracts) = - compile_contracts(package, compile_options, np_language, &is_opcode_supported)?; + let contracts = compile_contracts(package, compile_options, np_language, &is_opcode_supported)?; let (language, _) = backend.get_backend_info()?; - try_vecmap(contracts, |contract| { + try_vecmap(contracts, |(contract, _)| { let functions = try_vecmap(contract.functions, |function| -> Result<_, BackendError> { Ok(FunctionInfo { name: function.name, diff --git a/crates/nargo_cli/src/cli/prove_cmd.rs b/crates/nargo_cli/src/cli/prove_cmd.rs index 417a0872d0a..b2a2ff1856b 100644 --- a/crates/nargo_cli/src/cli/prove_cmd.rs +++ b/crates/nargo_cli/src/cli/prove_cmd.rs @@ -101,14 +101,14 @@ pub(crate) fn prove_package( (program, None) } else { - let (context, program) = + let (program, debug_artifact) = compile_package(package, compile_options, np_language, &is_opcode_supported)?; let preprocessed_program = PreprocessedProgram { backend: String::from(BACKEND_IDENTIFIER), abi: program.abi, bytecode: program.circuit, }; - (preprocessed_program, Some((program.debug, context))) + (preprocessed_program, Some(debug_artifact)) }; let PreprocessedProgram { abi, bytecode, .. } = preprocessed_program; diff --git a/crates/nargo_cli/src/cli/verify_cmd.rs b/crates/nargo_cli/src/cli/verify_cmd.rs index d9cea3ae586..4b57c70875f 100644 --- a/crates/nargo_cli/src/cli/verify_cmd.rs +++ b/crates/nargo_cli/src/cli/verify_cmd.rs @@ -85,7 +85,7 @@ fn verify_package( let preprocessed_program = if circuit_build_path.exists() { read_program_from_file(circuit_build_path)? } else { - let (_, program) = + let (program, _) = compile_package(package, compile_options, np_language, &is_opcode_supported)?; PreprocessedProgram { From a49bc88de459438922b1ebd226bcbf06ec680273 Mon Sep 17 00:00:00 2001 From: Tom French Date: Mon, 11 Sep 2023 20:27:12 +0100 Subject: [PATCH 2/3] chore: typo fix --- crates/nargo_cli/src/cli/execute_cmd.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/nargo_cli/src/cli/execute_cmd.rs b/crates/nargo_cli/src/cli/execute_cmd.rs index c8fae842c91..7abebfb5daf 100644 --- a/crates/nargo_cli/src/cli/execute_cmd.rs +++ b/crates/nargo_cli/src/cli/execute_cmd.rs @@ -150,8 +150,8 @@ fn report_error_with_opcode_locations( let source_locations: Vec<_> = opcode_locations .iter() .flat_map(|opcode_location| { - // This assumes that the we're executing the circuit which corresponds to the first `DebugInfo`. - // This holds for all binary crates. + // 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`. let locations = debug_artifact.debug_symbols[0].opcode_location(opcode_location); locations.unwrap_or_default() }) From 657404ffb2a8c5ba10ee3ed37ce6cdcd64b2261c Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 12 Sep 2023 09:50:21 +0100 Subject: [PATCH 3/3] chore: add assertion on the length of `debug_artifacts.debug_symbols` --- tooling/nargo_cli/src/cli/execute_cmd.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tooling/nargo_cli/src/cli/execute_cmd.rs b/tooling/nargo_cli/src/cli/execute_cmd.rs index 7abebfb5daf..bce14027587 100644 --- a/tooling/nargo_cli/src/cli/execute_cmd.rs +++ b/tooling/nargo_cli/src/cli/execute_cmd.rs @@ -152,6 +152,7 @@ fn report_error_with_opcode_locations( .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() })