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

crates/nargo_cli/src/cli/check_cmd.rs

@@ -94,7 +94,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;
@@ -117,13 +117,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/noirc_abi/src/input_parser/mod.rs

@@ -49,9 +49,12 @@ impl InputValue {
                 if map.len() != fields.len() {
                     return false;
                 }
+
+                let field_types = fields.iter().cloned().collect::<BTreeMap<_, _>>();
+
                 // 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

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,

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,28 +19,26 @@ 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)]
@@ -123,13 +121,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/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)
     }
@@ -1076,7 +1072,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);
@@ -1113,10 +1109,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

@@ -443,9 +443,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);
                         }
                     }
                 }

crates/noirc_frontend/src/hir_def/types.rs

@@ -1,11 +1,11 @@
 use std::{
     cell::RefCell,
-    collections::{BTreeMap, BTreeSet, HashMap},
+    collections::{BTreeSet, HashMap},
     rc::Rc,
 };
 
 use crate::{hir::type_check::TypeCheckError, node_interner::NodeInterner};
-use iter_extended::{btree_map, vecmap};
+use iter_extended::vecmap;
 use noirc_abi::AbiType;
 use noirc_errors::Span;
 
@@ -112,7 +112,7 @@ pub struct StructType {
     /// Fields are ordered and private, they should only
     /// be accessed through get_field(), get_fields(), or instantiate()
     /// since these will handle applying generic arguments to fields as well.
-    fields: BTreeMap<Ident, Type>,
+    fields: Vec<(Ident, Type)>,
 
     pub generics: Generics,
     pub span: Span,
@@ -141,7 +141,7 @@ impl StructType {
         id: StructId,
         name: Ident,
         span: Span,
-        fields: BTreeMap<Ident, Type>,
+        fields: Vec<(Ident, Type)>,
         generics: Generics,
     ) -> StructType {
         StructType { id, fields, name, span, generics }
@@ -151,7 +151,7 @@ impl StructType {
     /// fields are resolved strictly after the struct itself is initially
     /// created. Therefore, this method is used to set the fields once they
     /// become known.
-    pub fn set_fields(&mut self, fields: BTreeMap<Ident, Type>) {
+    pub fn set_fields(&mut self, fields: Vec<(Ident, Type)>) {
         assert!(self.fields.is_empty());
         self.fields = fields;
     }
@@ -179,7 +179,7 @@ impl StructType {
     }
 
     /// Returns all the fields of this type, after being applied to the given generic arguments.
-    pub fn get_fields(&self, generic_args: &[Type]) -> BTreeMap<String, Type> {
+    pub fn get_fields(&self, generic_args: &[Type]) -> Vec<(String, Type)> {
         assert_eq!(self.generics.len(), generic_args.len());
 
         let substitutions = self
@@ -189,17 +189,14 @@ impl StructType {
             .map(|((old_id, old_var), new)| (*old_id, (old_var.clone(), new.clone())))
             .collect();
 
-        self.fields
-            .iter()
-            .map(|(name, typ)| {
-                let name = name.0.contents.clone();
-                (name, typ.substitute(&substitutions))
-            })
-            .collect()
+        vecmap(&self.fields, |(name, typ)| {
+            let name = name.0.contents.clone();
+            (name, typ.substitute(&substitutions))
+        })
     }
 
     pub fn field_names(&self) -> BTreeSet<Ident> {
-        self.fields.keys().cloned().collect()
+        self.fields.iter().map(|(name, _)| name.clone()).collect()
     }
 
     /// True if the given index is the same index as a generic type of this struct
@@ -1181,8 +1178,8 @@ impl Type {
             Type::Struct(def, args) => {
                 let struct_type = def.borrow();
                 let fields = struct_type.get_fields(args);
-                let abi_map = btree_map(fields, |(name, typ)| (name, typ.as_abi_type()));
-                AbiType::Struct { fields: abi_map }
+                let fields = vecmap(fields, |(name, typ)| (name, typ.as_abi_type()));
+                AbiType::Struct { fields }
             }
             Type::Tuple(_) => todo!("as_abi_type not yet implemented for tuple types"),
             Type::TypeVariable(_) => unreachable!(),

crates/noirc_frontend/src/monomorphization/mod.rs

@@ -227,10 +227,18 @@ impl<'interner> Monomorphizer<'interner> {
             }
             HirPattern::Struct(_, fields, _) => {
                 let struct_field_types = unwrap_struct_type(typ);
+                assert_eq!(struct_field_types.len(), fields.len());
 
-                for (name, field) in fields {
-                    let typ = &struct_field_types[&name.0.contents];
-                    self.parameter(field, typ, new_params);
+                let mut fields = btree_map(fields, |(name, field)| (name.0.contents, field));
+
+                // Iterate over struct_field_types since unwrap_struct_type will always
+                // return the fields in the order defined by the struct type.
+                for (field_name, field_type) in struct_field_types {
+                    let field = fields.remove(&field_name).unwrap_or_else(|| {
+                        unreachable!("Expected a field named '{field_name}' in the struct pattern")
+                    });
+
+                    self.parameter(field, &field_type, new_params);
                 }
             }
         }
@@ -460,14 +468,16 @@ impl<'interner> Monomorphizer<'interner> {
         let typ = self.interner.id_type(id);
         let field_types = unwrap_struct_type(&typ);
 
+        let field_type_map = btree_map(&field_types, |x| x.clone());
+
         // Create let bindings for each field value first to preserve evaluation order before
         // they are reordered and packed into the resulting tuple
         let mut field_vars = BTreeMap::new();
         let mut new_exprs = Vec::with_capacity(constructor.fields.len());
 
         for (field_name, expr_id) in constructor.fields {
             let new_id = self.next_local_id();
-            let field_type = field_types.get(&field_name.0.contents).unwrap();
+            let field_type = field_type_map.get(&field_name.0.contents).unwrap();
             let typ = Self::convert_type(field_type);
 
             field_vars.insert(field_name.0.contents.clone(), (new_id, typ));
@@ -481,14 +491,21 @@ impl<'interner> Monomorphizer<'interner> {
             }));
         }
 
-        let sorted_fields = vecmap(field_vars, |(name, (id, typ))| {
+        // We must ensure the tuple created from the variables here matches the order
+        // of the fields as defined in the type. To do this, we iterate over field_types,
+        // rather than field_type_map which is a sorted BTreeMap.
+        let field_idents = vecmap(field_types, |(name, _)| {
+            let (id, typ) = field_vars.remove(&name).unwrap_or_else(|| {
+                unreachable!("Expected field {name} to be present in constructor for {typ}")
+            });
+
             let definition = Definition::Local(id);
             let mutable = false;
             ast::Expression::Ident(ast::Ident { definition, mutable, location: None, name, typ })
         });
 
         // Finally we can return the created Tuple from the new block
-        new_exprs.push(ast::Expression::Tuple(sorted_fields));
+        new_exprs.push(ast::Expression::Tuple(field_idents));
         ast::Expression::Block(new_exprs)
     }
 
@@ -522,13 +539,18 @@ impl<'interner> Monomorphizer<'interner> {
             }
             HirPattern::Struct(_, patterns, _) => {
                 let fields = unwrap_struct_type(typ);
-                // We map each pattern to its respective field in a BTreeMap
-                // Fields in struct types are ordered, and doing this map guarantees we extract the correct field index
-                let patterns_map = btree_map(patterns, |(ident, pattern)| {
-                    let typ = fields[&ident.0.contents].clone();
-                    (ident.0.contents, (pattern, typ))
+                assert_eq!(patterns.len(), fields.len());
+
+                let mut patterns =
+                    btree_map(patterns, |(name, pattern)| (name.0.contents, pattern));
+
+                // We iterate through the type's fields to match the order defined in the struct type
+                let patterns_iter = fields.into_iter().map(|(field_name, field_type)| {
+                    let pattern = patterns.remove(&field_name).unwrap();
+                    (pattern, field_type)
                 });
-                self.unpack_tuple_pattern(value, patterns_map.into_values())
+
+                self.unpack_tuple_pattern(value, patterns_iter)
             }
         }
     }
@@ -998,7 +1020,7 @@ fn unwrap_tuple_type(typ: &HirType) -> Vec<HirType> {
     }
 }
 
-fn unwrap_struct_type(typ: &HirType) -> BTreeMap<String, HirType> {
+fn unwrap_struct_type(typ: &HirType) -> Vec<(String, HirType)> {
     match typ {
         HirType::Struct(def, args) => def.borrow().get_fields(args),
         HirType::TypeVariable(binding) => match &*binding.borrow() {