fix(acir): Attach locations to MemoryOps in ACIR

crates/nargo_cli/src/cli/execute_cmd.rs

@@ -82,7 +82,6 @@ fn execute_package<B: Backend>(
     // Parse the initial witness values from Prover.toml
     let (inputs_map, _) =
         read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &abi)?;
-
     let solved_witness =
         execute_program(backend, circuit, &abi, &inputs_map, Some((debug, context)))?;
     let public_abi = abi.public_abi();
@@ -91,38 +90,68 @@ fn execute_package<B: Backend>(
     Ok((return_value, solved_witness))
 }
 
-fn extract_unsatisfied_constraint_from_nargo_error(
+/// There are certain errors that contain an [acvm::pwg::ErrorLocation].
+/// We need to determine whether the error location has been resolving during execution.
+/// If the location has been resolved we return the contained [OpcodeLocation].
+fn extract_opcode_error_from_nargo_error(
     nargo_err: &NargoError,
-) -> Option<OpcodeLocation> {
+) -> Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)> {
     let solving_err = match nargo_err {
         nargo::NargoError::SolvingError(err) => err,
         _ => return None,
     };
 
     match solving_err {
-        acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain {
+        acvm::pwg::OpcodeResolutionError::IndexOutOfBounds {
+            opcode_location: error_location,
+            ..
+        }
+        | acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain {
             opcode_location: error_location,
         } => match error_location {
             ErrorLocation::Unresolved => {
                 unreachable!("Cannot resolve index for unsatisfied constraint")
             }
-            ErrorLocation::Resolved(opcode_location) => Some(*opcode_location),
+            ErrorLocation::Resolved(opcode_location) => Some((*opcode_location, solving_err)),
         },
         _ => None,
     }
 }
 
-fn report_unsatisfied_constraint_error(
-    opcode_location: Option<OpcodeLocation>,
+/// Resolve an [OpcodeLocation] using debug information generated during compilation
+/// to determine an opcode's call stack. Then report the error using the resolved
+/// call stack and any other relevant error information returned from the ACVM.
+fn report_error_with_opcode_location(
+    opcode_err_info: Option<(OpcodeLocation, &acvm::pwg::OpcodeResolutionError)>,
     debug: &DebugInfo,
     context: &Context,
 ) {
-    if let Some(opcode_location) = opcode_location {
+    if let Some((opcode_location, opcode_err)) = opcode_err_info {
         if let Some(locations) = debug.opcode_location(&opcode_location) {
             // The location of the error itself will be the location at the top
             // of the call stack (the last item in the Vec).
             if let Some(location) = locations.last() {
-                let message = "Failed constraint".into();
+                let message = match opcode_err {
+                    acvm::pwg::OpcodeResolutionError::IndexOutOfBounds {
+                        index,
+                        array_size,
+                        ..
+                    } => {
+                        format!(
+                            "Index out of bounds, array has size {array_size:?}, but index was {index:?}"
+                        )
+                    }
+                    acvm::pwg::OpcodeResolutionError::UnsatisfiedConstrain { .. } => {
+                        "Failed constraint".into()
+                    }
+                    _ => {
+                        // All other errors that do not have corresponding opcode locations
+                        // should not be reported in this method.
+                        // If an error with an opcode location is not handled in this match statement
+                        // the basic message attached to the original error from the ACVM should be reported.
+                        return;
+                    }
+                };
                 CustomDiagnostic::simple_error(message, String::new(), location.span)
                     .in_file(location.file)
                     .with_call_stack(locations)
@@ -145,8 +174,8 @@ pub(crate) fn execute_program<B: Backend>(
         Ok(solved_witness) => Ok(solved_witness),
         Err(err) => {
             if let Some((debug, context)) = debug_data {
-                let opcode_location = extract_unsatisfied_constraint_from_nargo_error(&err);
-                report_unsatisfied_constraint_error(opcode_location, &debug, &context);
+                let opcode_err_info = extract_opcode_error_from_nargo_error(&err);
+                report_error_with_opcode_location(opcode_err_info, &debug, &context);
             }
 
             Err(crate::errors::CliError::NargoError(err))

crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Nargo.toml

@@ -0,0 +1,7 @@
+[package]
+name = "dynamic_index_failure"
+type = "bin"
+authors = [""]
+compiler_version = "0.10.3"
+
+[dependencies]

crates/nargo_cli/tests/compile_failure/dynamic_index_failure/Prover.toml

@@ -0,0 +1,2 @@
+x = [104, 101, 108, 108, 111]
+z = "4"

crates/nargo_cli/tests/compile_failure/dynamic_index_failure/src/main.nr

@@ -0,0 +1,23 @@
+fn main(mut x: [u32; 5], z: Field) {
+    let idx = z + 10;
+
+    x[z] = 4;
+
+    // Dynamic index is greater than length of the array
+    assert(x[idx] != 0);
+
+    // TODO(#2133): Provide more accurate call stacks for arrays merged in if statements
+    // if z != 20 {
+    //     x[0] = x[4];
+    // } else {
+    //     // TODO: Dynamic predicate still gives index out of bounds error
+    //     if idx as u32 < 3 {
+    //         x[idx] = 10;
+    //     }
+    //     x[idx] = 10;
+    //     for i in 0..5 {
+    //         x[idx] = x[i];
+    //     }
+    // }
+    // assert(x[idx] != 0);
+}

crates/noirc_evaluator/src/ssa.rs

@@ -87,14 +87,12 @@ pub fn create_circuit(
     enable_brillig_logging: bool,
 ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> {
     let func_sig = program.main_function_signature.clone();
+    let mut generated_acir =
+        optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?;
+    let opcodes = generated_acir.take_opcodes();
     let GeneratedAcir {
-        current_witness_index,
-        opcodes,
-        return_witnesses,
-        locations,
-        input_witnesses,
-        ..
-    } = optimize_into_acir(program, enable_ssa_logging, enable_brillig_logging)?;
+        current_witness_index, return_witnesses, locations, input_witnesses, ..
+    } = generated_acir;
 
     let abi = gen_abi(&context.def_interner, func_sig, &input_witnesses, return_witnesses.clone());
     let public_abi = abi.clone().public_abi();

crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

@@ -514,7 +514,7 @@
     /// Note that this is not the euclidian division, where we have instead remainder < |rhs|
     fn signed_division_var(
         &mut self,
         lhs: AcirVar,
         rhs: AcirVar,
         bit_size: u32,
     ) -> Result<(AcirVar, AcirVar), RuntimeError> {
@@ -599,7 +599,7 @@
 
     /// Returns an `AcirVar` which will be constrained to be lhs mod 2^{rhs}
     /// In order to do this, we simply perform euclidian division of lhs by 2^{rhs}
     /// The remainder of the division is then lhs mod 2^{rhs}
     pub(crate) fn truncate_var(
         &mut self,
         lhs: AcirVar,
@@ -933,7 +933,7 @@
 
     /// Recursively create acir values for returned arrays. This is necessary because a brillig returned array can have nested arrays as elements.
     /// A singular array of witnesses is collected for a top level array, by deflattening the assigned witnesses at each level.
     fn brillig_array_output(
         &mut self,
         element_types: &[AcirType],
         size: usize,
@@ -1081,7 +1081,7 @@
 
         // Add the memory read operation to the list of opcodes
         let op = MemOp::read_at_mem_index(index_witness.into(), value_read_witness);
-        self.acir_ir.opcodes.push(Opcode::MemoryOp { block_id, op });
+        self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op });
 
         Ok(value_read_var)
     }
@@ -1102,7 +1102,8 @@
 
         // Add the memory write operation to the list of opcodes
         let op = MemOp::write_to_mem_index(index_witness.into(), value_write_witness.into());
-        self.acir_ir.opcodes.push(Opcode::MemoryOp { block_id, op });
+        self.acir_ir.push_opcode(Opcode::MemoryOp { block_id, op });
+
         Ok(())
     }
 
@@ -1128,7 +1129,7 @@
             })?,
         };
 
-        self.acir_ir.opcodes.push(Opcode::MemoryInit { block_id, init: initialized_values });
+        self.acir_ir.push_opcode(Opcode::MemoryInit { block_id, init: initialized_values });
         Ok(())
     }
 }

crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs

@@ -34,7 +34,7 @@
     pub(crate) current_witness_index: u32,
 
     /// The opcodes of which the compiled ACIR will comprise.
-    pub(crate) opcodes: Vec<AcirOpcode>,
+    opcodes: Vec<AcirOpcode>,
 
     /// All witness indices that comprise the final return value of the program
     ///
@@ -45,7 +45,7 @@
     /// All witness indices which are inputs to the main function
     pub(crate) input_witnesses: Vec<Witness>,
 
     /// Correspondance between an opcode index (in opcodes) and the source code call stack which generated it
     pub(crate) locations: BTreeMap<OpcodeLocation, CallStack>,
 
     /// Source code location of the current instruction being processed
@@ -60,14 +60,18 @@
     }
 
     /// Adds a new opcode into ACIR.
-    fn push_opcode(&mut self, opcode: AcirOpcode) {
+    pub(crate) fn push_opcode(&mut self, opcode: AcirOpcode) {
         self.opcodes.push(opcode);
         if !self.call_stack.is_empty() {
             self.locations
                 .insert(OpcodeLocation::Acir(self.opcodes.len() - 1), self.call_stack.clone());
         }
     }
 
+    pub(crate) fn take_opcodes(&mut self) -> Vec<AcirOpcode> {
+        std::mem::take(&mut self.opcodes)
+    }
+
     /// Updates the witness index counter and returns
     /// the next witness index.
     pub(crate) fn next_witness_index(&mut self) -> Witness {
@@ -228,7 +232,7 @@
             }
         };
 
-        self.opcodes.push(AcirOpcode::BlackBoxFuncCall(black_box_func_call));
+        self.push_opcode(AcirOpcode::BlackBoxFuncCall(black_box_func_call));
 
         Ok(outputs_clone)
     }
@@ -345,8 +349,8 @@
     }
 
     /// Signed division lhs /  rhs
     /// We derive the signed division from the unsigned euclidian division.
     /// note that this is not euclidian division!
     // if x is a signed integer, then sign(x)x >= 0
     // so if a and b are signed integers, we can do the unsigned division:
     // sign(a)a = q1*sign(b)b + r1
@@ -747,7 +751,7 @@
         let two_max_bits: FieldElement = two.pow(&FieldElement::from(max_bits as i128));
         let comparison_evaluation = (a - b) + two_max_bits;
 
         // Euclidian division by 2^{max_bits}  : 2^{max_bits} + a - b = q * 2^{max_bits} + r
         //
         // 2^{max_bits} is of max_bits+1 bit size
         // If a>b, then a-b is less than 2^{max_bits} - 1, so 2^{max_bits} + a - b is less than 2^{max_bits} + 2^{max_bits} - 1 = 2^{max_bits+1} - 1

crates/noirc_evaluator/src/ssa/acir_gen/mod.rs

@@ -1171,11 +1171,11 @@ mod tests {
         let ssa = builder.finish();
 
         let context = Context::new();
-        let acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap();
+        let mut acir = context.convert_ssa(ssa, Brillig::default(), &HashMap::new()).unwrap();
 
         let expected_opcodes =
             vec![Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1)))];
-        assert_eq!(acir.opcodes, expected_opcodes);
+        assert_eq!(acir.take_opcodes(), expected_opcodes);
         assert_eq!(acir.return_witnesses, vec![Witness(1)]);
     }
 }

crates/noirc_evaluator/src/ssa/ssa_gen/context.rs

@@ -551,7 +551,9 @@
                     LValue::Ident
                 }
             }
-            ast::LValue::Index { array, index, .. } => self.index_lvalue(array, index).2,
+            ast::LValue::Index { array, index, location, .. } => {
+                self.index_lvalue(array, index, location).2
+            }
             ast::LValue::MemberAccess { object, field_index } => {
                 let (old_object, object_lvalue) = self.extract_current_value_recursive(object);
                 let object_lvalue = Box::new(object_lvalue);
@@ -590,20 +592,27 @@
         &mut self,
         array: &ast::LValue,
         index: &ast::Expression,
+        location: &Location,
     ) -> (ValueId, ValueId, LValue, Option<ValueId>) {
         let (old_array, array_lvalue) = self.extract_current_value_recursive(array);
         let index = self.codegen_non_tuple_expression(index);
         let array_lvalue = Box::new(array_lvalue);
         let array_values = old_array.clone().into_value_list(self);
 
+        let location = *location;
         // A slice is represented as a tuple (length, slice contents).
         // We need to fetch the second value.
         if array_values.len() > 1 {
-            let slice_lvalue =
-                LValue::SliceIndex { old_slice: old_array, index, slice_lvalue: array_lvalue };
+            let slice_lvalue = LValue::SliceIndex {
+                old_slice: old_array,
+                index,
+                slice_lvalue: array_lvalue,
+                location,
+            };
             (array_values[1], index, slice_lvalue, Some(array_values[0]))
         } else {
-            let array_lvalue = LValue::Index { old_array: array_values[0], index, array_lvalue };
+            let array_lvalue =
+                LValue::Index { old_array: array_values[0], index, array_lvalue, location };
             (array_values[0], index, array_lvalue, None)
         }
     }
@@ -620,7 +629,8 @@
                 }
             }
             ast::LValue::Index { array, index, element_type, location } => {
-                let (old_array, index, index_lvalue, max_length) = self.index_lvalue(array, index);
+                let (old_array, index, index_lvalue, max_length) =
+                    self.index_lvalue(array, index, location);
                 let element =
                     self.codegen_array_index(old_array, index, element_type, *location, max_length);
                 (element, index_lvalue)
@@ -647,14 +657,15 @@
     pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) {
         match lvalue {
             LValue::Ident => unreachable!("Cannot assign to a variable without a reference"),
-            LValue::Index { old_array: mut array, index, array_lvalue } => {
-                array = self.assign_lvalue_index(new_value, array, index);
+            LValue::Index { old_array: mut array, index, array_lvalue, location } => {
+                array = self.assign_lvalue_index(new_value, array, index, location);
                 self.assign_new_value(*array_lvalue, array.into());
             }
-            LValue::SliceIndex { old_slice: slice, index, slice_lvalue } => {
+            LValue::SliceIndex { old_slice: slice, index, slice_lvalue, location } => {
                 let mut slice_values = slice.into_value_list(self);
 
-                slice_values[1] = self.assign_lvalue_index(new_value, slice_values[1], index);
+                slice_values[1] =
+                    self.assign_lvalue_index(new_value, slice_values[1], index, location);
 
                 // The size of the slice does not change in a slice index assignment so we can reuse the same length value
                 let new_slice = Tree::Branch(vec![slice_values[0].into(), slice_values[1].into()]);
@@ -675,11 +686,13 @@
         new_value: Values,
         mut array: ValueId,
         index: ValueId,
+        location: Location,
     ) -> ValueId {
         let element_size = self.builder.field_constant(self.element_size(array));
 
         // The actual base index is the user's index * the array element type's size
-        let mut index = self.builder.insert_binary(index, BinaryOp::Mul, element_size);
+        let mut index =
+            self.builder.set_location(location).insert_binary(index, BinaryOp::Mul, element_size);
         let one = self.builder.field_constant(FieldElement::one());
 
         new_value.for_each(|value| {
@@ -839,7 +852,7 @@
     /// and return this new id.
     pub(super) fn get_or_queue_function(&self, id: ast::FuncId) -> IrFunctionId {
         // Start a new block to guarantee the destructor for the map lock is released
         // before map needs to be aquired again in self.functions.write() below
         {
             let map = self.functions.read().expect("Failed to read self.functions");
             if let Some(existing_id) = map.get(&id) {
@@ -862,8 +875,8 @@
 #[derive(Debug)]
 pub(super) enum LValue {
     Ident,
-    Index { old_array: ValueId, index: ValueId, array_lvalue: Box<LValue> },
-    SliceIndex { old_slice: Values, index: ValueId, slice_lvalue: Box<LValue> },
+    Index { old_array: ValueId, index: ValueId, array_lvalue: Box<LValue>, location: Location },
+    SliceIndex { old_slice: Values, index: ValueId, slice_lvalue: Box<LValue>, location: Location },
     MemberAccess { old_object: Values, index: usize, object_lvalue: Box<LValue> },
     Dereference { reference: Values },
 }

crates/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -130,7 +130,10 @@
                         let slice_contents = self.codegen_array(elements, typ[1].clone());
                         Tree::Branch(vec![slice_length.into(), slice_contents])
                     }
-                    _ => unreachable!("ICE: array literal type must be an array or a slice, but got {}", array.typ),
+                    _ => unreachable!(
+                        "ICE: array literal type must be an array or a slice, but got {}",
+                        array.typ
+                    ),
                 }
             }
             ast::Literal::Integer(value, typ) => {
@@ -344,7 +347,7 @@
         self.builder.insert_cast(lhs, typ).into()
     }
 
     /// Codegens a for loop, creating three new blocks in the process.
     /// The return value of a for loop is always a unit literal.
     ///
     /// For example, the loop `for i in start .. end { body }` is codegen'd as:
@@ -354,7 +357,7 @@
     ///   br loop_entry(v0)
     /// loop_entry(i: Field):
     ///   v2 = lt i v1
     ///   brif v2, then: loop_body, else: loop_end
     /// loop_body():
     ///   v3 = ... codegen body ...
     ///   v4 = add 1, i