Skip to content

Commit

Permalink
feat: Add full call stacks to runtime errors (#2310)
Browse files Browse the repository at this point in the history
  • Loading branch information
jfecher authored Aug 15, 2023
1 parent cfd4fcd commit 9004181
Show file tree
Hide file tree
Showing 28 changed files with 349 additions and 227 deletions.
2 changes: 1 addition & 1 deletion crates/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ fn on_did_save_text_document(
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 @@ -108,23 +108,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 @@ pub fn compile_main(
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 Down Expand Up @@ -226,13 +224,13 @@ fn compile_contract(
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 @@ fn compile_contract(
});
}

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

0 comments on commit 9004181

Please sign in to comment.