Improve error handling of ids variables (#851)

* Get rid of uninformative `FailedToComputeOperands` error

* Move results to options, use internal erros, refactor assert_not_equal hint

* Use String in error

* nFix tests

* Add specific error for wrong ids type on pack_from_var_name + remove pack_from_relocatable

* Restore fn

* Add specific error for pow

* Start using concrete types for structs in hints

* Move functions to methods

* Use concrete types for EcPoint

* Add cows

* Fix some tests

* Fix some tests

* Fix ids name

* Fix ids name

* Fix flipped coordinates

* clippy

* Add tests for BigInt3

* Add tests for EcPoint

* remove commented code

* Add addr info

src/hint_processor/builtin_hint_processor/blake2s_utils.rs

@@ -270,9 +270,7 @@ mod tests {
         //Execute the hint
         assert_matches!(
             run_hint!(vm, ids_data, hint_code),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((2, 0))
+            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(x))) if x == MaybeRelocatable::from((2,0))
         );
     }
 
@@ -290,9 +288,8 @@ mod tests {
         //Execute the hint
         assert_matches!(
             run_hint!(vm, ids_data, hint_code),
-            Err(HintError::Internal(
-                VirtualMachineError::ExpectedRelocatable(x)
-            )) if x == MaybeRelocatable::from((1, 0))
+            Err(HintError::IdentifierNotRelocatable(x, y)
+            ) if x == "output" && y == (1,0).into()
         );
     }
 
@@ -338,9 +335,7 @@ mod tests {
         //Execute the hint
         assert_matches!(
             run_hint!(vm, ids_data, hint_code),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((2, 0))
+            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(x))) if x == MaybeRelocatable::from((2,0))
         );
     }
 
@@ -425,7 +420,7 @@ mod tests {
         //Execute the hint
         assert_matches!(
             run_hint!(vm, HashMap::new(), hint_code),
-            Err(HintError::FailedToGetIds)
+            Err(HintError::UnknownIdentifier(x)) if x == "blake2s_ptr_end"
         );
     }
 

src/hint_processor/builtin_hint_processor/builtin_hint_processor_definition.rs

@@ -557,9 +557,8 @@ mod tests {
         let ids_data = ids_data!["len"];
         assert_matches!(
             run_hint!(vm, ids_data, hint_code),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 1))
+            Err(HintError::IdentifierNotInteger(x, y))
+            if x == "len" && y == (1,1).into()
         );
     }
 

src/hint_processor/builtin_hint_processor/dict_hint_utils.rs

@@ -442,7 +442,7 @@ mod tests {
         let ids_data = ids_data!["default_value"];
         assert_matches!(
             run_hint!(vm, ids_data, hint_code),
-            Err(HintError::FailedToGetIds)
+            Err(HintError::UnknownIdentifier(x)) if x == "default_value"
         );
     }
 

src/hint_processor/builtin_hint_processor/find_element_hint.rs

@@ -144,7 +144,7 @@ mod tests {
             },
             hint_processor_definition::HintProcessor,
         },
-        types::relocatable::{MaybeRelocatable, Relocatable},
+        types::relocatable::MaybeRelocatable,
         utils::test_utils::*,
         vm::vm_core::VirtualMachine,
     };
@@ -269,9 +269,8 @@ mod tests {
         let ids_data = ids_data!["array_ptr", "elm_size", "n_elms", "index", "key"];
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 4))
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "key" && y == (1,4).into()
         );
     }
 
@@ -283,9 +282,8 @@ mod tests {
         )]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 1))
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "elm_size" && y == (1,1).into()
         );
     }
 
@@ -317,12 +315,11 @@ mod tests {
     fn find_elm_not_int_n_elms() {
         let relocatable = MaybeRelocatable::from((1, 2));
         let (mut vm, ids_data) =
-            init_vm_ids_data(HashMap::from([("n_elms".to_string(), relocatable.clone())]));
+            init_vm_ids_data(HashMap::from([("n_elms".to_string(), relocatable)]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                inner
-            ))) if inner == relocatable
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "n_elms" && y == (1,2).into()
         );
     }
 
@@ -361,12 +358,7 @@ mod tests {
             init_vm_ids_data(HashMap::from([("key".to_string(), relocatable)]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                MaybeRelocatable::RelocatableValue(Relocatable {
-                    segment_index: 1,
-                    offset: 4
-                })
-            )))
+            Err(HintError::IdentifierNotInteger(x, y)) if x == "key" && y == (1,4).into()
         );
     }
 
@@ -402,9 +394,8 @@ mod tests {
         )]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 1))
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "elm_size" && y == (1,1).into()
         );
     }
 
@@ -440,9 +431,8 @@ mod tests {
         )]));
         assert_matches!(
             run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 2))
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "n_elms" && y == (1,2).into()
         );
     }
 

src/hint_processor/builtin_hint_processor/hint_utils.rs

@@ -1,7 +1,9 @@
 use felt::Felt;
 
 use crate::hint_processor::hint_processor_definition::HintReference;
-use crate::hint_processor::hint_processor_utils::compute_addr_from_reference;
+use crate::hint_processor::hint_processor_utils::{
+    compute_addr_from_reference, get_ptr_from_reference,
+};
 use crate::hint_processor::hint_processor_utils::{
     get_integer_from_reference, get_maybe_relocatable_from_reference,
 };
@@ -42,16 +44,14 @@ pub fn get_ptr_from_var_name(
     ids_data: &HashMap<String, HintReference>,
     ap_tracking: &ApTracking,
 ) -> Result<Relocatable, HintError> {
-    let var_addr = get_relocatable_from_var_name(var_name, vm, ids_data, ap_tracking)?;
-    //Add immediate if present in reference
-    let hint_reference = ids_data
-        .get(&String::from(var_name))
-        .ok_or(HintError::FailedToGetIds)?;
-    if hint_reference.dereference {
-        let value = vm.get_relocatable(var_addr)?;
-        Ok(value)
-    } else {
-        Ok(var_addr)
+    let reference = get_reference_from_var_name(var_name, ids_data)?;
+    match get_ptr_from_reference(vm, reference, ap_tracking) {
+        // Map internal errors into more descriptive variants
+        Ok(val) => Ok(val),
+        Err(HintError::WrongIdentifierTypeInternal(var_addr)) => Err(
+            HintError::IdentifierNotRelocatable(var_name.to_string(), var_addr),
+        ),
+        _ => Err(HintError::UnknownIdentifier(var_name.to_string())),
     }
 }
 
@@ -62,11 +62,7 @@ pub fn get_address_from_var_name(
     ids_data: &HashMap<String, HintReference>,
     ap_tracking: &ApTracking,
 ) -> Result<MaybeRelocatable, HintError> {
-    Ok(MaybeRelocatable::from(compute_addr_from_reference(
-        ids_data.get(var_name).ok_or(HintError::FailedToGetIds)?,
-        vm,
-        ap_tracking,
-    )?))
+    get_relocatable_from_var_name(var_name, vm, ids_data, ap_tracking).map(|x| x.into())
 }
 
 //Gets the address, as a Relocatable of the variable given by the ids name
@@ -76,24 +72,30 @@ pub fn get_relocatable_from_var_name(
     ids_data: &HashMap<String, HintReference>,
     ap_tracking: &ApTracking,
 ) -> Result<Relocatable, HintError> {
-    compute_addr_from_reference(
-        ids_data.get(var_name).ok_or(HintError::FailedToGetIds)?,
-        vm,
-        ap_tracking,
-    )
+    ids_data
+        .get(var_name)
+        .and_then(|x| compute_addr_from_reference(x, vm, ap_tracking))
+        .ok_or_else(|| HintError::UnknownIdentifier(var_name.to_string()))
 }
 
 //Gets the value of a variable name.
 //If the value is an MaybeRelocatable::Int(Bigint) return &Bigint
 //else raises Err
 pub fn get_integer_from_var_name<'a>(
-    var_name: &str,
+    var_name: &'a str,
     vm: &'a VirtualMachine,
     ids_data: &'a HashMap<String, HintReference>,
     ap_tracking: &ApTracking,
 ) -> Result<Cow<'a, Felt>, HintError> {
     let reference = get_reference_from_var_name(var_name, ids_data)?;
-    get_integer_from_reference(vm, reference, ap_tracking)
+    match get_integer_from_reference(vm, reference, ap_tracking) {
+        // Map internal errors into more descriptive variants
+        Ok(val) => Ok(val),
+        Err(HintError::WrongIdentifierTypeInternal(var_addr)) => Err(
+            HintError::IdentifierNotInteger(var_name.to_string(), var_addr),
+        ),
+        _ => Err(HintError::UnknownIdentifier(var_name.to_string())),
+    }
 }
 
 //Gets the value of a variable name as a MaybeRelocatable
@@ -105,13 +107,16 @@ pub fn get_maybe_relocatable_from_var_name<'a>(
 ) -> Result<MaybeRelocatable, HintError> {
     let reference = get_reference_from_var_name(var_name, ids_data)?;
     get_maybe_relocatable_from_reference(vm, reference, ap_tracking)
+        .ok_or_else(|| HintError::UnknownIdentifier(var_name.to_string()))
 }
 
 pub fn get_reference_from_var_name<'a>(
-    var_name: &str,
+    var_name: &'a str,
     ids_data: &'a HashMap<String, HintReference>,
 ) -> Result<&'a HintReference, HintError> {
-    ids_data.get(var_name).ok_or(HintError::FailedToGetIds)
+    ids_data
+        .get(var_name)
+        .ok_or(HintError::UnknownIdentifier(var_name.to_string()))
 }
 
 #[cfg(test)]
@@ -124,9 +129,7 @@ mod tests {
         serde::deserialize_program::OffsetValue,
         utils::test_utils::*,
         vm::{
-            errors::{memory_errors::MemoryError, vm_errors::VirtualMachineError},
-            vm_core::VirtualMachine,
-            vm_memory::memory::Memory,
+            errors::memory_errors::MemoryError, vm_core::VirtualMachine, vm_memory::memory::Memory,
         },
     };
     use assert_matches::assert_matches;
@@ -167,7 +170,7 @@ mod tests {
 
         assert_matches!(
             get_maybe_relocatable_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
-            Err(HintError::FailedToGetIds)
+            Err(HintError::UnknownIdentifier(x)) if x == *"value"
         );
     }
 
@@ -193,9 +196,8 @@ mod tests {
 
         assert_matches!(
             get_ptr_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
-            Err(HintError::Internal(
-                VirtualMachineError::ExpectedRelocatable(x)
-            )) if x == MaybeRelocatable::from((1, 0))
+            Err(HintError::IdentifierNotRelocatable(x,y
+            )) if x == "value" && y == (1,0).into()
         );
     }
 
@@ -221,7 +223,7 @@ mod tests {
 
         assert_matches!(
             get_relocatable_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
-            Err(HintError::FailedToGetIds)
+            Err(HintError::UnknownIdentifier(x)) if x == "value"
         );
     }
 
@@ -247,9 +249,8 @@ mod tests {
 
         assert_matches!(
             get_integer_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
-            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                x
-            ))) if x == MaybeRelocatable::from((1, 0))
+            Err(HintError::IdentifierNotInteger(x, y
+            )) if x == "value" && y == (1,0).into()
         );
     }
 }