From eddb50869bd84912be21d1343e549473df435897 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 16 Oct 2023 15:57:23 +0100 Subject: [PATCH] refactor: insert_subgraph just return HashMap, make InsertionResult new_root compulsory (#609) insert_subgraph reused InsertionResult by making new_root into an Option. The "reuse" is trivial - what's left is a single field - so this avoids confusion as to when `new_root` is or is not None and thus a bunch of unwraps. --- src/builder/build_traits.rs | 4 +-- src/hugr/hugrmut.rs | 50 ++++++++++++------------------ src/hugr/rewrite/outline_cfg.rs | 2 +- src/hugr/views/sibling_subgraph.rs | 4 +-- 4 files changed, 24 insertions(+), 36 deletions(-) diff --git a/src/builder/build_traits.rs b/src/builder/build_traits.rs index a1411dcd2..e4ec74191 100644 --- a/src/builder/build_traits.rs +++ b/src/builder/build_traits.rs @@ -231,7 +231,7 @@ pub trait Dataflow: Container { input_wires: impl IntoIterator, ) -> Result, BuildError> { let num_outputs = hugr.get_optype(hugr.root()).signature().output_count(); - let node = self.add_hugr(hugr)?.new_root.unwrap(); + let node = self.add_hugr(hugr)?.new_root; let inputs = input_wires.into_iter().collect(); wire_up_inputs(inputs, node, self)?; @@ -252,7 +252,7 @@ pub trait Dataflow: Container { input_wires: impl IntoIterator, ) -> Result, BuildError> { let num_outputs = hugr.get_optype(hugr.root()).signature().output_count(); - let node = self.add_hugr_view(hugr)?.new_root.unwrap(); + let node = self.add_hugr_view(hugr)?.new_root; let inputs = input_wires.into_iter().collect(); wire_up_inputs(inputs, node, self)?; diff --git a/src/hugr/hugrmut.rs b/src/hugr/hugrmut.rs index 464646401..7f91752b5 100644 --- a/src/hugr/hugrmut.rs +++ b/src/hugr/hugrmut.rs @@ -164,8 +164,8 @@ pub trait HugrMut: HugrMutInternals { /// /// Sibling order is not preserved. /// - /// The returned `InsertionResult` does not contain a `new_root` value, since - /// a subgraph may not have a defined root. + /// The return value is a map from indices in `other` to the indices of the + /// corresponding new nodes in `self`. // // TODO: Try to preserve the order when possible? We cannot always ensure // it, since the subgraph may have arbitrary nodes without including their @@ -175,7 +175,7 @@ pub trait HugrMut: HugrMutInternals { root: Node, other: &impl HugrView, subgraph: &SiblingSubgraph, - ) -> Result { + ) -> Result, HugrError> { self.valid_node(root)?; self.hugr_mut().insert_subgraph(root, other, subgraph) } @@ -195,24 +195,14 @@ pub struct InsertionResult { /// The node, after insertion, that was the root of the inserted Hugr. /// /// That is, the value in [InsertionResult::node_map] under the key that was the [HugrView::root] - /// - /// When inserting a subgraph, this value is `None`. - pub new_root: Option, + pub new_root: Node, /// Map from nodes in the Hugr/view that was inserted, to their new /// positions in the Hugr into which said was inserted. pub node_map: HashMap, } -impl InsertionResult { - fn translating_indices( - new_root: Option, - node_map: HashMap, - ) -> Self { - Self { - new_root, - node_map: HashMap::from_iter(node_map.into_iter().map(|(k, v)| (k.into(), v.into()))), - } - } +fn translate_indices(node_map: HashMap) -> HashMap { + HashMap::from_iter(node_map.into_iter().map(|(k, v)| (k.into(), v.into()))) } /// Impl for non-wrapped Hugrs. Overwrites the recursive default-impls to directly use the hugr. @@ -298,7 +288,7 @@ impl + AsMut> HugrMut for T { } fn insert_hugr(&mut self, root: Node, mut other: Hugr) -> Result { - let (other_root, node_map) = insert_hugr_internal(self.as_mut(), root, &other)?; + let (new_root, node_map) = insert_hugr_internal(self.as_mut(), root, &other)?; // Update the optypes and metadata, taking them from the other graph. for (&node, &new_node) in node_map.iter() { let optype = other.op_types.take(node); @@ -306,11 +296,11 @@ impl + AsMut> HugrMut for T { let meta = other.metadata.take(node); self.as_mut().set_metadata(new_node.into(), meta).unwrap(); } - debug_assert_eq!(Some(&other_root.index), node_map.get(&other.root().index)); - Ok(InsertionResult::translating_indices( - Some(other_root), - node_map, - )) + debug_assert_eq!(Some(&new_root.index), node_map.get(&other.root().index)); + Ok(InsertionResult { + new_root, + node_map: translate_indices(node_map), + }) } fn insert_from_view( @@ -318,7 +308,7 @@ impl + AsMut> HugrMut for T { root: Node, other: &impl HugrView, ) -> Result { - let (other_root, node_map) = insert_hugr_internal(self.as_mut(), root, other)?; + let (new_root, node_map) = insert_hugr_internal(self.as_mut(), root, other)?; // Update the optypes and metadata, copying them from the other graph. for (&node, &new_node) in node_map.iter() { let nodetype = other.get_nodetype(node.into()); @@ -328,11 +318,11 @@ impl + AsMut> HugrMut for T { .set_metadata(new_node.into(), meta.clone()) .unwrap(); } - debug_assert_eq!(Some(&other_root.index), node_map.get(&other.root().index)); - Ok(InsertionResult::translating_indices( - Some(other_root), - node_map, - )) + debug_assert_eq!(Some(&new_root.index), node_map.get(&other.root().index)); + Ok(InsertionResult { + new_root, + node_map: translate_indices(node_map), + }) } fn insert_subgraph( @@ -340,7 +330,7 @@ impl + AsMut> HugrMut for T { root: Node, other: &impl HugrView, subgraph: &SiblingSubgraph, - ) -> Result { + ) -> Result, HugrError> { // Create a portgraph view with the explicit list of nodes defined by the subgraph. let portgraph: NodeFiltered<_, NodeFilter<&[Node]>, &[Node]> = NodeFiltered::new_node_filtered( @@ -358,7 +348,7 @@ impl + AsMut> HugrMut for T { .set_metadata(new_node.into(), meta.clone()) .unwrap(); } - Ok(InsertionResult::translating_indices(None, node_map)) + Ok(translate_indices(node_map)) } } diff --git a/src/hugr/rewrite/outline_cfg.rs b/src/hugr/rewrite/outline_cfg.rs index 691fb8a40..bb0546ae3 100644 --- a/src/hugr/rewrite/outline_cfg.rs +++ b/src/hugr/rewrite/outline_cfg.rs @@ -150,7 +150,7 @@ impl Rewrite for OutlineCfg { .insert_hugr(outer_cfg, new_block_bldr.hugr().clone()) .unwrap(); ( - ins_res.new_root.unwrap(), + ins_res.new_root, *ins_res.node_map.get(&cfg.node()).unwrap(), ) }; diff --git a/src/hugr/views/sibling_subgraph.rs b/src/hugr/views/sibling_subgraph.rs index 2dbc03a32..312088d4d 100644 --- a/src/hugr/views/sibling_subgraph.rs +++ b/src/hugr/views/sibling_subgraph.rs @@ -424,9 +424,7 @@ impl SiblingSubgraph { // Take the unfinished Hugr from the builder, to avoid unnecessary // validation checks that require connecting the inputs and outputs. let mut extracted = mem::take(builder.hugr_mut()); - let node_map = extracted - .insert_subgraph(extracted.root(), hugr, self)? - .node_map; + let node_map = extracted.insert_subgraph(extracted.root(), hugr, self)?; // Connect the inserted nodes in-between the input and output nodes. let [inp, out] = extracted.get_io(extracted.root()).unwrap();