fix: Make def collector ordering more deterministic (#2515)

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

@@ -2,9 +2,9 @@ type Foo<T> = [T; 2];
 
 type Bar = Field;
 
-type Three = Two<u8>;
-type Two<A> = One<A, u32>;
 type One<A, B> = (A, B);
+type Two<A> = One<A, u32>;
+type Three = Two<u8>;
 
 struct MyStruct {
     foo: Bar,

crates/noirc_frontend/src/ast/mod.rs

@@ -124,17 +124,17 @@ impl std::fmt::Display for UnresolvedTypeData {
             },
             FormatString(len, elements) => write!(f, "fmt<{len}, {elements}"),
             Function(args, ret, env) => {
-                let args = vecmap(args, ToString::to_string);
+                let args = vecmap(args, ToString::to_string).join(", ");
 
                 match &env.as_ref().typ {
                     UnresolvedTypeData::Unit => {
-                        write!(f, "fn({}) -> {ret}", args.join(", "))
+                        write!(f, "fn({args}) -> {ret}")
                     }
                     UnresolvedTypeData::Tuple(env_types) => {
-                        let env_types = vecmap(env_types, |arg| ToString::to_string(&arg.typ));
-                        write!(f, "fn[{}]({}) -> {ret}", env_types.join(", "), args.join(", "))
+                        let env_types = vecmap(env_types, |arg| arg.typ.to_string()).join(", ");
+                        write!(f, "fn[{env_types}]({args}) -> {ret}")
                     }
-                    _ => unreachable!(),
+                    other => write!(f, "fn[{other}]({args}) -> {ret}"),
                 }
             }
             MutableReference(element) => write!(f, "&mut {element}"),

crates/noirc_frontend/src/graph/mod.rs

@@ -10,7 +10,7 @@ use fm::FileId;
 use rustc_hash::{FxHashMap, FxHashSet};
 use smol_str::SmolStr;
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub enum CrateId {
     Root(usize),
     Crate(usize),

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

@@ -21,7 +21,7 @@ use fm::FileId;
 use iter_extended::vecmap;
 use noirc_errors::Span;
 use noirc_errors::{CustomDiagnostic, FileDiagnostic};
-use std::collections::HashMap;
+use std::collections::{BTreeMap, HashMap};
 use std::rc::Rc;
 use std::vec;
 
@@ -69,9 +69,9 @@ pub struct DefCollector {
     pub(crate) def_map: CrateDefMap,
     pub(crate) collected_imports: Vec<ImportDirective>,
     pub(crate) collected_functions: Vec<UnresolvedFunctions>,
-    pub(crate) collected_types: HashMap<StructId, UnresolvedStruct>,
-    pub(crate) collected_type_aliases: HashMap<TypeAliasId, UnresolvedTypeAlias>,
-    pub(crate) collected_traits: HashMap<TraitId, UnresolvedTrait>,
+    pub(crate) collected_types: BTreeMap<StructId, UnresolvedStruct>,
+    pub(crate) collected_type_aliases: BTreeMap<TypeAliasId, UnresolvedTypeAlias>,
+    pub(crate) collected_traits: BTreeMap<TraitId, UnresolvedTrait>,
     pub(crate) collected_globals: Vec<UnresolvedGlobal>,
     pub(crate) collected_impls: ImplMap,
     pub(crate) collected_traits_impls: ImplMap,
@@ -80,6 +80,10 @@ pub struct DefCollector {
 /// Maps the type and the module id in which the impl is defined to the functions contained in that
 /// impl along with the generics declared on the impl itself. This also contains the Span
 /// of the object_type of the impl, used to issue an error if the object type fails to resolve.
+///
+/// Note that because these are keyed by unresolved types, the impl map is one of the few instances
+/// of HashMap rather than BTreeMap. For this reason, we should be careful not to iterate over it
+/// since it would be non-deterministic.
 type ImplMap =
     HashMap<(UnresolvedType, LocalModuleId), Vec<(UnresolvedGenerics, Span, UnresolvedFunctions)>>;
 
@@ -89,9 +93,9 @@ impl DefCollector {
             def_map,
             collected_imports: vec![],
             collected_functions: vec![],
-            collected_types: HashMap::new(),
-            collected_type_aliases: HashMap::new(),
-            collected_traits: HashMap::new(),
+            collected_types: BTreeMap::new(),
+            collected_type_aliases: BTreeMap::new(),
+            collected_traits: BTreeMap::new(),
             collected_impls: HashMap::new(),
             collected_traits_impls: HashMap::new(),
             collected_globals: vec![],
@@ -375,7 +379,7 @@ fn type_check_globals(
 /// so that expressions can access the fields of structs
 fn resolve_structs(
     context: &mut Context,
-    structs: HashMap<StructId, UnresolvedStruct>,
+    structs: BTreeMap<StructId, UnresolvedStruct>,
     crate_id: CrateId,
     errors: &mut Vec<FileDiagnostic>,
 ) {
@@ -481,7 +485,7 @@ fn take_errors_filter_self_not_resolved(resolver: Resolver<'_>) -> Vec<ResolverE
 /// so that expressions can access the elements of traits
 fn resolve_traits(
     context: &mut Context,
-    traits: HashMap<TraitId, UnresolvedTrait>,
+    traits: BTreeMap<TraitId, UnresolvedTrait>,
     crate_id: CrateId,
     errors: &mut Vec<FileDiagnostic>,
 ) {
@@ -524,7 +528,7 @@ fn resolve_struct_fields(
 
 fn resolve_type_aliases(
     context: &mut Context,
-    type_aliases: HashMap<TypeAliasId, UnresolvedTypeAlias>,
+    type_aliases: BTreeMap<TypeAliasId, UnresolvedTypeAlias>,
     crate_id: CrateId,
     all_errors: &mut Vec<FileDiagnostic>,
 ) {
@@ -546,7 +550,7 @@ fn resolve_type_aliases(
 fn resolve_impls(
     interner: &mut NodeInterner,
     crate_id: CrateId,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     collected_impls: ImplMap,
     errors: &mut Vec<FileDiagnostic>,
 ) -> Vec<(FileId, FuncId)> {
@@ -600,7 +604,7 @@ fn resolve_impls(
 fn resolve_free_functions(
     interner: &mut NodeInterner,
     crate_id: CrateId,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     collected_functions: Vec<UnresolvedFunctions>,
     self_type: Option<Type>,
     errors: &mut Vec<FileDiagnostic>,
@@ -625,7 +629,7 @@ fn resolve_free_functions(
 fn resolve_function_set(
     interner: &mut NodeInterner,
     crate_id: CrateId,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     unresolved_functions: UnresolvedFunctions,
     self_type: Option<Type>,
     impl_generics: Vec<(Rc<String>, Shared<TypeBinding>, Span)>,

crates/noirc_frontend/src/hir/def_map/mod.rs

@@ -7,7 +7,7 @@ use crate::token::{Attribute, TestScope};
 use arena::{Arena, Index};
 use fm::{FileId, FileManager};
 use noirc_errors::{FileDiagnostic, Location};
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 mod module_def;
 pub use module_def::*;
@@ -27,7 +27,7 @@ pub const MAIN_FUNCTION: &str = "main";
 // XXX: Ultimately, we want to constrain an index to be of a certain type just like in RA
 /// Lets first check if this is offered by any external crate
 /// XXX: RA has made this a crate on crates.io
-#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash)]
+#[derive(Debug, PartialEq, Eq, Copy, Clone, Hash, PartialOrd, Ord)]
 pub struct LocalModuleId(pub Index);
 
 impl LocalModuleId {
@@ -36,7 +36,7 @@ impl LocalModuleId {
     }
 }
 
-#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)]
 pub struct ModuleId {
     pub krate: CrateId,
     pub local_id: LocalModuleId,
@@ -49,7 +49,7 @@ impl ModuleId {
 }
 
 impl ModuleId {
-    pub fn module(self, def_maps: &HashMap<CrateId, CrateDefMap>) -> &ModuleData {
+    pub fn module(self, def_maps: &BTreeMap<CrateId, CrateDefMap>) -> &ModuleData {
         &def_maps[&self.krate].modules()[self.local_id.0]
     }
 }
@@ -65,7 +65,7 @@ pub struct CrateDefMap {
 
     pub(crate) krate: CrateId,
 
-    pub(crate) extern_prelude: HashMap<String, ModuleId>,
+    pub(crate) extern_prelude: BTreeMap<String, ModuleId>,
 }
 
 impl CrateDefMap {
@@ -100,7 +100,7 @@ impl CrateDefMap {
             root: LocalModuleId(root),
             modules,
             krate: crate_id,
-            extern_prelude: HashMap::new(),
+            extern_prelude: BTreeMap::new(),
         };
 
         // Now we want to populate the CrateDefMap using the DefCollector

crates/noirc_frontend/src/hir/mod.rs

@@ -9,7 +9,7 @@ use crate::hir_def::function::FuncMeta;
 use crate::node_interner::{FuncId, NodeInterner, StructId};
 use def_map::{Contract, CrateDefMap};
 use fm::FileManager;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 use self::def_map::TestFunction;
 
@@ -19,12 +19,12 @@ use self::def_map::TestFunction;
 pub struct Context {
     pub def_interner: NodeInterner,
     pub crate_graph: CrateGraph,
-    pub(crate) def_maps: HashMap<CrateId, CrateDefMap>,
+    pub(crate) def_maps: BTreeMap<CrateId, CrateDefMap>,
     pub file_manager: FileManager,
 
     /// Maps a given (contract) module id to the next available storage slot
     /// for that contract.
-    pub storage_slots: HashMap<def_map::ModuleId, StorageSlot>,
+    pub storage_slots: BTreeMap<def_map::ModuleId, StorageSlot>,
 }
 
 #[derive(Debug, Copy, Clone)]
@@ -40,10 +40,10 @@ impl Context {
     pub fn new(file_manager: FileManager, crate_graph: CrateGraph) -> Context {
         Context {
             def_interner: NodeInterner::default(),
-            def_maps: HashMap::new(),
+            def_maps: BTreeMap::new(),
             crate_graph,
             file_manager,
-            storage_slots: HashMap::new(),
+            storage_slots: BTreeMap::new(),
         }
     }
 

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

@@ -2,7 +2,7 @@ use iter_extended::partition_results;
 use noirc_errors::CustomDiagnostic;
 
 use crate::graph::CrateId;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs};
 use crate::{Ident, Path, PathKind};
@@ -52,7 +52,7 @@ impl From<PathResolutionError> for CustomDiagnostic {
 pub fn resolve_imports(
     crate_id: CrateId,
     imports_to_resolve: Vec<ImportDirective>,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
 ) -> (Vec<ResolvedImport>, Vec<(PathResolutionError, LocalModuleId)>) {
     let def_map = &def_maps[&crate_id];
 
@@ -71,7 +71,7 @@ pub fn resolve_imports(
 }
 
 pub(super) fn allow_referencing_contracts(
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     krate: CrateId,
     local_id: LocalModuleId,
 ) -> bool {
@@ -81,7 +81,7 @@ pub(super) fn allow_referencing_contracts(
 pub fn resolve_path_to_ns(
     import_directive: &ImportDirective,
     def_map: &CrateDefMap,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     allow_contracts: bool,
 ) -> PathResolution {
     let import_path = &import_directive.path.segments;
@@ -111,7 +111,7 @@ pub fn resolve_path_to_ns(
 fn resolve_path_from_crate_root(
     def_map: &CrateDefMap,
     import_path: &[Ident],
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     allow_contracts: bool,
 ) -> PathResolution {
     resolve_name_in_module(def_map, import_path, def_map.root, def_maps, allow_contracts)
@@ -121,7 +121,7 @@ fn resolve_name_in_module(
     def_map: &CrateDefMap,
     import_path: &[Ident],
     starting_mod: LocalModuleId,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     allow_contracts: bool,
 ) -> PathResolution {
     let mut current_mod = &def_map.modules[starting_mod.0];
@@ -185,7 +185,7 @@ fn resolve_path_name(import_directive: &ImportDirective) -> Ident {
 fn resolve_external_dep(
     current_def_map: &CrateDefMap,
     directive: &ImportDirective,
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     allow_contracts: bool,
 ) -> PathResolution {
     // Use extern_prelude to get the dep

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

@@ -2,7 +2,7 @@ use super::import::{
     allow_referencing_contracts, resolve_path_to_ns, ImportDirective, PathResolutionError,
 };
 use crate::Path;
-use std::collections::HashMap;
+use std::collections::BTreeMap;
 
 use crate::graph::CrateId;
 use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId};
@@ -11,7 +11,7 @@ pub trait PathResolver {
     /// Resolve the given path returning the resolved ModuleDefId.
     fn resolve(
         &self,
-        def_maps: &HashMap<CrateId, CrateDefMap>,
+        def_maps: &BTreeMap<CrateId, CrateDefMap>,
         path: Path,
     ) -> Result<ModuleDefId, PathResolutionError>;
 
@@ -34,7 +34,7 @@ impl StandardPathResolver {
 impl PathResolver for StandardPathResolver {
     fn resolve(
         &self,
-        def_maps: &HashMap<CrateId, CrateDefMap>,
+        def_maps: &BTreeMap<CrateId, CrateDefMap>,
         path: Path,
     ) -> Result<ModuleDefId, PathResolutionError> {
         resolve_path(def_maps, self.module_id, path)
@@ -52,7 +52,7 @@ impl PathResolver for StandardPathResolver {
 /// Resolve the given path to a function or a type.
 /// In the case of a conflict, functions are given priority
 pub fn resolve_path(
-    def_maps: &HashMap<CrateId, CrateDefMap>,
+    def_maps: &BTreeMap<CrateId, CrateDefMap>,
     module_id: ModuleId,
     path: Path,
 ) -> Result<ModuleDefId, PathResolutionError> {