Skip to content

Commit

Permalink
fix: Correct stack trace order in comptime assertion failures (#6066)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #6065

## Summary\*

Ensures the stack trace used in assertion failure messages at comptime
have the correct ordering. The old scheme was grouping together each
location by file and ordering by line number so I changed it to just
show filenames like we do in SSA when there's a failing assert.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
jfecher authored Sep 17, 2024
1 parent eda9043 commit 04f1636
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 31 deletions.
11 changes: 1 addition & 10 deletions compiler/noirc_errors/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,11 @@ 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
FileDiagnostic { file_id, diagnostic }
}
}

Expand Down
17 changes: 14 additions & 3 deletions compiler/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ pub struct CustomDiagnostic {
pub kind: DiagnosticKind,
pub deprecated: bool,
pub unnecessary: bool,

/// 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>,
}

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
Expand All @@ -39,6 +43,7 @@ impl CustomDiagnostic {
kind: DiagnosticKind::Error,
deprecated: false,
unnecessary: false,
call_stack: Default::default(),
}
}

Expand All @@ -55,6 +60,7 @@ impl CustomDiagnostic {
kind,
deprecated: false,
unnecessary: false,
call_stack: Default::default(),
}
}

Expand Down Expand Up @@ -109,13 +115,19 @@ impl CustomDiagnostic {
kind: DiagnosticKind::Bug,
deprecated: false,
unnecessary: false,
call_stack: Default::default(),
}
}

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

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

pub fn add_note(&mut self, message: String) {
self.notes.push(message);
}
Expand Down Expand Up @@ -204,7 +216,7 @@ impl FileDiagnostic {
files: &'files impl Files<'files, FileId = fm::FileId>,
deny_warnings: bool,
) -> bool {
report(files, &self.diagnostic, Some(self.file_id), &self.call_stack, deny_warnings)
report(files, &self.diagnostic, Some(self.file_id), deny_warnings)
}
}

Expand All @@ -213,15 +225,14 @@ pub fn report<'files>(
files: &'files impl Files<'files, FileId = fm::FileId>,
custom_diagnostic: &CustomDiagnostic,
file: Option<fm::FileId>,
call_stack: &[Location],
deny_warnings: bool,
) -> bool {
let color_choice =
if std::io::stderr().is_terminal() { ColorChoice::Auto } else { ColorChoice::Never };
let writer = StandardStream::stderr(color_choice);
let config = codespan_reporting::term::Config::default();

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

Expand Down
6 changes: 3 additions & 3 deletions compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl From<SsaReport> for FileDiagnostic {
let location = call_stack.last().expect("Expected RuntimeError to have a location");
let diagnostic =
Diagnostic::simple_warning(message, secondary_message, location.span);
diagnostic.in_file(file_id).with_call_stack(call_stack)
diagnostic.with_call_stack(call_stack).in_file(file_id)
}
SsaReport::Bug(bug) => {
let message = bug.to_string();
Expand All @@ -101,7 +101,7 @@ impl From<SsaReport> for FileDiagnostic {
let file_id = call_stack.last().map(|location| location.file).unwrap_or_default();
let location = call_stack.last().expect("Expected RuntimeError to have a location");
let diagnostic = Diagnostic::simple_bug(message, secondary_message, location.span);
diagnostic.in_file(file_id).with_call_stack(call_stack)
diagnostic.with_call_stack(call_stack).in_file(file_id)
}
}
}
Expand Down Expand Up @@ -178,7 +178,7 @@ impl From<RuntimeError> for FileDiagnostic {
let call_stack = vecmap(error.call_stack(), |location| *location);
let file_id = call_stack.last().map(|location| location.file).unwrap_or_default();
let diagnostic = error.into_diagnostic();
diagnostic.in_file(file_id).with_call_stack(call_stack)
diagnostic.with_call_stack(call_stack).in_file(file_id)
}
}

Expand Down
10 changes: 2 additions & 8 deletions compiler/noirc_frontend/src/hir/comptime/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,15 +391,9 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic {
Some(msg) => (msg.clone(), "Assertion failed".into()),
None => ("Assertion failed".into(), String::new()),
};
let mut diagnostic =
CustomDiagnostic::simple_error(primary, secondary, location.span);
let diagnostic = CustomDiagnostic::simple_error(primary, secondary, location.span);

// Only take at most 5 frames starting from the top of the stack to avoid producing too much output
for frame in call_stack.iter().rev().take(5) {
diagnostic.add_secondary_with_file("".to_string(), frame.span, frame.file);
}

diagnostic
diagnostic.with_call_stack(call_stack.into_iter().copied().collect())
}
InterpreterError::NoMethodFound { name, typ, location } => {
let msg = format!("No method named `{name}` found for type `{typ}`");
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/monomorphization/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl From<MonomorphizationError> for FileDiagnostic {
let location = error.location();
let call_stack = vec![location];
let diagnostic = error.into_diagnostic();
diagnostic.in_file(location.file).with_call_stack(call_stack)
diagnostic.with_call_stack(call_stack).in_file(location.file)
}
}

Expand Down
9 changes: 3 additions & 6 deletions tooling/nargo/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,11 +238,8 @@ pub fn try_to_diagnose_runtime_error(
};
// 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 location = *source_locations.last()?;
let message = extract_message_from_error(&abi.error_types, nargo_err);
Some(
CustomDiagnostic::simple_error(message, String::new(), location.span)
.in_file(location.file)
.with_call_stack(source_locations),
)
let error = CustomDiagnostic::simple_error(message, String::new(), location.span);
Some(error.with_call_stack(source_locations).in_file(location.file))
}

0 comments on commit 04f1636

Please sign in to comment.