Implement substitute_error_message_attribute_references (#689)

* Add flow_tracking_data to Attribute

* Start substitute_error_message_references method

* First draft

* Finish first draft

* Remove duplicated error_attr management

* Fix string format

* Fix invalid reference string + filter out ap-based references

* Copy get_maybe_relocatable_from_reference from PR 687

* Filter out complex cairo types

* Clippy

* Add tests for ap-based references in error attribute messages

* Add tests for complex references in error attribute messages

* Start changelog entry

* Fix eof

* Add changelog entry

* Fix test

* Update changelog

* Fix chanhgelog

* Fix chanhgelog

* Remove debug print

* Simplify test

Co-authored-by: Juan Rigada <62958725+Jrigada@users.noreply.github.com>

CHANGELOG.md

@@ -26,6 +26,13 @@
         * `DictManager`, its dictionaries, and all dict module hints implemented in rust now use `MaybeRelocatable` for keys and values instead of `BigInt`
         * Add helper functions that allow extracting ids variables as `MaybeRelocatable`: `get_maybe_relocatable_from_var_name` & `get_maybe_relocatable_from_reference`
         * Change inner value type of dict-related `HintError` variants to `MaybeRelocatable`
+
+* Implement `substitute_error_message_attribute_references` [#689] (https://github.com/lambdaclass/cairo-rs/pull/689)
+    * Public Api changes:
+        * Remove `error_message_attributes` field from `VirtualMachine`, and `VirtualMachine::new`
+        * Add `flow_tracking_data` field to `Attribute`
+        * `get_error_attr_value` now replaces the references in the error message with the corresponding cairo values.
+        * Remove duplicated handling of error attribute messages leading to duplicated into in the final error display.
 * Fix multiplicative inverse bug [#697](https://github.com/lambdaclass/cairo-rs/pull/697) [#698](https://github.com/lambdaclass/cairo-rs/pull/698). The VM was using integer division rather than prime field inverse when deducing `op0` or `op1` for the multiplication opcode
 
 #### [0.1.0] - 2022-12-30

cairo_programs/bad_programs/error_msg_attr_struct.cairo

@@ -0,0 +1,16 @@
+%builtins range_check
+from starkware.cairo.common.math import assert_le
+
+struct Cat {
+    paws: felt,
+    lives: felt,
+}
+
+func main{range_check_ptr}() {
+    alloc_locals;
+    local cat: Cat = Cat(2, 10);
+    with_attr error_message("Cats cannot have more than nine lives: {cat}") {
+        assert_le(cat.lives, 9);
+    }
+    return();
+}

cairo_programs/bad_programs/error_msg_attr_tempvar.cairo

@@ -0,0 +1,7 @@
+func main() {
+    tempvar x = 3;
+    with_attr error_message("SafeUint256: addition overflow: {x}") {
+        assert x = 2;
+    }
+    return();
+}

src/cairo_run.rs

@@ -32,7 +32,7 @@ pub fn cairo_run(
     };
 
     let mut cairo_runner = CairoRunner::new(&program, layout, proof_mode)?;
-    let mut vm = VirtualMachine::new(trace_enabled, program.error_message_attributes);
+    let mut vm = VirtualMachine::new(trace_enabled);
     let end = cairo_runner.initialize(&mut vm)?;
 
     cairo_runner

src/hint_processor/hint_processor_definition.rs

@@ -1,13 +1,17 @@
+use crate::any_box;
+use crate::serde::deserialize_program::ApTracking;
+use crate::serde::deserialize_program::OffsetValue;
+use crate::serde::deserialize_program::Reference;
+use crate::types::exec_scope::ExecutionScopes;
+use crate::types::instruction::Register;
+use crate::vm::errors::hint_errors::HintError;
+use crate::vm::errors::vm_errors::VirtualMachineError;
+use crate::vm::vm_core::VirtualMachine;
+use std::any::Any;
+use std::collections::HashMap;
+
 use super::builtin_hint_processor::builtin_hint_processor_definition::HintProcessorData;
-use crate::{
-    any_box,
-    serde::deserialize_program::{ApTracking, OffsetValue},
-    types::{exec_scope::ExecutionScopes, instruction::Register},
-    vm::errors::{hint_errors::HintError, vm_errors::VirtualMachineError},
-    vm::vm_core::VirtualMachine,
-};
 use felt::Felt;
-use std::{any::Any, collections::HashMap};
 
 pub trait HintProcessor {
     //Executes the hint which's data is provided by a dynamic structure previously created by compile_hint
@@ -97,3 +101,25 @@ impl HintReference {
         }
     }
 }
+
+impl From<Reference> for HintReference {
+    fn from(reference: Reference) -> Self {
+        HintReference {
+            offset1: reference.value_address.offset1.clone(),
+            offset2: reference.value_address.offset2.clone(),
+            dereference: reference.value_address.dereference,
+            // only store `ap` tracking data if the reference is referred to it
+            ap_tracking_data: match (
+                &reference.value_address.offset1,
+                &reference.value_address.offset2,
+            ) {
+                (OffsetValue::Reference(Register::AP, _, _), _)
+                | (_, OffsetValue::Reference(Register::AP, _, _)) => {
+                    Some(reference.ap_tracking_data.clone())
+                }
+                _ => None,
+            },
+            cairo_type: Some(reference.value_address.value_type.clone()),
+        }
+    }
+}

src/serde/deserialize_program.rs

@@ -83,6 +83,7 @@ pub struct Attribute {
     pub start_pc: usize,
     pub end_pc: usize,
     pub value: String,
+    pub flow_tracking_data: Option<FlowTrackingData>,
 }
 
 #[derive(Deserialize, Clone, Debug, PartialEq, Eq)]
@@ -1044,12 +1045,26 @@ mod tests {
                 start_pc: 379,
                 end_pc: 381,
                 value: String::from("SafeUint256: addition overflow"),
+                flow_tracking_data: Some(FlowTrackingData {
+                    ap_tracking: ApTracking {
+                        group: 14,
+                        offset: 35,
+                    },
+                    reference_ids: HashMap::new(),
+                }),
             },
             Attribute {
                 name: String::from("error_message"),
                 start_pc: 402,
                 end_pc: 404,
                 value: String::from("SafeUint256: subtraction overflow"),
+                flow_tracking_data: Some(FlowTrackingData {
+                    ap_tracking: ApTracking {
+                        group: 15,
+                        offset: 60,
+                    },
+                    reference_ids: HashMap::new(),
+                }),
             },
         ];
 

src/types/program.rs

@@ -109,6 +109,7 @@ impl Default for Program {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::serde::deserialize_program::{ApTracking, FlowTrackingData};
     use crate::utils::test_utils::mayberelocatable;
     use felt::{felt_str, NewFelt};
     use num_traits::Zero;
@@ -366,6 +367,13 @@ mod tests {
             start_pc: 379,
             end_pc: 381,
             value: String::from("SafeUint256: addition overflow"),
+            flow_tracking_data: Some(FlowTrackingData {
+                ap_tracking: ApTracking {
+                    group: 14,
+                    offset: 35,
+                },
+                reference_ids: HashMap::new(),
+            }),
         }];
 
         let data: Vec<MaybeRelocatable> = vec![

src/utils.rs

@@ -201,7 +201,7 @@ pub mod test_utils {
 
     macro_rules! vm_with_range_check {
         () => {{
-            let mut vm = VirtualMachine::new(false, Vec::new());
+            let mut vm = VirtualMachine::new(false);
             vm.builtin_runners = vec![(
                 "range_check".to_string(),
                 RangeCheckBuiltinRunner::new(8, 8, true).into(),
@@ -265,11 +265,11 @@ pub mod test_utils {
 
     macro_rules! vm {
         () => {{
-            VirtualMachine::new(false, Vec::new())
+            VirtualMachine::new(false)
         }};
 
         ($use_trace:expr) => {{
-            VirtualMachine::new($use_trace, Vec::new())
+            VirtualMachine::new($use_trace)
         }};
     }
     pub(crate) use vm;