Use hint location instead of instruction location when building VmExceptions from hint failure

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<HashMap<usize, Location>>` to `Option<HashMap<usize, InstructionLocation>>`
+        * `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

src/serde/deserialize_program.rs

@@ -100,16 +100,23 @@ pub struct DebugInfo {
     instruction_locations: HashMap<usize, InstructionLocation>,
 }
 
-#[derive(Deserialize, Debug, PartialEq)]
+#[derive(Deserialize, Debug, PartialEq, Eq, Clone)]
 pub struct InstructionLocation {
-    inst: Location,
+    pub inst: Location,
+    pub hints: Vec<HintLocation>,
 }
 
 #[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
 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<Option<BigInt>, 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![],
                 }),
             ]
         ) };

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<String, Identifier>,
     pub error_message_attributes: Vec<Attribute>,
-    pub instruction_locations: Option<HashMap<usize, Location>>,
+    pub instruction_locations: Option<HashMap<usize, InstructionLocation>>,
 }
 
 impl Program {
@@ -36,7 +36,7 @@ impl Program {
         reference_manager: ReferenceManager,
         identifiers: HashMap<String, Identifier>,
         error_message_attributes: Vec<Attribute>,
-        instruction_locations: Option<HashMap<usize, Location>>,
+        instruction_locations: Option<HashMap<usize, InstructionLocation>>,
     ) -> Result<Program, ProgramError> {
         Ok(Self {
             builtins,

src/vm/errors/vm_errors.rs

@@ -240,4 +240,6 @@ pub enum VirtualMachineError {
     InvalidArgCount(usize, usize),
     #[error("{0}, {1}")]
     ErrorMessageAttribute(String, Box<VirtualMachineError>),
+    #[error("Got an exception while executing a hint: {1}")]
+    Hint(usize, Box<VirtualMachineError>),
 }

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<String> {
     (!errors.is_empty()).then(|| errors)
 }
 
-pub fn get_location(pc: usize, runner: &CairoRunner) -> Option<Location> {
-    runner
-        .program
-        .instruction_locations
-        .as_ref()?
-        .get(&pc)
-        .cloned()
+pub fn get_location(
+    pc: usize,
+    runner: &CairoRunner,
+    hint_index: Option<usize>,
+) -> Option<Location> {
+    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<String
         if let Some(ref attr) = get_error_attr_value(traceback_pc.offset, runner) {
             traceback.push_str(attr)
         }
-        match get_location(traceback_pc.offset, runner) {
+
+        match get_location(traceback_pc.offset, runner, None) {
             Some(location) => 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);
+    }
 }

src/vm/vm_core.rs

@@ -525,8 +525,10 @@ impl VirtualMachine {
         constants: &HashMap<String, BigInt>,
     ) -> 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(())