address comments

Author: lmondada

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

@@ -127,15 +127,15 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
     ///
     /// The returned port will be in `replacement`, unless the wire in the
     /// replacement is empty and `return_invalid` is
-    /// [`IncludeReplacementNodes::Valid`] (the default), in which case it
-    /// will be another `host` port. If [`IncludeReplacementNodes::All`] 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: IncludeReplacementNodes,
+        return_invalid: BoundaryMode,
     ) -> Option<BoundaryPort<HostNode, OutgoingPort>> {
         let HostPort(node, port) = port.into();
         let pos = self
@@ -155,7 +155,7 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
         &self,
         pos: usize,
         host: &impl HugrView<Node = HostNode>,
-        return_invalid: IncludeReplacementNodes,
+        return_invalid: 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 == IncludeReplacementNodes::All {
+        if out_node != repl_inp || return_invalid == BoundaryMode::IncludeIO {
             BoundaryPort::Replacement(out_node, out_port)
         } else {
             let (in_node, in_port) = *self.subgraph.incoming_ports()[out_port.index()]
@@ -214,16 +214,16 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
     ///
     /// The returned ports will be in `replacement`, unless the wires in the
     /// replacement are empty and `return_invalid` is
-    /// [`IncludeReplacementNodes::Valid`] (the default), in which case they
-    /// will be other `host` ports. If [`IncludeReplacementNodes::All`] 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: IncludeReplacementNodes,
+        return_invalid: BoundaryMode,
     ) -> impl Iterator<Item = BoundaryPort<HostNode, IncomingPort>> + 'a {
         let HostPort(node, port) = port.into();
         let positions = self
@@ -245,15 +245,15 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
         &self,
         pos: usize,
         host: &impl HugrView<Node = HostNode>,
-        return_invalid: IncludeReplacementNodes,
+        return_invalid: 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 == IncludeReplacementNodes::All {
+                if in_node != repl_out || return_invalid == 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()];
@@ -327,12 +327,8 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
         subgraph_outgoing_ports
             .enumerate()
             .flat_map(|(pos, subg_np)| {
-                self.linked_replacement_inputs_by_position(
-                    pos,
-                    host,
-                    IncludeReplacementNodes::Valid,
-                )
-                .filter_map(move |np| Some((np.as_replacement()?, subg_np)))
+                self.linked_replacement_inputs_by_position(pos, host, BoundaryMode::SnapToHost)
+                    .filter_map(move |np| Some((np.as_replacement()?, subg_np)))
             })
             .map(|((repl_node, repl_port), (subgraph_node, subgraph_port))| {
                 (
@@ -374,11 +370,7 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
             .enumerate()
             .filter_map(|(pos, subg_all)| {
                 let np = self
-                    .linked_replacement_output_by_position(
-                        pos,
-                        host,
-                        IncludeReplacementNodes::Valid,
-                    )
+                    .linked_replacement_output_by_position(pos, host, BoundaryMode::SnapToHost)
                     .as_replacement()?;
                 Some((np, subg_all))
             })
@@ -425,12 +417,8 @@ impl<HostNode: HugrNode> SimpleReplacement<HostNode> {
             .enumerate()
             .filter_map(|(pos, subg_all)| {
                 Some((
-                    self.linked_replacement_output_by_position(
-                        pos,
-                        host,
-                        IncludeReplacementNodes::Valid,
-                    )
-                    .as_host()?,
+                    self.linked_replacement_output_by_position(pos, host, BoundaryMode::SnapToHost)
+                        .as_host()?,
                     subg_all,
                 ))
             })
@@ -567,15 +555,19 @@ impl<HostNode: HugrNode> PatchVerification for SimpleReplacement<HostNode> {
 /// In [`SimpleReplacement`], some nodes in the replacement may not be valid
 /// after the replacement is applied.
 ///
-/// This enum allows to filter out such nodes.
+/// This enum allows specifying whether these invalid nodes on the boundary
+/// should be returned or should be resolved to valid nodes in the host.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Default)]
-#[non_exhaustive]
-pub enum IncludeReplacementNodes {
+pub enum BoundaryMode {
     /// Only consider nodes that are valid after the replacement is applied.
+    ///
+    /// This means that nodes in hosts may be returned in places where nodes in
+    /// the replacement would be typically expected.
     #[default]
-    Valid,
-    /// Include all nodes, including potentially invalid ones.
-    All,
+    SnapToHost,
+    /// Include all nodes, including potentially invalid ones (inputs and
+    /// outputs of replacements).
+    IncludeIO,
 }
 
 /// Result of applying a [`SimpleReplacement`].
@@ -690,7 +682,7 @@ pub(in crate::hugr::patch) mod test {
         ModuleBuilder, endo_sig, inout_sig,
     };
     use crate::extension::prelude::{bool_t, qb_t};
-    use crate::hugr::patch::simple_replace::{IncludeReplacementNodes, Outcome};
+    use crate::hugr::patch::simple_replace::{BoundaryMode, Outcome};
     use crate::hugr::patch::{BoundaryPort, HostPort, PatchVerification, ReplacementPort};
     use crate::hugr::views::{HugrView, SiblingSubgraph};
     use crate::hugr::{Hugr, HugrMut, Patch};
@@ -1186,7 +1178,7 @@ pub(in crate::hugr::patch) mod test {
             .linked_replacement_inputs(
                 (inp, OutgoingPort::from(0)),
                 &hugr,
-                IncludeReplacementNodes::Valid,
+                BoundaryMode::SnapToHost,
             )
             .collect();
 
@@ -1203,7 +1195,7 @@ pub(in crate::hugr::patch) mod test {
                 repl.linked_replacement_output(
                     (out, IncomingPort::from(i)),
                     &hugr,
-                    IncludeReplacementNodes::Valid,
+                    BoundaryMode::SnapToHost,
                 )
                 .unwrap()
             })
@@ -1240,7 +1232,7 @@ pub(in crate::hugr::patch) mod test {
             .linked_replacement_inputs(
                 (inp, OutgoingPort::from(0)),
                 &hugr,
-                IncludeReplacementNodes::Valid,
+                BoundaryMode::SnapToHost,
             )
             .collect();
 
@@ -1256,7 +1248,7 @@ pub(in crate::hugr::patch) mod test {
                 repl.linked_replacement_output(
                     (out, IncomingPort::from(i)),
                     &hugr,
-                    IncludeReplacementNodes::Valid,
+                    BoundaryMode::SnapToHost,
                 )
                 .unwrap()
             })
@@ -1298,7 +1290,7 @@ pub(in crate::hugr::patch) mod test {
             .linked_replacement_inputs(
                 (inp, OutgoingPort::from(0)),
                 &hugr,
-                IncludeReplacementNodes::Valid,
+                BoundaryMode::SnapToHost,
             )
             .collect();
 
@@ -1318,7 +1310,7 @@ pub(in crate::hugr::patch) mod test {
                 repl.linked_replacement_output(
                     (out, IncomingPort::from(i)),
                     &hugr,
-                    IncludeReplacementNodes::Valid,
+                    BoundaryMode::SnapToHost,
                 )
                 .unwrap()
             })

hugr-persistent/src/persistent_hugr.rs

@@ -618,14 +618,13 @@ fn get_parent_commits<R>(
     replacement: &PersistentReplacement,
     graph: &CommitStateSpace<R>,
 ) -> Result<Vec<Commit>, InvalidCommit> {
-    let parent_ids = replacement.invalidation_set().map(|n| n.0).unique();
+    let parent_ids = replacement.invalidation_set().map(|n| n.owner()).unique();
     parent_ids
         .map(|id| {
-            if graph.contains_id(id) {
-                Ok(graph.get_commit(id).clone())
-            } else {
-                Err(InvalidCommit::UnknownParent(id))
-            }
+            graph
+                .try_get_commit(id)
+                .cloned()
+                .ok_or(InvalidCommit::UnknownParent(id))
         })
         .collect()
 }

hugr-persistent/src/state_space.rs

@@ -11,7 +11,7 @@ use hugr_core::{
         internal::HugrInternals,
         patch::{
             BoundaryPort,
-            simple_replace::{IncludeReplacementNodes, InvalidReplacement},
+            simple_replace::{BoundaryMode, InvalidReplacement},
         },
         views::{InvalidSignature, sibling_subgraph::InvalidSubgraph},
     },
@@ -253,6 +253,12 @@ impl<R> CommitStateSpace<R> {
         self.graph.get_node(commit_id).into()
     }
 
+    /// 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())
+    }
+
     /// Get an iterator over all commit IDs in the state space.
     pub fn all_commit_ids(&self) -> impl Iterator<Item = CommitId> + Clone + '_ {
         let vec = self.graph.all_node_ids().collect_vec();
@@ -330,8 +336,10 @@ impl<R> CommitStateSpace<R> {
 
     /// Get the boundary inputs linked to `(node, port)` in `child`.
     ///
-    /// The returned ports will be in the `child` commit unless the child commit
-    /// is empty, in which case they will be in one of the parents of `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` should be a child commit of the owner of `node`.
     ///
@@ -344,7 +352,7 @@ impl<R> CommitStateSpace<R> {
         node: PatchNode,
         port: OutgoingPort,
         child: CommitId,
-        return_invalid: IncludeReplacementNodes,
+        return_invalid: BoundaryMode,
     ) -> impl Iterator<Item = (PatchNode, IncomingPort)> + '_ {
         assert!(
             self.is_boundary_edge(node, port, child),
@@ -364,8 +372,10 @@ impl<R> CommitStateSpace<R> {
 
     /// Get the single boundary output linked to `(node, port)` in `child`.
     ///
-    /// The returned port will be in the `child` commit unless the child commit
-    /// is empty, in which case it will be in one of the parents of `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`.
     ///
     /// `child` should be a child commit of the owner of `node` (or `None` will
     /// be returned).
@@ -378,7 +388,7 @@ impl<R> CommitStateSpace<R> {
         node: PatchNode,
         port: IncomingPort,
         child: CommitId,
-        return_invalid: IncludeReplacementNodes,
+        return_invalid: BoundaryMode,
     ) -> Option<(PatchNode, OutgoingPort)> {
         let parent_hugrs = ParentsView::from_commit(child, self);
         let repl = self.replacement(child)?;
@@ -400,7 +410,7 @@ impl<R> CommitStateSpace<R> {
         node: PatchNode,
         port: impl Into<Port>,
         child: CommitId,
-        return_invalid: IncludeReplacementNodes,
+        return_invalid: BoundaryMode,
     ) -> impl Iterator<Item = (PatchNode, Port)> + '_ {
         match port.into().as_directed() {
             Either::Left(incoming) => Either::Left(

hugr-persistent/src/walker.rs

@@ -66,7 +66,7 @@ use thiserror::Error;
 use hugr_core::{
     Direction, Hugr, HugrView, Port, PortIndex,
     hugr::{
-        patch::simple_replace::IncludeReplacementNodes,
+        patch::simple_replace::BoundaryMode,
         views::{RootCheckable, SiblingSubgraph},
     },
     ops::handle::DfgID,
@@ -303,11 +303,12 @@ impl<'a, R: Resolver> Walker<'a, R> {
             wire_ports_incoming.extend(w.all_incoming_ports(self.as_hugr_view()));
             wire_ports_outgoing.extend(w.single_outgoing_port(self.as_hugr_view()));
             for id in w.owners() {
-                if self.state_space.contains_id(id) {
-                    additional_parents.insert(id, self.state_space.get_commit(id).clone());
-                } else {
-                    return Err(InvalidCommit::UnknownParent(id));
-                }
+                let commit = self
+                    .state_space
+                    .try_get_commit(id)
+                    .ok_or(InvalidCommit::UnknownParent(id))?
+                    .clone();
+                additional_parents.insert(id, commit);
             }
         }
 
@@ -421,7 +422,7 @@ impl<R: Clone> Walker<'_, R> {
                     opp_node,
                     opp_port,
                     child_id,
-                    IncludeReplacementNodes::Valid,
+                    BoundaryMode::SnapToHost,
                 ) {
                     let mut empty_commits = empty_commits.clone();
                     if node.0 != child_id {
@@ -836,7 +837,5 @@ mod tests {
                 BTreeSet::from_iter([base_commit, empty_commit])
             ])
         );
-
-        panic!("explicit")
     }
 }

hugr-persistent/src/wire.rs

@@ -2,9 +2,9 @@ use std::collections::{BTreeSet, VecDeque};
 
 use hugr_core::{
     Direction, HugrView, IncomingPort, OutgoingPort, Port, Wire,
-    hugr::patch::simple_replace::IncludeReplacementNodes,
+    hugr::patch::simple_replace::BoundaryMode,
 };
-use itertools::{Either, Itertools};
+use itertools::Itertools;
 
 use crate::{CommitId, PatchNode, PersistentHugr, Resolver, Walker};
 
@@ -52,6 +52,12 @@ impl CommitWire {
     fn commit_id(&self) -> CommitId {
         self.0.node().0
     }
+
+    delegate::delegate! {
+        to self.0 {
+            fn node(&self) -> PatchNode;
+        }
+    }
 }
 
 /// A node in a commit of a [`PersistentHugr`] is either a valid node of the
@@ -120,7 +126,7 @@ impl PersistentWire {
                                     opp_node,
                                     opp_port,
                                     deleted_by,
-                                    IncludeReplacementNodes::All,
+                                    BoundaryMode::IncludeIO,
                                 )
                             {
                                 debug_assert_eq!(child_node.owner(), deleted_by);