diff --git a/CHANGELOG.md b/CHANGELOG.md index 81ef45c475..7d4006d5a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,3 +10,10 @@ * 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 + +* Use hint location instead of instruction location when building VmExceptions from hint failure [#673](https://github.com/lambdaclass/cairo-rs/pull/673/files) + * Public Api changes: + * `hints` field added to `InstructionLocation` + * `Program.instruction_locations` type changed from `Option>` to `Option>` + * `VirtualMachineError`s produced by `HintProcessor::execute_hint()` will be wrapped in a `VirtualMachineError::Hint` error containing their hint_index + * `get_location()` now receives an an optional usize value `hint_index`, used to obtain hint locations diff --git a/src/serde/deserialize_program.rs b/src/serde/deserialize_program.rs index 2cdb29e6ab..dea2f9351c 100644 --- a/src/serde/deserialize_program.rs +++ b/src/serde/deserialize_program.rs @@ -100,9 +100,10 @@ pub struct DebugInfo { instruction_locations: HashMap, } -#[derive(Deserialize, Debug, PartialEq)] +#[derive(Deserialize, Debug, PartialEq, Eq, Clone)] pub struct InstructionLocation { - inst: Location, + pub inst: Location, + pub hints: Vec, } #[derive(Deserialize, Clone, Debug, PartialEq, Eq)] @@ -110,6 +111,12 @@ pub struct InputFile { pub filename: String, } +#[derive(Deserialize, Debug, PartialEq, Eq, Clone)] +pub struct HintLocation { + pub location: Location, + pub n_prefix_newlines: u32, +} + fn bigint_from_number<'de, D>(deserializer: D) -> Result, D::Error> where D: Deserializer<'de>, @@ -361,13 +368,9 @@ pub fn deserialize_program( .into_iter() .filter(|attr| attr.name == "error_message") .collect(), - instruction_locations: program_json.debug_info.map(|debug_info| { - debug_info - .instruction_locations - .into_iter() - .map(|(offset, instruction_location)| (offset, instruction_location.inst)) - .collect() - }), + instruction_locations: program_json + .debug_info + .map(|debug_info| debug_info.instruction_locations), }) } @@ -1147,6 +1150,7 @@ mod tests { start_line: 7, start_col: 5, }, + hints: vec![], }, ), ( @@ -1160,6 +1164,7 @@ mod tests { start_line: 5, start_col: 5, }, + hints: vec![], }, ), ]), @@ -1260,6 +1265,7 @@ mod tests { start_col: 18, }), String::from( "While expanding the reference 'syscall_ptr' in:")) ), start_line: 9, start_col: 18 }, + hints: vec![], }), ] ) }; diff --git a/src/types/program.rs b/src/types/program.rs index a349daacb7..5e94ebd84f 100644 --- a/src/types/program.rs +++ b/src/types/program.rs @@ -1,5 +1,5 @@ use crate::serde::deserialize_program::{ - deserialize_program, Attribute, HintParams, Identifier, Location, ReferenceManager, + deserialize_program, Attribute, HintParams, Identifier, InstructionLocation, ReferenceManager, }; use crate::types::errors::program_errors::ProgramError; use crate::types::relocatable::MaybeRelocatable; @@ -22,7 +22,7 @@ pub struct Program { pub reference_manager: ReferenceManager, pub identifiers: HashMap, pub error_message_attributes: Vec, - pub instruction_locations: Option>, + pub instruction_locations: Option>, } impl Program { @@ -36,7 +36,7 @@ impl Program { reference_manager: ReferenceManager, identifiers: HashMap, error_message_attributes: Vec, - instruction_locations: Option>, + instruction_locations: Option>, ) -> Result { Ok(Self { builtins, diff --git a/src/vm/errors/vm_errors.rs b/src/vm/errors/vm_errors.rs index f89ac0c7ce..917197a731 100644 --- a/src/vm/errors/vm_errors.rs +++ b/src/vm/errors/vm_errors.rs @@ -240,4 +240,6 @@ pub enum VirtualMachineError { InvalidArgCount(usize, usize), #[error("{0}, {1}")] ErrorMessageAttribute(String, Box), + #[error("Got an exception while executing a hint: {1}")] + Hint(usize, Box), } diff --git a/src/vm/errors/vm_exception.rs b/src/vm/errors/vm_exception.rs index a043b2e850..905247114f 100644 --- a/src/vm/errors/vm_exception.rs +++ b/src/vm/errors/vm_exception.rs @@ -30,9 +30,14 @@ impl VmException { ) -> Self { let pc = vm.run_context.pc.offset; let error_attr_value = get_error_attr_value(pc, runner); + let hint_index = if let VirtualMachineError::Hint(hint_index, _) = error { + Some(hint_index) + } else { + None + }; VmException { pc, - inst_location: get_location(pc, runner), + inst_location: get_location(pc, runner, hint_index), inner_exc: error, error_attr_value, traceback: get_traceback(vm, runner), @@ -50,13 +55,20 @@ pub fn get_error_attr_value(pc: usize, runner: &CairoRunner) -> Option { (!errors.is_empty()).then(|| errors) } -pub fn get_location(pc: usize, runner: &CairoRunner) -> Option { - runner - .program - .instruction_locations - .as_ref()? - .get(&pc) - .cloned() +pub fn get_location( + pc: usize, + runner: &CairoRunner, + hint_index: Option, +) -> Option { + let instruction_location = runner.program.instruction_locations.as_ref()?.get(&pc)?; + if let Some(index) = hint_index { + instruction_location + .hints + .get(index) + .map(|hint_location| hint_location.location.clone()) + } else { + Some(instruction_location.inst.clone()) + } } // Returns the traceback at the current pc. @@ -66,7 +78,8 @@ pub fn get_traceback(vm: &VirtualMachine, runner: &CairoRunner) -> Option traceback.push_str(&format!( "{}\n", location.to_string_with_content(&format!("(pc=0:{})", traceback_pc.offset)) @@ -169,7 +182,9 @@ mod test { use std::path::Path; use crate::hint_processor::builtin_hint_processor::builtin_hint_processor_definition::BuiltinHintProcessor; - use crate::serde::deserialize_program::{Attribute, InputFile}; + use crate::serde::deserialize_program::{ + Attribute, HintLocation, InputFile, InstructionLocation, + }; use crate::types::program::Program; use crate::types::relocatable::Relocatable; use crate::utils::test_utils::*; @@ -188,8 +203,13 @@ mod test { start_line: 1, start_col: 1, }; - let program = - program!(instruction_locations = Some(HashMap::from([(pc, location.clone())])),); + let instruction_location = InstructionLocation { + inst: location.clone(), + hints: vec![], + }; + let program = program!( + instruction_locations = Some(HashMap::from([(pc, instruction_location.clone())])), + ); let runner = cairo_runner!(program); let vm_excep = VmException { pc, @@ -408,10 +428,15 @@ mod test { start_line: 1, start_col: 1, }; - let program = - program!(instruction_locations = Some(HashMap::from([(2, location.clone())])),); + let instruction_location = InstructionLocation { + inst: location.clone(), + hints: vec![], + }; + let program = program!( + instruction_locations = Some(HashMap::from([(2, instruction_location.clone())])), + ); let runner = cairo_runner!(program); - assert_eq!(get_location(2, &runner), Some(location)); + assert_eq!(get_location(2, &runner, None), Some(location)); } #[test] @@ -426,9 +451,51 @@ mod test { start_line: 1, start_col: 1, }; - let program = program!(instruction_locations = Some(HashMap::from([(2, location)])),); + let instruction_location = InstructionLocation { + inst: location, + hints: vec![], + }; + let program = + program!(instruction_locations = Some(HashMap::from([(2, instruction_location)])),); let runner = cairo_runner!(program); - assert_eq!(get_location(3, &runner), None); + assert_eq!(get_location(3, &runner, None), None); + } + + #[test] + fn get_location_some_hint_index() { + let location_a = Location { + end_line: 2, + end_col: 2, + input_file: InputFile { + filename: String::from("Folder/file_a.cairo"), + }, + parent_location: None, + start_line: 1, + start_col: 1, + }; + let location_b = Location { + end_line: 3, + end_col: 2, + input_file: InputFile { + filename: String::from("Folder/file_b.cairo"), + }, + parent_location: None, + start_line: 1, + start_col: 5, + }; + let hint_location = HintLocation { + location: location_b.clone(), + n_prefix_newlines: 2, + }; + let instruction_location = InstructionLocation { + inst: location_a, + hints: vec![hint_location], + }; + let program = program!( + instruction_locations = Some(HashMap::from([(2, instruction_location.clone())])), + ); + let runner = cairo_runner!(program); + assert_eq!(get_location(2, &runner, Some(0)), Some(location_b)); } #[test] @@ -605,4 +672,39 @@ cairo_programs/bad_programs/bad_range_check.cairo:11:5: (pc=0:6) let vm_excepction = VmException::from_vm_error(&cairo_runner, &vm, error); assert_eq!(vm_excepction.to_string(), expected_error_string); } + + #[test] + fn run_bad_usort_and_check_error_displayed() { + let expected_error_string = r#"cairo_programs/bad_programs/bad_usort.cairo:79:5: Error at pc=0:75: +Got an exception while executing a hint: unexpected verify multiplicity fail: positions length != 0 + %{ assert len(positions) == 0 %} + ^******************************^ +Cairo traceback (most recent call last): +cairo_programs/bad_programs/bad_usort.cairo:91:48: (pc=0:97) + let (output_len, output, multiplicities) = usort(input_len=3, input=input_array); + ^***********************************^ +cairo_programs/bad_programs/bad_usort.cairo:36:5: (pc=0:30) + verify_usort{output=output}( + ^**************************^ +cairo_programs/bad_programs/bad_usort.cairo:64:5: (pc=0:60) + verify_multiplicity(multiplicity=multiplicity, input_len=input_len, input=input, value=value); + ^*******************************************************************************************^ +"#; + let program = Program::from_file( + Path::new("cairo_programs/bad_programs/bad_usort.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 7b08a5e6ac..05333969d0 100644 --- a/src/vm/vm_core.rs +++ b/src/vm/vm_core.rs @@ -525,8 +525,10 @@ impl VirtualMachine { constants: &HashMap, ) -> Result<(), VirtualMachineError> { if let Some(hint_list) = hint_data_dictionary.get(&self.run_context.pc.offset) { - for hint_data in hint_list.iter() { - hint_executor.execute_hint(self, exec_scopes, hint_data, constants)? + for (hint_index, hint_data) in hint_list.iter().enumerate() { + hint_executor + .execute_hint(self, exec_scopes, hint_data, constants) + .map_err(|err| VirtualMachineError::Hint(hint_index, Box::new(err)))? } } Ok(())