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,16 @@ 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();
-        for node in self.nodes() {
+        // Equivalent to `self.nodes()` that does not borrow the full structure, so we can mutate `self.hierarchy` while iterating.
+        let nodes = self.graph.nodes_iter().map_into();
+        for node in 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);
-        }
-        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(_)) {
@@ -787,6 +791,10 @@ fn check_directives<SRC: HugrView, TN: HugrNode>(
         replace: HashMap::default(),
         use_existing: HashMap::default(),
     };
+    #[expect(
+        clippy::iter_over_hash_type,
+        reason = "Nondeterminism only affects the error reported."
+    )]
     for (&sn, dirv) in children {
         if other.get_parent(sn) != Some(other.module_root()) {
             return Err(NodeLinkingError::NotChildOfRoot(sn));
@@ -814,12 +822,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),
         }
     }
 
@@ -485,6 +485,7 @@ mod test {
         assert!(blocks.iter().all(|b| other_blocks.remove(b)));
         let [new_block, new_cfg] = h.apply_patch(OutlineCfg::new(blocks.clone())).unwrap();
 
+        #[expect(clippy::iter_over_hash_type, reason = "order does not matter")]
         for n in other_blocks {
             assert_eq!(h.get_parent(n), Some(cfg));
         }

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

@@ -1,6 +1,6 @@
 //! Implementation of the `Replace` operation.
 
-use std::collections::{HashMap, HashSet, VecDeque};
+use std::collections::{BTreeSet, HashMap, HashSet, VecDeque};
 
 use itertools::Itertools;
 use thiserror::Error;
@@ -71,12 +71,16 @@ pub struct Replacement<HostNode = Node> {
     pub replacement: Hugr,
     /// Describes how parts of the Hugr that would otherwise be removed should
     /// instead be preserved but with new parents amongst the newly-inserted
-    /// nodes.  This is a Map from container nodes in [`Self::replacement`]
+    /// nodes.
+    ///
+    /// This is a Map from container nodes in [`Self::replacement`]
     /// that have no children, to container nodes that are descended from
     /// [`Self::removal`]. The keys are the new parents for the children of
     /// the values.  Note no value may be ancestor or descendant of another.
     /// This is "B" in the spec; "R" is the set of descendants of
     /// [`Self::removal`]  that are not descendants of values here.
+    ///
+    /// The nodes in the values must be distinct descendants of the removed nodes.
     pub adoptions: HashMap<Node, HostNode>,
     /// Edges from nodes in the existing Hugr that are not removed
     /// ([`NewEdgeSpec::src`] in Gamma\R) to inserted nodes
@@ -146,7 +150,7 @@ impl<HostNode: HugrNode, N: Clone> NewEdgeSpec<N, HostNode> {
     fn check_existing_edge(
         &self,
         h: &impl HugrView<Node = HostNode>,
-        legal_src_ancestors: &HashSet<HostNode>,
+        legal_src_ancestors: &BTreeSet<HostNode>,
         err_edge: impl Fn(Self) -> WhichEdgeSpec<HostNode>,
     ) -> Result<(), ReplaceError<HostNode>> {
         if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind
@@ -202,7 +206,7 @@ impl<HostNode: HugrNode> Replacement<HostNode> {
     fn get_removed_nodes(
         &self,
         h: &impl HugrView<Node = HostNode>,
-    ) -> Result<HashSet<HostNode>, ReplaceError<HostNode>> {
+    ) -> Result<BTreeSet<HostNode>, ReplaceError<HostNode>> {
         // Check the keys of the transfer map too, the values we'll use imminently
         self.adoptions.keys().try_for_each(|&n| {
             (self.replacement.contains_node(n)
@@ -222,7 +226,7 @@ impl<HostNode: HugrNode> Replacement<HostNode> {
             ));
         }
 
-        let mut removed = HashSet::new();
+        let mut removed = BTreeSet::new();
         let mut queue = VecDeque::from_iter(self.removal.iter().copied());
         while let Some(n) = queue.pop_front() {
             let new = removed.insert(n);
@@ -400,9 +404,18 @@ 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. Repeated values return an error."
+        )]
         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());
+
+            if h.children(old_parent).next().is_none() {
+                // `old_parent` appeared more than once on the right-hand side of `adoptions`.
+                return Err(ReplaceError::RepeatedAdoptee { node: old_parent });
+            }
+
             while let Some(ch) = h.first_child(old_parent) {
                 h.set_parent(ch, *new_parent);
             }
@@ -422,7 +435,7 @@ fn transfer_edges<'a, SrcNode, TgtNode, HostNode>(
     trans_src: impl Fn(SrcNode) -> Option<HostNode>,
     trans_tgt: impl Fn(TgtNode) -> Option<HostNode>,
     err_spec: impl Fn(NewEdgeSpec<SrcNode, TgtNode>) -> WhichEdgeSpec<HostNode>,
-    legal_src_ancestors: Option<&HashSet<HostNode>>,
+    legal_src_ancestors: Option<&BTreeSet<HostNode>>,
 ) -> Result<(), ReplaceError<HostNode>>
 where
     SrcNode: 'a + HugrNode,
@@ -509,6 +522,12 @@ pub enum ReplaceError<HostNode = Node> {
     /// The [`NewEdgeKind`] was not applicable for the source/target node(s)
     #[error("The edge kind was not applicable to the {0:?} node: {1:?}")]
     BadEdgeKind(Direction, WhichEdgeSpec<HostNode>),
+    /// Some value in [`Replacement::adoptions`] was repeated for multiple parents. The nodes are indicated on a best-effort basis.
+    #[error("Node {node:?} adopted by multiple new parents")]
+    RepeatedAdoptee {
+        /// The node that was repeated
+        node: HostNode,
+    },
 }
 
 /// The three kinds of [`NewEdgeSpec`] that may appear in a [`ReplaceError`]

hugr-core/src/hugr/views/sibling_subgraph.rs

@@ -550,7 +550,7 @@ impl<N: HugrNode> SiblingSubgraph<N> {
     }
 
     /// Check the validity of the subgraph, as described in the docs of
-    /// [`SiblingSubgraph::try_new`], but do not check convexity.    
+    /// [`SiblingSubgraph::try_new`], but do not check convexity.
     pub fn validate_skip_convexity(
         &self,
         hugr: &impl HugrView<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);
             }