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

feat: Add full call stacks to runtime errors #2310

Merged
merged 7 commits into from
Aug 15, 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
2 changes: 1 addition & 1 deletion crates/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@
let file_id = location.file;

// Ignore diagnostics for any file that wasn't the file we saved
// TODO: In the future, we could create "related" diagnostics for these files

Check warning on line 188 in crates/lsp/src/lib.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (codelenses)
// TODO: This currently just appends the `.nr` file extension that we store as a constant,
// but that won't work if we accept other extensions
if fm.path(file_id).with_extension(FILE_EXTENSION) != file_path {
Expand Down Expand Up @@ -307,7 +307,7 @@
let fm = &context.file_manager;
let files = fm.as_simple_files();

for FileDiagnostic { file_id, diagnostic } in file_diagnostics {
for FileDiagnostic { file_id, diagnostic, call_stack: _ } in file_diagnostics {
// Ignore diagnostics for any file that wasn't the file we saved
// TODO: In the future, we could create "related" diagnostics for these files
// TODO: This currently just appends the `.nr` file extension that we store as a constant,
Expand Down
7 changes: 6 additions & 1 deletion crates/nargo/src/artifacts/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ impl DebugArtifact {

let files_with_debug_symbols: BTreeSet<FileId> = debug_symbols
.iter()
.flat_map(|function_symbols| function_symbols.locations.values().map(|loc| loc.file))
.flat_map(|function_symbols| {
function_symbols
.locations
.values()
.filter_map(|call_stack| call_stack.last().map(|location| location.file))
})
.collect();

for file_id in files_with_debug_symbols {
Expand Down
22 changes: 11 additions & 11 deletions crates/nargo_cli/src/cli/execute_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,23 +101,23 @@ fn extract_unsatisfied_constraint_from_nargo_error(nargo_err: &NargoError) -> Op
_ => None,
}
}

fn report_unsatisfied_constraint_error(
opcode_idx: Option<usize>,
debug: &DebugInfo,
context: &Context,
) {
if let Some(opcode_index) = opcode_idx {
if let Some(loc) = debug.opcode_location(opcode_index) {
noirc_errors::reporter::report(
&context.file_manager,
&CustomDiagnostic::simple_error(
"Unsatisfied constraint".to_string(),
"Constraint failed".to_string(),
loc.span,
),
Some(loc.file),
false,
);
if let Some(locations) = debug.opcode_location(opcode_index) {
// 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) = locations.last() {
let message = "Failed constraint".into();
CustomDiagnostic::simple_error(message, String::new(), location.span)
.in_file(location.file)
.with_call_stack(locations)
.report(&context.file_manager, false);
}
}
}
}
Expand Down
20 changes: 9 additions & 11 deletions crates/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,10 @@
Some(m) => m,
None => {
// TODO(#2155): This error might be a better to exist in Nargo
let err = FileDiagnostic {
file_id: FileId::default(),
diagnostic: CustomDiagnostic::from_message(
"cannot compile crate into a program as it does not contain a `main` function",
),
};
let err = CustomDiagnostic::from_message(
"cannot compile crate into a program as it does not contain a `main` function",
)
.in_file(FileId::default());
return Err(vec![err]);
}
};
Expand All @@ -164,7 +162,7 @@
let compiled_program = compile_no_check(context, options, main)?;

if options.print_acir {
println!("Compiled ACIR for main (unoptimized):");

Check warning on line 165 in crates/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (unoptimized)
println!("{}", compiled_program.circuit);
}

Expand Down Expand Up @@ -198,7 +196,7 @@
for compiled_contract in &compiled_contracts {
for contract_function in &compiled_contract.functions {
println!(
"Compiled ACIR for {}::{} (unoptimized):",

Check warning on line 199 in crates/noirc_driver/src/lib.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (unoptimized)
compiled_contract.name, contract_function.name
);
println!("{}", contract_function.bytecode);
Expand Down Expand Up @@ -226,13 +224,13 @@
options: &CompileOptions,
) -> Result<CompiledContract, Vec<FileDiagnostic>> {
let mut functions = Vec::new();
let mut errs = Vec::new();
let mut errors = Vec::new();
for function_id in &contract.functions {
let name = context.function_name(function_id).to_owned();
let function = match compile_no_check(context, options, *function_id) {
Ok(function) => function,
Err(err) => {
errs.push(err);
Err(new_error) => {
errors.push(new_error);
continue;
}
};
Expand All @@ -253,10 +251,10 @@
});
}

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

Expand Down
10 changes: 5 additions & 5 deletions crates/noirc_errors/src/debug_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ use serde::{Deserialize, Serialize};
#[derive(Default, Debug, Clone, Deserialize, Serialize)]
pub struct DebugInfo {
/// Map opcode index of an ACIR circuit into the source code location
pub locations: BTreeMap<usize, Location>,
pub locations: BTreeMap<usize, Vec<Location>>,
}

impl DebugInfo {
pub fn new(locations: BTreeMap<usize, Location>) -> Self {
pub fn new(locations: BTreeMap<usize, Vec<Location>>) -> Self {
DebugInfo { locations }
}

Expand All @@ -27,13 +27,13 @@ impl DebugInfo {
let mut new_locations = BTreeMap::new();
for (i, idx) in opcode_indices.iter().enumerate() {
if self.locations.contains_key(idx) {
new_locations.insert(i, self.locations[idx]);
new_locations.insert(i, self.locations[idx].clone());
}
}
self.locations = new_locations;
}

pub fn opcode_location(&self, idx: usize) -> Option<&Location> {
self.locations.get(&idx)
pub fn opcode_location(&self, idx: usize) -> Option<Vec<Location>> {
self.locations.get(&idx).cloned()
}
}
15 changes: 15 additions & 0 deletions crates/noirc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,21 @@ pub use reporter::{CustomDiagnostic, DiagnosticKind};
pub struct FileDiagnostic {
pub file_id: fm::FileId,
pub diagnostic: CustomDiagnostic,

/// An optional call stack to display the full runtime call stack
/// leading up to a runtime error. If this is empty it will not be displayed.
pub call_stack: Vec<Location>,
}

impl FileDiagnostic {
pub fn new(file_id: fm::FileId, diagnostic: CustomDiagnostic) -> FileDiagnostic {
FileDiagnostic { file_id, diagnostic, call_stack: Vec::new() }
}

pub fn with_call_stack(mut self, call_stack: Vec<Location>) -> Self {
self.call_stack = call_stack;
self
}
}

impl From<FileDiagnostic> for Vec<FileDiagnostic> {
Expand Down
64 changes: 56 additions & 8 deletions crates/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{FileDiagnostic, Span};
use crate::{FileDiagnostic, Location, Span};
use codespan_reporting::diagnostic::{Diagnostic, Label};
use codespan_reporting::term;
use codespan_reporting::term::termcolor::{ColorChoice, StandardStream};
Expand Down Expand Up @@ -60,7 +60,7 @@ impl CustomDiagnostic {
}

pub fn in_file(self, file_id: fm::FileId) -> FileDiagnostic {
FileDiagnostic { file_id, diagnostic: self }
FileDiagnostic::new(file_id, self)
}

pub fn add_note(&mut self, message: String) {
Expand Down Expand Up @@ -115,25 +115,31 @@ pub fn report_all(
diagnostics: &[FileDiagnostic],
deny_warnings: bool,
) -> ReportedErrors {
let error_count = diagnostics
.iter()
.map(|error| report(files, &error.diagnostic, Some(error.file_id), deny_warnings) as u32)
.sum();
let error_count =
diagnostics.iter().map(|error| error.report(files, deny_warnings) as u32).sum();

ReportedErrors { error_count }
}

impl FileDiagnostic {
pub fn report(&self, files: &fm::FileManager, deny_warnings: bool) -> bool {
report(files, &self.diagnostic, Some(self.file_id), &self.call_stack, deny_warnings)
}
}

/// Report the given diagnostic, and return true if it was an error
pub fn report(
files: &fm::FileManager,
custom_diagnostic: &CustomDiagnostic,
file: Option<fm::FileId>,
call_stack: &[Location],
deny_warnings: bool,
) -> bool {
let writer = StandardStream::stderr(ColorChoice::Always);
let config = codespan_reporting::term::Config::default();

let diagnostic = convert_diagnostic(custom_diagnostic, file, deny_warnings);
let stack_trace = stack_trace(files, call_stack);
let diagnostic = convert_diagnostic(custom_diagnostic, file, stack_trace, deny_warnings);
term::emit(&mut writer.lock(), &config, files.as_simple_files(), &diagnostic).unwrap();

deny_warnings || custom_diagnostic.is_error()
Expand All @@ -142,6 +148,7 @@ pub fn report(
fn convert_diagnostic(
cd: &CustomDiagnostic,
file: Option<fm::FileId>,
stack_trace: String,
deny_warnings: bool,
) -> Diagnostic<usize> {
let diagnostic = match (cd.kind, deny_warnings) {
Expand All @@ -162,5 +169,46 @@ fn convert_diagnostic(
vec![]
};

diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(cd.notes.clone())
let mut notes = cd.notes.clone();
notes.push(stack_trace);

diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(notes)
}

fn stack_trace(files: &fm::FileManager, call_stack: &[Location]) -> String {
if call_stack.is_empty() {
return String::new();
}

let mut result = "Call stack:\n".to_string();

for (i, call_item) in call_stack.iter().enumerate() {
let path = files.path(call_item.file);
let source = files.fetch_file(call_item.file).source();

let (line, column) = location(source, call_item.span.start());
result += &format!("{}. {}.nr:{}:{}\n", i + 1, path.display(), line, column);
}

result
}

fn location(source: &str, span_start: u32) -> (u32, u32) {
let mut line = 1;
let mut column = 0;

for (i, char) in source.chars().enumerate() {
column += 1;

if char == '\n' {
line += 1;
column = 0;
}

if span_start <= i as u32 {
break;
}
}

(line, column)
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ impl<'block> BrilligBlock<'block> {
self.create_block_label_for_current_function(*else_destination),
);
}
TerminatorInstruction::Jmp { destination, arguments, location: _ } => {
TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => {
let target = &dfg[*destination];
for (src, dest) in arguments.iter().zip(target.parameters()) {
// Destination variable might have already been created by another block that jumps to this target
Expand Down
Loading
Loading