address comments

lmondada · lmondada · commit e13ab9016bed · 2025-07-09T13:07:21.000+02:00
diff --git a/hugr-core/src/hugr/patch/simple_replace.rs b/hugr-core/src/hugr/patch/simple_replace.rs
@@ -126,16 +126,16 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
     /// of `self`.
     ///
     /// The returned port will be in `replacement`, unless the wire in the
-    /// replacement is empty and `return_invalid` is
-    /// [`BoundaryMode::SnapToHost`] (the default), in which case it
-    /// will be another `host` port. If [`BoundaryMode::IncludeIO`] is
-    /// passed, the returned port will always be in `replacement` even if it
-    /// is invalid (i.e. it is an IO node in the replacement).
+    /// replacement is empty and `boundary` is [`BoundaryMode::SnapToHost`] (the
+    /// default), in which case it will be another `host` port. If
+    /// [`BoundaryMode::IncludeIO`] is passed, the returned port will always
+    /// be in `replacement` even if it is invalid (i.e. it is an IO node in
+    /// the replacement).
     pub fn linked_replacement_output(
         &self,
         port: impl Into<HostPort<HostNode, IncomingPort>>,
         host: &impl HugrView<Node = HostNode>,
-        return_invalid: BoundaryMode,
+        boundary: BoundaryMode,
     ) -> Option<BoundaryPort<HostNode, OutgoingPort>> {
         let HostPort(node, port) = port.into();
         let pos = self
@@ -144,7 +144,7 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
             .iter()
             .position(move |&(n, p)| host.linked_inputs(n, p).contains(&(node, port)))?;
 
-        Some(self.linked_replacement_output_by_position(pos, host, return_invalid))
+        Some(self.linked_replacement_output_by_position(pos, host, boundary))
     }
 
     /// The outgoing port linked to the i-th output boundary edge of `subgraph`.
@@ -155,7 +155,7 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
         &self,
         pos: usize,
         host: &impl HugrView<Node = HostNode>,
-        return_invalid: BoundaryMode,
+        boundary: BoundaryMode,
     ) -> BoundaryPort<HostNode, OutgoingPort> {
         debug_assert!(pos < self.subgraph().signature(host).output_count());
 
@@ -166,7 +166,7 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
             .single_linked_output(repl_out, pos)
             .expect("valid dfg wire");
 
-        if out_node != repl_inp || return_invalid == BoundaryMode::IncludeIO {
+        if out_node != repl_inp || boundary == BoundaryMode::IncludeIO {
             BoundaryPort::Replacement(out_node, out_port)
         } else {
             let (in_node, in_port) = *self.subgraph.incoming_ports()[out_port.index()]
@@ -213,17 +213,16 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
     /// of `self`.
     ///
     /// The returned ports will be in `replacement`, unless the wires in the
-    /// replacement are empty and `return_invalid` is
-    /// [`BoundaryMode::SnapToHost`] (the default), in which case they
-    /// will be other `host` ports. If [`BoundaryMode::IncludeIO`] is
-    /// passed, the returned ports will always be in
-    /// `replacement` even if they are invalid (i.e. they are an IO node in
-    /// the replacement).
+    /// replacement are empty and `boundary` is [`BoundaryMode::SnapToHost`]
+    /// (the default), in which case they will be other `host` ports. If
+    /// [`BoundaryMode::IncludeIO`] is passed, the returned ports will
+    /// always be in `replacement` even if they are invalid (i.e. they are
+    /// an IO node in the replacement).
     pub fn linked_replacement_inputs<'a>(
         &'a self,
         port: impl Into<HostPort<HostNode, OutgoingPort>>,
         host: &'a impl HugrView<Node = HostNode>,
-        return_invalid: BoundaryMode,
+        boundary: BoundaryMode,
     ) -> impl Iterator<Item = BoundaryPort<HostNode, IncomingPort>> + 'a {
         let HostPort(node, port) = port.into();
         let positions = self
@@ -235,25 +234,24 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
                 host.single_linked_output(n, p).expect("valid dfg wire") == (node, port)
             });
 
-        positions.flat_map(move |pos| {
-            self.linked_replacement_inputs_by_position(pos, host, return_invalid)
-        })
+        positions
+            .flat_map(move |pos| self.linked_replacement_inputs_by_position(pos, host, boundary))
     }
 
     /// The incoming ports linked to the i-th input boundary edge of `subgraph`.
     fn linked_replacement_inputs_by_position(
         &self,
         pos: usize,
         host: &impl HugrView<Node = HostNode>,
-        return_invalid: BoundaryMode,
+        boundary: BoundaryMode,
     ) -> impl Iterator<Item = BoundaryPort<HostNode, IncomingPort>> {
         debug_assert!(pos < self.subgraph().signature(host).input_count());
 
         let [repl_inp, repl_out] = self.get_replacement_io();
         self.replacement
             .linked_inputs(repl_inp, pos)
             .flat_map(move |(in_node, in_port)| {
-                if in_node != repl_out || return_invalid == BoundaryMode::IncludeIO {
+                if in_node != repl_out || boundary == BoundaryMode::IncludeIO {
                     Either::Left(std::iter::once(BoundaryPort::Replacement(in_node, in_port)))
                 } else {
                     let (out_node, out_port) = self.subgraph.outgoing_ports()[in_port.index()];
@@ -552,8 +550,8 @@ impl<HostNode: HugrNode> PatchVerification for SimpleReplacement<HostNode> {
     }
 }
 
-/// In [`SimpleReplacement`], some nodes in the replacement may not be valid
-/// after the replacement is applied.
+/// In [`SimpleReplacement::replacement`], IO nodes marking the boundary will
+/// not be valid nodes in the host after the replacement is applied.
 ///
 /// This enum allows specifying whether these invalid nodes on the boundary
 /// should be returned or should be resolved to valid nodes in the host.
diff --git a/hugr-persistent/src/state_space.rs b/hugr-persistent/src/state_space.rs
@@ -256,7 +256,7 @@ impl<R> CommitStateSpace<R> {
     /// Check whether `commit_id` exists and return it.
     pub fn try_get_commit(&self, commit_id: CommitId) -> Option<&Commit> {
         self.contains_id(commit_id)
-            .then(|| self.graph.get_node(commit_id).into())
+            .then(|| self.get_commit(commit_id))
     }
 
     /// Get an iterator over all commit IDs in the state space.
@@ -337,9 +337,9 @@ impl<R> CommitStateSpace<R> {
     /// Get the boundary inputs linked to `(node, port)` in `child`.
     ///
     /// The returned ports will be ports on successors of the input node in the
-    /// `child` commit, unless (node, port) is connected to an empty wire in
-    /// `child` (i.e. a wire from input node to output node), in which case
-    /// they will be in one of the parents of `child`.
+    /// `child` commit, unless (node, port) is connected to a passthrough wire
+    /// in `child` (i.e. a wire from input node to output node), in which
+    /// case they will be in one of the parents of `child`.
     ///
     /// `child` should be a child commit of the owner of `node`.
     ///
@@ -372,10 +372,10 @@ impl<R> CommitStateSpace<R> {
 
     /// Get the single boundary output linked to `(node, port)` in `child`.
     ///
-    /// The returned port will be ports on predecessors of the output node in
-    /// the `child` commit, unless (node, port) is connected to an empty wire
-    /// in `child` (i.e. a wire from input node to output node), in which
-    /// case it will be in one of the parents of `child`.
+    /// The returned port will be a port on a predecessor of the output node in
+    /// the `child` commit, unless (node, port) is connected to a passthrough
+    /// wire in `child` (i.e. a wire from input node to output node), in
+    /// which case it will be in one of the parents of `child`.
     ///
     /// `child` should be a child commit of the owner of `node` (or `None` will
     /// be returned).
@@ -425,8 +425,11 @@ impl<R> CommitStateSpace<R> {
         }
     }
 
-    /// Get the single output boundary port linked to `(node, port)` in a
-    /// parent of the commit of `node`.
+    /// Get the single output port linked to `(node, port)` in a parent of the
+    /// commit of `node`.
+    ///
+    /// The returned port belongs to the input boundary of the subgraph in
+    /// parent.
     ///
     /// ## Panics
     ///
@@ -450,8 +453,11 @@ impl<R> CommitStateSpace<R> {
         repl.linked_host_input((node, port), &parent_hugrs).into()
     }
 
-    /// Get the input boundary ports linked to `(node, port)` in a
-    /// parent of the commit of `node`.
+    /// Get the input ports linked to `(node, port)` in a parent of the commit
+    /// of `node`.
+    ///
+    /// The returned ports belong to the output boundary of the subgraph in
+    /// parent.
     ///
     /// ## Panics
     ///
diff --git a/hugr-persistent/src/tests.rs b/hugr-persistent/src/tests.rs
@@ -300,15 +300,14 @@ pub(super) fn persistent_hugr_empty_child() -> (PersistentHugr, [CommitId; 2], [
     let (triple_not_hugr, not_nodes) = {
         let mut dfg_builder = DFGBuilder::new(endo_sig(bool_t())).unwrap();
         let [mut w] = dfg_builder.input_wires_arr();
-        let mut not_nodes = Vec::with_capacity(3);
-        for _ in 0..3 {
+        let not_nodes = [(); 3].map(|()| {
             let handle = dfg_builder.add_dataflow_op(LogicOp::Not, vec![w]).unwrap();
             [w] = handle.outputs_arr();
-            not_nodes.push(handle.node());
-        }
+            handle.node()
+        });
         (
             dfg_builder.finish_hugr_with_outputs([w]).unwrap(),
-            not_nodes.into_iter().collect_array::<3>().unwrap(),
+            not_nodes,
         )
     };
     let mut hugr = PersistentHugr::with_base(triple_not_hugr);
diff --git a/hugr-persistent/src/walker.rs b/hugr-persistent/src/walker.rs
@@ -344,7 +344,7 @@ impl<'a, R: Resolver> Walker<'a, R> {
             let mut repl = repl.try_into_checked().expect("replacement is not DFG");
             let new_inputs = incoming
                 .iter()
-                .flatten()
+                .flatten() // because of singleton-vec wrapping above
                 .map(|&(n, p)| {
                     map_boundary(n, p.into())
                         .as_outgoing()
@@ -450,7 +450,7 @@ impl<R: Clone> Walker<'_, R> {
 impl<R: Resolver> Walker<'_, R> {
     // Check walker equality by comparing pointers to the state space and
     // other fields. Only for testing purposes.
-    fn ptr_eq(&self, other: &Self) -> bool {
+    fn component_wise_ptr_eq(&self, other: &Self) -> bool {
         std::ptr::eq(self.state_space.as_ref(), other.state_space.as_ref())
             && self.pinned_nodes == other.pinned_nodes
             && BTreeSet::from_iter(self.selected_commits.all_commit_ids())
@@ -463,7 +463,7 @@ impl<R: Resolver> Walker<'_, R> {
         let Some([new_walker]) = self.expand(wire, dir).collect_array() else {
             return false;
         };
-        new_walker.ptr_eq(self)
+        new_walker.component_wise_ptr_eq(self)
     }
 }
 
diff --git a/hugr-persistent/tests/persistent_walker_example.rs b/hugr-persistent/tests/persistent_walker_example.rs
@@ -271,32 +271,31 @@ fn create_commit(wire: PersistentWire, walker: &Walker) -> Option<Commit> {
                 //  - the first qubit is the one that is shared between the two CZ gates
                 //  - the second qubit only touches the first CZ (out_node)
                 //  - the third qubit only touches the second CZ (in_node)
-                match (port.as_directed(), node == out_node) {
-                    (Either::Left(incoming), true) if incoming.index() == shared_qb_out => {
-                        // out_node on the shared qubit -> port 0
-                        OutgoingPort::from(0).into()
+                match port.as_directed() {
+                    Either::Left(incoming) => {
+                        let in_boundary: [(_, IncomingPort); 3] = [
+                            (out_node, shared_qb_out.into()),
+                            (out_node, (1 - shared_qb_out).into()),
+                            (in_node, (1 - shared_qb_in).into()),
+                        ];
+                        let out_index = in_boundary
+                            .iter()
+                            .position(|&(n, p)| n == node && p == incoming)
+                            .expect("invalid input port");
+                        OutgoingPort::from(out_index).into()
                     }
-                    (Either::Left(incoming), true) if incoming.index() == 1 - shared_qb_out => {
-                        // out_node on the not shared qubit -> port 1
-                        OutgoingPort::from(1).into()
+                    Either::Right(outgoing) => {
+                        let out_boundary: [(_, OutgoingPort); 3] = [
+                            (in_node, shared_qb_in.into()),
+                            (out_node, (1 - shared_qb_out).into()),
+                            (in_node, (1 - shared_qb_in).into()),
+                        ];
+                        let in_index = out_boundary
+                            .iter()
+                            .position(|&(n, p)| n == node && p == outgoing)
+                            .expect("invalid output port");
+                        IncomingPort::from(in_index).into()
                     }
-                    (Either::Left(incoming), false) if incoming.index() == 1 - shared_qb_in => {
-                        // in_node on the not shared qubit -> port 2
-                        OutgoingPort::from(2).into()
-                    }
-                    (Either::Right(outgoing), false) if outgoing.index() == shared_qb_in => {
-                        // in_node on the shared qubit -> port 0
-                        IncomingPort::from(0).into()
-                    }
-                    (Either::Right(outgoing), true) if outgoing.index() == 1 - shared_qb_out => {
-                        // out_node on the not shared qubit -> port 1
-                        IncomingPort::from(1).into()
-                    }
-                    (Either::Right(outgoing), false) if outgoing.index() == 1 - shared_qb_in => {
-                        // in_node on the not shared qubit -> port 2
-                        IncomingPort::from(2).into()
-                    }
-                    _ => panic!("unexpected boundary port"),
                 }
             })
         }
@@ -305,7 +304,7 @@ fn create_commit(wire: PersistentWire, walker: &Walker) -> Option<Commit> {
     .ok()
 }
 
-#[ignore = "takes 10s (to be optimised)"]
+#[ignore = "takes 10s (todo: optimise)"]
 #[test]
 fn walker_example() {
     let state_space = build_state_space();
@@ -340,7 +339,6 @@ fn walker_example() {
     // and such that the resulting HUGR is empty
     let mut empty_hugr = None;
     for cs in empty_commits.iter().combinations(3) {
-        // for cs in empty_commits.iter().combinations(2) {
         let cs = cs.into_iter().copied();
         if let Ok(hugr) = state_space.try_extract_hugr(cs) {
             empty_hugr = Some(hugr);
