Merge branch 'master' into e/ssa_error

crates/nargo/src/ops/foreign_calls.rs

@@ -3,7 +3,7 @@ use acvm::{
     pwg::ForeignCallWaitInfo,
 };
 use iter_extended::vecmap;
-use noirc_abi::{decode_string_value, decode_value, input_parser::json::JsonTypes, AbiType};
+use noirc_abi::{decode_string_value, input_parser::InputValueDisplay, AbiType};
 
 use crate::errors::ForeignCallError;
 
@@ -68,11 +68,11 @@ impl ForeignCall {
         // We must use a flat map here as each value in a struct will be in a separate input value
         let mut input_values_as_fields =
             input_values.iter().flat_map(|values| values.iter().map(|value| value.to_field()));
-        let decoded_value = decode_value(&mut input_values_as_fields, &abi_type)?;
 
-        let json_value = JsonTypes::try_from_input_value(&decoded_value, &abi_type)?;
+        let input_value_display =
+            InputValueDisplay::try_from_fields(&mut input_values_as_fields, abi_type)?;
 
-        println!("{json_value}");
+        println!("{input_value_display}");
         Ok(())
     }
 }

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

@@ -30,6 +30,8 @@ fn main(mut x: Field) {
     };
     *c.bar.array = [3, 4];
     assert(*c.bar.array == [3, 4]);
+
+    regression_1887();
 }
 
 fn add1(x: &mut Field) {
@@ -58,3 +60,20 @@ impl S {
 fn mutate_copy(mut a: Field) {
     a = 7;
 }
+
+// Previously the `foo.bar` in `foo.bar.mutate()` would insert an automatic dereference
+// of `foo` which caused the method to wrongly be mutating a copy of bar rather than the original.
+fn regression_1887() {
+    let foo = &mut Foo { bar: Bar { x: 0 } };
+    foo.bar.mutate();
+    assert(foo.bar.x == 32);
+}
+
+struct Foo { bar: Bar }
+struct Bar { x: Field }
+
+impl Bar {
+    fn mutate(&mut self) {
+        self.x = 32;
+    }
+}

crates/noirc_abi/src/input_parser/json.rs

@@ -59,7 +59,7 @@ pub(crate) fn serialize_to_json(
 
 #[derive(Debug, Deserialize, Serialize, Clone)]
 #[serde(untagged)]
-pub enum JsonTypes {
+pub(super) enum JsonTypes {
     // This is most likely going to be a hex string
     // But it is possible to support UTF-8
     String(String),
@@ -78,7 +78,7 @@ pub enum JsonTypes {
 }
 
 impl JsonTypes {
-    pub fn try_from_input_value(
+    pub(super) fn try_from_input_value(
         value: &InputValue,
         abi_type: &AbiType,
     ) -> Result<JsonTypes, InputParserError> {

crates/noirc_abi/src/input_parser/mod.rs

@@ -1,13 +1,13 @@
-pub mod json;
+mod json;
 mod toml;
 
 use std::collections::BTreeMap;
 
 use acvm::FieldElement;
 use serde::Serialize;
 
-use crate::errors::InputParserError;
-use crate::{Abi, AbiType};
+use crate::errors::{AbiError, InputParserError};
+use crate::{decode_value, Abi, AbiType};
 /// This is what all formats eventually transform into
 /// For example, a toml file will parse into TomlTypes
 /// and those TomlTypes will be mapped to Value
@@ -67,6 +67,35 @@ impl InputValue {
     }
 }
 
+/// In order to display an `InputValue` we need an `AbiType` to accurately
+/// convert the value into a human-readable format.
+pub struct InputValueDisplay {
+    input_value: InputValue,
+    abi_type: AbiType,
+}
+
+impl InputValueDisplay {
+    pub fn try_from_fields(
+        field_iterator: &mut impl Iterator<Item = FieldElement>,
+        abi_type: AbiType,
+    ) -> Result<Self, AbiError> {
+        let input_value = decode_value(field_iterator, &abi_type)?;
+        Ok(InputValueDisplay { input_value, abi_type })
+    }
+}
+
+impl std::fmt::Display for InputValueDisplay {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // From the docs: https://doc.rust-lang.org/std/fmt/struct.Error.html
+        // This type does not support transmission of an error other than that an error
+        // occurred. Any extra information must be arranged to be transmitted through
+        // some other means.
+        let json_value = json::JsonTypes::try_from_input_value(&self.input_value, &self.abi_type)
+            .map_err(|_| std::fmt::Error)?;
+        write!(f, "{}", serde_json::to_string(&json_value).map_err(|_| std::fmt::Error)?)
+    }
+}
+
 /// The different formats that are supported when parsing
 /// the initial witness values
 #[cfg_attr(test, derive(strum_macros::EnumIter))]

crates/noirc_abi/src/lib.rs

@@ -368,7 +368,7 @@ impl Abi {
     }
 }
 
-pub fn decode_value(
+fn decode_value(
     field_iterator: &mut impl Iterator<Item = FieldElement>,
     value_type: &AbiType,
 ) -> Result<InputValue, AbiError> {

crates/noirc_evaluator/src/ssa_refactor.rs

@@ -77,10 +77,16 @@ pub fn create_circuit(
     show_output: bool,
 ) -> Result<(Circuit, DebugInfo, Abi), RuntimeError> {
     let func_sig = program.main_function_signature.clone();
-    let GeneratedAcir { current_witness_index, opcodes, return_witnesses, locations, .. } =
-        optimize_into_acir(program, show_output, enable_ssa_logging, enable_brillig_logging)?;
+    let GeneratedAcir {
+        current_witness_index,
+        opcodes,
+        return_witnesses,
+        locations,
+        input_witnesses,
+        ..
+    } = optimize_into_acir(program, show_output, enable_ssa_logging, enable_brillig_logging)?;
 
-    let abi = gen_abi(func_sig, return_witnesses.clone());
+    let abi = gen_abi(func_sig, &input_witnesses, return_witnesses.clone());
     let public_abi = abi.clone().public_abi();
 
     let public_parameters =

crates/noirc_evaluator/src/ssa_refactor/abi_gen/mod.rs

@@ -1,29 +1,36 @@
 use std::collections::BTreeMap;
 
 use acvm::acir::native_types::Witness;
-use iter_extended::{btree_map, vecmap};
+use iter_extended::btree_map;
 use noirc_abi::{Abi, AbiParameter, FunctionSignature};
 
 /// Arranges a function signature and a generated circuit's return witnesses into a
 /// `noirc_abi::Abi`.
-pub(crate) fn gen_abi(func_sig: FunctionSignature, return_witnesses: Vec<Witness>) -> Abi {
+pub(crate) fn gen_abi(
+    func_sig: FunctionSignature,
+    input_witnesses: &[Witness],
+    return_witnesses: Vec<Witness>,
+) -> Abi {
     let (parameters, return_type) = func_sig;
-    let param_witnesses = param_witnesses_from_abi_param(&parameters);
+    let param_witnesses = param_witnesses_from_abi_param(&parameters, input_witnesses);
     Abi { parameters, return_type, param_witnesses, return_witnesses }
 }
 
 // Takes each abi parameter and shallowly maps to the expected witness range in which the
 // parameter's constituent values live.
 fn param_witnesses_from_abi_param(
     abi_params: &Vec<AbiParameter>,
+    input_witnesses: &[Witness],
 ) -> BTreeMap<String, Vec<Witness>> {
-    let mut offset = 1;
+    let mut idx = 0_usize;
+
     btree_map(abi_params, |param| {
         let num_field_elements_needed = param.typ.field_count();
-        let idx_start = offset;
-        let idx_end = idx_start + num_field_elements_needed;
-        let witnesses = vecmap(idx_start..idx_end, Witness);
-        offset += num_field_elements_needed;
-        (param.name.clone(), witnesses)
+        let mut wit = Vec::new();
+        for _ in 0..num_field_elements_needed {
+            wit.push(input_witnesses[idx]);
+            idx += 1;
+        }
+        (param.name.clone(), wit)
     })
 }

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

@@ -110,6 +110,26 @@ pub(crate) struct AcirContext {
 }
 
 impl AcirContext {
+    pub(crate) fn current_witness_index(&self) -> Witness {
+        self.acir_ir.current_witness_index()
+    }
+
+    pub(crate) fn extract_witness(&self, inputs: &[AcirValue]) -> Vec<u32> {
+        inputs
+            .iter()
+            .flat_map(|value| value.clone().flatten())
+            .map(|value| {
+                self.vars
+                    .get(&value.0)
+                    .expect("ICE: undeclared AcirVar")
+                    .to_expression()
+                    .to_witness()
+                    .expect("ICE - cannot extract a witness")
+                    .0
+            })
+            .collect()
+    }
+
     /// Adds a constant to the context and assigns a Variable to represent it
     pub(crate) fn add_constant(&mut self, constant: FieldElement) -> AcirVar {
         let constant_data = AcirVarData::Const(constant);
@@ -837,7 +857,8 @@ impl AcirContext {
     }
 
     /// Terminates the context and takes the resulting `GeneratedAcir`
-    pub(crate) fn finish(self) -> GeneratedAcir {
+    pub(crate) fn finish(mut self, inputs: Vec<u32>) -> GeneratedAcir {
+        self.acir_ir.input_witnesses = vecmap(inputs, Witness);
         self.acir_ir
     }
 

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

@@ -42,6 +42,9 @@ pub(crate) struct GeneratedAcir {
     /// abi's return type.
     pub(crate) return_witnesses: Vec<Witness>,
 
+    /// 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 location which generated it
     pub(crate) locations: HashMap<usize, Location>,
 

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

@@ -3,6 +3,7 @@ mod acir_ir;
 
 use std::collections::{HashMap, HashSet};
 use std::fmt::Debug;
+use std::ops::RangeInclusive;
 
 use self::acir_ir::acir_variable::{AcirContext, AcirType, AcirVar};
 use super::{
@@ -165,16 +166,15 @@ impl Context {
     ) -> Result<GeneratedAcir, RuntimeError> {
         let dfg = &main_func.dfg;
         let entry_block = &dfg[main_func.entry_block()];
-
-        self.convert_ssa_block_params(entry_block.parameters(), dfg)?;
+        let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?;
 
         for instruction_id in entry_block.instructions() {
             self.convert_ssa_instruction(*instruction_id, dfg, ssa, &brillig, allow_log_ops)?;
         }
 
         self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?;
 
-        Ok(self.acir_context.finish())
+        Ok(self.acir_context.finish(input_witness.collect()))
     }
 
     fn convert_brillig_main(
@@ -188,6 +188,7 @@ impl Context {
             let typ = dfg.type_of_value(*param_id);
             self.create_value_from_type(&typ, &mut |this, _| Ok(this.acir_context.add_variable()))
         })?;
+        let witness_inputs = self.acir_context.extract_witness(&inputs);
 
         let outputs: Vec<AcirType> =
             vecmap(main_func.returns(), |result_id| dfg.type_of_value(*result_id).into());
@@ -210,15 +211,17 @@ impl Context {
             self.acir_context.return_var(acir_var)?;
         }
 
-        Ok(self.acir_context.finish())
+        Ok(self.acir_context.finish(witness_inputs))
     }
 
     /// Adds and binds `AcirVar`s for each numeric block parameter or block parameter array element.
     fn convert_ssa_block_params(
         &mut self,
         params: &[ValueId],
         dfg: &DataFlowGraph,
-    ) -> Result<(), RuntimeError> {
+    ) -> Result<RangeInclusive<u32>, RuntimeError> {
+        // The first witness (if any) is the next one
+        let start_witness = self.acir_context.current_witness_index().0 + 1;
         for param_id in params {
             let typ = dfg.type_of_value(*param_id);
             let value = self.convert_ssa_block_param(&typ)?;
@@ -235,7 +238,8 @@ impl Context {
             }
             self.ssa_values.insert(*param_id, value);
         }
-        Ok(())
+        let end_witness = self.acir_context.current_witness_index().0;
+        Ok(start_witness..=end_witness)
     }
 
     fn convert_ssa_block_param(&mut self, param_type: &Type) -> Result<AcirValue, RuntimeError> {

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

@@ -185,7 +185,9 @@ impl<'a> FunctionContext<'a> {
                     }
                 })
             }
-            noirc_frontend::UnaryOp::Dereference => self.dereference(&rhs, &unary.result_type),
+            noirc_frontend::UnaryOp::Dereference { .. } => {
+                self.dereference(&rhs, &unary.result_type)
+            }
         }
     }
 

crates/noirc_frontend/src/ast/expression.rs

@@ -271,7 +271,14 @@ pub enum UnaryOp {
     Minus,
     Not,
     MutableReference,
-    Dereference,
+
+    /// If implicitly_added is true, this operation was implicitly added by the compiler for a
+    /// field dereference. The compiler may undo some of these implicitly added dereferences if
+    /// the reference later turns out to be needed (e.g. passing a field by reference to a function
+    /// requiring an &mut parameter).
+    Dereference {
+        implicitly_added: bool,
+    },
 }
 
 impl UnaryOp {
@@ -496,7 +503,7 @@ impl Display for UnaryOp {
             UnaryOp::Minus => write!(f, "-"),
             UnaryOp::Not => write!(f, "!"),
             UnaryOp::MutableReference => write!(f, "&mut"),
-            UnaryOp::Dereference => write!(f, "*"),
+            UnaryOp::Dereference { .. } => write!(f, "*"),
         }
     }
 }

crates/noirc_frontend/src/ast/statement.rs

@@ -456,7 +456,7 @@ impl LValue {
             })),
             LValue::Dereference(lvalue) => {
                 ExpressionKind::Prefix(Box::new(crate::PrefixExpression {
-                    operator: crate::UnaryOp::Dereference,
+                    operator: crate::UnaryOp::Dereference { implicitly_added: false },
                     rhs: lvalue.as_expression(span),
                 }))
             }