From 1723b489b2f82e644a1ada8ddac9aa3c55532c48 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 11 Aug 2025 13:28:55 +0100 Subject: [PATCH 01/26] Add HugrMut::insert_forest / insert_view_forest, implement insert_hugr/from_view --- hugr-core/src/hugr/hugrmut.rs | 103 +++++++++++++++++++-------- hugr-core/src/hugr/views/impls.rs | 7 +- hugr-core/src/hugr/views/rerooted.rs | 7 +- 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 74a6d1461d..d34179c42d 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -188,7 +188,9 @@ pub trait HugrMut: HugrMutInternals { Self::insert_region(self, root, other, region) } - /// Insert a sub-region of another hugr into this one, under a given parent node. + /// Insert a subtree of another hugr into this one, under a given parent node. + /// Edges into the inserted subtree (i.e. nonlocal or static) will be disconnected + /// in `self`. /// /// # Panics /// @@ -199,9 +201,17 @@ pub trait HugrMut: HugrMutInternals { root: Self::Node, other: Hugr, region: Node, - ) -> InsertionResult; + ) -> InsertionResult { + let node_map = self.insert_forest(other, HashMap::from([(region, root)])); + InsertionResult { + inserted_entrypoint: node_map[®ion], + node_map, + } + } - /// Copy another hugr into this one, under a given parent node. + /// Copy the entrypoint subtree of another hugr into this one, under a given parent node. + /// Edges into the inserted subtree (i.e. nonlocal or static) will be disconnected + /// in `self`. /// /// # Panics /// @@ -210,7 +220,15 @@ pub trait HugrMut: HugrMutInternals { &mut self, root: Self::Node, other: &H, - ) -> InsertionResult; + ) -> InsertionResult { + let ep = other.entrypoint(); + let node_map = + self.insert_view_forest(other, other.descendants(ep), HashMap::from([(ep, root)])); + InsertionResult { + inserted_entrypoint: node_map[&ep], + node_map, + } + } /// Copy a subgraph from another hugr into this one, under a given parent node. /// @@ -233,6 +251,41 @@ pub trait HugrMut: HugrMutInternals { subgraph: &SiblingSubgraph, ) -> HashMap; + /// Insert a forest of nodes from another Hugr into this one. + /// `root_parents` maps from roots of regions in the other Hugr to insert, + /// to the node in this Hugr that shall be parent for that region. + /// + /// # Panics + /// + /// If any of the values in `roots` are not nodes in `self`. + // TODO: Error for doubly-included subtrees + fn insert_forest( + &mut self, + other: Hugr, + root_parents: HashMap, + ) -> HashMap; + + /// Copy a forest of nodes from a view into this one. + /// `nodes` enumerates all nodes to copy; `root_parents` identifies + /// those nodes from `other` which should be placed under the given + /// parent nodes in `self`. + /// + /// Note that item in `nodes` must *either* be present in `root_parents`, + /// or follow its parent (in `other`) in the iteration order. + /// + /// # Panics + /// + /// If any of the values in `roots` are not nodes in `self`. + /// ?? If `nodes` contains any duplicate elements, or does not adhere to the ordering + /// requirement above + // TODO: turn double-inclusion into error? + fn insert_view_forest( + &mut self, + other: &H, + nodes: impl IntoIterator, + roots: HashMap, + ) -> HashMap; + /// Applies a patch to the graph. fn apply_patch(&mut self, rw: impl Patch) -> Result where @@ -412,15 +465,17 @@ impl HugrMut for Hugr { (src_port, dst_port) } - fn insert_region( + fn insert_forest( &mut self, - root: Self::Node, mut other: Hugr, - region: Node, - ) -> InsertionResult { - let node_map = insert_hugr_internal(self, &other, other.descendants(region), |&n| { - if n == region { Some(root) } else { None } - }); + roots: HashMap, + ) -> HashMap { + let node_map = insert_hugr_internal( + self, + &other, + roots.keys().flat_map(|n| other.descendants(*n)), + |k| roots.get(k).cloned(), + ); // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, taking them from the other graph. @@ -434,24 +489,17 @@ impl HugrMut for Hugr { let meta = other.metadata.take(node_pg); self.metadata.set(new_node_pg, meta); } - InsertionResult { - inserted_entrypoint: node_map[®ion], - node_map, - } + node_map } - fn insert_from_view( + fn insert_view_forest( &mut self, - root: Self::Node, other: &H, - ) -> InsertionResult { - let node_map = insert_hugr_internal(self, other, other.entry_descendants(), |&n| { - if n == other.entrypoint() { - Some(root) - } else { - None - } - }); + nodes: impl IntoIterator, + roots: HashMap, + ) -> HashMap { + let node_map = + insert_hugr_internal(self, other, nodes.into_iter(), |k| roots.get(k).cloned()); // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, copying them from the other graph. @@ -467,10 +515,7 @@ impl HugrMut for Hugr { .set(new_node.into_portgraph(), Some(meta.clone())); } } - InsertionResult { - inserted_entrypoint: node_map[&other.entrypoint()], - node_map, - } + node_map } fn insert_subgraph( diff --git a/hugr-core/src/hugr/views/impls.rs b/hugr-core/src/hugr/views/impls.rs index dbc9ad7b8f..befaa0c72e 100644 --- a/hugr-core/src/hugr/views/impls.rs +++ b/hugr-core/src/hugr/views/impls.rs @@ -1,6 +1,6 @@ //! Implementation of the core hugr traits for different wrappers of a `Hugr`. -use std::{borrow::Cow, rc::Rc, sync::Arc}; +use std::{borrow::Cow, collections::HashMap, rc::Rc, sync::Arc}; use super::HugrView; use crate::hugr::HugrMut; @@ -114,9 +114,8 @@ macro_rules! hugr_mut_methods { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_hugr(&mut self, root: Self::Node, other: crate::Hugr) -> crate::hugr::hugrmut::InsertionResult; - fn insert_region(&mut self, root: Self::Node, other: crate::Hugr, region: crate::Node) -> crate::hugr::hugrmut::InsertionResult; - fn insert_from_view(&mut self, root: Self::Node, other: &Other) -> crate::hugr::hugrmut::InsertionResult; + fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; + fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; fn insert_subgraph(&mut self, root: Self::Node, other: &Other, subgraph: &crate::hugr::views::SiblingSubgraph) -> std::collections::HashMap; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; diff --git a/hugr-core/src/hugr/views/rerooted.rs b/hugr-core/src/hugr/views/rerooted.rs index 18821cfe29..2b4a241b0b 100644 --- a/hugr-core/src/hugr/views/rerooted.rs +++ b/hugr-core/src/hugr/views/rerooted.rs @@ -1,6 +1,8 @@ //! A HUGR wrapper with a modified entrypoint node, returned by //! [`HugrView::with_entrypoint`] and [`HugrMut::with_entrypoint_mut`]. +use std::collections::HashMap; + use crate::hugr::HugrMut; use crate::hugr::internal::{HugrInternals, HugrMutInternals}; @@ -137,9 +139,8 @@ impl HugrMut for Rerooted { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_hugr(&mut self, root: Self::Node, other: crate::Hugr) -> crate::hugr::hugrmut::InsertionResult; - fn insert_region(&mut self, root: Self::Node, other: crate::Hugr, region: crate::Node) -> crate::hugr::hugrmut::InsertionResult; - fn insert_from_view(&mut self, root: Self::Node, other: &Other) -> crate::hugr::hugrmut::InsertionResult; + fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; + fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; fn insert_subgraph(&mut self, root: Self::Node, other: &Other, subgraph: &crate::hugr::views::SiblingSubgraph) -> std::collections::HashMap; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; From 2e665426a7a954caba3800b61832df359a900d1b Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 11 Aug 2025 17:19:54 +0100 Subject: [PATCH 02/26] insert_subgraph delegates to insert_forest (copying nodes list) --- hugr-core/src/hugr/hugrmut.rs | 35 ++++++---------------------- hugr-core/src/hugr/views/impls.rs | 1 - hugr-core/src/hugr/views/rerooted.rs | 1 - 3 files changed, 7 insertions(+), 30 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index d34179c42d..b0f46da518 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -249,7 +249,13 @@ pub trait HugrMut: HugrMutInternals { root: Self::Node, other: &H, subgraph: &SiblingSubgraph, - ) -> HashMap; + ) -> HashMap { + self.insert_view_forest( + other, + subgraph.nodes().iter().cloned(), + subgraph.nodes().iter().map(|n| (*n, root)).collect(), + ) + } /// Insert a forest of nodes from another Hugr into this one. /// `root_parents` maps from roots of regions in the other Hugr to insert, @@ -518,33 +524,6 @@ impl HugrMut for Hugr { node_map } - fn insert_subgraph( - &mut self, - root: Self::Node, - other: &H, - subgraph: &SiblingSubgraph, - ) -> HashMap { - let node_map = insert_hugr_internal(self, other, subgraph.nodes().iter().copied(), |_| { - Some(root) - }); - // Update the optypes and metadata, copying them from the other graph. - for (&node, &new_node) in &node_map { - let nodetype = other.get_optype(node); - self.op_types - .set(new_node.into_portgraph(), nodetype.clone()); - let meta = other.node_metadata_map(node); - if !meta.is_empty() { - self.metadata - .set(new_node.into_portgraph(), Some(meta.clone())); - } - // Add the required extensions to the registry. - if let Ok(exts) = nodetype.used_extensions() { - self.use_extensions(exts); - } - } - node_map - } - fn copy_descendants( &mut self, root: Self::Node, diff --git a/hugr-core/src/hugr/views/impls.rs b/hugr-core/src/hugr/views/impls.rs index befaa0c72e..3c17100c5c 100644 --- a/hugr-core/src/hugr/views/impls.rs +++ b/hugr-core/src/hugr/views/impls.rs @@ -116,7 +116,6 @@ macro_rules! hugr_mut_methods { fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; - fn insert_subgraph(&mut self, root: Self::Node, other: &Other, subgraph: &crate::hugr::views::SiblingSubgraph) -> std::collections::HashMap; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } diff --git a/hugr-core/src/hugr/views/rerooted.rs b/hugr-core/src/hugr/views/rerooted.rs index 2b4a241b0b..ef29e8ea2c 100644 --- a/hugr-core/src/hugr/views/rerooted.rs +++ b/hugr-core/src/hugr/views/rerooted.rs @@ -141,7 +141,6 @@ impl HugrMut for Rerooted { fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; - fn insert_subgraph(&mut self, root: Self::Node, other: &Other, subgraph: &crate::hugr::views::SiblingSubgraph) -> std::collections::HashMap; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } From 02fea7312bc734270f7916c8f6cc416f8c7f2187 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 11 Aug 2025 17:56:48 +0100 Subject: [PATCH 03/26] errors; docs --- hugr-core/src/hugr/hugrmut.rs | 93 +++++++++++++++++++++------- hugr-core/src/hugr/views/impls.rs | 6 +- hugr-core/src/hugr/views/rerooted.rs | 6 +- 3 files changed, 76 insertions(+), 29 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index b0f46da518..8ce30f20f6 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -202,7 +202,9 @@ pub trait HugrMut: HugrMutInternals { other: Hugr, region: Node, ) -> InsertionResult { - let node_map = self.insert_forest(other, HashMap::from([(region, root)])); + let node_map = self + .insert_forest(other, HashMap::from([(region, root)])) + .expect("No errors possible for single subtree"); InsertionResult { inserted_entrypoint: node_map[®ion], node_map, @@ -222,8 +224,9 @@ pub trait HugrMut: HugrMutInternals { other: &H, ) -> InsertionResult { let ep = other.entrypoint(); - let node_map = - self.insert_view_forest(other, other.descendants(ep), HashMap::from([(ep, root)])); + let node_map = self + .insert_view_forest(other, other.descendants(ep), HashMap::from([(ep, root)])) + .expect("No errors possible for single subtree"); InsertionResult { inserted_entrypoint: node_map[&ep], node_map, @@ -255,42 +258,58 @@ pub trait HugrMut: HugrMutInternals { subgraph.nodes().iter().cloned(), subgraph.nodes().iter().map(|n| (*n, root)).collect(), ) + .expect("SiblingSubgraph nodes are a set") } /// Insert a forest of nodes from another Hugr into this one. /// `root_parents` maps from roots of regions in the other Hugr to insert, /// to the node in this Hugr that shall be parent for that region. /// + /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` + /// and where each value is the corresponding (new) node in `self`. + /// + /// # Errors + /// + /// [InsertForestError::DoubleCopy] if the subtrees of the keys of `root_parents` + /// are not disjoint (the error indicates the root of the _inner_ subtree). + /// /// # Panics /// /// If any of the values in `roots` are not nodes in `self`. - // TODO: Error for doubly-included subtrees fn insert_forest( &mut self, other: Hugr, root_parents: HashMap, - ) -> HashMap; + ) -> InsertForestResult; /// Copy a forest of nodes from a view into this one. - /// `nodes` enumerates all nodes to copy; `root_parents` identifies - /// those nodes from `other` which should be placed under the given - /// parent nodes in `self`. /// - /// Note that item in `nodes` must *either* be present in `root_parents`, - /// or follow its parent (in `other`) in the iteration order. + /// `nodes` enumerates all nodes in `other` to copy; every item must *either* be + /// present in `root_parents`, or follow its parent (in `other`) in the iteration order. + /// + /// `root_parents` identifies those nodes in `nodes` which should be placed under + /// the given parent nodes in `self`. Note that unlike [Self::insert_forest] this + /// allows inserting most of a subtree in one location but with subparts of that + /// subtree placed elsewhere. + /// + /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` + /// and where each value is the corresponding (new) node in `self`. + /// + /// # Errors + /// + /// [InsertForestError::DoubleCopy] if any node appears in `nodes` more than once /// /// # Panics /// /// If any of the values in `roots` are not nodes in `self`. - /// ?? If `nodes` contains any duplicate elements, or does not adhere to the ordering - /// requirement above - // TODO: turn double-inclusion into error? + /// + /// If `nodes` does not adhere to the ordering requirement above fn insert_view_forest( &mut self, other: &H, nodes: impl IntoIterator, roots: HashMap, - ) -> HashMap; + ) -> InsertForestResult; /// Applies a patch to the graph. fn apply_patch(&mut self, rw: impl Patch) -> Result @@ -322,6 +341,22 @@ pub trait HugrMut: HugrMutInternals { ExtensionRegistry: Extend; } +/// Result of inserting a forest from a hugr of `SN` nodes, into a hugr with +/// `TN` nodes. +/// +/// On success, a map giving the new indices; or an error in the request. +/// Used by [HugrMut::insert_forest] and [HugrMut::insert_view_forest]. +pub type InsertForestResult = Result, InsertForestError>; + +/// An error from [HugrMut::insert_forest] or [HugrMut::insert_view_forest] +#[derive(Clone, Debug, derive_more::Display, derive_more::Error, PartialEq)] +#[non_exhaustive] +pub enum InsertForestError { + /// The specified source node would be copied twice into the target + #[display("Node/subtree {_0} would be copied twice")] + DoubleCopy(N), +} + /// Records the result of inserting a Hugr or view via [`HugrMut::insert_hugr`], /// [`HugrMut::insert_from_view`], or [`HugrMut::insert_region`]. /// @@ -475,13 +510,23 @@ impl HugrMut for Hugr { &mut self, mut other: Hugr, roots: HashMap, - ) -> HashMap { + ) -> Result, InsertForestError> { + for &r in roots.keys() { + let mut n = r; + while let Some(p) = other.get_parent(n) { + if roots.contains_key(&p) { + return Err(InsertForestError::DoubleCopy(r)); + } + n = p; + } + } let node_map = insert_hugr_internal( self, &other, roots.keys().flat_map(|n| other.descendants(*n)), |k| roots.get(k).cloned(), - ); + ) + .expect("Trees disjoint so no repeated nodes"); // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, taking them from the other graph. @@ -495,7 +540,7 @@ impl HugrMut for Hugr { let meta = other.metadata.take(node_pg); self.metadata.set(new_node_pg, meta); } - node_map + Ok(node_map) } fn insert_view_forest( @@ -503,9 +548,9 @@ impl HugrMut for Hugr { other: &H, nodes: impl IntoIterator, roots: HashMap, - ) -> HashMap { + ) -> Result, InsertForestError> { let node_map = - insert_hugr_internal(self, other, nodes.into_iter(), |k| roots.get(k).cloned()); + insert_hugr_internal(self, other, nodes.into_iter(), |k| roots.get(k).cloned())?; // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, copying them from the other graph. @@ -521,7 +566,7 @@ impl HugrMut for Hugr { .set(new_node.into_portgraph(), Some(meta.clone())); } } - node_map + Ok(node_map) } fn copy_descendants( @@ -597,7 +642,7 @@ fn insert_hugr_internal( other: &H, other_nodes: impl Iterator, reroot: impl Fn(&H::Node) -> Option, -) -> HashMap { +) -> Result, InsertForestError> { let new_node_count_hint = other_nodes.size_hint().1.unwrap_or_default(); // Insert the nodes from the other graph into this one. @@ -609,7 +654,9 @@ fn insert_hugr_internal( // correct optype, avoiding cloning if possible. let op = OpType::default(); let new = hugr.add_node(op); - node_map.insert(old, new); + if node_map.insert(old, new).is_some() { + return Err(InsertForestError::DoubleCopy(old)); + } hugr.set_num_ports(new, other.num_inputs(old), other.num_outputs(old)); @@ -644,7 +691,7 @@ fn insert_hugr_internal( } } } - node_map + Ok(node_map) } #[cfg(test)] diff --git a/hugr-core/src/hugr/views/impls.rs b/hugr-core/src/hugr/views/impls.rs index 3c17100c5c..3fcaea0f8b 100644 --- a/hugr-core/src/hugr/views/impls.rs +++ b/hugr-core/src/hugr/views/impls.rs @@ -3,8 +3,8 @@ use std::{borrow::Cow, collections::HashMap, rc::Rc, sync::Arc}; use super::HugrView; -use crate::hugr::HugrMut; use crate::hugr::internal::{HugrInternals, HugrMutInternals}; +use crate::hugr::{HugrMut, hugrmut::InsertForestError}; macro_rules! hugr_internal_methods { // The extra ident here is because invocations of the macro cannot pass `self` as argument @@ -114,8 +114,8 @@ macro_rules! hugr_mut_methods { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; - fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; + fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> Result, InsertForestError>; + fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> Result, InsertForestError>; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } diff --git a/hugr-core/src/hugr/views/rerooted.rs b/hugr-core/src/hugr/views/rerooted.rs index ef29e8ea2c..61e39a3316 100644 --- a/hugr-core/src/hugr/views/rerooted.rs +++ b/hugr-core/src/hugr/views/rerooted.rs @@ -3,8 +3,8 @@ use std::collections::HashMap; -use crate::hugr::HugrMut; use crate::hugr::internal::{HugrInternals, HugrMutInternals}; +use crate::hugr::{HugrMut, hugrmut::InsertForestError}; use super::{HugrView, panic_invalid_node}; @@ -139,8 +139,8 @@ impl HugrMut for Rerooted { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> HashMap; - fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> HashMap; + fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> Result, InsertForestError>; + fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> Result, InsertForestError>; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } From fdee9444e2fd36aac4e7ae3fd2a534bd3f4492e5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 12 Aug 2025 09:43:42 +0100 Subject: [PATCH 04/26] Tests --- hugr-core/src/hugr/hugrmut.rs | 180 +++++++++++++++++++++++++++++++++- 1 file changed, 175 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 8ce30f20f6..23dd5498ce 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -696,12 +696,17 @@ fn insert_hugr_internal( #[cfg(test)] mod test { + use cool_asserts::assert_matches; + use itertools::Itertools; + + use crate::builder::test::simple_dfg_hugr; + use crate::builder::{DFGBuilder, Dataflow, DataflowHugr, DataflowSubContainer, HugrBuilder}; use crate::extension::PRELUDE; - use crate::{ - extension::prelude::{Noop, usize_t}, - ops::{self, FuncDefn, Input, Output, dataflow::IOTrait}, - types::Signature, - }; + use crate::extension::prelude::{Noop, bool_t, usize_t}; + use crate::hugr::ValidationError; + use crate::ops::handle::{FuncID, NodeHandle}; + use crate::ops::{self, FuncDefn, Input, Output, dataflow::IOTrait}; + use crate::types::Signature; use super::*; @@ -780,4 +785,169 @@ mod test { hugr.validate().unwrap(); assert_eq!(hugr.num_nodes(), 1); } + + /// A DFG-entrypoint Hugr (no inputs, one bool_t output) containing two calls, + /// to a FuncDefn and a FuncDecl each bool_t->bool_t, and their handles. + pub(crate) fn dfg_calling_defn_decl() -> (Hugr, FuncID, FuncID) { + let mut dfb = DFGBuilder::new(Signature::new(vec![], bool_t())).unwrap(); + let new_defn = { + let mut mb = dfb.module_root_builder(); + let fb = mb + .define_function("helper_id", Signature::new_endo(bool_t())) + .unwrap(); + let [f_inp] = fb.input_wires_arr(); + fb.finish_with_outputs([f_inp]).unwrap() + }; + let new_decl = dfb + .module_root_builder() + .declare("helper2", Signature::new_endo(bool_t()).into()) + .unwrap(); + let cst = dfb.add_load_value(ops::Value::true_val()); + let [c1] = dfb + .call(new_defn.handle(), &[], [cst]) + .unwrap() + .outputs_arr(); + let [c2] = dfb.call(&new_decl, &[], [c1]).unwrap().outputs_arr(); + ( + dfb.finish_hugr_with_outputs([c2]).unwrap(), + *new_defn.handle(), + new_decl, + ) + } + + #[test] + fn test_insert_forest() { + // Specify which decls to transfer + for (call1, call2) in [(false, false), (false, true), (true, false), (true, true)] { + let mut h = simple_dfg_hugr(); + let (insert, defn, decl) = dfg_calling_defn_decl(); + let roots = HashMap::from_iter( + std::iter::once((insert.entrypoint(), h.entrypoint())) + .chain(call1.then_some((defn.node(), h.module_root())).into_iter()) + .chain(call2.then_some((decl.node(), h.module_root())).into_iter()), + ); + h.insert_forest(insert, roots).unwrap(); + if call1 && call2 { + h.validate().unwrap(); + } else { + assert!(matches!( + h.validate(), + Err(ValidationError::UnconnectedPort { .. }) + )); + } + assert_eq!( + h.children(h.module_root()).count(), + 1 + (call1 as usize) + (call2 as usize) + ); + let [calln1, calln2] = h + .nodes() + .filter(|n| h.get_optype(*n).is_call()) + .collect_array() + .unwrap(); + + let tgt1 = h.nodes().find(|n| { + h.get_optype(*n) + .as_func_defn() + .is_some_and(|fd| fd.func_name() == "helper_id") + }); + assert_eq!(tgt1.is_some(), call1); + assert_eq!(h.static_source(calln1), tgt1); + + let tgt2 = h.nodes().find(|n| { + h.get_optype(*n) + .as_func_decl() + .is_some_and(|fd| fd.func_name() == "helper2") + }); + assert_eq!(tgt2.is_some(), call2); + assert_eq!(h.static_source(calln2), tgt2); + } + } + + #[test] + fn test_insert_view_forest() { + let (insert, defn, decl) = dfg_calling_defn_decl(); + let mut h = simple_dfg_hugr(); + + let mut roots = HashMap::from([ + (insert.entrypoint(), h.entrypoint()), + (defn.node(), h.module_root()), + (decl.node(), h.module_root()), + ]); + + // Straightforward case: three complete subtrees + h.insert_view_forest( + &insert, + insert + .entry_descendants() + .chain(insert.descendants(defn.node())) + .chain(std::iter::once(decl.node())), + roots.clone(), + ) + .unwrap(); + h.validate().unwrap(); + + // Copy the FuncDefn node but not its children + let mut h = simple_dfg_hugr(); + let node_map = h + .insert_view_forest( + &insert, + insert.entry_descendants().chain([defn.node(), decl.node()]), + roots.clone(), + ) + .unwrap(); + assert_matches!(h.validate(), + Err(ValidationError::ContainerWithoutChildren { node, optype: _ }) => assert_eq!(node, node_map[&defn.node()])); + + // Copy the FuncDefn *containing* the entrypoint but transplant the entrypoint + let func_containing_entry = insert.get_parent(insert.entrypoint()).unwrap(); + assert!(matches!( + insert.get_optype(func_containing_entry), + OpType::FuncDefn(_) + )); + roots.insert(func_containing_entry, h.module_root()); + let mut h = simple_dfg_hugr(); + let node_map = h + .insert_view_forest(&insert, insert.nodes().skip(1), roots) + .unwrap(); + assert!(matches!( + h.validate(), + Err(ValidationError::InterGraphEdgeError(_)) + )); + for c in h.nodes().filter(|n| h.get_optype(*n).is_call()) { + assert!(h.static_source(c).is_some()); + } + let new_defn = node_map[&func_containing_entry]; + // The DFG (entrypoint) has been moved elsewhere + let [inp, outp] = h.get_io(new_defn).unwrap(); + assert_eq!(h.children(new_defn).count(), 2); + assert!(matches!(h.get_optype(inp), OpType::Input(_))); + assert!(matches!(h.get_optype(outp), OpType::Output(_))); + let inserted_ep = node_map[&insert.entrypoint()]; + assert_eq!(h.output_neighbours(inp).next().unwrap(), inserted_ep); + assert_eq!(h.get_parent(inserted_ep), Some(h.entrypoint())); + } + + #[test] + fn bad_insert_forest() { + let backup = simple_dfg_hugr(); + let mut h = backup.clone(); + + let (insert, _, _) = dfg_calling_defn_decl(); + let ep = insert.entrypoint(); + let epp = insert.get_parent(ep).unwrap(); + let roots = HashMap::from([(epp, h.module_root()), (ep, h.entrypoint())]); + let r = h.insert_view_forest( + &insert, + insert.descendants(epp).chain(insert.descendants(ep)), + roots.clone(), + ); + assert_eq!(r, Err(InsertForestError::DoubleCopy(ep))); + assert!(h.validate().is_err()); + + let mut h = backup.clone(); + let r = h.insert_forest(insert, roots); + assert_eq!(r, Err(InsertForestError::DoubleCopy(ep))); + // Here the error is detected in building `nodes` from `roots` so before any mutation + assert_eq!(h, backup); + } } From 0eaf3eb4c368192e8f223de16e277e17fb44d759 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 12 Aug 2025 14:08:43 +0100 Subject: [PATCH 05/26] fix test - edges removed rather than delocalized --- hugr-core/src/hugr/hugrmut.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 23dd5498ce..d85e30ff1a 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -916,15 +916,18 @@ mod test { for c in h.nodes().filter(|n| h.get_optype(*n).is_call()) { assert!(h.static_source(c).is_some()); } + // The DFG (entrypoint) has been moved: + let inserted_ep = node_map[&insert.entrypoint()]; + assert_eq!(h.get_parent(inserted_ep), Some(h.entrypoint())); let new_defn = node_map[&func_containing_entry]; - // The DFG (entrypoint) has been moved elsewhere - let [inp, outp] = h.get_io(new_defn).unwrap(); assert_eq!(h.children(new_defn).count(), 2); + + let [inp, outp] = h.get_io(new_defn).unwrap(); assert!(matches!(h.get_optype(inp), OpType::Input(_))); assert!(matches!(h.get_optype(outp), OpType::Output(_))); - let inserted_ep = node_map[&insert.entrypoint()]; - assert_eq!(h.output_neighbours(inp).next().unwrap(), inserted_ep); - assert_eq!(h.get_parent(inserted_ep), Some(h.entrypoint())); + // It seems the edge from Input is disconnected, but the edge to Output preserved + assert_eq!(h.all_neighbours(inp).next(), None); + assert_eq!(h.input_neighbours(outp).next(), Some(inserted_ep)); } #[test] From f55c288ca3e1d0daecd5144291af8c8909a62fa1 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 13 Aug 2025 12:04:28 +0100 Subject: [PATCH 06/26] More docs/links --- hugr-core/src/hugr/hugrmut.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index d85e30ff1a..f18d2dce4e 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -178,7 +178,9 @@ pub trait HugrMut: HugrMutInternals { /// If the node is not in the graph, or if the port is invalid. fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (OutgoingPort, IncomingPort); - /// Insert another hugr into this one, under a given parent node. + /// Insert another hugr into this one, under a given parent node. Edges into the + /// inserted subtree (i.e. nonlocal or static) will be disconnected in `self`. + /// (See [Self::insert_forest] for a way to insert sources of such edges as well.) /// /// # Panics /// @@ -190,7 +192,8 @@ pub trait HugrMut: HugrMutInternals { /// Insert a subtree of another hugr into this one, under a given parent node. /// Edges into the inserted subtree (i.e. nonlocal or static) will be disconnected - /// in `self`. + /// in `self`. (See [Self::insert_forest] for a way to preserve such edges by + /// inserting their sources as well.) /// /// # Panics /// @@ -213,7 +216,8 @@ pub trait HugrMut: HugrMutInternals { /// Copy the entrypoint subtree of another hugr into this one, under a given parent node. /// Edges into the inserted subtree (i.e. nonlocal or static) will be disconnected - /// in `self`. + /// in `self`. (See [Self::insert_view_forest] for a way to insert sources of such edges + /// as well.) /// /// # Panics /// @@ -262,8 +266,10 @@ pub trait HugrMut: HugrMutInternals { } /// Insert a forest of nodes from another Hugr into this one. + /// /// `root_parents` maps from roots of regions in the other Hugr to insert, /// to the node in this Hugr that shall be parent for that region. + /// If `root_parents` is empty, nothing is inserted. /// /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` /// and where each value is the corresponding (new) node in `self`. From 10778cef26aba75fd762e239ffd74049b40158d0 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 12 Aug 2025 15:45:40 +0100 Subject: [PATCH 07/26] Optimize by turning HashMap into impl IntoIterator, avoid size_hint --- hugr-core/src/hugr/hugrmut.rs | 114 +++++++++++++++------------ hugr-core/src/hugr/views/impls.rs | 4 +- hugr-core/src/hugr/views/rerooted.rs | 4 +- 3 files changed, 68 insertions(+), 54 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index f18d2dce4e..440def21ac 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -206,7 +206,7 @@ pub trait HugrMut: HugrMutInternals { region: Node, ) -> InsertionResult { let node_map = self - .insert_forest(other, HashMap::from([(region, root)])) + .insert_forest(other, [(region, root)]) .expect("No errors possible for single subtree"); InsertionResult { inserted_entrypoint: node_map[®ion], @@ -229,7 +229,7 @@ pub trait HugrMut: HugrMutInternals { ) -> InsertionResult { let ep = other.entrypoint(); let node_map = self - .insert_view_forest(other, other.descendants(ep), HashMap::from([(ep, root)])) + .insert_view_forest(other, other.descendants(ep), [(ep, root)]) .expect("No errors possible for single subtree"); InsertionResult { inserted_entrypoint: node_map[&ep], @@ -260,7 +260,7 @@ pub trait HugrMut: HugrMutInternals { self.insert_view_forest( other, subgraph.nodes().iter().cloned(), - subgraph.nodes().iter().map(|n| (*n, root)).collect(), + subgraph.nodes().iter().map(|n| (*n, root)), ) .expect("SiblingSubgraph nodes are a set") } @@ -281,17 +281,16 @@ pub trait HugrMut: HugrMutInternals { /// /// # Panics /// - /// If any of the values in `roots` are not nodes in `self`. + /// If any of the keys in `roots` are not nodes in `other`, or any of the values not in `self`. fn insert_forest( &mut self, other: Hugr, - root_parents: HashMap, + root_parents: impl IntoIterator, ) -> InsertForestResult; /// Copy a forest of nodes from a view into this one. /// - /// `nodes` enumerates all nodes in `other` to copy; every item must *either* be - /// present in `root_parents`, or follow its parent (in `other`) in the iteration order. + /// `nodes` enumerates all nodes in `other` to copy. /// /// `root_parents` identifies those nodes in `nodes` which should be placed under /// the given parent nodes in `self`. Note that unlike [Self::insert_forest] this @@ -303,18 +302,16 @@ pub trait HugrMut: HugrMutInternals { /// /// # Errors /// - /// [InsertForestError::DoubleCopy] if any node appears in `nodes` more than once + /// [InsertForestError::DoubleCopy] if any node appears in `nodes` more than once. /// /// # Panics /// - /// If any of the values in `roots` are not nodes in `self`. - /// - /// If `nodes` does not adhere to the ordering requirement above + /// If any of the keys in `roots` are not in `nodes`, or any of the values not nodes in `self`. fn insert_view_forest( &mut self, other: &H, - nodes: impl IntoIterator, - roots: HashMap, + nodes: impl Iterator + Clone, + roots: impl IntoIterator, ) -> InsertForestResult; /// Applies a patch to the graph. @@ -515,23 +512,42 @@ impl HugrMut for Hugr { fn insert_forest( &mut self, mut other: Hugr, - roots: HashMap, + roots: impl IntoIterator, ) -> Result, InsertForestError> { - for &r in roots.keys() { - let mut n = r; - while let Some(p) = other.get_parent(n) { - if roots.contains_key(&p) { - return Err(InsertForestError::DoubleCopy(r)); + let mut roots = roots.into_iter(); + let Some(fst_root) = roots.next() else { + return Ok(HashMap::new()); + }; + let node_map = match roots.next() { + None => { + // Skip DoubleCopy check and avoid allocating singleton HashMap + insert_hugr_internal( + self, + &other, + other.descendants(fst_root.0), + std::iter::once(fst_root), + ) + } + Some(snd_root) => { + let roots: HashMap = + [fst_root, snd_root].into_iter().chain(roots).collect(); + for &r in roots.keys() { + let mut n = r; + while let Some(p) = other.get_parent(n) { + if roots.contains_key(&p) { + return Err(InsertForestError::DoubleCopy(r)); + } + n = p; + } } - n = p; + insert_hugr_internal( + self, + &other, + roots.keys().flat_map(|n| other.descendants(*n)), + roots.iter().map(|(r, p)| (*r, *p)), + ) } } - let node_map = insert_hugr_internal( - self, - &other, - roots.keys().flat_map(|n| other.descendants(*n)), - |k| roots.get(k).cloned(), - ) .expect("Trees disjoint so no repeated nodes"); // Merge the extension sets. self.extensions.extend(other.extensions()); @@ -552,11 +568,10 @@ impl HugrMut for Hugr { fn insert_view_forest( &mut self, other: &H, - nodes: impl IntoIterator, - roots: HashMap, + nodes: impl Iterator + Clone, + roots: impl IntoIterator, ) -> Result, InsertForestError> { - let node_map = - insert_hugr_internal(self, other, nodes.into_iter(), |k| roots.get(k).cloned())?; + let node_map = insert_hugr_internal(self, other, nodes, roots)?; // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, copying them from the other graph. @@ -646,8 +661,8 @@ impl HugrMut for Hugr { fn insert_hugr_internal( hugr: &mut Hugr, other: &H, - other_nodes: impl Iterator, - reroot: impl Fn(&H::Node) -> Option, + other_nodes: impl Iterator + Clone, + root_parents: impl IntoIterator, ) -> Result, InsertForestError> { let new_node_count_hint = other_nodes.size_hint().1.unwrap_or_default(); @@ -655,7 +670,7 @@ fn insert_hugr_internal( let mut node_map = HashMap::with_capacity(new_node_count_hint); hugr.reserve(new_node_count_hint, 0); - for old in other_nodes { + for old in other_nodes.clone() { // We use a dummy optype here. The callers take care of updating the // correct optype, avoiding cloning if possible. let op = OpType::default(); @@ -666,16 +681,6 @@ fn insert_hugr_internal( hugr.set_num_ports(new, other.num_inputs(old), other.num_outputs(old)); - let new_parent = if let Some(new_parent) = reroot(&old) { - new_parent - } else { - let old_parent = other.get_parent(old).unwrap(); - *node_map - .get(&old_parent) - .expect("Child node came before parent in `other_nodes` iterator") - }; - hugr.set_parent(new, new_parent); - // Reconnect the edges to the new node. for tgt in other.node_inputs(old) { for (neigh, src) in other.linked_outputs(old, tgt) { @@ -697,6 +702,17 @@ fn insert_hugr_internal( } } } + for (r, p) in root_parents { + hugr.set_parent(node_map[&r], p); + } + for old in other_nodes { + let new = node_map[&old]; + if hugr.get_parent(new).is_none() { + let old_parent = other.get_parent(old).unwrap(); + let new_parent = node_map[&old_parent]; + hugr.set_parent(new, new_parent); + } + } Ok(node_map) } @@ -827,11 +843,9 @@ mod test { for (call1, call2) in [(false, false), (false, true), (true, false), (true, true)] { let mut h = simple_dfg_hugr(); let (insert, defn, decl) = dfg_calling_defn_decl(); - let roots = HashMap::from_iter( - std::iter::once((insert.entrypoint(), h.entrypoint())) - .chain(call1.then_some((defn.node(), h.module_root())).into_iter()) - .chain(call2.then_some((decl.node(), h.module_root())).into_iter()), - ); + let roots = std::iter::once((insert.entrypoint(), h.entrypoint())) + .chain(call1.then_some((defn.node(), h.module_root())).into_iter()) + .chain(call2.then_some((decl.node(), h.module_root())).into_iter()); h.insert_forest(insert, roots).unwrap(); if call1 && call2 { h.validate().unwrap(); @@ -944,11 +958,11 @@ mod test { let (insert, _, _) = dfg_calling_defn_decl(); let ep = insert.entrypoint(); let epp = insert.get_parent(ep).unwrap(); - let roots = HashMap::from([(epp, h.module_root()), (ep, h.entrypoint())]); + let roots = [(epp, h.module_root()), (ep, h.entrypoint())]; let r = h.insert_view_forest( &insert, insert.descendants(epp).chain(insert.descendants(ep)), - roots.clone(), + roots, ); assert_eq!(r, Err(InsertForestError::DoubleCopy(ep))); assert!(h.validate().is_err()); diff --git a/hugr-core/src/hugr/views/impls.rs b/hugr-core/src/hugr/views/impls.rs index 3fcaea0f8b..f5cf9dfc40 100644 --- a/hugr-core/src/hugr/views/impls.rs +++ b/hugr-core/src/hugr/views/impls.rs @@ -114,8 +114,8 @@ macro_rules! hugr_mut_methods { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> Result, InsertForestError>; - fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> Result, InsertForestError>; + fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> Result, InsertForestError>; + fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> Result, InsertForestError>; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } diff --git a/hugr-core/src/hugr/views/rerooted.rs b/hugr-core/src/hugr/views/rerooted.rs index 61e39a3316..33a3d0c49b 100644 --- a/hugr-core/src/hugr/views/rerooted.rs +++ b/hugr-core/src/hugr/views/rerooted.rs @@ -139,8 +139,8 @@ impl HugrMut for Rerooted { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: HashMap) -> Result, InsertForestError>; - fn insert_view_forest(&mut self, other: &Other, nodes: impl IntoIterator, roots: HashMap) -> Result, InsertForestError>; + fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> Result, InsertForestError>; + fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> Result, InsertForestError>; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } From 826990dd8fb30b6ed076edada9ebd64f78b5095d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 15 Aug 2025 17:12:31 +0100 Subject: [PATCH 08/26] struct InsertedForest; use InsertForestResult alias more widely --- hugr-core/src/hugr/hugrmut.rs | 55 +++++++++++++++++++--------- hugr-core/src/hugr/views/impls.rs | 8 ++-- hugr-core/src/hugr/views/rerooted.rs | 8 ++-- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 440def21ac..94a9f65b92 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -207,7 +207,8 @@ pub trait HugrMut: HugrMutInternals { ) -> InsertionResult { let node_map = self .insert_forest(other, [(region, root)]) - .expect("No errors possible for single subtree"); + .expect("No errors possible for single subtree") + .node_map; InsertionResult { inserted_entrypoint: node_map[®ion], node_map, @@ -230,7 +231,8 @@ pub trait HugrMut: HugrMutInternals { let ep = other.entrypoint(); let node_map = self .insert_view_forest(other, other.descendants(ep), [(ep, root)]) - .expect("No errors possible for single subtree"); + .expect("No errors possible for single subtree") + .node_map; InsertionResult { inserted_entrypoint: node_map[&ep], node_map, @@ -263,6 +265,7 @@ pub trait HugrMut: HugrMutInternals { subgraph.nodes().iter().map(|n| (*n, root)), ) .expect("SiblingSubgraph nodes are a set") + .node_map } /// Insert a forest of nodes from another Hugr into this one. @@ -349,7 +352,7 @@ pub trait HugrMut: HugrMutInternals { /// /// On success, a map giving the new indices; or an error in the request. /// Used by [HugrMut::insert_forest] and [HugrMut::insert_view_forest]. -pub type InsertForestResult = Result, InsertForestError>; +pub type InsertForestResult = Result, InsertForestError>; /// An error from [HugrMut::insert_forest] or [HugrMut::insert_view_forest] #[derive(Clone, Debug, derive_more::Display, derive_more::Error, PartialEq)] @@ -377,6 +380,18 @@ pub struct InsertionResult { pub node_map: HashMap, } +/// Records the result of inserting a Hugr or view via [`HugrMut::insert_forest`] +/// or [`HugrMut::insert_view_forest`]. +/// +/// Contains a map from the nodes in the source HUGR to the nodes in the target +/// HUGR, using their respective `Node` types. +#[derive(Clone, Debug, Default)] +pub struct InsertedForest { + /// Map from the nodes from the source Hugr/view that were inserted, + /// to the corresponding nodes in the Hugr into which said was inserted. + pub node_map: HashMap, +} + /// Translate a portgraph node index map into a map from nodes in the source /// HUGR to nodes in the target HUGR. /// @@ -513,12 +528,14 @@ impl HugrMut for Hugr { &mut self, mut other: Hugr, roots: impl IntoIterator, - ) -> Result, InsertForestError> { + ) -> InsertForestResult { let mut roots = roots.into_iter(); let Some(fst_root) = roots.next() else { - return Ok(HashMap::new()); + return Ok(InsertedForest { + node_map: HashMap::new(), + }); }; - let node_map = match roots.next() { + let inserted = match roots.next() { None => { // Skip DoubleCopy check and avoid allocating singleton HashMap insert_hugr_internal( @@ -554,7 +571,7 @@ 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. - for (&node, &new_node) in &node_map { + for (&node, &new_node) in &inserted.node_map { let node_pg = node.into_portgraph(); let new_node_pg = new_node.into_portgraph(); let optype = other.op_types.take(node_pg); @@ -562,7 +579,7 @@ impl HugrMut for Hugr { let meta = other.metadata.take(node_pg); self.metadata.set(new_node_pg, meta); } - Ok(node_map) + Ok(inserted) } fn insert_view_forest( @@ -570,14 +587,14 @@ impl HugrMut for Hugr { other: &H, nodes: impl Iterator + Clone, roots: impl IntoIterator, - ) -> Result, InsertForestError> { - let node_map = insert_hugr_internal(self, other, nodes, roots)?; + ) -> InsertForestResult { + let inserted = insert_hugr_internal(self, other, nodes, roots)?; // Merge the extension sets. self.extensions.extend(other.extensions()); // 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. - for (&node, &new_node) in &node_map { + for (&node, &new_node) in &inserted.node_map { let nodetype = other.get_optype(node); self.op_types .set(new_node.into_portgraph(), nodetype.clone()); @@ -587,7 +604,7 @@ impl HugrMut for Hugr { .set(new_node.into_portgraph(), Some(meta.clone())); } } - Ok(node_map) + Ok(inserted) } fn copy_descendants( @@ -663,7 +680,7 @@ fn insert_hugr_internal( other: &H, other_nodes: impl Iterator + Clone, root_parents: impl IntoIterator, -) -> Result, InsertForestError> { +) -> InsertForestResult { let new_node_count_hint = other_nodes.size_hint().1.unwrap_or_default(); // Insert the nodes from the other graph into this one. @@ -713,7 +730,7 @@ fn insert_hugr_internal( hugr.set_parent(new, new_parent); } } - Ok(node_map) + Ok(InsertedForest { node_map }) } #[cfg(test)] @@ -914,7 +931,8 @@ mod test { insert.entry_descendants().chain([defn.node(), decl.node()]), roots.clone(), ) - .unwrap(); + .unwrap() + .node_map; assert_matches!(h.validate(), Err(ValidationError::ContainerWithoutChildren { node, optype: _ }) => assert_eq!(node, node_map[&defn.node()])); @@ -928,7 +946,8 @@ mod test { let mut h = simple_dfg_hugr(); let node_map = h .insert_view_forest(&insert, insert.nodes().skip(1), roots) - .unwrap(); + .unwrap() + .node_map; assert!(matches!( h.validate(), Err(ValidationError::InterGraphEdgeError(_)) @@ -964,12 +983,12 @@ mod test { insert.descendants(epp).chain(insert.descendants(ep)), roots, ); - assert_eq!(r, Err(InsertForestError::DoubleCopy(ep))); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); assert!(h.validate().is_err()); let mut h = backup.clone(); let r = h.insert_forest(insert, roots); - assert_eq!(r, Err(InsertForestError::DoubleCopy(ep))); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); // Here the error is detected in building `nodes` from `roots` so before any mutation assert_eq!(h, backup); } diff --git a/hugr-core/src/hugr/views/impls.rs b/hugr-core/src/hugr/views/impls.rs index f5cf9dfc40..3be4e76beb 100644 --- a/hugr-core/src/hugr/views/impls.rs +++ b/hugr-core/src/hugr/views/impls.rs @@ -1,10 +1,10 @@ //! Implementation of the core hugr traits for different wrappers of a `Hugr`. -use std::{borrow::Cow, collections::HashMap, rc::Rc, sync::Arc}; +use std::{borrow::Cow, rc::Rc, sync::Arc}; use super::HugrView; use crate::hugr::internal::{HugrInternals, HugrMutInternals}; -use crate::hugr::{HugrMut, hugrmut::InsertForestError}; +use crate::hugr::{HugrMut, hugrmut::InsertForestResult}; macro_rules! hugr_internal_methods { // The extra ident here is because invocations of the macro cannot pass `self` as argument @@ -114,8 +114,8 @@ macro_rules! hugr_mut_methods { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> Result, InsertForestError>; - fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> Result, InsertForestError>; + fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> InsertForestResult; + fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> InsertForestResult; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } diff --git a/hugr-core/src/hugr/views/rerooted.rs b/hugr-core/src/hugr/views/rerooted.rs index 33a3d0c49b..73fbe0cba9 100644 --- a/hugr-core/src/hugr/views/rerooted.rs +++ b/hugr-core/src/hugr/views/rerooted.rs @@ -1,10 +1,8 @@ //! A HUGR wrapper with a modified entrypoint node, returned by //! [`HugrView::with_entrypoint`] and [`HugrMut::with_entrypoint_mut`]. -use std::collections::HashMap; - use crate::hugr::internal::{HugrInternals, HugrMutInternals}; -use crate::hugr::{HugrMut, hugrmut::InsertForestError}; +use crate::hugr::{HugrMut, hugrmut::InsertForestResult}; use super::{HugrView, panic_invalid_node}; @@ -139,8 +137,8 @@ impl HugrMut for Rerooted { fn connect(&mut self, src: Self::Node, src_port: impl Into, dst: Self::Node, dst_port: impl Into); fn disconnect(&mut self, node: Self::Node, port: impl Into); fn add_other_edge(&mut self, src: Self::Node, dst: Self::Node) -> (crate::OutgoingPort, crate::IncomingPort); - fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> Result, InsertForestError>; - fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> Result, InsertForestError>; + fn insert_forest(&mut self, other: crate::Hugr, roots: impl IntoIterator) -> InsertForestResult; + fn insert_view_forest(&mut self, other: &Other, nodes: impl Iterator + Clone, roots: impl IntoIterator) -> InsertForestResult; fn use_extension(&mut self, extension: impl Into>); fn use_extensions(&mut self, registry: impl IntoIterator) where crate::extension::ExtensionRegistry: Extend; } From f9eee4423d5dbe3653496bfddbbefe9cf019d0df Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 15 Aug 2025 17:34:08 +0100 Subject: [PATCH 09/26] N -> SN --- hugr-core/src/hugr/hugrmut.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 94a9f65b92..cd32063807 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -354,13 +354,15 @@ pub trait HugrMut: HugrMutInternals { /// Used by [HugrMut::insert_forest] and [HugrMut::insert_view_forest]. pub type InsertForestResult = Result, InsertForestError>; -/// An error from [HugrMut::insert_forest] or [HugrMut::insert_view_forest] +/// An error from [HugrMut::insert_forest] or [HugrMut::insert_view_forest]. +/// +/// `SN` is the type of nodes in the source Hugr #[derive(Clone, Debug, derive_more::Display, derive_more::Error, PartialEq)] #[non_exhaustive] -pub enum InsertForestError { +pub enum InsertForestError { /// The specified source node would be copied twice into the target #[display("Node/subtree {_0} would be copied twice")] - DoubleCopy(N), + DoubleCopy(SN), } /// Records the result of inserting a Hugr or view via [`HugrMut::insert_hugr`], From caecc43f50cb3372bbff122434aaf53f1542e882 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 15 Aug 2025 17:35:04 +0100 Subject: [PATCH 10/26] Make DoubleCopy contain a named field --- hugr-core/src/hugr/hugrmut.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index cd32063807..efef819c55 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -361,8 +361,11 @@ pub type InsertForestResult = Result, InsertFores #[non_exhaustive] pub enum InsertForestError { /// The specified source node would be copied twice into the target - #[display("Node/subtree {_0} would be copied twice")] - DoubleCopy(SN), + #[display("Node/subtree {node} would be copied twice")] + DoubleCopy { + /// The node or root of subtree that would be copied twice + node: SN, + }, } /// Records the result of inserting a Hugr or view via [`HugrMut::insert_hugr`], @@ -550,13 +553,13 @@ impl HugrMut for Hugr { Some(snd_root) => { let roots: HashMap = [fst_root, snd_root].into_iter().chain(roots).collect(); - for &r in roots.keys() { - let mut n = r; - while let Some(p) = other.get_parent(n) { + for &node in roots.keys() { + let mut anc = node; + while let Some(p) = other.get_parent(anc) { if roots.contains_key(&p) { - return Err(InsertForestError::DoubleCopy(r)); + return Err(InsertForestError::DoubleCopy { node }); } - n = p; + anc = p; } } insert_hugr_internal( @@ -695,7 +698,7 @@ fn insert_hugr_internal( let op = OpType::default(); let new = hugr.add_node(op); if node_map.insert(old, new).is_some() { - return Err(InsertForestError::DoubleCopy(old)); + return Err(InsertForestError::DoubleCopy { node: old }); } hugr.set_num_ports(new, other.num_inputs(old), other.num_outputs(old)); @@ -985,12 +988,12 @@ mod test { insert.descendants(epp).chain(insert.descendants(ep)), roots, ); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy { node: ep })); assert!(h.validate().is_err()); let mut h = backup.clone(); let r = h.insert_forest(insert, roots); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy { node: ep })); // Here the error is detected in building `nodes` from `roots` so before any mutation assert_eq!(h, backup); } From bd12196eaa941bac27f7a974e04823d490d4fad2 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 16 Aug 2025 09:46:28 +0100 Subject: [PATCH 11/26] Revert "Make DoubleCopy contain a named field" This reverts commit caecc43f50cb3372bbff122434aaf53f1542e882. --- hugr-core/src/hugr/hugrmut.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index efef819c55..cd32063807 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -361,11 +361,8 @@ pub type InsertForestResult = Result, InsertFores #[non_exhaustive] pub enum InsertForestError { /// The specified source node would be copied twice into the target - #[display("Node/subtree {node} would be copied twice")] - DoubleCopy { - /// The node or root of subtree that would be copied twice - node: SN, - }, + #[display("Node/subtree {_0} would be copied twice")] + DoubleCopy(SN), } /// Records the result of inserting a Hugr or view via [`HugrMut::insert_hugr`], @@ -553,13 +550,13 @@ impl HugrMut for Hugr { Some(snd_root) => { let roots: HashMap = [fst_root, snd_root].into_iter().chain(roots).collect(); - for &node in roots.keys() { - let mut anc = node; - while let Some(p) = other.get_parent(anc) { + for &r in roots.keys() { + let mut n = r; + while let Some(p) = other.get_parent(n) { if roots.contains_key(&p) { - return Err(InsertForestError::DoubleCopy { node }); + return Err(InsertForestError::DoubleCopy(r)); } - anc = p; + n = p; } } insert_hugr_internal( @@ -698,7 +695,7 @@ fn insert_hugr_internal( let op = OpType::default(); let new = hugr.add_node(op); if node_map.insert(old, new).is_some() { - return Err(InsertForestError::DoubleCopy { node: old }); + return Err(InsertForestError::DoubleCopy(old)); } hugr.set_num_ports(new, other.num_inputs(old), other.num_outputs(old)); @@ -988,12 +985,12 @@ mod test { insert.descendants(epp).chain(insert.descendants(ep)), roots, ); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy { node: ep })); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); assert!(h.validate().is_err()); let mut h = backup.clone(); let r = h.insert_forest(insert, roots); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy { node: ep })); + assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); // Here the error is detected in building `nodes` from `roots` so before any mutation assert_eq!(h, backup); } From bdc0ff1402c075c2a87c8ac5d0bb79f7bf675d3f Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 16 Aug 2025 10:03:30 +0100 Subject: [PATCH 12/26] Use rstest::fixture --- hugr-core/src/hugr/hugrmut.rs | 99 ++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 48 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index cd32063807..51cee318dd 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -739,6 +739,7 @@ fn insert_hugr_internal( mod test { use cool_asserts::assert_matches; use itertools::Itertools; + use rstest::{fixture, rstest}; use crate::builder::test::simple_dfg_hugr; use crate::builder::{DFGBuilder, Dataflow, DataflowHugr, DataflowSubContainer, HugrBuilder}; @@ -829,6 +830,7 @@ mod test { /// A DFG-entrypoint Hugr (no inputs, one bool_t output) containing two calls, /// to a FuncDefn and a FuncDecl each bool_t->bool_t, and their handles. + #[fixture] pub(crate) fn dfg_calling_defn_decl() -> (Hugr, FuncID, FuncID) { let mut dfb = DFGBuilder::new(Signature::new(vec![], bool_t())).unwrap(); let new_defn = { @@ -856,55 +858,56 @@ mod test { ) } - #[test] - fn test_insert_forest() { - // Specify which decls to transfer - for (call1, call2) in [(false, false), (false, true), (true, false), (true, true)] { - let mut h = simple_dfg_hugr(); - let (insert, defn, decl) = dfg_calling_defn_decl(); - let roots = std::iter::once((insert.entrypoint(), h.entrypoint())) - .chain(call1.then_some((defn.node(), h.module_root())).into_iter()) - .chain(call2.then_some((decl.node(), h.module_root())).into_iter()); - h.insert_forest(insert, roots).unwrap(); - if call1 && call2 { - h.validate().unwrap(); - } else { - assert!(matches!( - h.validate(), - Err(ValidationError::UnconnectedPort { .. }) - )); - } - assert_eq!( - h.children(h.module_root()).count(), - 1 + (call1 as usize) + (call2 as usize) - ); - let [calln1, calln2] = h - .nodes() - .filter(|n| h.get_optype(*n).is_call()) - .collect_array() - .unwrap(); + #[rstest] + fn test_insert_forest( + dfg_calling_defn_decl: (Hugr, FuncID, FuncID), + #[values(false, true)] copy_defn: bool, + #[values(false, true)] copy_decl: bool, + ) { + let (insert, defn, decl) = dfg_calling_defn_decl; + let mut h = simple_dfg_hugr(); + let roots = std::iter::once((insert.entrypoint(), h.entrypoint())) + .chain(copy_defn.then_some((defn.node(), h.module_root()))) + .chain(copy_decl.then_some((decl.node(), h.module_root()))); + h.insert_forest(insert, roots).unwrap(); + if copy_defn && copy_decl { + h.validate().unwrap(); + } else { + assert!(matches!( + h.validate(), + Err(ValidationError::UnconnectedPort { .. }) + )); + } + assert_eq!( + h.children(h.module_root()).count(), + 1 + (copy_defn as usize) + (copy_decl as usize) + ); + let [calln1, calln2] = h + .nodes() + .filter(|n| h.get_optype(*n).is_call()) + .collect_array() + .unwrap(); - let tgt1 = h.nodes().find(|n| { - h.get_optype(*n) - .as_func_defn() - .is_some_and(|fd| fd.func_name() == "helper_id") - }); - assert_eq!(tgt1.is_some(), call1); - assert_eq!(h.static_source(calln1), tgt1); + let tgt1 = h.nodes().find(|n| { + h.get_optype(*n) + .as_func_defn() + .is_some_and(|fd| fd.func_name() == "helper_id") + }); + assert_eq!(tgt1.is_some(), copy_defn); + assert_eq!(h.static_source(calln1), tgt1); - let tgt2 = h.nodes().find(|n| { - h.get_optype(*n) - .as_func_decl() - .is_some_and(|fd| fd.func_name() == "helper2") - }); - assert_eq!(tgt2.is_some(), call2); - assert_eq!(h.static_source(calln2), tgt2); - } + let tgt2 = h.nodes().find(|n| { + h.get_optype(*n) + .as_func_decl() + .is_some_and(|fd| fd.func_name() == "helper2") + }); + assert_eq!(tgt2.is_some(), copy_decl); + assert_eq!(h.static_source(calln2), tgt2); } - #[test] - fn test_insert_view_forest() { - let (insert, defn, decl) = dfg_calling_defn_decl(); + #[rstest] + fn test_insert_view_forest(dfg_calling_defn_decl: (Hugr, FuncID, FuncID)) { + let (insert, defn, decl) = dfg_calling_defn_decl; let mut h = simple_dfg_hugr(); let mut roots = HashMap::from([ @@ -971,12 +974,12 @@ mod test { assert_eq!(h.input_neighbours(outp).next(), Some(inserted_ep)); } - #[test] - fn bad_insert_forest() { + #[rstest] + fn bad_insert_forest(dfg_calling_defn_decl: (Hugr, FuncID, FuncID)) { let backup = simple_dfg_hugr(); let mut h = backup.clone(); - let (insert, _, _) = dfg_calling_defn_decl(); + let (insert, _, _) = dfg_calling_defn_decl; let ep = insert.entrypoint(); let epp = insert.get_parent(ep).unwrap(); let roots = [(epp, h.module_root()), (ep, h.entrypoint())]; From 58dccdd383e52a10b3a54a9523f694020fdb42ae Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sat, 16 Aug 2025 10:07:11 +0100 Subject: [PATCH 13/26] test tidy --- hugr-core/src/hugr/hugrmut.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 51cee318dd..db64c39c4b 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -878,10 +878,8 @@ mod test { Err(ValidationError::UnconnectedPort { .. }) )); } - assert_eq!( - h.children(h.module_root()).count(), - 1 + (copy_defn as usize) + (copy_decl as usize) - ); + let expected_mod_children = 1 + (copy_defn as usize) + (copy_decl as usize); + assert_eq!(h.children(h.module_root()).count(), expected_mod_children); let [calln1, calln2] = h .nodes() .filter(|n| h.get_optype(*n).is_call()) From b1c0d2a2df6abdb06135f5b1215616690678568d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 17 Aug 2025 10:38:20 +0100 Subject: [PATCH 14/26] add benchmark --- hugr/benches/benchmarks/hugr.rs | 46 ++++++++++++++++++++++-- hugr/benches/benchmarks/hugr/examples.rs | 29 ++++++++++++++- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index e5f2d4de2b..31a5bfca00 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -3,13 +3,17 @@ pub mod examples; use criterion::{AxisScale, BenchmarkId, Criterion, PlotConfiguration, criterion_group}; -use hugr::Hugr; use hugr::envelope::{EnvelopeConfig, EnvelopeFormat}; +use hugr::hugr::hugrmut::HugrMut; +use hugr::ops::handle::NodeHandle; #[allow(unused)] use hugr::std_extensions::STD_REG; +use hugr::{Hugr, HugrView}; use std::hint::black_box; -pub use examples::{BENCH_EXTENSIONS, circuit, simple_cfg_hugr, simple_dfg_hugr}; +pub use examples::{ + BENCH_EXTENSIONS, circuit, dfg_calling_defn_decl, simple_cfg_hugr, simple_dfg_hugr, +}; trait Serializer { fn serialize(&self, hugr: &Hugr) -> Vec; @@ -60,6 +64,44 @@ fn bench_builder(c: &mut Criterion) { group.finish(); } +fn bench_insertion(c: &mut Criterion) { + let mut group = c.benchmark_group("insertion"); + group.plot_config(PlotConfiguration::default().summary_scale(AxisScale::Logarithmic)); + group.bench_function("insert_from_view", |b| { + let mut h1 = simple_dfg_hugr(); + let h2 = simple_cfg_hugr(); + b.iter(|| black_box(h1.insert_from_view(h1.entrypoint(), &h2))) + }); + group.bench_function("insert_hugr_view", |b| { + let mut h = simple_dfg_hugr(); + // Note it's possible that creation of simple_dfg_hugr may dominate the cost of insertion! + b.iter(|| black_box(h.insert_hugr(h.entrypoint(), simple_dfg_hugr()))) + }); + group.bench_function("insert_view_forest", |b| { + let mut h = simple_dfg_hugr(); + let (insert, decl, defn) = dfg_calling_defn_decl(); + let nodes = insert.entry_descendants().chain([defn.node(), decl.node()]); + let roots = [ + (insert.entrypoint(), h.entrypoint()), + (defn.node(), h.module_root()), + (decl.node(), h.module_root()), + ]; + b.iter(|| black_box(h.insert_view_forest(&insert, nodes.clone(), roots.iter().cloned()))) + }); + group.bench_function("insert_forest", |b| { + let mut h = simple_dfg_hugr(); + b.iter(|| { + let (insert, decl, defn) = dfg_calling_defn_decl(); + let roots = [ + (insert.entrypoint(), h.entrypoint()), + (defn.node(), h.module_root()), + (decl.node(), h.module_root()), + ]; + black_box(h.insert_forest(insert, roots.into_iter())) + }) + }); +} + fn bench_serialization(c: &mut Criterion) { c.bench_function("simple_cfg_serialize/json", |b| { let h = simple_cfg_hugr(); diff --git a/hugr/benches/benchmarks/hugr/examples.rs b/hugr/benches/benchmarks/hugr/examples.rs index 8e274ac8ff..bad373cb98 100644 --- a/hugr/benches/benchmarks/hugr/examples.rs +++ b/hugr/benches/benchmarks/hugr/examples.rs @@ -8,7 +8,7 @@ use hugr::builder::{ }; use hugr::extension::ExtensionRegistry; use hugr::extension::prelude::{bool_t, qb_t, usize_t}; -use hugr::ops::OpName; +use hugr::ops::{OpName, Value, handle::FuncID}; use hugr::std_extensions::STD_REG; use hugr::std_extensions::arithmetic::float_types::{ConstF64, float64_type}; use hugr::types::Signature; @@ -52,6 +52,33 @@ pub fn simple_cfg_hugr() -> Hugr { cfg_builder.finish_hugr().unwrap() } +pub fn dfg_calling_defn_decl() -> (Hugr, FuncID, FuncID) { + let mut dfb = DFGBuilder::new(Signature::new(vec![], bool_t())).unwrap(); + let new_defn = { + let mut mb = dfb.module_root_builder(); + let fb = mb + .define_function("helper_id", Signature::new_endo(bool_t())) + .unwrap(); + let [f_inp] = fb.input_wires_arr(); + fb.finish_with_outputs([f_inp]).unwrap() + }; + let new_decl = dfb + .module_root_builder() + .declare("helper2", Signature::new_endo(bool_t()).into()) + .unwrap(); + let cst = dfb.add_load_value(Value::true_val()); + let [c1] = dfb + .call(new_defn.handle(), &[], [cst]) + .unwrap() + .outputs_arr(); + let [c2] = dfb.call(&new_decl, &[], [c1]).unwrap().outputs_arr(); + ( + dfb.finish_hugr_with_outputs([c2]).unwrap(), + *new_defn.handle(), + new_decl, + ) +} + pub static QUANTUM_EXT: LazyLock> = LazyLock::new(|| { Extension::new_arc( "bench.quantum".try_into().unwrap(), From 88a4987295f22a8ff1c5dc1132f5d0b631d7fbf3 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 17 Aug 2025 20:53:21 +0100 Subject: [PATCH 15/26] lint benchmark and add to criterion group --- hugr/benches/benchmarks/hugr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index 31a5bfca00..5a55c16426 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -97,7 +97,7 @@ fn bench_insertion(c: &mut Criterion) { (defn.node(), h.module_root()), (decl.node(), h.module_root()), ]; - black_box(h.insert_forest(insert, roots.into_iter())) + black_box(h.insert_forest(insert, roots)) }) }); } @@ -151,5 +151,5 @@ criterion_group! { name = benches; config = Criterion::default(); targets = - bench_builder, bench_serialization + bench_builder, bench_insertion, bench_serialization } From cb92e0c01341df67a05dcb501861026fc838d831 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Sun, 17 Aug 2025 21:37:54 +0100 Subject: [PATCH 16/26] comments --- hugr-core/src/hugr/hugrmut.rs | 27 ++++++++++++++++----------- hugr/benches/benchmarks/hugr.rs | 1 + 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index db64c39c4b..d055892553 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -270,8 +270,11 @@ pub trait HugrMut: HugrMutInternals { /// Insert a forest of nodes from another Hugr into this one. /// - /// `root_parents` maps from roots of regions in the other Hugr to insert, - /// to the node in this Hugr that shall be parent for that region. + /// `root_parents` contains pairs of + /// * the root of a region in `other` to insert, + /// * the node in `self` that shall be parent for that region. + /// + /// Later entries for the same region override earlier ones. /// If `root_parents` is empty, nothing is inserted. /// /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` @@ -279,8 +282,8 @@ pub trait HugrMut: HugrMutInternals { /// /// # Errors /// - /// [InsertForestError::DoubleCopy] if the subtrees of the keys of `root_parents` - /// are not disjoint (the error indicates the root of the _inner_ subtree). + /// [InsertForestError::DoubleCopy] if the regions in `root_parents` are not disjount + /// (the error indicates the root of the _inner_ subtree). /// /// # Panics /// @@ -295,13 +298,15 @@ pub trait HugrMut: HugrMutInternals { /// /// `nodes` enumerates all nodes in `other` to copy. /// - /// `root_parents` identifies those nodes in `nodes` which should be placed under - /// the given parent nodes in `self`. Note that unlike [Self::insert_forest] this - /// allows inserting most of a subtree in one location but with subparts of that - /// subtree placed elsewhere. + /// `root_parents` contains pairs of a node in `nodes` and the parent in `self` under which + /// it should be to placed. Later entries (for the same node) override earlier ones. + /// Note that unlike [Self::insert_forest] this allows inserting most of a subtree in one + /// location but with subparts of that subtree placed elsewhere. /// - /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` - /// and where each value is the corresponding (new) node in `self`. + /// Nodes in `nodes` which are not mentioned in `root_parents` and whose parent in `other` + /// is not in `nodes`, will have no parent in `self`. + /// + /// Returns a [`HashMap`] from each node in `nodes` to the corresponding (new) node in `self`. /// /// # Errors /// @@ -309,7 +314,7 @@ pub trait HugrMut: HugrMutInternals { /// /// # Panics /// - /// If any of the keys in `roots` are not in `nodes`, or any of the values not nodes in `self`. + /// If any of the keys in `root_parents` are not in `nodes`, or any of the values not nodes in `self`. fn insert_view_forest( &mut self, other: &H, diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index 5a55c16426..75ab7780a8 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -91,6 +91,7 @@ fn bench_insertion(c: &mut Criterion) { group.bench_function("insert_forest", |b| { let mut h = simple_dfg_hugr(); b.iter(|| { + // Note the cost of constructing `insert`` here may dominate the cost of insertion! let (insert, decl, defn) = dfg_calling_defn_decl(); let roots = [ (insert.entrypoint(), h.entrypoint()), From a73ce4f23e103353f018374e84b6c51c04dff654 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 19 Aug 2025 12:40:13 +0100 Subject: [PATCH 17/26] Better benchmarks via iter_batched --- hugr/benches/benchmarks/hugr.rs | 38 +++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index 75ab7780a8..bc1f609b7b 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -2,7 +2,7 @@ pub mod examples; -use criterion::{AxisScale, BenchmarkId, Criterion, PlotConfiguration, criterion_group}; +use criterion::{AxisScale, BatchSize, BenchmarkId, Criterion, PlotConfiguration, criterion_group}; use hugr::envelope::{EnvelopeConfig, EnvelopeFormat}; use hugr::hugr::hugrmut::HugrMut; use hugr::ops::handle::NodeHandle; @@ -72,14 +72,17 @@ fn bench_insertion(c: &mut Criterion) { let h2 = simple_cfg_hugr(); b.iter(|| black_box(h1.insert_from_view(h1.entrypoint(), &h2))) }); - group.bench_function("insert_hugr_view", |b| { - let mut h = simple_dfg_hugr(); - // Note it's possible that creation of simple_dfg_hugr may dominate the cost of insertion! - b.iter(|| black_box(h.insert_hugr(h.entrypoint(), simple_dfg_hugr()))) + group.bench_function("insert_hugr", |b| { + b.iter_batched( + || (simple_dfg_hugr(), simple_cfg_hugr()), + |(mut h, insert)| black_box(h.insert_hugr(h.entrypoint(), insert)), + BatchSize::SmallInput, + ) }); group.bench_function("insert_view_forest", |b| { let mut h = simple_dfg_hugr(); let (insert, decl, defn) = dfg_calling_defn_decl(); + // Note it would be better to use iter_batched to avoid cloning nodes/roots. let nodes = insert.entry_descendants().chain([defn.node(), decl.node()]); let roots = [ (insert.entrypoint(), h.entrypoint()), @@ -89,17 +92,20 @@ fn bench_insertion(c: &mut Criterion) { b.iter(|| black_box(h.insert_view_forest(&insert, nodes.clone(), roots.iter().cloned()))) }); group.bench_function("insert_forest", |b| { - let mut h = simple_dfg_hugr(); - b.iter(|| { - // Note the cost of constructing `insert`` here may dominate the cost of insertion! - let (insert, decl, defn) = dfg_calling_defn_decl(); - let roots = [ - (insert.entrypoint(), h.entrypoint()), - (defn.node(), h.module_root()), - (decl.node(), h.module_root()), - ]; - black_box(h.insert_forest(insert, roots)) - }) + b.iter_batched( + || { + let h = simple_dfg_hugr(); + let (insert, decl, defn) = dfg_calling_defn_decl(); + let roots = [ + (insert.entrypoint(), h.entrypoint()), + (defn.node(), h.module_root()), + (decl.node(), h.module_root()), + ]; + (h, insert, roots) + }, + |(mut h, insert, roots)| black_box(h.insert_forest(insert, roots)), + BatchSize::SmallInput, + ) }); } From 481648ec69ed6c5f07fa5a9e019691f044bb9f4d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 19 Aug 2025 15:04:03 +0100 Subject: [PATCH 18/26] Always convert to HashMap; update insert_forest_internal; roots->root_parents --- hugr-core/src/hugr/hugrmut.rs | 61 ++++++++++++----------------------- 1 file changed, 21 insertions(+), 40 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index d055892553..1c8c314e69 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -319,7 +319,7 @@ pub trait HugrMut: HugrMutInternals { &mut self, other: &H, nodes: impl Iterator + Clone, - roots: impl IntoIterator, + root_parents: impl IntoIterator, ) -> InsertForestResult; /// Applies a patch to the graph. @@ -534,44 +534,24 @@ impl HugrMut for Hugr { fn insert_forest( &mut self, mut other: Hugr, - roots: impl IntoIterator, + root_parents: impl IntoIterator, ) -> InsertForestResult { - let mut roots = roots.into_iter(); - let Some(fst_root) = roots.next() else { - return Ok(InsertedForest { - node_map: HashMap::new(), - }); - }; - let inserted = match roots.next() { - None => { - // Skip DoubleCopy check and avoid allocating singleton HashMap - insert_hugr_internal( - self, - &other, - other.descendants(fst_root.0), - std::iter::once(fst_root), - ) - } - Some(snd_root) => { - let roots: HashMap = - [fst_root, snd_root].into_iter().chain(roots).collect(); - for &r in roots.keys() { - let mut n = r; - while let Some(p) = other.get_parent(n) { - if roots.contains_key(&p) { - return Err(InsertForestError::DoubleCopy(r)); - } - n = p; - } + let roots: HashMap<_, _> = root_parents.into_iter().collect(); + for &r in roots.keys() { + let mut n = r; + while let Some(p) = other.get_parent(n) { + if roots.contains_key(&p) { + return Err(InsertForestError::DoubleCopy(r)); } - insert_hugr_internal( - self, - &other, - roots.keys().flat_map(|n| other.descendants(*n)), - roots.iter().map(|(r, p)| (*r, *p)), - ) + n = p; } } + let inserted = insert_forest_internal( + self, + &other, + roots.keys().flat_map(|n| other.descendants(*n)), + &roots, + ) .expect("Trees disjoint so no repeated nodes"); // Merge the extension sets. self.extensions.extend(other.extensions()); @@ -593,9 +573,10 @@ impl HugrMut for Hugr { &mut self, other: &H, nodes: impl Iterator + Clone, - roots: impl IntoIterator, + root_parents: impl IntoIterator, ) -> InsertForestResult { - let inserted = insert_hugr_internal(self, other, nodes, roots)?; + let inserted = + insert_forest_internal(self, other, nodes, &root_parents.into_iter().collect())?; // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, copying them from the other graph. @@ -682,11 +663,11 @@ impl HugrMut for Hugr { /// - `reroot`: A function that returns the new parent for each inserted node. /// If `None`, the parent is set to the original parent after it has been inserted into `hugr`. /// If that is the case, the parent must come before the child in the `other_nodes` iterator. -fn insert_hugr_internal( +fn insert_forest_internal( hugr: &mut Hugr, other: &H, other_nodes: impl Iterator + Clone, - root_parents: impl IntoIterator, + root_parents: &HashMap, ) -> InsertForestResult { let new_node_count_hint = other_nodes.size_hint().1.unwrap_or_default(); @@ -727,7 +708,7 @@ fn insert_hugr_internal( } } for (r, p) in root_parents { - hugr.set_parent(node_map[&r], p); + hugr.set_parent(node_map[r], *p); } for old in other_nodes { let new = node_map[&old]; From 9a9c32528202fade55cf960eed79362bda5bddbf Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 19 Aug 2025 15:26:14 +0100 Subject: [PATCH 19/26] Do not require HashMap, so insert_view_forest can avoid building one --- hugr-core/src/hugr/hugrmut.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 1c8c314e69..1a1da8116a 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -550,7 +550,7 @@ impl HugrMut for Hugr { self, &other, roots.keys().flat_map(|n| other.descendants(*n)), - &roots, + roots.iter().map(|(r, p)| (*r, *p)), ) .expect("Trees disjoint so no repeated nodes"); // Merge the extension sets. @@ -575,8 +575,7 @@ impl HugrMut for Hugr { nodes: impl Iterator + Clone, root_parents: impl IntoIterator, ) -> InsertForestResult { - let inserted = - insert_forest_internal(self, other, nodes, &root_parents.into_iter().collect())?; + let inserted = insert_forest_internal(self, other, nodes, root_parents.into_iter())?; // Merge the extension sets. self.extensions.extend(other.extensions()); // Update the optypes and metadata, copying them from the other graph. @@ -667,7 +666,7 @@ fn insert_forest_internal( hugr: &mut Hugr, other: &H, other_nodes: impl Iterator + Clone, - root_parents: &HashMap, + root_parents: impl Iterator, ) -> InsertForestResult { let new_node_count_hint = other_nodes.size_hint().1.unwrap_or_default(); @@ -708,7 +707,7 @@ fn insert_forest_internal( } } for (r, p) in root_parents { - hugr.set_parent(node_map[r], *p); + hugr.set_parent(node_map[&r], p); } for old in other_nodes { let new = node_map[&old]; From ec01c365389497775eebf1986834416d3586d6cc Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 19 Aug 2025 15:47:52 +0100 Subject: [PATCH 20/26] Separate errors: DoubleCopy => DuplicateNode + SubtreeAlreadyCopied --- hugr-core/src/hugr/hugrmut.rs | 46 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 1a1da8116a..1e049c8c6b 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -282,8 +282,7 @@ pub trait HugrMut: HugrMutInternals { /// /// # Errors /// - /// [InsertForestError::DoubleCopy] if the regions in `root_parents` are not disjount - /// (the error indicates the root of the _inner_ subtree). + /// [InsertForestError::SubtreeAlreadyCopied] if the regions in `root_parents` are not disjount /// /// # Panics /// @@ -310,7 +309,7 @@ pub trait HugrMut: HugrMutInternals { /// /// # Errors /// - /// [InsertForestError::DoubleCopy] if any node appears in `nodes` more than once. + /// [InsertForestError::DuplicateNode] if any node appears in `nodes` more than once. /// /// # Panics /// @@ -365,9 +364,20 @@ pub type InsertForestResult = Result, InsertFores #[derive(Clone, Debug, derive_more::Display, derive_more::Error, PartialEq)] #[non_exhaustive] pub enum InsertForestError { - /// The specified source node would be copied twice into the target - #[display("Node/subtree {_0} would be copied twice")] - DoubleCopy(SN), + /// A source node was specified twice in a call to [HugrMut::insert_view_forest] + #[display("Node {_0} would be copied twice")] + DuplicateNode(SN), + /// A subtree would be copied twice (i.e. it is contained in another) in a call to + /// [HugrMut::insert_forest] + #[display( + "Subtree rooted at {subtree} is already being copied as part of that rooted at {parent}" + )] + SubtreeAlreadyCopied { + /// Root of the inner subtree + subtree: SN, + /// Root of the outer subtree that also contains the inner + parent: SN, + }, } /// Records the result of inserting a Hugr or view via [`HugrMut::insert_hugr`], @@ -537,13 +547,13 @@ impl HugrMut for Hugr { root_parents: impl IntoIterator, ) -> InsertForestResult { let roots: HashMap<_, _> = root_parents.into_iter().collect(); - for &r in roots.keys() { - let mut n = r; - while let Some(p) = other.get_parent(n) { - if roots.contains_key(&p) { - return Err(InsertForestError::DoubleCopy(r)); + for &subtree in roots.keys() { + let mut n = subtree; + while let Some(parent) = other.get_parent(n) { + if roots.contains_key(&parent) { + return Err(InsertForestError::SubtreeAlreadyCopied { subtree, parent }); } - n = p; + n = parent; } } let inserted = insert_forest_internal( @@ -680,7 +690,7 @@ fn insert_forest_internal( let op = OpType::default(); let new = hugr.add_node(op); if node_map.insert(old, new).is_some() { - return Err(InsertForestError::DoubleCopy(old)); + return Err(InsertForestError::DuplicateNode(old)); } hugr.set_num_ports(new, other.num_inputs(old), other.num_outputs(old)); @@ -971,12 +981,18 @@ mod test { insert.descendants(epp).chain(insert.descendants(ep)), roots, ); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); + assert_eq!(r.err(), Some(InsertForestError::DuplicateNode(ep))); assert!(h.validate().is_err()); let mut h = backup.clone(); let r = h.insert_forest(insert, roots); - assert_eq!(r.err(), Some(InsertForestError::DoubleCopy(ep))); + assert_eq!( + r.err(), + Some(InsertForestError::SubtreeAlreadyCopied { + subtree: ep, + parent: epp + }) + ); // Here the error is detected in building `nodes` from `roots` so before any mutation assert_eq!(h, backup); } From 84264987a6531051f33ee27bbe064fb5cb33d319 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 20 Aug 2025 09:46:14 +0100 Subject: [PATCH 21/26] docs --- hugr-core/src/hugr/hugrmut.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 1e049c8c6b..791ac2959d 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -277,16 +277,14 @@ pub trait HugrMut: HugrMutInternals { /// Later entries for the same region override earlier ones. /// If `root_parents` is empty, nothing is inserted. /// - /// Returns a [`HashMap`] whose keys are all the inserted nodes of `other` - /// and where each value is the corresponding (new) node in `self`. - /// /// # Errors /// - /// [InsertForestError::SubtreeAlreadyCopied] if the regions in `root_parents` are not disjount + /// [InsertForestError::SubtreeAlreadyCopied] if the regions in `root_parents` are not disjoint /// /// # Panics /// - /// If any of the keys in `roots` are not nodes in `other`, or any of the values not in `self`. + /// If any of the keys in `root_parents` are not nodes in `other`, + /// or any of the values not in `self`. fn insert_forest( &mut self, other: Hugr, @@ -305,8 +303,6 @@ pub trait HugrMut: HugrMutInternals { /// Nodes in `nodes` which are not mentioned in `root_parents` and whose parent in `other` /// is not in `nodes`, will have no parent in `self`. /// - /// Returns a [`HashMap`] from each node in `nodes` to the corresponding (new) node in `self`. - /// /// # Errors /// /// [InsertForestError::DuplicateNode] if any node appears in `nodes` more than once. @@ -400,8 +396,8 @@ pub struct InsertionResult { /// Records the result of inserting a Hugr or view via [`HugrMut::insert_forest`] /// or [`HugrMut::insert_view_forest`]. /// -/// Contains a map from the nodes in the source HUGR to the nodes in the target -/// HUGR, using their respective `Node` types. +/// Contains a map from the nodes in the source HUGR that were copied, to the +/// corresponding nodes in the target HUGR, using the respective `Node` types. #[derive(Clone, Debug, Default)] pub struct InsertedForest { /// Map from the nodes from the source Hugr/view that were inserted, @@ -669,9 +665,7 @@ impl HugrMut for Hugr { /// - `hugr`: The hugr to insert into. /// - `other`: The other graph to insert from. /// - `other_nodes`: The nodes in the other graph to insert. -/// - `reroot`: A function that returns the new parent for each inserted node. -/// If `None`, the parent is set to the original parent after it has been inserted into `hugr`. -/// If that is the case, the parent must come before the child in the `other_nodes` iterator. +/// - `root_parents`: a list of pairs of (node in `other`, parent to assign in `hugr`) fn insert_forest_internal( hugr: &mut Hugr, other: &H, From f8bdaa11eb8bfc26e28fbd8d344caac8459f7946 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 21 Aug 2025 18:14:26 +0100 Subject: [PATCH 22/26] Builder docs for #2496 --- hugr-core/src/builder/build_traits.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/hugr-core/src/builder/build_traits.rs b/hugr-core/src/builder/build_traits.rs index eb3cf2730c..ef5076f2cf 100644 --- a/hugr-core/src/builder/build_traits.rs +++ b/hugr-core/src/builder/build_traits.rs @@ -99,6 +99,8 @@ pub trait Container { } /// Insert a copy of a HUGR as a child of the container. + /// (Only the portion below the entrypoint will be inserted, with any incoming + /// edges broken; see [Dataflow::add_hugr_view_with_wires_link_nodes]) fn add_hugr_view(&mut self, child: &H) -> InsertionResult { let parent = self.container_node(); self.hugr_mut().insert_from_view(parent, child) @@ -256,7 +258,9 @@ pub trait Dataflow: Container { } /// Copy a hugr-defined op into the sibling graph, wiring up the - /// `input_wires` to the incoming ports of the resulting root node. + /// `input_wires` to the incoming ports of the node that was the entrypoint. + /// (Note, any part of `hugr` outside the entrypoint is not copied; + /// this may lead to new input ports being disconnected.) /// /// # Errors /// From 17a8de4e0e43689d6c2014c64f7bc77cc27298bd Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 21 Aug 2025 21:41:14 +0100 Subject: [PATCH 23/26] remove not-yet-existent doclink --- hugr-core/src/builder/build_traits.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hugr-core/src/builder/build_traits.rs b/hugr-core/src/builder/build_traits.rs index ef5076f2cf..3421963d87 100644 --- a/hugr-core/src/builder/build_traits.rs +++ b/hugr-core/src/builder/build_traits.rs @@ -99,8 +99,9 @@ pub trait Container { } /// Insert a copy of a HUGR as a child of the container. - /// (Only the portion below the entrypoint will be inserted, with any incoming - /// edges broken; see [Dataflow::add_hugr_view_with_wires_link_nodes]) + /// + /// Only the portion below the entrypoint will be inserted, with any incoming + /// edges broken. fn add_hugr_view(&mut self, child: &H) -> InsertionResult { let parent = self.container_node(); self.hugr_mut().insert_from_view(parent, child) From 191119f85c3326dfd8d1695dba7a83f0ec124c94 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 22 Aug 2025 17:24:15 +0100 Subject: [PATCH 24/26] Improve doc of dfg_calling_defn_decl --- hugr-core/src/hugr/hugrmut.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/hugr-core/src/hugr/hugrmut.rs b/hugr-core/src/hugr/hugrmut.rs index 791ac2959d..69434bb71a 100644 --- a/hugr-core/src/hugr/hugrmut.rs +++ b/hugr-core/src/hugr/hugrmut.rs @@ -817,8 +817,9 @@ mod test { assert_eq!(hugr.num_nodes(), 1); } - /// A DFG-entrypoint Hugr (no inputs, one bool_t output) containing two calls, - /// to a FuncDefn and a FuncDecl each bool_t->bool_t, and their handles. + /// Builds a DFG-entrypoint Hugr (no inputs, one bool_t output) containing two calls, + /// to a FuncDefn and a FuncDecl each bool_t->bool_t. + /// Returns the Hugr and both function handles. #[fixture] pub(crate) fn dfg_calling_defn_decl() -> (Hugr, FuncID, FuncID) { let mut dfb = DFGBuilder::new(Signature::new(vec![], bool_t())).unwrap(); From 685e19fc1a168c8886420e24b149bb102eb4690b Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 1 Sep 2025 18:13:13 +0100 Subject: [PATCH 25/26] insert_view_forest uses iter_batched and recreates host each time --- hugr/benches/benchmarks/hugr.rs | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index bc1f609b7b..9e42b696eb 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -80,16 +80,21 @@ fn bench_insertion(c: &mut Criterion) { ) }); group.bench_function("insert_view_forest", |b| { - let mut h = simple_dfg_hugr(); let (insert, decl, defn) = dfg_calling_defn_decl(); - // Note it would be better to use iter_batched to avoid cloning nodes/roots. - let nodes = insert.entry_descendants().chain([defn.node(), decl.node()]); - let roots = [ - (insert.entrypoint(), h.entrypoint()), - (defn.node(), h.module_root()), - (decl.node(), h.module_root()), - ]; - b.iter(|| black_box(h.insert_view_forest(&insert, nodes.clone(), roots.iter().cloned()))) + b.iter_batched( + || { + let h = simple_dfg_hugr(); + let nodes = insert.entry_descendants().chain([defn.node(), decl.node()]); + let roots = [ + (insert.entrypoint(), h.entrypoint()), + (defn.node(), h.module_root()), + (decl.node(), h.module_root()), + ]; + (h, insert, nodes, roots) + }, + |(mut h, insert, nodes, roots)| black_box(h.insert_view_forest(&insert, nodes, roots)), + BatchSize::SmallInput, + ) }); group.bench_function("insert_forest", |b| { b.iter_batched( From 57e58e29610165b5b99c96f4e185dfcc2a5fe3aa Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 1 Sep 2025 18:18:20 +0100 Subject: [PATCH 26/26] oops, fix borrow in bench --- hugr/benches/benchmarks/hugr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hugr/benches/benchmarks/hugr.rs b/hugr/benches/benchmarks/hugr.rs index 9e42b696eb..d434209411 100644 --- a/hugr/benches/benchmarks/hugr.rs +++ b/hugr/benches/benchmarks/hugr.rs @@ -90,9 +90,9 @@ fn bench_insertion(c: &mut Criterion) { (defn.node(), h.module_root()), (decl.node(), h.module_root()), ]; - (h, insert, nodes, roots) + (h, &insert, nodes, roots) }, - |(mut h, insert, nodes, roots)| black_box(h.insert_view_forest(&insert, nodes, roots)), + |(mut h, insert, nodes, roots)| black_box(h.insert_view_forest(insert, nodes, roots)), BatchSize::SmallInput, ) });