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

chore: use DebugArtifacts instead of FileManager to report errors #2641

Merged
merged 5 commits into from
Sep 12, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions Cargo.lock

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

2 changes: 1 addition & 1 deletion compiler/fm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down
1 change: 1 addition & 0 deletions tooling/nargo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ iter-extended.workspace = true
serde.workspace = true
thiserror.workspace = true
base64.workspace = true
codespan-reporting.workspace = true
32 changes: 31 additions & 1 deletion tooling/nargo/src/artifacts/debug.rs
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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::Name, Error> {
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::Source, Error> {
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<usize, 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_index((), byte_index)
})
}

fn line_range(&self, file_id: Self::FileId, line_index: usize) -> Result<Range<usize>, 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)
})
}
}
2 changes: 1 addition & 1 deletion tooling/nargo_cli/src/cli/codegen_verifier_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
76 changes: 41 additions & 35 deletions tooling/nargo_cli/src/cli/compile_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelec
use noirc_driver::{
compile_main, CompilationResult, CompileOptions, CompiledContract, CompiledProgram,
};
use noirc_errors::debug_info::DebugInfo;
use noirc_frontend::graph::CrateName;

use clap::Args;
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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());
}
Expand All @@ -105,27 +109,38 @@ 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(
package: &Package,
compile_options: &CompileOptions,
np_language: Language,
is_opcode_supported: &impl Fn(&Opcode) -> bool,
) -> Result<(FileManager, Vec<CompiledContract>), CliError> {
) -> Result<Vec<(CompiledContract, DebugArtifact)>, 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)?;

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,
Expand All @@ -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<CompiledContract>,
contracts: Vec<(CompiledContract, DebugArtifact)>,
package: &Package,
circuit_dir: &Path,
output_debug: bool,
Expand All @@ -157,43 +170,36 @@ 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<DebugInfo>)> =
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),
circuit_dir,
);

if output_debug {
let debug_artifact = DebugArtifact::new(debug_infos, file_manager);
save_debug_artifact_to_file(
&debug_artifact,
&format!("{}-{}", package.name, contract.name),
Expand Down
27 changes: 15 additions & 12 deletions tooling/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ 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;
use nargo_toml::{get_package_manifest, resolve_workspace_from_toml, PackageSelection};
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;
Expand Down Expand Up @@ -86,15 +87,15 @@ fn execute_package(
np_language: Language,
is_opcode_supported: &impl Fn(&Opcode) -> bool,
) -> Result<(Option<InputValue>, 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)?;

Expand Down Expand Up @@ -143,14 +144,16 @@ 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<OpcodeLocation>, &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 we're executing the circuit which corresponds to the first `DebugInfo`.
// This holds for all binary crates as there is only one `DebugInfo`.
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
assert_eq!(debug_artifact.debug_symbols.len(), 1);
let locations = debug_artifact.debug_symbols[0].opcode_location(opcode_location);
locations.unwrap_or_default()
})
.collect();
Expand Down Expand Up @@ -184,7 +187,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);
}
}
}
Expand All @@ -194,7 +197,7 @@ pub(crate) fn execute_program(
circuit: Circuit,
abi: &Abi,
inputs_map: &InputMap,
debug_data: Option<(DebugInfo, FileManager)>,
debug_data: Option<DebugArtifact>,
) -> Result<WitnessMap, CliError> {
#[allow(deprecated)]
let blackbox_solver = acvm::blackbox_solver::BarretenbergSolver::new();
Expand All @@ -206,9 +209,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))
Expand Down
7 changes: 3 additions & 4 deletions tooling/nargo_cli/src/cli/info_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ fn count_opcodes_and_gates_in_program(
np_language: Language,
is_opcode_supported: &impl Fn(&Opcode) -> bool,
) -> Result<ProgramInfo, CliError> {
let (_, compiled_program) =
let (compiled_program, _) =
compile_package(package, compile_options, np_language, &is_opcode_supported)?;
let (language, _) = backend.get_backend_info()?;

Expand All @@ -190,11 +190,10 @@ fn count_opcodes_and_gates_in_contracts(
np_language: Language,
is_opcode_supported: &impl Fn(&Opcode) -> bool,
) -> Result<Vec<ContractInfo>, 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,
Expand Down
4 changes: 2 additions & 2 deletions tooling/nargo_cli/src/cli/prove_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion tooling/nargo_cli/src/cli/verify_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading