fix!: Change serialization of struct field order to match the user de…

…fined order (#1166)

* Change struct field order to the user defined order

* Remove remnants of old test_data folder

* Fix test

* Fix tests

* move poseidon tests to nargo

* Update crates/noirc_abi/src/input_parser/mod.rs

Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>

* add simple example of using structs which will fail

* chore: remove unused import

* fix: respect struct ordering when abi encoding

* chore: run only `struct_inputs_simple` test

* chore: update from `constrain` to `assert`

* fix: maintain field ordering when generating struct witnesses

* chore: run all tests

* fix: fix serialisation test

* chore: nit refactor

* chore: fix nits

* chore: switch to using `.remove`

* fix: prevent panics on nested structs

* chore: replace `into_iter()` with equivalent `iter()`

* chore: rename test to make it clearer what it's testing

---------

Co-authored-by: Kevaundray Wedderburn <kevtheappdev@gmail.com>
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
Co-authored-by: Tom French <tom@tomfren.ch>

crates/nargo_cli/src/cli/check_cmd.rs

@@ -100,7 +100,7 @@ fn create_input_toml_template(
 
 #[cfg(test)]
 mod tests {
-    use std::{collections::BTreeMap, path::PathBuf};
+    use std::path::PathBuf;
 
     use noirc_abi::{AbiParameter, AbiType, AbiVisibility, Sign};
     use noirc_driver::CompileOptions;
@@ -123,13 +123,13 @@ mod tests {
             typed_param(
                 "d",
                 AbiType::Struct {
-                    fields: BTreeMap::from([
+                    fields: vec![
                         (String::from("d1"), AbiType::Field),
                         (
                             String::from("d2"),
                             AbiType::Array { length: 3, typ: Box::new(AbiType::Field) },
                         ),
-                    ]),
+                    ],
                 },
             ),
             typed_param("e", AbiType::Boolean),

crates/nargo_cli/tests/test_data/struct_fields_ordering/Nargo.toml

@@ -0,0 +1,5 @@
+[package]
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]

crates/nargo_cli/tests/test_data/struct_fields_ordering/Prover.toml

@@ -0,0 +1,3 @@
+[y]
+foo = "5"
+bar = "7"

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

@@ -0,0 +1,14 @@
+use dep::std;
+
+// Note that fields are not in alphabetical order.
+// We want to check that this ordering is maintained
+struct myStruct {
+    foo: u32,
+    bar: Field,
+}
+
+fn main(y : pub myStruct) {
+    assert(y.foo == 5);
+    assert(y.bar == 7);
+}
+

crates/noirc_abi/src/input_parser/mod.rs

@@ -50,9 +50,12 @@ impl InputValue {
                 if map.len() != fields.len() {
                     return false;
                 }
+
+                let field_types = BTreeMap::from_iter(fields.iter().cloned());
+
                 // Check that all of the struct's fields' values match the ABI as well.
                 map.iter().all(|(field_name, field_value)| {
-                    if let Some(field_type) = fields.get(field_name) {
+                    if let Some(field_type) = field_types.get(field_name) {
                         field_value.matches_abi(field_type)
                     } else {
                         false
@@ -137,13 +140,13 @@ mod serialization_tests {
                 AbiParameter {
                     name: "bar".into(),
                     typ: AbiType::Struct {
-                        fields: BTreeMap::from([
+                        fields: vec![
                             ("field1".into(), AbiType::Integer { sign: Sign::Unsigned, width: 8 }),
                             (
                                 "field2".into(),
                                 AbiType::Array { length: 2, typ: Box::new(AbiType::Boolean) },
                             ),
-                        ]),
+                        ],
                     },
                     visibility: AbiVisibility::Private,
                 },

crates/noirc_abi/src/lib.rs

@@ -59,7 +59,7 @@ pub enum AbiType {
             serialize_with = "serialization::serialize_struct_fields",
             deserialize_with = "serialization::deserialize_struct_fields"
         )]
-        fields: BTreeMap<String, AbiType>,
+        fields: Vec<(String, AbiType)>,
     },
     String {
         length: u64,
@@ -249,7 +249,7 @@ impl Abi {
                     return Err(AbiError::TypeMismatch { param, value });
                 }
 
-                Self::encode_value(value).map(|v| (param_name, v))
+                Self::encode_value(value, &expected_type).map(|v| (param_name, v))
             })
             .collect::<Result<_, _>>()?;
 
@@ -275,7 +275,7 @@ impl Abi {
                         value: return_value,
                     });
                 }
-                let encoded_return_fields = Self::encode_value(return_value)?;
+                let encoded_return_fields = Self::encode_value(return_value, return_type)?;
 
                 // We need to be more careful when writing the return value's witness values.
                 // This is as it may share witness indices with other public inputs so we must check that when
@@ -300,7 +300,7 @@ impl Abi {
         Ok(witness_map.into())
     }
 
-    fn encode_value(value: InputValue) -> Result<Vec<FieldElement>, AbiError> {
+    fn encode_value(value: InputValue, abi_type: &AbiType) -> Result<Vec<FieldElement>, AbiError> {
         let mut encoded_value = Vec::new();
         match value {
             InputValue::Field(elem) => encoded_value.push(elem),
@@ -310,11 +310,14 @@ impl Abi {
                     string.bytes().map(|byte| FieldElement::from_be_bytes_reduce(&[byte]));
                 encoded_value.extend(str_as_fields);
             }
-            InputValue::Struct(object) => {
-                for value in object.into_values() {
-                    encoded_value.extend(Self::encode_value(value)?);
+            InputValue::Struct(object) => match abi_type {
+                AbiType::Struct { fields } => {
+                    for (field, typ) in fields {
+                        encoded_value.extend(Self::encode_value(object[field].clone(), typ)?);
+                    }
                 }
-            }
+                _ => unreachable!("value should have already been checked to match abi type"),
+            },
         }
         Ok(encoded_value)
     }

crates/noirc_abi/src/serialization.rs

@@ -1,5 +1,5 @@
+use iter_extended::vecmap;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
-use std::collections::BTreeMap;
 
 use crate::AbiType;
 
@@ -19,34 +19,30 @@ struct StructField {
 }
 
 pub(crate) fn serialize_struct_fields<S>(
-    fields: &BTreeMap<String, AbiType>,
+    fields: &[(String, AbiType)],
     s: S,
 ) -> Result<S::Ok, S::Error>
 where
     S: Serializer,
 {
-    let fields_vector: Vec<StructField> = fields
-        .iter()
-        .map(|(name, typ)| StructField { name: name.to_owned(), typ: typ.to_owned() })
-        .collect();
+    let fields_vector =
+        vecmap(fields, |(name, typ)| StructField { name: name.to_owned(), typ: typ.to_owned() });
+
     fields_vector.serialize(s)
 }
 
 pub(crate) fn deserialize_struct_fields<'de, D>(
     deserializer: D,
-) -> Result<BTreeMap<String, AbiType>, D::Error>
+) -> Result<Vec<(String, AbiType)>, D::Error>
 where
     D: Deserializer<'de>,
 {
     let fields_vector = Vec::<StructField>::deserialize(deserializer)?;
-    let fields = fields_vector.into_iter().map(|StructField { name, typ }| (name, typ)).collect();
-    Ok(fields)
+    Ok(vecmap(fields_vector, |StructField { name, typ }| (name, typ)))
 }
 
 #[cfg(test)]
 mod tests {
-    use std::collections::BTreeMap;
-
     use crate::{AbiParameter, AbiType, AbiVisibility, Sign};
 
     #[test]
@@ -123,13 +119,13 @@ mod tests {
         let expected_struct = AbiParameter {
             name: "thing3".to_string(),
             typ: AbiType::Struct {
-                fields: BTreeMap::from([
+                fields: vec![
                     ("field1".to_string(), AbiType::Integer { sign: Sign::Unsigned, width: 3 }),
                     (
                         "field2".to_string(),
                         AbiType::Array { length: 2, typ: Box::new(AbiType::Field) },
                     ),
-                ]),
+                ],
             },
             visibility: AbiVisibility::Private,
         };

crates/noirc_evaluator/src/lib.rs

@@ -17,7 +17,7 @@ use acvm::{
     Language,
 };
 use errors::{RuntimeError, RuntimeErrorKind};
-use iter_extended::btree_map;
+use iter_extended::vecmap;
 use noirc_abi::{Abi, AbiType, AbiVisibility};
 use noirc_frontend::monomorphization::ast::*;
 use ssa::{node::ObjectType, ssa_gen::IrGenerator};
@@ -220,7 +220,7 @@ impl Evaluator {
                 vec![witness]
             }
             AbiType::Struct { fields } => {
-                let new_fields = btree_map(fields, |(inner_name, value)| {
+                let new_fields = vecmap(fields, |(inner_name, value)| {
                     let new_name = format!("{name}.{inner_name}");
                     (new_name, value.clone())
                 });
@@ -229,7 +229,44 @@ impl Evaluator {
                 self.generate_struct_witnesses(&mut struct_witnesses, &new_fields)?;
 
                 ir_gen.abi_struct(name, Some(def), fields, &struct_witnesses);
-                struct_witnesses.values().flatten().copied().collect()
+
+                // This is a dirty hack and should be removed in future.
+                //
+                // `struct_witnesses` is a flat map where structs are represented by multiple entries
+                // i.e. a struct `foo` with fields `bar` and `baz` is stored under the keys
+                // `foo.bar` and `foo.baz` each holding the witnesses for fields `bar` and `baz` respectively.
+                //
+                // We've then lost the information on ordering of these fields. To reconstruct this we iterate
+                // over `fields` recursively to calculate the proper ordering of this `BTreeMap`s keys.
+                //
+                // Ideally we wouldn't lose this information in the first place.
+                fn get_field_ordering(prefix: String, fields: &[(String, AbiType)]) -> Vec<String> {
+                    fields
+                        .iter()
+                        .flat_map(|(field_name, field_type)| {
+                            let flattened_name = format!("{prefix}.{field_name}");
+                            if let AbiType::Struct { fields } = field_type {
+                                get_field_ordering(flattened_name, fields)
+                            } else {
+                                vec![flattened_name]
+                            }
+                        })
+                        .collect()
+                }
+                let field_ordering = get_field_ordering(name.to_owned(), fields);
+
+                // We concatenate the witness vectors in the order of the struct's fields.
+                // This ensures that struct fields are mapped to the correct witness indices during ABI encoding.
+                field_ordering
+                    .iter()
+                    .flat_map(|field_name| {
+                        struct_witnesses.remove(field_name).unwrap_or_else(|| {
+                            unreachable!(
+                                "Expected a field named '{field_name}' in the struct pattern"
+                            )
+                        })
+                    })
+                    .collect()
             }
             AbiType::String { length } => {
                 let typ = AbiType::Integer { sign: noirc_abi::Sign::Unsigned, width: 8 };
@@ -250,7 +287,7 @@ impl Evaluator {
     fn generate_struct_witnesses(
         &mut self,
         struct_witnesses: &mut BTreeMap<String, Vec<Witness>>,
-        fields: &BTreeMap<String, AbiType>,
+        fields: &[(String, AbiType)],
     ) -> Result<(), RuntimeErrorKind> {
         for (name, typ) in fields {
             match typ {
@@ -273,11 +310,10 @@ impl Evaluator {
                     struct_witnesses.insert(name.clone(), internal_arr_witnesses);
                 }
                 AbiType::Struct { fields, .. } => {
-                    let mut new_fields: BTreeMap<String, AbiType> = BTreeMap::new();
-                    for (inner_name, value) in fields {
-                        let new_name = format!("{name}.{inner_name}");
-                        new_fields.insert(new_name, value.clone());
-                    }
+                    let new_fields = vecmap(fields, |(field_name, typ)| {
+                        let new_name = format!("{name}.{field_name}");
+                        (new_name, typ.clone())
+                    });
                     self.generate_struct_witnesses(struct_witnesses, &new_fields)?;
                 }
                 AbiType::String { length } => {

crates/noirc_evaluator/src/ssa/ssa_gen.rs

@@ -124,7 +124,7 @@ impl IrGenerator {
         &mut self,
         struct_name: &str,
         ident_def: Option<Definition>,
-        fields: &BTreeMap<String, noirc_abi::AbiType>,
+        fields: &[(String, noirc_abi::AbiType)],
         witnesses: &BTreeMap<String, Vec<Witness>>,
     ) -> Value {
         let values = vecmap(fields, |(name, field_typ)| {

crates/noirc_frontend/src/hir/def_collector/dc_crate.rs

@@ -19,7 +19,7 @@ use fm::FileId;
 use iter_extended::vecmap;
 use noirc_errors::Span;
 use noirc_errors::{CustomDiagnostic, FileDiagnostic};
-use std::collections::{BTreeMap, HashMap};
+use std::collections::HashMap;
 use std::rc::Rc;
 
 /// Stores all of the unresolved functions in a particular file/mod
@@ -346,7 +346,7 @@ fn resolve_struct_fields(
     krate: CrateId,
     unresolved: UnresolvedStruct,
     all_errors: &mut Vec<FileDiagnostic>,
-) -> (Generics, BTreeMap<Ident, Type>) {
+) -> (Generics, Vec<(Ident, Type)>) {
     let path_resolver =
         StandardPathResolver::new(ModuleId { local_id: unresolved.module_id, krate });
 

crates/noirc_frontend/src/hir/resolution/resolver.rs

@@ -18,7 +18,7 @@ use crate::hir_def::expr::{
     HirMethodCallExpression, HirPrefixExpression,
 };
 use crate::token::Attribute;
-use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
+use std::collections::{HashMap, HashSet};
 use std::rc::Rc;
 
 use crate::graph::CrateId;
@@ -567,17 +567,13 @@ impl<'a> Resolver<'a> {
     pub fn resolve_struct_fields(
         mut self,
         unresolved: NoirStruct,
-    ) -> (Generics, BTreeMap<Ident, Type>, Vec<ResolverError>) {
+    ) -> (Generics, Vec<(Ident, Type)>, Vec<ResolverError>) {
         let generics = self.add_generics(&unresolved.generics);
 
         // Check whether the struct definition has globals in the local module and add them to the scope
         self.resolve_local_globals();
 
-        let fields = unresolved
-            .fields
-            .into_iter()
-            .map(|(ident, typ)| (ident, self.resolve_type(typ)))
-            .collect();
+        let fields = vecmap(unresolved.fields, |(ident, typ)| (ident, self.resolve_type(typ)));
 
         (generics, fields, self.errors)
     }
@@ -1094,7 +1090,7 @@ impl<'a> Resolver<'a> {
     ) -> Vec<(Ident, U)> {
         let mut ret = Vec::with_capacity(fields.len());
         let mut seen_fields = HashSet::new();
-        let mut unseen_fields = self.get_field_names_of_type(&struct_type);
+        let mut unseen_fields = struct_type.borrow().field_names();
 
         for (field, expr) in fields {
             let resolved = resolve_function(self, expr);
@@ -1131,10 +1127,6 @@ impl<'a> Resolver<'a> {
         self.interner.get_struct(type_id)
     }
 
-    fn get_field_names_of_type(&self, typ: &Shared<StructType>) -> BTreeSet<Ident> {
-        typ.borrow().field_names()
-    }
-
     fn lookup<T: TryFromModuleDefId>(&mut self, path: Path) -> Result<T, ResolverError> {
         let span = path.span();
         let id = self.resolve_path(path)?;

crates/noirc_frontend/src/hir/type_check/expr.rs

@@ -427,9 +427,10 @@ impl<'interner> TypeChecker<'interner> {
         // Note that we use a Vec to store the original arguments (rather than a BTreeMap) to
         // preserve the evaluation order of the source code.
         let mut args = constructor.fields;
-        args.sort_by_key(|arg| arg.0.clone());
+        args.sort_by_key(|(name, _)| name.clone());
 
-        let fields = typ.borrow().get_fields(&generics);
+        let mut fields = typ.borrow().get_fields(&generics);
+        fields.sort_by_key(|(name, _)| name.clone());
 
         for ((param_name, param_type), (arg_ident, arg)) in fields.into_iter().zip(args) {
             // This can be false if the user provided an incorrect field count. That error should

crates/noirc_frontend/src/hir/type_check/stmt.rs

@@ -81,15 +81,13 @@ impl<'interner> TypeChecker<'interner> {
                 });
 
                 if let Type::Struct(struct_type, generics) = struct_type {
-                    let mut pattern_fields = fields.clone();
-                    pattern_fields.sort_by_key(|(ident, _)| ident.clone());
                     let struct_type = struct_type.borrow();
 
-                    for (field_name, field_pattern) in pattern_fields {
+                    for (field_name, field_pattern) in fields {
                         if let Some((type_field, _)) =
                             struct_type.get_field(&field_name.0.contents, generics)
                         {
-                            self.bind_pattern(&field_pattern, type_field);
+                            self.bind_pattern(field_pattern, type_field);
                         }
                     }
                 }