diff --git a/Cargo.lock b/Cargo.lock index 5d6b0dac07..2dddd1159c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1171,6 +1171,7 @@ dependencies = [ "delegate", "derive_more 2.1.1", "hugr-core", + "indexmap 2.14.0", "insta", "itertools 0.14.0", "petgraph", diff --git a/Cargo.toml b/Cargo.toml index 4d547f037b..1b2299072a 100644 --- a/Cargo.toml +++ b/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" diff --git a/hugr-core/src/extension/declarative/ops.rs b/hugr-core/src/extension/declarative/ops.rs index 0987b5166b..8d89be51e2 100644 --- a/hugr-core/src/extension/declarative/ops.rs +++ b/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()); } diff --git a/hugr-core/src/hugr.rs b/hugr-core/src/hugr.rs index dcbeff4f2a..99eeb32237 100644 --- a/hugr-core/src/hugr.rs +++ b/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> = 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 diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index d8168db9ee..f36c0844a7 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/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 diff --git a/hugr-core/src/hugr/linking.rs b/hugr-core/src/hugr/linking.rs index 709ddc5959..0aea858ce6 100644 --- a/hugr-core/src/hugr/linking.rs +++ b/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( 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( ) { // 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); diff --git a/hugr-core/src/hugr/patch/outline_cfg.rs b/hugr-core/src/hugr/patch/outline_cfg.rs index e37e749dde..2735455b02 100644 --- a/hugr-core/src/hugr/patch/outline_cfg.rs +++ b/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, + blocks: BTreeSet, } impl OutlineCfg { /// Create a new `OutlineCfg` rewrite that will move the provided blocks. pub fn new(blocks: impl IntoIterator) -> 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)); } diff --git a/hugr-core/src/hugr/patch/replace.rs b/hugr-core/src/hugr/patch/replace.rs index bdd01914c5..a90efaf565 100644 --- a/hugr-core/src/hugr/patch/replace.rs +++ b/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 { 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, /// Edges from nodes in the existing Hugr that are not removed /// ([`NewEdgeSpec::src`] in Gamma\R) to inserted nodes @@ -146,7 +150,7 @@ impl NewEdgeSpec { fn check_existing_edge( &self, h: &impl HugrView, - legal_src_ancestors: &HashSet, + legal_src_ancestors: &BTreeSet, err_edge: impl Fn(Self) -> WhichEdgeSpec, ) -> Result<(), ReplaceError> { if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind @@ -202,7 +206,7 @@ impl Replacement { fn get_removed_nodes( &self, h: &impl HugrView, - ) -> Result, ReplaceError> { + ) -> Result, ReplaceError> { // 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 Replacement { )); } - 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 PatchHugrMut for Replacement { 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, trans_tgt: impl Fn(TgtNode) -> Option, err_spec: impl Fn(NewEdgeSpec) -> WhichEdgeSpec, - legal_src_ancestors: Option<&HashSet>, + legal_src_ancestors: Option<&BTreeSet>, ) -> Result<(), ReplaceError> where SrcNode: 'a + HugrNode, @@ -509,6 +522,12 @@ pub enum ReplaceError { /// 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), + /// 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`] diff --git a/hugr-core/src/hugr/views/sibling_subgraph.rs b/hugr-core/src/hugr/views/sibling_subgraph.rs index 704e5a2d5a..f4a698e81d 100644 --- a/hugr-core/src/hugr/views/sibling_subgraph.rs +++ b/hugr-core/src/hugr/views/sibling_subgraph.rs @@ -550,7 +550,7 @@ impl SiblingSubgraph { } /// 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, diff --git a/hugr-core/src/import.rs b/hugr-core/src/import.rs index 9f775f0b98..dbde9d19be 100644 --- a/hugr-core/src/import.rs +++ b/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. diff --git a/hugr-persistent/Cargo.toml b/hugr-persistent/Cargo.toml index a94bca1530..b79d8a527b 100644 --- a/hugr-persistent/Cargo.toml +++ b/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"] } diff --git a/hugr-persistent/src/persistent_hugr.rs b/hugr-persistent/src/persistent_hugr.rs index 01bbfd6bae..3ebc0e5a8c 100644 --- a/hugr-persistent/src/persistent_hugr.rs +++ b/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 { // 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::>(); + 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); diff --git a/hugr-persistent/src/tests.rs b/hugr-persistent/src/tests.rs index 045b517460..4664606c30 100644 --- a/hugr-persistent/src/tests.rs +++ b/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); }