fix: Replace non-deterministic iterations on hash maps

Cargo.toml

@@ -37,6 +37,7 @@ missing_docs = "warn"
 # https://github.com/rust-lang/rust-clippy/issues/5112
 debug_assert_with_mut_call = "warn"
 too_long_first_doc_paragraph = "warn"
+iter_over_hash_type = "warn"
 
 [workspace.dependencies]
 anyhow = "1.0.101"

hugr-core/src/extension/declarative/ops.rs

@@ -100,6 +100,10 @@ impl OperationDeclaration {
             extension_ref,
         )?;
 
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "copying independent map entries is deterministic"
+        )]
         for (k, v) in &self.misc {
             op_def.add_misc(k, v.clone());
         }

hugr-core/src/hugr.rs

@@ -11,7 +11,6 @@ pub mod validate;
 pub mod views;
 
 use std::cmp::Ordering;
-use std::collections::HashMap;
 use std::collections::VecDeque;
 use std::io;
 use std::iter;
@@ -444,17 +443,17 @@ impl Hugr {
     /// (since these must be the entry and exit blocks) and in each dataflow
     /// sibling graph (since these must be the input and output nodes).
     fn order_siblings_by_node_index(&mut self) {
-        let mut node_children: HashMap<Node, Vec<Node>> = HashMap::default();
+        let mut node_children = Vec::new();
         for node in self.nodes() {
             let mut children = self.children(node).collect_vec();
             if self.contains_dsg_or_csg(node) {
                 children[2..].sort();
             } else {
                 children.sort();
             }
-            node_children.insert(node, children);
+            node_children.push((node, children));
         }
-        for (node, children) in &node_children {
+        for (node, children) in node_children {
             self.hierarchy.detach_children(node.into_portgraph());
             for child in children {
                 self.hierarchy

hugr-core/src/hugr/hugrmut.rs

@@ -663,6 +663,10 @@ impl HugrMut for Hugr {
         // Update the optypes and metadata, taking them from the other graph.
         //
         // No need to compute each node's extensions here, as we merge `other.extensions` directly.
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "updating per-node payloads is independent for each node"
+        )]
         for (&node, &new_node) in &inserted.node_map {
             let node_pg = node.into_portgraph();
             let new_node_pg = new_node.into_portgraph();
@@ -686,6 +690,10 @@ impl HugrMut for Hugr {
         // Update the optypes and metadata, copying them from the other graph.
         //
         // No need to compute each node's extensions here, as we merge `other.extensions` directly.
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "updating per-node payloads is independent for each node"
+        )]
         for (&node, &new_node) in &inserted.node_map {
             let nodetype = other.get_optype(node);
             self.op_types

hugr-core/src/hugr/linking.rs

@@ -91,6 +91,10 @@ pub trait HugrLinking: HugrMut {
             roots.insert(other.entrypoint(), parent);
             other.set_parent(other.entrypoint(), other.module_root());
         };
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "roots are stored in a BTreeMap and child pruning is independent"
+        )]
         for (ch, dirv) in children.iter() {
             roots.insert(*ch, self.module_root());
             if matches!(dirv, NodeLinkingDirective::UseExisting(_)) {
@@ -759,10 +763,22 @@ fn check_directives<SRC: HugrView, TN: HugrNode>(
     parent: Option<TN>,
     children: &NodeLinkingDirectives<SRC::Node, TN>,
 ) -> Result<Transfers<SRC::Node, TN>, NodeLinkingError<SRC::Node, TN>> {
+    /// Returns the minimum key in `children` for which `pred` returns true, or
+    /// `None` if there is no such key.
+    ///
+    /// Helper function to avoid indeterminism in error reporting from iterating
+    /// over `children` (a `HashMap`).
+    fn min_directive_key<SN: HugrNode, TN>(
+        children: &NodeLinkingDirectives<SN, TN>,
+        mut pred: impl FnMut(SN) -> bool,
+    ) -> Option<SN> {
+        children.keys().copied().filter(|&n| pred(n)).min()
+    }
+
     if parent.is_some() {
         if other.entrypoint() == other.module_root() {
-            if let Some(c) = children.keys().next() {
-                return Err(NodeLinkingError::ChildOfEntrypoint(*c));
+            if let Some(c) = min_directive_key(children, |_| true) {
+                return Err(NodeLinkingError::ChildOfEntrypoint(c));
             }
         } else {
             let mut n = other.entrypoint();
@@ -787,10 +803,15 @@ fn check_directives<SRC: HugrView, TN: HugrNode>(
         replace: HashMap::default(),
         use_existing: HashMap::default(),
     };
-    for (&sn, dirv) in children {
-        if other.get_parent(sn) != Some(other.module_root()) {
-            return Err(NodeLinkingError::NotChildOfRoot(sn));
-        }
+    if let Some(sn) = min_directive_key(children, |sn| {
+        other.get_parent(sn) != Some(other.module_root())
+    }) {
+        return Err(NodeLinkingError::NotChildOfRoot(sn));
+    }
+    for sn in other.children(other.module_root()) {
+        let Some(dirv) = children.get(&sn) else {
+            continue;
+        };
         match dirv {
             NodeLinkingDirective::Add { replace } => {
                 for &r in replace {
@@ -814,12 +835,20 @@ fn link_by_node<SN: HugrNode, TGT: HugrLinking + ?Sized>(
 ) {
     // Resolve `use_existing` first in case the existing node is also replaced by
     // a new node (which we know will not be in RHS of any entry in `replace`).
+    #[expect(
+        clippy::iter_over_hash_type,
+        reason = "each source is replaced independently, so order cannot affect the result"
+    )]
     for (sn, tn) in transfers.use_existing {
         let copy = node_map.remove(&sn).unwrap();
         // Because of `UseExisting` we avoided adding `sn`s descendants
         debug_assert_eq!(hugr.children(copy).next(), None);
         replace_static_src(hugr, copy, tn);
     }
+    #[expect(
+        clippy::iter_over_hash_type,
+        reason = "each source is replaced independently, so order cannot affect the result"
+    )]
     for (tn, sn) in transfers.replace {
         let new_node = *node_map.get(&sn).unwrap();
         replace_static_src(hugr, tn, new_node);

hugr-core/src/hugr/patch/outline_cfg.rs

@@ -1,6 +1,6 @@
 //! Rewrite for inserting a CFG-node into the hierarchy containing a subsection
 //! of an existing CFG
-use std::collections::HashSet;
+use std::collections::BTreeSet;
 
 use itertools::Itertools;
 use thiserror::Error;
@@ -19,14 +19,14 @@ use super::{PatchHugrMut, PatchVerification};
 /// Moves some of the blocks in a Control-flow region into a new CFG-node that
 /// is the only child of a new Basic Block in the original region.
 pub struct OutlineCfg {
-    blocks: HashSet<Node>,
+    blocks: BTreeSet<Node>,
 }
 
 impl OutlineCfg {
     /// Create a new `OutlineCfg` rewrite that will move the provided blocks.
     pub fn new(blocks: impl IntoIterator<Item = Node>) -> Self {
         Self {
-            blocks: HashSet::from_iter(blocks),
+            blocks: BTreeSet::from_iter(blocks),
         }
     }
 
@@ -236,7 +236,7 @@ pub enum OutlineCfgError {
 
 #[cfg(test)]
 mod test {
-    use std::collections::HashSet;
+    use std::collections::{BTreeSet, HashSet};
 
     use crate::builder::{
         BlockBuilder, BuildError, CFGBuilder, Container, Dataflow, DataflowSubContainer,
@@ -481,7 +481,7 @@ mod test {
         cfg: Node,
         blocks: Vec<Node>,
     ) -> (Node, Node, Node) {
-        let mut other_blocks = h.children(cfg).collect::<HashSet<_>>();
+        let mut other_blocks = h.children(cfg).collect::<BTreeSet<_>>();
         assert!(blocks.iter().all(|b| other_blocks.remove(b)));
         let [new_block, new_cfg] = h.apply_patch(OutlineCfg::new(blocks.clone())).unwrap();
 

hugr-core/src/hugr/patch/replace.rs

@@ -400,6 +400,10 @@ impl<HostNode: HugrNode> PatchHugrMut for Replacement<HostNode> {
         h.remove_node(inserted_entrypoint);
 
         // 6. Transfer to keys of `transfers` children of the corresponding values.
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "adoptions move disjoint child sets, so order cannot affect the result"
+        )]
         for (new_parent, &old_parent) in &self.adoptions {
             let new_parent = node_map.get(new_parent).unwrap();
             debug_assert!(h.children(old_parent).next().is_some());
@@ -409,6 +413,10 @@ impl<HostNode: HugrNode> PatchHugrMut for Replacement<HostNode> {
         }
 
         // 7. Remove remaining nodes
+        #[expect(
+            clippy::iter_over_hash_type,
+            reason = "all nodes in this set are removed, so removal order does not affect the result"
+        )]
         for n in to_remove {
             h.remove_node(n);
         }

hugr-core/src/import.rs

@@ -30,8 +30,9 @@ use crate::{
 };
 use hugr_model::v0::table;
 use hugr_model::v0::{self as model};
+use indexmap::IndexMap;
 use itertools::{Either, Itertools};
-use rustc_hash::FxHashMap;
+use rustc_hash::{FxBuildHasher, FxHashMap};
 use serde::Deserialize as _;
 use smol_str::{SmolStr, ToSmolStr};
 use thiserror::Error;
@@ -247,7 +248,7 @@ pub(crate) fn import_described_hugr(
     let mut ctx = Context {
         module,
         hugr: Hugr::new(),
-        link_ports: FxHashMap::default(),
+        link_ports: IndexMap::default(),
         static_edges: Vec::new(),
         extensions,
         nodes: FxHashMap::default(),
@@ -287,7 +288,7 @@ struct Context<'a> {
 
     /// The ports that are part of each link. This is used to connect the ports at the end of the
     /// import process.
-    link_ports: FxHashMap<(table::RegionId, table::LinkIndex), Vec<(Node, Port)>>,
+    link_ports: IndexMap<(table::RegionId, table::LinkIndex), Vec<(Node, Port)>, FxBuildHasher>,
 
     /// Pairs of nodes that should be connected by a static edge.
     /// These are collected during the import process and connected at the end.

hugr-persistent/Cargo.toml

@@ -26,6 +26,7 @@ derive_more = { workspace = true, features = [
 ] }
 delegate.workspace = true
 itertools.workspace = true
+indexmap.workspace = true
 petgraph.workspace = true
 portgraph.workspace = true
 relrc = { workspace = true, features = ["petgraph", "serde"] }

hugr-persistent/src/persistent_hugr.rs

@@ -8,6 +8,7 @@ use hugr_core::{
     Hugr, HugrView, Node,
     hugr::patch::{Patch, simple_replace},
 };
+use indexmap::IndexMap;
 use itertools::Itertools;
 use relrc::HistoryGraph;
 
@@ -187,13 +188,10 @@ impl PersistentHugr {
     ) -> Result<CommitId, InvalidCommit> {
         // Check that `replacement` does not conflict with siblings at any of its
         // parents
-        let new_invalid_nodes = replacement
-            .subgraph()
-            .nodes()
-            .iter()
-            .map(|&PatchNode(id, node)| (id, node))
-            .into_grouping_map()
-            .collect::<BTreeSet<_>>();
+        let mut new_invalid_nodes = IndexMap::<_, BTreeSet<_>>::new();
+        for &PatchNode(id, node) in replacement.subgraph().nodes() {
+            new_invalid_nodes.entry(id).or_default().insert(node);
+        }
         for (parent, new_invalid_nodes) in new_invalid_nodes {
             let invalidation_set = self.deleted_nodes(parent).collect();
             if let Some(&node) = new_invalid_nodes.intersection(&invalidation_set).next() {
@@ -394,6 +392,10 @@ impl PersistentHugr {
                 hugr.mermaid_string()
             );
 
+            #[expect(
+                clippy::iter_over_hash_type,
+                reason = "inserting independent node mappings is order-independent"
+            )]
             for (old_node, new_node) in new_node_map {
                 let old_patch_node = PatchNode(commit_id, old_node);
                 node_map.insert(old_patch_node, new_node);

hugr-persistent/src/tests.rs

@@ -276,6 +276,10 @@ pub(crate) fn test_state_space() -> TestStateSpace {
         // and nodes in the state space
         let inv_node_map = {
             let mut inv = BTreeMap::new();
+            #[expect(
+                clippy::iter_over_hash_type,
+                reason = "insertion order of non-colliding keys does not matter for BTreeMaps"
+            )]
             for (repl_node, hugr_node) in node_map {
                 inv.insert(hugr_node, repl_node);
             }