Skip to content

Commit

Permalink
fix: Compute the correct slice length when coercing from a literal ar…
Browse files Browse the repository at this point in the history
…ray of complex types (#4986)

# Description

## Problem\*

Resolves #4967

## Summary\*

There is a problem in the [array to slice
coercion](https://github.com/noir-lang/noir/blob/07930d4373a393146210efae69e6ec40171f047b/compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs#L87)
code. When evaluating the coercion, it does not take into account that
for SSA, complex types such as tuples and structs are flattened,
resulting in an array representation with length of elements times the
size of the complex type (as a result of the flattening). This results
in the replacement slice having an incorrect size.

Then, because the slice has an inflated size, the code which decodes the
elements of the slice for printing or tracking (in the debugger) crashes
when it attempts to decode beyond the values that it has available.

## Additional Context

There are two additional minor changes introduced in this PR:

1. Removed the `Optional<>` for `PrintableType::Array`'s length. This
was introduced to be able to print slices (setting the length to
`None`), but those are now represented properly with
`PrintableType::Slice`.
2. Added support for printing slices.

Both are pretty small changes which I made while investigating the
issue, but I'm willing to send separate PRs for those if required.

I also expanded the existing test cases to verify both the coerced slice
length and printing of slices.

## Documentation\*

Check one:
- [X] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [X] I have tested the changes locally.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
ggiraldez committed May 6, 2024
1 parent 07930d4 commit f3f1150
Show file tree
Hide file tree
Showing 6 changed files with 43 additions and 25 deletions.
12 changes: 11 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs
Expand Up @@ -87,8 +87,18 @@ pub(super) fn simplify_call(
Intrinsic::AsSlice => {
let array = dfg.get_array_constant(arguments[0]);
if let Some((array, array_type)) = array {
let slice_length = dfg.make_constant(array.len().into(), Type::length_type());
// Compute the resulting slice length by dividing the flattened
// array length by the size of each array element
let elements_size = array_type.element_size();
let inner_element_types = array_type.element_types();
assert_eq!(
0,
array.len() % elements_size,
"expected array length to be multiple of its elements size"
);
let slice_length_value = array.len() / elements_size;
let slice_length =
dfg.make_constant(slice_length_value.into(), Type::length_type());
let new_slice = dfg.make_array(array, Type::Slice(inner_element_types));
SimplifyResult::SimplifiedToMultiple(vec![slice_length, new_slice])
} else {
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir_def/types.rs
Expand Up @@ -1787,7 +1787,7 @@ impl From<&Type> for PrintableType {
match value {
Type::FieldElement => PrintableType::Field,
Type::Array(size, typ) => {
let length = size.evaluate_to_u64();
let length = size.evaluate_to_u64().expect("Cannot print variable sized arrays");
let typ = typ.as_ref();
PrintableType::Array { length, typ: Box::new(typ.into()) }
}
Expand Down
19 changes: 4 additions & 15 deletions compiler/noirc_printable_type/src/lib.rs
Expand Up @@ -11,7 +11,7 @@ use thiserror::Error;
pub enum PrintableType {
Field,
Array {
length: Option<u64>,
length: u64,
#[serde(rename = "type")]
typ: Box<PrintableType>,
},
Expand Down Expand Up @@ -186,7 +186,8 @@ fn to_string(value: &PrintableValue, typ: &PrintableType) -> Option<String> {
(_, PrintableType::MutableReference { .. }) => {
output.push_str("<<mutable ref>>");
}
(PrintableValue::Vec { array_elements, is_slice }, PrintableType::Array { typ, .. }) => {
(PrintableValue::Vec { array_elements, is_slice }, PrintableType::Array { typ, .. })
| (PrintableValue::Vec { array_elements, is_slice }, PrintableType::Slice { typ }) => {
if *is_slice {
output.push('&')
}
Expand Down Expand Up @@ -317,19 +318,7 @@ pub fn decode_value(

PrintableValue::Field(field_element)
}
PrintableType::Array { length: None, typ } => {
let length = field_iterator
.next()
.expect("not enough data to decode variable array length")
.to_u128() as usize;
let mut array_elements = Vec::with_capacity(length);
for _ in 0..length {
array_elements.push(decode_value(field_iterator, typ));
}

PrintableValue::Vec { array_elements, is_slice: false }
}
PrintableType::Array { length: Some(length), typ } => {
PrintableType::Array { length, typ } => {
let length = *length as usize;
let mut array_elements = Vec::with_capacity(length);
for _ in 0..length {
Expand Down
13 changes: 13 additions & 0 deletions test_programs/execution_success/debug_logs/src/main.nr
Expand Up @@ -71,6 +71,8 @@ fn main(x: Field, y: pub Field) {
let closured_lambda = |x| x + one;
println(f"closured_lambda: {closured_lambda}, sentinel: {sentinel}");
println(closured_lambda);

regression_4967();
}

fn string_identity(string: fmtstr<14, (Field, Field)>) -> fmtstr<14, (Field, Field)> {
Expand Down Expand Up @@ -122,3 +124,14 @@ fn regression_2906() {
println(f"array_five_vals: {array_five_vals}, label_five_vals: {label_five_vals}");
}

fn regression_4967() {
let sentinel: u32 = 8888;

let slice_of_tuples: [(i32, u8)] = &[(11, 22), (33, 44)];
println(f"slice_of_tuples: {slice_of_tuples}, sentinel: {sentinel}");
println(slice_of_tuples);

let slice_of_tuples_coerced: [(i32, u8)] = [(11, 22), (33, 44)];
println(f"slice_of_tuples: {slice_of_tuples_coerced}, sentinel: {sentinel}");
println(slice_of_tuples_coerced);
}
8 changes: 8 additions & 0 deletions test_programs/execution_success/slice_coercion/src/main.nr
Expand Up @@ -16,4 +16,12 @@ fn main(expected: pub Field, first: Field) {
let mut hasher = Hasher::new();
hasher.add(first);
assert(hasher.fields[0] == expected);

regression_4967();
}

fn regression_4967() {
let var1: [(i32, u8)] = [(1, 2)];
assert(var1.len() == 1);
dep::std::println(var1);
}
14 changes: 6 additions & 8 deletions tooling/nargo/src/artifacts/debug_vars.rs
Expand Up @@ -96,21 +96,19 @@ impl DebugVars {
PrintableType::Array { length, typ },
) => {
assert!(!*is_slice, "slice has array type");
if let Some(len) = length {
if *index as u64 >= *len {
panic!("unexpected field index past array length")
}
if *len != array_elements.len() as u64 {
panic!("type/array length mismatch")
}
if *index as u64 >= *length {
panic!("unexpected field index past array length")
}
if *length != array_elements.len() as u64 {
panic!("type/array length mismatch")
}
(array_elements.get_mut(*index as usize).unwrap(), &*Box::leak(typ.clone()))
}
(
PrintableValue::Vec { array_elements, is_slice },
PrintableType::Slice { typ },
) => {
assert!(*is_slice, "array has slice type");
assert!(*is_slice, "slice doesn't have slice type");
(array_elements.get_mut(*index as usize).unwrap(), &*Box::leak(typ.clone()))
}
(
Expand Down

0 comments on commit f3f1150

Please sign in to comment.