Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: Don't assume outer deref when fetching integer values from references #1732

Merged
merged 12 commits into from
Apr 26, 2024
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
* Add `CairoPie` methods `run_validity_checks` & `check_pie_compatibility`
* Add `Program` method `from_stripped_program`

* bugfix: Don't assume outer deref when fetching integer values from references[#1732](https://github.com/lambdaclass/cairo-vm/pull/1732)

* feat: Implement `extend_additional_data` for `BuiltinRunner`[#1726](https://github.com/lambdaclass/cairo-vm/pull/1726)

* BREAKING: Set dynamic params as null by default on air public input [#1716](https://github.com/lambdaclass/cairo-vm/pull/1716)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ mod tests {
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotRelocatable(bx))
if *bx == ("output".to_string(), (1,0).into())
if bx.as_ref() == "output"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1003,7 +1003,7 @@ mod tests {
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx))
if *bx == ("len".to_string(), (1,1).into())
if bx.as_ref() == "len"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ 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::IdentifierNotInteger(bx)) if *bx == ("key".to_string(), (1,4).into())
Err(HintError::UnknownIdentifier(bx)) if bx.as_ref() == "key"
);
}

Expand All @@ -283,7 +283,7 @@ mod tests {
)]));
assert_matches!(
run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("elm_size".to_string(), (1,1).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "elm_size"
);
}

Expand Down Expand Up @@ -321,7 +321,7 @@ mod tests {
init_vm_ids_data(HashMap::from([("n_elms".to_string(), relocatable)]));
assert_matches!(
run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("n_elms".to_string(), (1,2).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "n_elms"
);
}

Expand Down Expand Up @@ -364,7 +364,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::IdentifierNotInteger(bx)) if *bx == ("key".to_string(), (1,4).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "key"
);
}

Expand Down Expand Up @@ -403,7 +403,7 @@ mod tests {
)]));
assert_matches!(
run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("elm_size".to_string(), (1,1).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "elm_size"
);
}

Expand All @@ -429,7 +429,7 @@ mod tests {
)]));
assert_matches!(
run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("n_elms".to_string(), (1,2).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "n_elms"
);
}

Expand Down
29 changes: 12 additions & 17 deletions vm/src/hint_processor/builtin_hint_processor/hint_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,10 @@
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(Box::new((var_name.to_string(), *var_addr))),
),
_ => Err(HintError::UnknownIdentifier(
var_name.to_string().into_boxed_str(),
Err(HintError::WrongIdentifierTypeInternal) => Err(HintError::IdentifierNotRelocatable(
Box::<str>::from(var_name),
)),
_ => Err(HintError::UnknownIdentifier(Box::<str>::from(var_name))),

Check warning on line 54 in vm/src/hint_processor/builtin_hint_processor/hint_utils.rs

View check run for this annotation

Codecov / codecov/patch

vm/src/hint_processor/builtin_hint_processor/hint_utils.rs#L54

Added line #L54 was not covered by tests
}
}

Expand All @@ -77,7 +75,7 @@
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().into_boxed_str()))
.ok_or_else(|| HintError::UnknownIdentifier(Box::<str>::from(var_name)))
}

//Gets the value of a variable name.
Expand All @@ -93,12 +91,10 @@
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(Box::new((var_name.to_string(), *var_addr))),
),
_ => Err(HintError::UnknownIdentifier(
var_name.to_string().into_boxed_str(),
)),
Err(HintError::WrongIdentifierTypeInternal) => {
Err(HintError::IdentifierNotInteger(Box::<str>::from(var_name)))
}
_ => Err(HintError::UnknownIdentifier(Box::<str>::from(var_name))),
}
}

Expand All @@ -111,7 +107,7 @@
) -> 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().into_boxed_str()))
.ok_or_else(|| HintError::UnknownIdentifier(Box::<str>::from(var_name)))
}

pub fn get_reference_from_var_name<'a>(
Expand All @@ -120,7 +116,7 @@
) -> Result<&'a HintReference, HintError> {
ids_data
.get(var_name)
.ok_or_else(|| HintError::UnknownIdentifier(var_name.to_string().into_boxed_str()))
.ok_or_else(|| HintError::UnknownIdentifier(Box::<str>::from(var_name)))
}

pub fn get_constant_from_var_name<'a>(
Expand All @@ -137,7 +133,6 @@
#[cfg(test)]
mod tests {
use super::*;
use crate::stdlib::string::ToString;

use crate::{
hint_processor::hint_processor_definition::HintReference,
Expand Down Expand Up @@ -218,7 +213,7 @@

assert_matches!(
get_ptr_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
Err(HintError::IdentifierNotRelocatable(bx)) if *bx == ("value".to_string(), (1,0).into())
Err(HintError::IdentifierNotRelocatable(bx)) if bx.as_ref() == "value"
);
}

Expand Down Expand Up @@ -274,7 +269,7 @@

assert_matches!(
get_integer_from_var_name("value", &vm, &ids_data, &ApTracking::new()),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("value".to_string(), (1,0).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "value"
);
}
}
24 changes: 12 additions & 12 deletions vm/src/hint_processor/builtin_hint_processor/math_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,4).into())
Err(HintError::UnknownIdentifier(bx)) if bx.as_ref() == "a"
);
}

Expand All @@ -912,7 +912,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,4).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "a"
);
}

Expand Down Expand Up @@ -1061,14 +1061,14 @@ mod tests {
let hint_code = "from starkware.cairo.common.math_utils import assert_integer\nassert_integer(ids.a)\nassert 0 <= ids.a % PRIME < range_check_builtin.bound, f'a = {ids.a} is out of range.'";
let mut vm = vm_with_range_check!();
//Initialize fp
vm.run_context.fp = 4;
vm.run_context.fp = 1;
//Insert ids into memory
vm.segments = segments![((1, 0), (10, 10))];
let ids_data = ids_data!["a"];
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,3).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "a"
);
}

Expand Down Expand Up @@ -1103,7 +1103,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,3).into())
Err(HintError::UnknownIdentifier(bx)) if bx.as_ref() == "a"
);
}

Expand Down Expand Up @@ -1156,7 +1156,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code::ASSERT_LE_FELT, &mut exec_scopes, &constants),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,0).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "a"
);
}

Expand All @@ -1182,7 +1182,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code::ASSERT_LE_FELT, &mut exec_scopes, &constants),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("b".to_string(), (1,1).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "b"
);
}

Expand Down Expand Up @@ -1412,7 +1412,7 @@ mod tests {
let ids_data = ids_data!["value"];
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("value".to_string(), (1,4).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "value"
);
}

Expand Down Expand Up @@ -2234,7 +2234,7 @@ mod tests {
)
])
),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("value".to_string(), (1,3).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "value"
);
}

Expand Down Expand Up @@ -2404,7 +2404,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("a".to_string(), (1,1).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "a"
);
}

Expand All @@ -2421,7 +2421,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("b".to_string(), (1,2).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "b"
);
}

Expand All @@ -2439,7 +2439,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("b".to_string(), (1,2).into())
Err(HintError::UnknownIdentifier(bx)) if bx.as_ref() == "b"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ mod tests {

assert_matches!(
get_integer_from_var_name(var_name, &vm, &ids_data, &ApTracking::default()),
Err(HintError::IdentifierNotInteger(bx)) if *bx == (var_name.to_string(), (1,0).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == var_name
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ pub fn memset_step_loop(
#[cfg(test)]
mod tests {
use super::*;
use crate::stdlib::string::ToString;
use crate::types::relocatable::Relocatable;
use crate::{
any_box,
Expand Down Expand Up @@ -100,7 +99,7 @@ mod tests {
let ids_data = ids_data!["n"];
assert_matches!(
run_hint!(vm, ids_data, hint_code),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("n".to_string(), (1,1).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "n"
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ mod tests {
//Execute the hint
assert_matches!(
run_hint!(vm, ids_data, hint_code, &mut exec_scopes),
Err(HintError::IdentifierNotInteger(bx)) if *bx == ("n_used_accesses".to_string(), (1,0).into())
Err(HintError::IdentifierNotInteger(bx)) if bx.as_ref() == "n_used_accesses"
);
}

Expand Down
Loading
Loading