From cfd68d4c1bd1a2319698fca99d200a5d86ffa771 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 29 Aug 2024 12:25:08 -0300 Subject: [PATCH] feat: show backtrace on comptime assertion failures (#5842) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Resolves #5828 ## Summary `CustomDiagnostic` didn't have a `call_stack` property because so far this wasn't needed. I only added it for `FailingConstraint`, which is, _I think_, the only case where a backtrace might be needed. In all other cases (for example "wrong number of arguments" it might be clear why it's failing without needing a backtrace). Here's an example output: ``` error: Assertion failed ┌─ std/option.nr:33:16 │ 33 │ assert(self._is_some); │ ------------- │ ┌─ src/main.nr:8:9 │ 8 │ foo(); │ ----- │ ┌─ src/other.nr:2:5 │ 2 │ bar(); │ ----- · 7 │ let _ = expr.as_integer().unwrap(); │ -------------------------- │ Aborting due to 1 previous error ``` ## Additional Context None. ## 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. --- Cargo.lock | 1 + aztec_macros/Cargo.toml | 1 + compiler/noirc_errors/src/reporter.rs | 18 +++++++---- .../noirc_frontend/src/elaborator/comptime.rs | 1 + compiler/noirc_frontend/src/elaborator/mod.rs | 5 ++++ .../noirc_frontend/src/hir/comptime/errors.rs | 13 ++++++-- .../src/hir/comptime/interpreter.rs | 9 ++++-- .../src/hir/comptime/interpreter/builtin.rs | 30 +++++++++++++------ 8 files changed, 59 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 279d0b59ce1..cd936e4bca2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -451,6 +451,7 @@ version = "0.33.0" dependencies = [ "acvm", "convert_case 0.6.0", + "im", "iter-extended", "noirc_errors", "noirc_frontend", diff --git a/aztec_macros/Cargo.toml b/aztec_macros/Cargo.toml index c9d88e36e28..258379cd7b8 100644 --- a/aztec_macros/Cargo.toml +++ b/aztec_macros/Cargo.toml @@ -18,5 +18,6 @@ noirc_frontend.workspace = true noirc_errors.workspace = true iter-extended.workspace = true convert_case = "0.6.0" +im.workspace = true regex = "1.10" tiny-keccak = { version = "2.0.0", features = ["keccak"] } diff --git a/compiler/noirc_errors/src/reporter.rs b/compiler/noirc_errors/src/reporter.rs index 3ce0f268715..b21dc759f14 100644 --- a/compiler/noirc_errors/src/reporter.rs +++ b/compiler/noirc_errors/src/reporter.rs @@ -46,7 +46,7 @@ impl CustomDiagnostic { ) -> CustomDiagnostic { CustomDiagnostic { message: primary_message, - secondaries: vec![CustomLabel::new(secondary_message, secondary_span)], + secondaries: vec![CustomLabel::new(secondary_message, secondary_span, None)], notes: Vec::new(), kind, } @@ -98,7 +98,7 @@ impl CustomDiagnostic { ) -> CustomDiagnostic { CustomDiagnostic { message: primary_message, - secondaries: vec![CustomLabel::new(secondary_message, secondary_span)], + secondaries: vec![CustomLabel::new(secondary_message, secondary_span, None)], notes: Vec::new(), kind: DiagnosticKind::Bug, } @@ -113,7 +113,11 @@ impl CustomDiagnostic { } pub fn add_secondary(&mut self, message: String, span: Span) { - self.secondaries.push(CustomLabel::new(message, span)); + self.secondaries.push(CustomLabel::new(message, span, None)); + } + + pub fn add_secondary_with_file(&mut self, message: String, span: Span, file: fm::FileId) { + self.secondaries.push(CustomLabel::new(message, span, Some(file))); } pub fn is_error(&self) -> bool { @@ -153,11 +157,12 @@ impl std::fmt::Display for CustomDiagnostic { pub struct CustomLabel { pub message: String, pub span: Span, + pub file: Option, } impl CustomLabel { - fn new(message: String, span: Span) -> CustomLabel { - CustomLabel { message, span } + fn new(message: String, span: Span, file: Option) -> CustomLabel { + CustomLabel { message, span, file } } } @@ -234,7 +239,8 @@ fn convert_diagnostic( .map(|sl| { let start_span = sl.span.start() as usize; let end_span = sl.span.end() as usize; - Label::secondary(file_id, start_span..end_span).with_message(&sl.message) + let file = sl.file.unwrap_or(file_id); + Label::secondary(file, start_span..end_span).with_message(&sl.message) }) .collect() } else { diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 01b4585640f..12099b556b7 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -46,6 +46,7 @@ impl<'context> Elaborator<'context> { self.crate_id, self.debug_comptime_in_file, self.enable_arithmetic_generics, + self.interpreter_call_stack.clone(), ); elaborator.function_context.push(FunctionContext::default()); diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index e8b38193223..5bbd5f00ca8 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -168,6 +168,8 @@ pub struct Elaborator<'context> { /// Temporary flag to enable the experimental arithmetic generics feature enable_arithmetic_generics: bool, + + pub(crate) interpreter_call_stack: im::Vector, } #[derive(Default)] @@ -191,6 +193,7 @@ impl<'context> Elaborator<'context> { crate_id: CrateId, debug_comptime_in_file: Option, enable_arithmetic_generics: bool, + interpreter_call_stack: im::Vector, ) -> Self { Self { scopes: ScopeForest::default(), @@ -214,6 +217,7 @@ impl<'context> Elaborator<'context> { unresolved_globals: BTreeMap::new(), enable_arithmetic_generics, current_trait: None, + interpreter_call_stack, } } @@ -229,6 +233,7 @@ impl<'context> Elaborator<'context> { crate_id, debug_comptime_in_file, enable_arithmetic_generics, + im::Vector::new(), ) } diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index fd916485eaf..cfee6bcedac 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -56,6 +56,7 @@ pub enum InterpreterError { FailingConstraint { message: Option, location: Location, + call_stack: im::Vector, }, NoMethodFound { name: String, @@ -353,12 +354,20 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { let msg = format!("Expected a `bool` but found `{typ}`"); CustomDiagnostic::simple_error(msg, String::new(), location.span) } - InterpreterError::FailingConstraint { message, location } => { + InterpreterError::FailingConstraint { message, location, call_stack } => { let (primary, secondary) = match message { Some(msg) => (msg.clone(), "Assertion failed".into()), None => ("Assertion failed".into(), String::new()), }; - CustomDiagnostic::simple_error(primary, secondary, location.span) + let mut diagnostic = + CustomDiagnostic::simple_error(primary, secondary, location.span); + + // Only take at most 3 frames starting from the top of the stack to avoid producing too much output + for frame in call_stack.iter().rev().take(3) { + diagnostic.add_secondary_with_file("".to_string(), frame.span, frame.file); + } + + diagnostic } InterpreterError::NoMethodFound { name, typ, location } => { let msg = format!("No method named `{name}` found for type `{typ}`"); diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 33f8c9d8332..4980045c68d 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -70,7 +70,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { current_function: Option, ) -> Self { let bound_generics = Vec::new(); - Self { elaborator, crate_id, current_function, bound_generics, in_loop: false } + let in_loop = false; + Self { elaborator, crate_id, current_function, bound_generics, in_loop } } pub(crate) fn call_function( @@ -99,8 +100,11 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { } self.remember_bindings(&instantiation_bindings, &impl_bindings); + self.elaborator.interpreter_call_stack.push_back(location); + let result = self.call_function_inner(function, arguments, location); + self.elaborator.interpreter_call_stack.pop_back(); undo_instantiation_bindings(impl_bindings); undo_instantiation_bindings(instantiation_bindings); self.rebind_generics_from_previous_function(); @@ -1462,7 +1466,8 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { let message = constrain.2.and_then(|expr| self.evaluate(expr).ok()); let message = message.map(|value| value.display(self.elaborator.interner).to_string()); - Err(InterpreterError::FailingConstraint { location, message }) + let call_stack = self.elaborator.interpreter_call_stack.clone(); + Err(InterpreterError::FailingConstraint { location, message, call_stack }) } value => { let location = self.elaborator.interner.expr_location(&constrain.0); diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index bc8f473e08d..5ffc58004be 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -48,6 +48,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { location: Location, ) -> IResult { let interner = &mut self.elaborator.interner; + let call_stack = &self.elaborator.interpreter_call_stack; match name { "array_as_str_unchecked" => array_as_str_unchecked(interner, arguments, location), "array_len" => array_len(interner, arguments, location), @@ -110,11 +111,11 @@ impl<'local, 'context> Interpreter<'local, 'context> { "quoted_as_type" => quoted_as_type(self, arguments, location), "quoted_eq" => quoted_eq(arguments, location), "slice_insert" => slice_insert(interner, arguments, location), - "slice_pop_back" => slice_pop_back(interner, arguments, location), - "slice_pop_front" => slice_pop_front(interner, arguments, location), + "slice_pop_back" => slice_pop_back(interner, arguments, location, call_stack), + "slice_pop_front" => slice_pop_front(interner, arguments, location, call_stack), "slice_push_back" => slice_push_back(interner, arguments, location), "slice_push_front" => slice_push_front(interner, arguments, location), - "slice_remove" => slice_remove(interner, arguments, location), + "slice_remove" => slice_remove(interner, arguments, location, call_stack), "struct_def_as_type" => struct_def_as_type(interner, arguments, location), "struct_def_fields" => struct_def_fields(interner, arguments, location), "struct_def_generics" => struct_def_generics(interner, arguments, location), @@ -154,8 +155,16 @@ impl<'local, 'context> Interpreter<'local, 'context> { } } -fn failing_constraint(message: impl Into, location: Location) -> IResult { - Err(InterpreterError::FailingConstraint { message: Some(message.into()), location }) +fn failing_constraint( + message: impl Into, + location: Location, + call_stack: &im::Vector, +) -> IResult { + Err(InterpreterError::FailingConstraint { + message: Some(message.into()), + location, + call_stack: call_stack.clone(), + }) } fn array_len( @@ -287,6 +296,7 @@ fn slice_remove( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, location: Location, + call_stack: &im::Vector, ) -> IResult { let (slice, index) = check_two_arguments(arguments, location)?; @@ -294,7 +304,7 @@ fn slice_remove( let index = get_u32(index)? as usize; if values.is_empty() { - return failing_constraint("slice_remove called on empty slice", location); + return failing_constraint("slice_remove called on empty slice", location, call_stack); } if index >= values.len() { @@ -302,7 +312,7 @@ fn slice_remove( "slice_remove: index {index} is out of bounds for a slice of length {}", values.len() ); - return failing_constraint(message, location); + return failing_constraint(message, location, call_stack); } let element = values.remove(index); @@ -325,13 +335,14 @@ fn slice_pop_front( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, location: Location, + call_stack: &im::Vector, ) -> IResult { let argument = check_one_argument(arguments, location)?; let (mut values, typ) = get_slice(interner, argument)?; match values.pop_front() { Some(element) => Ok(Value::Tuple(vec![element, Value::Slice(values, typ)])), - None => failing_constraint("slice_pop_front called on empty slice", location), + None => failing_constraint("slice_pop_front called on empty slice", location, call_stack), } } @@ -339,13 +350,14 @@ fn slice_pop_back( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, location: Location, + call_stack: &im::Vector, ) -> IResult { let argument = check_one_argument(arguments, location)?; let (mut values, typ) = get_slice(interner, argument)?; match values.pop_back() { Some(element) => Ok(Value::Tuple(vec![Value::Slice(values, typ), element])), - None => failing_constraint("slice_pop_back called on empty slice", location), + None => failing_constraint("slice_pop_back called on empty slice", location, call_stack), } }