Merge branch 'master' into constant-brillig-execution

* master:
  fix: Fix assignment when both `mut` and `&mut` are used (#2264)
  feat: Add `assert_constant` (#2242)

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

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

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

@@ -0,0 +1,10 @@
+use dep::std::assert_constant;
+
+fn main(x: Field) {
+    foo(5, x);
+}
+
+fn foo(constant: Field, non_constant: Field) {
+    assert_constant(constant);
+    assert_constant(non_constant);
+}

crates/nargo_cli/tests/execution_success/references/src/main.nr

@@ -34,6 +34,7 @@ fn main(mut x: Field) {
     regression_1887();
     regression_2054();
     regression_2030();
+    regression_2255();
 }
 
 fn add1(x: &mut Field) {
@@ -97,3 +98,16 @@ fn regression_2030() {
     let _ = *array[0];
     *array[0] = 1;
 }
+
+// The `mut x: &mut ...` caught a bug handling lvalues where a double-dereference would occur internally
+// in one step rather than being tracked by two separate steps. This lead to assigning the 1 value to the
+// incorrect outer `mut` reference rather than the correct `&mut` reference.
+fn regression_2255() {
+    let x = &mut 0;
+    regression_2255_helper(x);
+    assert(*x == 1);
+}
+
+fn regression_2255_helper(mut x: &mut Field) {
+    *x = 1;
+}

crates/noirc_evaluator/src/errors.rs

@@ -32,6 +32,8 @@ pub enum RuntimeError {
     UnsupportedIntegerSize { num_bits: u32, max_num_bits: u32, location: Option<Location> },
     #[error("Could not determine loop bound at compile-time")]
     UnknownLoopBound { location: Option<Location> },
+    #[error("Argument is not constant")]
+    AssertConstantFailed { location: Option<Location> },
 }
 
 #[derive(Debug, PartialEq, Eq, Clone, Error)]
@@ -69,7 +71,8 @@ impl RuntimeError {
             | RuntimeError::InvalidRangeConstraint { location, .. }
             | RuntimeError::TypeConversion { location, .. }
             | RuntimeError::UnInitialized { location, .. }
-            | RuntimeError::UnknownLoopBound { location, .. }
+            | RuntimeError::UnknownLoopBound { location }
+            | RuntimeError::AssertConstantFailed { location }
             | RuntimeError::UnsupportedIntegerSize { location, .. } => *location,
         }
     }

crates/noirc_evaluator/src/ssa.rs

@@ -49,6 +49,7 @@ pub(crate) fn optimize_into_acir(
         ssa = ssa
             .inline_functions()
             .print(print_ssa_passes, "After Inlining:")
+            .evaluate_assert_constant()?
             .unroll_loops()?
             .print(print_ssa_passes, "After Unrolling:")
             .simplify_cfg()

crates/noirc_evaluator/src/ssa/ir/basic_block.rs

@@ -73,6 +73,11 @@ impl BasicBlock {
         &mut self.instructions
     }
 
+    /// Take the instructions in this block, replacing it with an empty Vec
+    pub(crate) fn take_instructions(&mut self) -> Vec<InstructionId> {
+        std::mem::take(&mut self.instructions)
+    }
+
     /// Sets the terminator instruction of this block.
     ///
     /// A properly-constructed block will always terminate with a TerminatorInstruction -

crates/noirc_evaluator/src/ssa/ir/dfg.rs

@@ -399,7 +399,7 @@ impl DataFlowGraph {
     /// source block.
     pub(crate) fn inline_block(&mut self, source: BasicBlockId, destination: BasicBlockId) {
         let source = &mut self.blocks[source];
-        let mut instructions = std::mem::take(source.instructions_mut());
+        let mut instructions = source.take_instructions();
         let terminator = source.take_terminator();
 
         let destination = &mut self.blocks[destination];
@@ -417,6 +417,11 @@ impl DataFlowGraph {
             _ => None,
         }
     }
+
+    /// True if the given ValueId refers to a constant value
+    pub(crate) fn is_constant(&self, argument: ValueId) -> bool {
+        !matches!(&self[self.resolve(argument)], Value::Instruction { .. } | Value::Param { .. })
+    }
 }
 
 impl std::ops::Index<InstructionId> for DataFlowGraph {
@@ -483,9 +488,7 @@ impl<'dfg> InsertInstructionResult<'dfg> {
             InsertInstructionResult::Results(results) => Cow::Borrowed(results),
             InsertInstructionResult::SimplifiedTo(result) => Cow::Owned(vec![result]),
             InsertInstructionResult::SimplifiedToMultiple(results) => Cow::Owned(results),
-            InsertInstructionResult::InstructionRemoved => {
-                panic!("InsertInstructionResult::results called on a removed instruction")
-            }
+            InsertInstructionResult::InstructionRemoved => Cow::Owned(vec![]),
         }
     }
 

crates/noirc_evaluator/src/ssa/ir/instruction.rs

@@ -34,6 +34,7 @@ pub(crate) type InstructionId = Id<Instruction>;
 pub(crate) enum Intrinsic {
     Sort,
     ArrayLen,
+    AssertConstant,
     SlicePushBack,
     SlicePushFront,
     SlicePopBack,
@@ -52,6 +53,7 @@ impl std::fmt::Display for Intrinsic {
             Intrinsic::Println => write!(f, "println"),
             Intrinsic::Sort => write!(f, "arraysort"),
             Intrinsic::ArrayLen => write!(f, "array_len"),
+            Intrinsic::AssertConstant => write!(f, "assert_constant"),
             Intrinsic::SlicePushBack => write!(f, "slice_push_back"),
             Intrinsic::SlicePushFront => write!(f, "slice_push_front"),
             Intrinsic::SlicePopBack => write!(f, "slice_pop_back"),
@@ -75,6 +77,7 @@ impl Intrinsic {
             "println" => Some(Intrinsic::Println),
             "arraysort" => Some(Intrinsic::Sort),
             "array_len" => Some(Intrinsic::ArrayLen),
+            "assert_constant" => Some(Intrinsic::AssertConstant),
             "slice_push_back" => Some(Intrinsic::SlicePushBack),
             "slice_push_front" => Some(Intrinsic::SlicePushFront),
             "slice_pop_back" => Some(Intrinsic::SlicePopBack),

crates/noirc_evaluator/src/ssa/ir/instruction/call.rs

@@ -165,6 +165,13 @@ pub(super) fn simplify_call(
                 SimplifyResult::None
             }
         }
+        Intrinsic::AssertConstant => {
+            if arguments.iter().all(|argument| dfg.is_constant(*argument)) {
+                SimplifyResult::Remove
+            } else {
+                SimplifyResult::None
+            }
+        }
         Intrinsic::BlackBox(bb_func) => simplify_black_box_func(bb_func, arguments, dfg),
         Intrinsic::Sort => simplify_sort(dfg, arguments),
         Intrinsic::Println => SimplifyResult::None,

crates/noirc_evaluator/src/ssa/opt/assert_constant.rs

@@ -0,0 +1,83 @@
+use crate::{
+    errors::RuntimeError,
+    ssa::{
+        ir::{
+            function::Function,
+            instruction::{Instruction, InstructionId, Intrinsic},
+            value::ValueId,
+        },
+        ssa_gen::Ssa,
+    },
+};
+
+impl Ssa {
+    /// A simple SSA pass to go through each instruction and evaluate each call
+    /// to `assert_constant`, issuing an error if any arguments to the function are
+    /// not constants.
+    ///
+    /// Note that this pass must be placed directly before loop unrolling to be
+    /// useful. Any optimization passes between this and loop unrolling will cause
+    /// the constants that this pass sees to be potentially different than the constants
+    /// seen by loop unrolling. Furthermore, this pass cannot be a part of loop unrolling
+    /// since we must go through every instruction to find all references to `assert_constant`
+    /// while loop unrolling only touches blocks with loops in them.
+    pub(crate) fn evaluate_assert_constant(mut self) -> Result<Ssa, RuntimeError> {
+        for function in self.functions.values_mut() {
+            for block in function.reachable_blocks() {
+                // Unfortunately we can't just use instructions.retain(...) here since
+                // check_instruction can also return an error
+                let instructions = function.dfg[block].take_instructions();
+                let mut filtered_instructions = Vec::with_capacity(instructions.len());
+
+                for instruction in instructions {
+                    if check_instruction(function, instruction)? {
+                        filtered_instructions.push(instruction);
+                    }
+                }
+
+                *function.dfg[block].instructions_mut() = filtered_instructions;
+            }
+        }
+        Ok(self)
+    }
+}
+
+/// During the loop unrolling pass we also evaluate calls to `assert_constant`.
+/// This is done in this pass because loop unrolling is the only pass that will error
+/// if a value (the loop bounds) are not known constants.
+///
+/// This returns Ok(true) if the given instruction should be kept in the block and
+/// Ok(false) if it should be removed.
+fn check_instruction(
+    function: &mut Function,
+    instruction: InstructionId,
+) -> Result<bool, RuntimeError> {
+    let assert_constant_id = function.dfg.import_intrinsic(Intrinsic::AssertConstant);
+    match &function.dfg[instruction] {
+        Instruction::Call { func, arguments } => {
+            if *func == assert_constant_id {
+                evaluate_assert_constant(function, instruction, arguments)
+            } else {
+                Ok(true)
+            }
+        }
+        _ => Ok(true),
+    }
+}
+
+/// Evaluate a call to `assert_constant`, returning an error if any of the elements are not
+/// constants. If all of the elements are constants, Ok(false) is returned. This signifies a
+/// success but also that the instruction need not be reinserted into the block being unrolled
+/// since it has already been evaluated.
+fn evaluate_assert_constant(
+    function: &Function,
+    instruction: InstructionId,
+    arguments: &[ValueId],
+) -> Result<bool, RuntimeError> {
+    if arguments.iter().all(|arg| function.dfg.is_constant(*arg)) {
+        Ok(false)
+    } else {
+        let location = function.dfg.get_location(&instruction);
+        Err(RuntimeError::AssertConstantFailed { location })
+    }
+}

crates/noirc_evaluator/src/ssa/opt/constant_folding.rs

@@ -50,7 +50,7 @@ struct Context {
 
 impl Context {
     fn fold_constants_in_block(&mut self, function: &mut Function, block: BasicBlockId) {
-        let instructions = std::mem::take(function.dfg[block].instructions_mut());
+        let instructions = function.dfg[block].take_instructions();
 
         for instruction in instructions {
             self.push_instruction(function, block, instruction);

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

@@ -4,6 +4,7 @@
 //! simpler form until the IR only has a single function remaining with 1 block within it.
 //! Generally, these passes are also expected to minimize the final amount of instructions.
 mod array_use;
+mod assert_constant;
 mod constant_folding;
 mod defunctionalize;
 mod die;

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

@@ -537,7 +537,14 @@ impl<'a> FunctionContext<'a> {
     /// version, as it is only needed for recursion.
     pub(super) fn extract_current_value(&mut self, lvalue: &ast::LValue) -> LValue {
         match lvalue {
-            ast::LValue::Ident(ident) => LValue::Ident(self.ident_lvalue(ident)),
+            ast::LValue::Ident(ident) => {
+                let (reference, should_auto_deref) = self.ident_lvalue(ident);
+                if should_auto_deref {
+                    LValue::Dereference { reference }
+                } else {
+                    LValue::Ident
+                }
+            }
             ast::LValue::Index { array, index, .. } => self.index_lvalue(array, index).2,
             ast::LValue::MemberAccess { object, field_index } => {
                 let (old_object, object_lvalue) = self.extract_current_value_recursive(object);
@@ -551,18 +558,19 @@ impl<'a> FunctionContext<'a> {
         }
     }
 
-    pub(super) fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values {
+    fn dereference_lvalue(&mut self, values: &Values, element_type: &ast::Type) -> Values {
         let element_types = Self::convert_type(element_type);
         values.map_both(element_types, |value, element_type| {
-            let reference = value.eval(self);
+            let reference = value.eval_reference();
             self.builder.insert_load(reference, element_type).into()
         })
     }
 
-    /// Compile the given identifier as a reference - ie. avoid calling .eval()
-    fn ident_lvalue(&self, ident: &ast::Ident) -> Values {
+    /// Compile the given identifier as a reference - ie. avoid calling .eval().
+    /// Returns the variable's value and whether the variable is mutable.
+    fn ident_lvalue(&self, ident: &ast::Ident) -> (Values, bool) {
         match &ident.definition {
-            ast::Definition::Local(id) => self.lookup(*id),
+            ast::Definition::Local(id) => (self.lookup(*id), ident.mutable),
             other => panic!("Unexpected definition found for mutable value: {other}"),
         }
     }
@@ -585,8 +593,13 @@ impl<'a> FunctionContext<'a> {
     fn extract_current_value_recursive(&mut self, lvalue: &ast::LValue) -> (Values, LValue) {
         match lvalue {
             ast::LValue::Ident(ident) => {
-                let variable = self.ident_lvalue(ident);
-                (variable.clone(), LValue::Ident(variable))
+                let (variable, should_auto_deref) = self.ident_lvalue(ident);
+                if should_auto_deref {
+                    let dereferenced = self.dereference_lvalue(&variable, &ident.typ);
+                    (dereferenced, LValue::Dereference { reference: variable })
+                } else {
+                    (variable.clone(), LValue::Ident)
+                }
             }
             ast::LValue::Index { array, index, element_type, location } => {
                 let (old_array, index, index_lvalue) = self.index_lvalue(array, index);
@@ -601,7 +614,7 @@ impl<'a> FunctionContext<'a> {
             }
             ast::LValue::Dereference { reference, element_type } => {
                 let (reference, _) = self.extract_current_value_recursive(reference);
-                let dereferenced = self.dereference(&reference, element_type);
+                let dereferenced = self.dereference_lvalue(&reference, element_type);
                 (dereferenced, LValue::Dereference { reference })
             }
         }
@@ -614,7 +627,7 @@ impl<'a> FunctionContext<'a> {
     /// `extract_current_value` for more details.
     pub(super) fn assign_new_value(&mut self, lvalue: LValue, new_value: Values) {
         match lvalue {
-            LValue::Ident(references) => self.assign(references, new_value),
+            LValue::Ident => unreachable!("Cannot assign to a variable without a reference"),
             LValue::Index { old_array: mut array, index, array_lvalue } => {
                 let element_size = self.builder.field_constant(self.element_size(array));
 
@@ -811,7 +824,7 @@ impl SharedContext {
 /// Used to remember the results of each step of extracting a value from an ast::LValue
 #[derive(Debug)]
 pub(super) enum LValue {
-    Ident(Values),
+    Ident,
     Index { old_array: ValueId, index: ValueId, array_lvalue: Box<LValue> },
     MemberAccess { old_object: Values, index: usize, object_lvalue: Box<LValue> },
     Dereference { reference: Values },

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

@@ -215,6 +215,14 @@ impl<'a> FunctionContext<'a> {
         }
     }
 
+    fn dereference(&mut self, values: &Values, element_type: &ast::Type) -> Values {
+        let element_types = Self::convert_type(element_type);
+        values.map_both(element_types, |value, element_type| {
+            let reference = value.eval(self);
+            self.builder.insert_load(reference, element_type).into()
+        })
+    }
+
     fn codegen_reference(&mut self, expr: &Expression) -> Values {
         match expr {
             Expression::Ident(ident) => self.codegen_ident_reference(ident),

noir_stdlib/src/lib.nr

@@ -27,3 +27,8 @@ unconstrained fn println<T>(input: T) {
 
 #[foreign(recursive_aggregation)]
 fn verify_proof<N>(_verification_key : [Field], _proof : [Field], _public_inputs : [Field], _key_hash : Field, _input_aggregation_object : [Field; N]) -> [Field; N] {}
+
+// Asserts that the given value is known at compile-time.
+// Useful for debugging for-loop bounds.
+#[builtin(assert_constant)]
+fn assert_constant<T>(_x: T) {}