diff --git a/CHANGELOG.md b/CHANGELOG.md index e17edf5742..81ef45c475 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,3 +5,8 @@ * `pub fn get_location(pc: &usize, runner: &CairoRunner) -> Option` is now `pub fn get_location(pc: usize, runner: &CairoRunner) -> Option` * `pub fn decode_instruction(encoded_instr: i64, mut imm: Option) -> Result` is now `pub fn decode_instruction(encoded_instr: i64, mut imm: Option<&BigInt>) -> Result` * `VmExcepion` field's string format now mirror their cairo-lang conterparts. + +* Add input file contents to traceback [#666](https://github.com/lambdaclass/cairo-rs/pull/666/files) + * Public Api changes: + * `VirtualMachineError` enum variants containing `MaybeRelocatable` and/or `Relocatable` values now use the `Display` format instead of `Debug` in their `Display` implementation + * `get_traceback` now adds the source code line to each traceback entry diff --git a/cairo_programs/bad_programs/bad_range_check.cairo b/cairo_programs/bad_programs/bad_range_check.cairo new file mode 100644 index 0000000000..86f6df5ae1 --- /dev/null +++ b/cairo_programs/bad_programs/bad_range_check.cairo @@ -0,0 +1,25 @@ +%builtins range_check + +func check_range{range_check_ptr: felt*}(num: felt) { + with_attr error_message("Failed range-check") { + [range_check_ptr] = num; + } + return(); +} + +func sub_1_check_range{range_check_ptr: felt*}(num: felt) -> felt { + check_range(num - 1); + return num - 1; +} + +func sub_by_1_check_range{range_check_ptr: felt*}(num: felt, sub_amount: felt) { + if (sub_amount == 0) { + return(); + } + return sub_by_1_check_range(sub_1_check_range(num), sub_amount -1); +} + +func main{range_check_ptr: felt*}() { + sub_by_1_check_range(6, 7); + return (); +} diff --git a/src/types/relocatable.rs b/src/types/relocatable.rs index b9aa8658d1..7725eff7b4 100644 --- a/src/types/relocatable.rs +++ b/src/types/relocatable.rs @@ -5,7 +5,10 @@ use crate::{ use num_bigint::BigInt; use num_integer::Integer; use num_traits::{FromPrimitive, Signed, ToPrimitive}; -use std::ops::Add; +use std::{ + fmt::{self, Display}, + ops::Add, +}; #[derive(Eq, Hash, PartialEq, PartialOrd, Clone, Copy, Debug)] pub struct Relocatable { @@ -64,6 +67,21 @@ impl From for MaybeRelocatable { } } +impl Display for MaybeRelocatable { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self { + MaybeRelocatable::RelocatableValue(rel) => rel.fmt(f), + MaybeRelocatable::Int(num) => write!(f, "{}", num), + } + } +} + +impl Display for Relocatable { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}:{}", self.segment_index, self.offset) + } +} + impl Add for Relocatable { type Output = Relocatable; fn add(self, other: usize) -> Self { @@ -850,4 +868,28 @@ mod tests { mayberelocatable!(3).get_relocatable() ) } + + #[test] + fn relocatable_display() { + assert_eq!( + format!("{}", Relocatable::from((1, 0))), + String::from("1:0") + ) + } + + #[test] + fn maybe_relocatable_relocatable_display() { + assert_eq!( + format!("{}", MaybeRelocatable::from((1, 0))), + String::from("1:0") + ) + } + + #[test] + fn maybe_relocatable_int_display() { + assert_eq!( + format!("{}", MaybeRelocatable::from(bigint!(6))), + String::from("6") + ) + } } diff --git a/src/vm/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index 9109703efa..f89ac0c7ce 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -28,11 +28,11 @@ pub enum VirtualMachineError { UnconstrainedResJumpRel, #[error("Res.UNCONSTRAINED cannot be used with Opcode.ASSERT_EQ")] UnconstrainedResAssertEq, - #[error("ASSERT_EQ instruction failed; {0:?} != {1:?}")] + #[error("An ASSERT_EQ instruction failed: {0} != {1}.")] DiffAssertValues(MaybeRelocatable, MaybeRelocatable), - #[error("Call failed to write return-pc (inconsistent op0): {0:?} != {1:?}. Did you forget to increment ap?")] + #[error("Call failed to write return-pc (inconsistent op0): {0} != {1}. Did you forget to increment ap?")] CantWriteReturnPc(MaybeRelocatable, MaybeRelocatable), - #[error("Call failed to write return-fp (inconsistent dst): {0:?} != {1:?}. Did you forget to increment ap?")] + #[error("Call failed to write return-fp (inconsistent dst): {0} != {1}. Did you forget to increment ap?")] CantWriteReturnFp(MaybeRelocatable, MaybeRelocatable), #[error("Couldn't get or load dst")] NoDst, @@ -50,11 +50,11 @@ pub enum VirtualMachineError { NotImplemented, #[error("Can only subtract two relocatable values of the same segment")] DiffIndexSub, - #[error("Inconsistent auto-deduction for builtin {0}, expected {1:?}, got {2:?}")] + #[error("Inconsistent auto-deduction for builtin {0}, expected {1}, got {2:?}")] InconsistentAutoDeduction(String, MaybeRelocatable, Option), #[error(transparent)] RunnerError(#[from] RunnerError), - #[error("Invalid hint encoding at pc: {0:?}")] + #[error("Invalid hint encoding at pc: {0}")] InvalidHintEncoding(MaybeRelocatable), #[error(transparent)] MemoryError(#[from] MemoryError), @@ -62,11 +62,11 @@ pub enum VirtualMachineError { NoRangeCheckBuiltin, #[error("Expected ecdsa builtin to be present")] NoSignatureBuiltin, - #[error("Failed to retrieve value from address {0:?}")] + #[error("Failed to retrieve value from address {0}")] MemoryGet(MaybeRelocatable), - #[error("Expected integer at address {0:?}")] + #[error("Expected integer at address {0}")] ExpectedInteger(MaybeRelocatable), - #[error("Expected relocatable at address {0:?}")] + #[error("Expected relocatable at address {0}")] ExpectedRelocatable(MaybeRelocatable), #[error("Failed to get ids for hint execution")] FailedToGetIds, @@ -74,7 +74,7 @@ pub enum VirtualMachineError { NonLeFelt(BigInt, BigInt), #[error("Div out of range: 0 < {0} <= {1}")] OutOfValidRange(BigInt, BigInt), - #[error("Assertion failed, 0 <= ids.a % PRIME < range_check_builtin.bound \n a = {0:?} is out of range")] + #[error("Assertion failed, 0 <= ids.a % PRIME < range_check_builtin.bound \n a = {0} is out of range")] ValueOutOfRange(BigInt), #[error("Value: {0} should be positive")] ValueNotPositive(BigInt), @@ -86,11 +86,11 @@ pub enum VirtualMachineError { SplitIntNotZero, #[error("split_int(): Limb {0} is out of range.")] SplitIntLimbOutOfRange(BigInt), - #[error("Failed to compare {0:?} and {1:?}, cant compare a relocatable to an integer value")] + #[error("Failed to compare {0} and {1}, cant compare a relocatable to an integer value")] DiffTypeComparison(MaybeRelocatable, MaybeRelocatable), - #[error("assert_not_equal failed: {0:?} = {1:?}")] + #[error("assert_not_equal failed: {0} = {1}")] AssertNotEqualFail(MaybeRelocatable, MaybeRelocatable), - #[error("Failed to compare {0:?} and {1:?}, cant compare two relocatable values of different segment indexes")] + #[error("Failed to compare {0} and {1}, cant compare two relocatable values of different segment indexes")] DiffIndexComp(Relocatable, Relocatable), #[error("Value: {0} is outside of the range [0, 2**250)")] ValueOutside250BitRange(BigInt), @@ -132,7 +132,7 @@ pub enum VirtualMachineError { NoneApTrackingData, #[error("Tracking groups should be the same, got {0} and {1}")] InvalidTrackingGroup(usize, usize), - #[error("Expected relocatable for ap, got {0:?}")] + #[error("Expected relocatable for ap, got {0}")] InvalidApValue(MaybeRelocatable), #[error("Dict Error: Tried to create a dict whithout an initial dict")] NoInitialDict, @@ -178,13 +178,13 @@ pub enum VirtualMachineError { CouldntPopPositions, #[error("unexpected verify multiplicity fail: last_pos not found")] LastPosNotFound, - #[error("Set's starting point {0:?} is bigger it's ending point {1:?}")] + #[error("Set's starting point {0} is bigger it's ending point {1}")] InvalidSetRange(MaybeRelocatable, MaybeRelocatable), #[error("Failed to construct a fixed size array of size: {0}")] FixedSizeArrayFail(usize), #[error("{0}")] AssertionFailed(String), - #[error("Wrong dict pointer supplied. Got {0:?}, expected {1:?}.")] + #[error("Wrong dict pointer supplied. Got {0}, expected {1}.")] MismatchedDictPtr(Relocatable, Relocatable), #[error("Integer must be postive or zero, got: {0}")] SecpSplitNegative(BigInt), @@ -218,7 +218,7 @@ pub enum VirtualMachineError { NoImm, #[error("Tried to compute an address but there was no register in the reference.")] NoRegisterInReference, - #[error("Couldn't compute operand {0} at address {1:?}")] + #[error("Couldn't compute operand {0} at address {1}")] FailedToComputeOperands(String, Relocatable), #[error("Custom Hint Error: {0}")] CustomHint(String), diff --git a/src/vm/errors/vm_exception.rs b/src/vm/errors/vm_exception.rs index 820c95caeb..a043b2e850 100644 --- a/src/vm/errors/vm_exception.rs +++ b/src/vm/errors/vm_exception.rs @@ -1,4 +1,9 @@ -use std::fmt::{self, Display}; +use std::{ + fmt::{self, Display}, + fs::File, + io::{BufReader, Read}, + path::Path, +}; use thiserror::Error; @@ -62,12 +67,11 @@ pub fn get_traceback(vm: &VirtualMachine, runner: &CairoRunner) -> Option traceback - .push_str(&location.to_string(&format!("(pc=0:{})\n", traceback_pc.offset))), - None => traceback.push_str(&format!( - "Unknown location (pc=0:{})\n", - traceback_pc.offset + Some(location) => traceback.push_str(&format!( + "{}\n", + location.to_string_with_content(&format!("(pc=0:{})", traceback_pc.offset)) )), + None => traceback.push_str(&format!("Unknown location (pc=0:{})", traceback_pc.offset)), } } (!traceback.is_empty()) @@ -88,7 +92,11 @@ impl Display for VmException { let mut location_msg = String::new(); let (mut location, mut message) = (location, &message); loop { - location_msg = format!("{}\n{}", location.to_string(message), location_msg); + location_msg = format!( + "{}\n{}", + location.to_string_with_content(message), + location_msg + ); // Add parent location info if let Some(parent) = &location.parent_location { (location, message) = (&parent.0, &parent.1) @@ -96,12 +104,12 @@ impl Display for VmException { break; } } - error_msg.push_str(&location_msg) + error_msg.push_str(&location_msg); } else { error_msg.push_str(&format!("{}\n", message)); } if let Some(ref string) = self.traceback { - error_msg.push_str(&format!("{}\n", string)); + error_msg.push_str(string); } // Write error message write!(f, "{}", error_msg) @@ -117,6 +125,42 @@ impl Location { self.input_file.filename, self.start_line, self.start_col, msg_prefix, message ) } + + pub fn to_string_with_content(&self, message: &String) -> String { + let mut string = self.to_string(message); + let input_file_path = Path::new(&self.input_file.filename); + if let Ok(file) = File::open(input_file_path) { + let mut reader = BufReader::new(file); + string.push_str(&format!("\n{}", self.get_location_marks(&mut reader))); + } + string + } + + pub fn get_location_marks(&self, file_contents: &mut impl Read) -> String { + let mut contents = String::new(); + // If this read fails, the string will be left empty, so we can ignore the result + let _ = file_contents.read_to_string(&mut contents); + let split_lines: Vec<&str> = contents.split('\n').collect(); + if !(0 < self.start_line && ((self.start_line - 1) as usize) < split_lines.len()) { + return String::new(); + } + let start_line = split_lines[((self.start_line - 1) as usize)]; + let start_col = self.start_col as usize; + let mut result = format!("{}\n", start_line); + let end_col = if self.start_line == self.end_line { + self.end_col as usize + } else { + start_line.len() + 1 + }; + let left_margin: String = vec![' '; start_col - 1].into_iter().collect(); + if end_col > start_col + 1 { + let highlight: String = vec!['*'; end_col - start_col - 2].into_iter().collect(); + result.push_str(&format!("{}^{}^", left_margin, highlight)); + } else { + result.push_str(&format!("{}^", left_margin)) + } + result + } } #[cfg(test)] mod test { @@ -388,7 +432,6 @@ mod test { } #[test] - // TEST CASE WITHOUT FILE CONTENTS fn get_traceback_bad_dict_update() { let program = Program::from_file( Path::new("cairo_programs/bad_programs/bad_dict_update.json"), @@ -404,12 +447,11 @@ mod test { assert!(cairo_runner .run_until_pc(end, &mut vm, &mut hint_processor) .is_err()); - let expected_traceback = String::from("Cairo traceback (most recent call last):\ncairo_programs/bad_programs/bad_dict_update.cairo:10:5: (pc=0:34)\n"); + let expected_traceback = String::from("Cairo traceback (most recent call last):\ncairo_programs/bad_programs/bad_dict_update.cairo:10:5: (pc=0:34)\n dict_update{dict_ptr=my_dict}(key=2, prev_value=3, new_value=4);\n ^*************************************************************^\n"); assert_eq!(get_traceback(&vm, &cairo_runner), Some(expected_traceback)); } #[test] - // TEST CASE WITHOUT FILE CONTENTS fn get_traceback_bad_usort() { let program = Program::from_file( Path::new("cairo_programs/bad_programs/bad_usort.json"), @@ -425,7 +467,142 @@ mod test { assert!(cairo_runner .run_until_pc(end, &mut vm, &mut hint_processor) .is_err()); - let expected_traceback = String::from("Cairo traceback (most recent call last):\ncairo_programs/bad_programs/bad_usort.cairo:91:48: (pc=0:97)\ncairo_programs/bad_programs/bad_usort.cairo:36:5: (pc=0:30)\ncairo_programs/bad_programs/bad_usort.cairo:64:5: (pc=0:60)\n"); + let expected_traceback = String::from("Cairo traceback (most recent call last):\ncairo_programs/bad_programs/bad_usort.cairo:91:48: (pc=0:97)\n let (output_len, output, multiplicities) = usort(input_len=3, input=input_array);\n ^***********************************^\ncairo_programs/bad_programs/bad_usort.cairo:36:5: (pc=0:30)\n verify_usort{output=output}(\n ^**************************^\ncairo_programs/bad_programs/bad_usort.cairo:64:5: (pc=0:60)\n verify_multiplicity(multiplicity=multiplicity, input_len=input_len, input=input, value=value);\n ^*******************************************************************************************^\n"); assert_eq!(get_traceback(&vm, &cairo_runner), Some(expected_traceback)); } + + #[test] + fn location_to_string_with_contents_no_contents() { + let location = Location { + end_line: 2, + end_col: 2, + input_file: InputFile { + filename: String::from("Folder/file.cairo"), + }, + parent_location: None, + start_line: 1, + start_col: 1, + }; + let message = String::from("While expanding the reference"); + assert_eq!( + location.to_string_with_content(&message), + String::from("Folder/file.cairo:1:1: While expanding the reference") + ) + } + + #[test] + fn location_to_string_with_contents() { + let location = Location { + end_line: 5, + end_col: 2, + input_file: InputFile { + filename: String::from("cairo_programs/bad_programs/bad_usort.cairo"), + }, + parent_location: None, + start_line: 5, + start_col: 1, + }; + let message = String::from("Error at pc=0:75:"); + assert_eq!( + location.to_string_with_content(&message), + String::from("cairo_programs/bad_programs/bad_usort.cairo:5:1: Error at pc=0:75:\nfunc usort{range_check_ptr}(input_len: felt, input: felt*) -> (\n^") + ) + } + + #[test] + fn location_to_string_with_contents_no_file() { + let location = Location { + end_line: 5, + end_col: 2, + input_file: InputFile { + filename: String::from("cairo_programs/bad_prtypoograms/bad_usort.cairo"), + }, + parent_location: None, + start_line: 5, + start_col: 1, + }; + let message = String::from("Error at pc=0:75:\n"); + assert_eq!( + location.to_string_with_content(&message), + String::from( + "cairo_programs/bad_prtypoograms/bad_usort.cairo:5:1: Error at pc=0:75:\n" + ) + ) + } + + #[test] + fn location_get_location_marks() { + let location = Location { + end_line: 5, + end_col: 2, + input_file: InputFile { + filename: String::from("cairo_programs/bad_programs/bad_usort.cairo"), + }, + parent_location: None, + start_line: 5, + start_col: 1, + }; + let input_file_path = Path::new(&location.input_file.filename); + let file = File::open(input_file_path).expect("Failed to open file"); + let mut reader = BufReader::new(file); + assert_eq!( + location.get_location_marks(&mut reader), + String::from("func usort{range_check_ptr}(input_len: felt, input: felt*) -> (\n^") + ) + } + + #[test] + fn location_get_location_marks_empty_file() { + let location = Location { + end_line: 5, + end_col: 2, + input_file: InputFile { + filename: String::from("cairo_programs/bad_programs/bad_usort.cairo"), + }, + parent_location: None, + start_line: 5, + start_col: 1, + }; + let mut reader: &[u8] = &[]; + assert_eq!(location.get_location_marks(&mut reader), String::from("")) + } + + #[test] + fn run_bad_range_check_and_check_error_displayed() { + let expected_error_string = r#"Error message: Failed range-check +cairo_programs/bad_programs/bad_range_check.cairo:5:9: Error at pc=0:0: +An ASSERT_EQ instruction failed: 4 != 5. + [range_check_ptr] = num; + ^*********************^ +Cairo traceback (most recent call last): +cairo_programs/bad_programs/bad_range_check.cairo:23:5: (pc=0:29) + sub_by_1_check_range(6, 7); + ^************************^ +cairo_programs/bad_programs/bad_range_check.cairo:19:12: (pc=0:21) + return sub_by_1_check_range(sub_1_check_range(num), sub_amount -1); + ^*********************************************************^ +cairo_programs/bad_programs/bad_range_check.cairo:19:33: (pc=0:17) + return sub_by_1_check_range(sub_1_check_range(num), sub_amount -1); + ^********************^ +cairo_programs/bad_programs/bad_range_check.cairo:11:5: (pc=0:6) + check_range(num - 1); + ^******************^ +"#; + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/bad_range_check.json"), + Some("main"), + ) + .expect("Call to `Program::from_file()` failed."); + + let mut hint_processor = BuiltinHintProcessor::new_empty(); + let mut cairo_runner = cairo_runner!(program, "all", false); + let mut vm = vm!(); + + let end = cairo_runner.initialize(&mut vm).unwrap(); + let error = cairo_runner + .run_until_pc(end, &mut vm, &mut hint_processor) + .unwrap_err(); + let vm_excepction = VmException::from_vm_error(&cairo_runner, &vm, error); + assert_eq!(vm_excepction.to_string(), expected_error_string); + } } diff --git a/src/vm/vm_core.rs b/src/vm/vm_core.rs index 2b073c9191..7b08a5e6ac 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -3313,7 +3313,7 @@ mod tests { ))) )) ); - assert_eq!(error.unwrap_err().to_string(), "Inconsistent auto-deduction for builtin ec_op, expected Int(2739017437753868763038285897969098325279422804143820990343394856167768859289), got Some(Int(2778063437308421278851140253538604815869848682781135193774472480292420096757))"); + assert_eq!(error.unwrap_err().to_string(), "Inconsistent auto-deduction for builtin ec_op, expected 2739017437753868763038285897969098325279422804143820990343394856167768859289, got Some(Int(2778063437308421278851140253538604815869848682781135193774472480292420096757))"); } #[test]