From 2467729ffb78bbd9827ded973e5e4a4b8b8b6864 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 16 Oct 2023 12:12:52 +0100 Subject: [PATCH 01/70] Add Replacement struct, NewEdgeSpec/NewEdgeKind, verify, invalidation_set --- src/hugr/rewrite.rs | 1 + src/hugr/rewrite/replace.rs | 149 ++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) create mode 100644 src/hugr/rewrite/replace.rs diff --git a/src/hugr/rewrite.rs b/src/hugr/rewrite.rs index 697644942..3f524e2db 100644 --- a/src/hugr/rewrite.rs +++ b/src/hugr/rewrite.rs @@ -2,6 +2,7 @@ pub mod insert_identity; pub mod outline_cfg; +pub mod replace; pub mod simple_replace; use crate::{Hugr, HugrView, Node}; diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs new file mode 100644 index 000000000..c3af650c9 --- /dev/null +++ b/src/hugr/rewrite/replace.rs @@ -0,0 +1,149 @@ +//! Implementation of the `Replace` operation. + +use std::collections::hash_map::Values; +use std::collections::hash_set::Iter; +use std::collections::{HashMap, HashSet, VecDeque}; +use std::iter::{Chain, Copied, Take}; + +use itertools::Itertools; +use thiserror::Error; + +use crate::ops::OpTrait; +use crate::types::Type; +use crate::{Hugr, HugrView, Node}; + +use super::Rewrite; + +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct NewEdgeSpec { + src: Node, + tgt: Node, + kind: NewEdgeKind, +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum NewEdgeKind { + Order, + Value { + ty: Type, + src_pos: usize, + tgt_pos: usize, + }, + Static { + ty: Type, + src_pos: usize, + tgt_pos: usize, + }, + ControlFlow { + src_pos: usize, + }, +} + +/// Specification of a `Replace` operation +#[derive(Debug, Clone)] // PartialEq? Eq probably doesn't make sense because of ordering. +pub struct Replacement { + /// The nodes to remove from the existing Hugr (known as Gamma). + /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. + removal: HashSet, + /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. + replacement: Hugr, + /// Map from container nodes in [replacement] that have no children, to container nodes + /// that are descended from [nodes]. The keys are the new parents for the children of the + /// values, i.e. said children are transferred to new locations rather than removed from the graph. + /// Note no value may be ancestor/descendant of another. "R" is the set of descendants of [nodes] + /// that are not descendants of values here. + transfers: HashMap, + /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) + /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. + in_edges: Vec, + /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed + /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. + out_edges: Vec, + /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). + /// For example, in cases where the source had an edge to a removed node, and the target had an + /// edge from a removed node, this would allow source to be directly connected to target. + new_edges: Vec, +} + +impl Rewrite for Replacement { + type Error = ReplaceError; + + type ApplyResult = (); + + type InvalidationSet<'a> = // as IntoIterator>::IntoIter + Chain< + Chain< + Copied>, + Copied>>, + Copied>>> + where + Self: 'a; + + const UNCHANGED_ON_FAILURE: bool = false; + + fn verify(&self, h: &impl crate::HugrView) -> Result<(), Self::Error> { + let parent = match self + .removal + .iter() + .map(|n| h.get_parent(*n)) + .unique() + .exactly_one() + { + Ok(Some(p)) => Ok(p), + Ok(None) => Err(ReplaceError( + "Cannot replace the top node of the Hugr".to_string(), + )), // maybe we SHOULD be able to?? + Err(e) => Err(ReplaceError(format!( + "Nodes to remove do not share a parent: {}", + e + ))), + }?; + // Should we require exactly equality? + let expected_tag = h.get_optype(parent).tag(); + let found_tag = self.replacement.root_type().tag(); + if expected_tag != found_tag { + return Err(ReplaceError(format!( + "Expected replacement to have root node w/tag {} but found {}", + expected_tag, found_tag + ))); + }; + let mut transferred: HashSet = self.transfers.values().copied().collect(); + if transferred.len() != self.transfers.values().len() { + return Err(ReplaceError( + "Repeated nodes in RHS of transfer map".to_string(), + )); + } + let mut removed = HashSet::new(); + let mut queue = VecDeque::from_iter(self.removal.iter().copied()); + while let Some(n) = queue.pop_front() { + let new = removed.insert(n); + debug_assert!(new); // Fails only if h's hierarchy has merges (is not a tree) + if !transferred.remove(&n) { + h.children(n).for_each(|ch| queue.push_back(ch)) + } + } + if !transferred.is_empty() { + return Err(ReplaceError( + "Some transferred nodes were not to be removed".to_string(), + )); + } + Ok(()) + } + + fn apply(self, _h: &mut impl crate::hugr::HugrMut) -> Result { + todo!() + } + + fn invalidation_set(&self) -> Self::InvalidationSet<'_> { + self.removal + .iter() + .copied() + .chain(self.transfers.values().copied()) + .chain(self.removal.iter().take(1).copied()) + } +} + +/// Error in a [`Replacement`] +#[derive(Clone, Debug, PartialEq, Eq, Error)] +#[error("{0}")] +pub struct ReplaceError(String); From 8e531d4110bd9eecc3d3f013935044f4e5630c32 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 16 Oct 2023 14:51:59 +0100 Subject: [PATCH 02/70] WIP Implement --- src/hugr/rewrite/replace.rs | 115 ++++++++++++++++++++++++++++++------ 1 file changed, 98 insertions(+), 17 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index c3af650c9..7df416b53 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -8,9 +8,11 @@ use std::iter::{Chain, Copied, Take}; use itertools::Itertools; use thiserror::Error; +use crate::hugr::hugrmut::InsertionResult; +use crate::hugr::HugrMut; use crate::ops::OpTrait; -use crate::types::Type; -use crate::{Hugr, HugrView, Node}; +use crate::types::EdgeKind; +use crate::{Hugr, HugrView, Node, Port}; use super::Rewrite; @@ -24,19 +26,9 @@ pub struct NewEdgeSpec { #[derive(Clone, Debug, PartialEq, Eq)] pub enum NewEdgeKind { Order, - Value { - ty: Type, - src_pos: usize, - tgt_pos: usize, - }, - Static { - ty: Type, - src_pos: usize, - tgt_pos: usize, - }, - ControlFlow { - src_pos: usize, - }, + Value { src_pos: usize, tgt_pos: usize }, + Static { src_pos: usize, tgt_pos: usize }, + ControlFlow { src_pos: usize }, } /// Specification of a `Replace` operation @@ -44,6 +36,8 @@ pub enum NewEdgeKind { pub struct Replacement { /// The nodes to remove from the existing Hugr (known as Gamma). /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. + /// Must be non-empty - otherwise there is no parent under which to place [replacement], + /// and no possible [in_edges], [out_edges] or [transfers]. removal: HashSet, /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. replacement: Hugr, @@ -127,11 +121,61 @@ impl Rewrite for Replacement { "Some transferred nodes were not to be removed".to_string(), )); } + // TODO check edges?? Ok(()) } - fn apply(self, _h: &mut impl crate::hugr::HugrMut) -> Result { - todo!() + fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { + let parent = h.get_parent(*self.removal.iter().next().unwrap()).unwrap(); + // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want + let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement)?; + + // 2. Add new edges from existing to copied nodes according to mu_in + let translate_idx = |n| *node_map.get(&n).unwrap(); + let id = |n| n; + transfer_edges(h, self.in_edges.iter(), id, translate_idx, false)?; + + // 3. Add new edges from copied to existing nodes according to mu_out, + // replacing existing value/static edges incoming to targets + transfer_edges(h, self.out_edges.iter(), translate_idx, id, true)?; + + //4. Add new edges between existing nodes according to mu_new, + // replacing existing value/static edges incoming to targets + transfer_edges(h, self.new_edges.iter(), id, id, true)?; + + // 5. Put newly-added copies into correct places in hierarchy + // (these will be correct places after removing nodes) + let mut remove = self.removal.iter(); + for new_node in h.children(new_root).collect::>().into_iter() { + let new_node = *node_map.get(&new_node).unwrap(); + if let Some(to_remove) = remove.next() { + h.move_before_sibling(new_node, *to_remove).unwrap(); + } else { + h.set_parent(new_node, parent).unwrap(); + } + } + debug_assert!(h.children(new_root).next().is_none()); + h.remove_node(new_root).unwrap(); + + // 6. Transfer to keys of `transfers` children of the corresponding values. + fn first_child(h: &impl HugrView, parent: Node) -> Option { + h.children(parent).next() + } + for (new_parent, old_parent) in self.transfers.iter() { + debug_assert!(h.children(*old_parent).next().is_some()); + while let Some(ch) = first_child(h, *old_parent) { + h.set_parent(ch, *new_parent).unwrap(); + } + } + + // 7. Remove remaining nodes + let mut to_remove = VecDeque::from_iter(self.removal); + while let Some(n) = to_remove.pop_front() { + // Nodes may have no children if they were moved in step 6 + to_remove.extend(h.children(n)); + h.remove_node(n).unwrap(); + } + Ok(()) } fn invalidation_set(&self) -> Self::InvalidationSet<'_> { @@ -143,6 +187,43 @@ impl Rewrite for Replacement { } } +fn transfer_edges<'a>( + h: &mut impl HugrMut, + edges: impl Iterator, + trans_src: impl Fn(Node) -> Node, + trans_tgt: impl Fn(Node) -> Node, + remove_existing: bool, +) -> Result<(), ReplaceError> { + for e in edges { + let src = trans_src(e.src); + let tgt = trans_tgt(e.tgt); + match e.kind { + NewEdgeKind::Order => { + if h.get_optype(src).other_output() != Some(EdgeKind::StateOrder) { + return Err(ReplaceError( + "Can't insert Order edge except from DFG node".to_string(), + )); + } + if h.get_parent(tgt) != h.get_parent(src) { + return Err(ReplaceError( + "Order edge target not at top level".to_string(), + )); + } + h.add_other_edge(src, tgt).unwrap(); + } + NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { + if remove_existing { + h.disconnect(tgt, Port::new(portgraph::Direction::Incoming, tgt_pos)) + .unwrap(); + } + h.connect(src, src_pos, tgt, tgt_pos).unwrap(); + } + NewEdgeKind::ControlFlow { src_pos } => h.connect(src, src_pos, tgt, 0).unwrap(), + } + } + Ok(()) +} + /// Error in a [`Replacement`] #[derive(Clone, Debug, PartialEq, Eq, Error)] #[error("{0}")] From e9f4ba856d67fa3fb99e6e725bf9d21a8a42b5da Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 16 Oct 2023 19:11:32 +0100 Subject: [PATCH 03/70] Allow returning HugrError or Msg --- src/hugr/rewrite/replace.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 7df416b53..5f3966048 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -9,7 +9,7 @@ use itertools::Itertools; use thiserror::Error; use crate::hugr::hugrmut::InsertionResult; -use crate::hugr::HugrMut; +use crate::hugr::{HugrError, HugrMut}; use crate::ops::OpTrait; use crate::types::EdgeKind; use crate::{Hugr, HugrView, Node, Port}; @@ -84,10 +84,10 @@ impl Rewrite for Replacement { .exactly_one() { Ok(Some(p)) => Ok(p), - Ok(None) => Err(ReplaceError( + Ok(None) => Err(ReplaceError::Msg( "Cannot replace the top node of the Hugr".to_string(), )), // maybe we SHOULD be able to?? - Err(e) => Err(ReplaceError(format!( + Err(e) => Err(ReplaceError::Msg(format!( "Nodes to remove do not share a parent: {}", e ))), @@ -96,14 +96,14 @@ impl Rewrite for Replacement { let expected_tag = h.get_optype(parent).tag(); let found_tag = self.replacement.root_type().tag(); if expected_tag != found_tag { - return Err(ReplaceError(format!( + return Err(ReplaceError::Msg(format!( "Expected replacement to have root node w/tag {} but found {}", expected_tag, found_tag ))); }; let mut transferred: HashSet = self.transfers.values().copied().collect(); if transferred.len() != self.transfers.values().len() { - return Err(ReplaceError( + return Err(ReplaceError::Msg( "Repeated nodes in RHS of transfer map".to_string(), )); } @@ -117,7 +117,7 @@ impl Rewrite for Replacement { } } if !transferred.is_empty() { - return Err(ReplaceError( + return Err(ReplaceError::Msg( "Some transferred nodes were not to be removed".to_string(), )); } @@ -200,12 +200,12 @@ fn transfer_edges<'a>( match e.kind { NewEdgeKind::Order => { if h.get_optype(src).other_output() != Some(EdgeKind::StateOrder) { - return Err(ReplaceError( + return Err(ReplaceError::Msg( "Can't insert Order edge except from DFG node".to_string(), )); } if h.get_parent(tgt) != h.get_parent(src) { - return Err(ReplaceError( + return Err(ReplaceError::Msg( "Order edge target not at top level".to_string(), )); } @@ -226,5 +226,9 @@ fn transfer_edges<'a>( /// Error in a [`Replacement`] #[derive(Clone, Debug, PartialEq, Eq, Error)] -#[error("{0}")] -pub struct ReplaceError(String); +pub enum ReplaceError { + #[error("{0}")] + Msg(String), + #[error(transparent)] + Hugr(#[from] HugrError), +} From 886fba6e1b2dbaa40f064685cad9fb4b251b1076 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 17 Oct 2023 09:24:34 +0100 Subject: [PATCH 04/70] Check edges --- src/hugr/rewrite/replace.rs | 59 +++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 5f3966048..651a47b4a 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -12,7 +12,7 @@ use crate::hugr::hugrmut::InsertionResult; use crate::hugr::{HugrError, HugrMut}; use crate::ops::OpTrait; use crate::types::EdgeKind; -use crate::{Hugr, HugrView, Node, Port}; +use crate::{Direction, Hugr, HugrView, Node, Port}; use super::Rewrite; @@ -117,11 +117,64 @@ impl Rewrite for Replacement { } } if !transferred.is_empty() { + // This also occurs if some RHS was reachable from another return Err(ReplaceError::Msg( "Some transferred nodes were not to be removed".to_string(), )); } - // TODO check edges?? + // Edge sources... + for e in self.in_edges.iter().chain(self.new_edges.iter()) { + if !h.contains_node(e.src) || removed.contains(&e.src) { + return Err(ReplaceError::Msg(format!( + "Edge source not in retained nodes: {:?}", + e.src + ))); + } + } + self.out_edges.iter().try_for_each(|e| { + self.replacement.valid_non_root(e.src).map_err(|_| { + ReplaceError::Msg(format!( + "Out-edge source not in replacement Hugr: {:?}", + e.src + )) + }) + })?; + // Edge targets... + self.in_edges.iter().try_for_each(|e| { + self.replacement.valid_non_root(e.tgt).map_err(|_| { + ReplaceError::Msg(format!( + "In-edge target not in replacement Hugr: {:?}", + e.tgt + )) + }) + })?; + for e in self.out_edges.iter().chain(self.new_edges.iter()) { + if !h.contains_node(e.tgt) || removed.contains(&e.tgt) { + return Err(ReplaceError::Msg(format!( + "Edge target not in retained nodes: {:?}", + e.tgt + ))); + } + match e.kind { + NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } => match h + .linked_ports(e.tgt, Port::new(Direction::Incoming, tgt_pos)) + .exactly_one() + { + // TODO What about an edge from a 'moved' subtree of the original Hugr. + // ATM we require this to be unchanged, yet moving could invalidate + // a nonlocal edge.... + Ok((src_n, _)) if removed.contains(&src_n) => (), + _ => { + return Err(ReplaceError::Msg(format!( + "Target of Edge {:?} did not have incoming edge being removed", + e + ))) + } + }, + _ => (), + } + } + Ok(()) } @@ -213,7 +266,7 @@ fn transfer_edges<'a>( } NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { if remove_existing { - h.disconnect(tgt, Port::new(portgraph::Direction::Incoming, tgt_pos)) + h.disconnect(tgt, Port::new(Direction::Incoming, tgt_pos)) .unwrap(); } h.connect(src, src_pos, tgt, tgt_pos).unwrap(); From ccf5200add446d29f7ac138255539ff02fdd0592 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 17 Oct 2023 14:01:28 +0100 Subject: [PATCH 05/70] Allow replacing edges from any node in S*; update spec --- specification/hugr.md | 4 ++-- src/hugr/rewrite/replace.rs | 23 +++++++++++++++++------ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/specification/hugr.md b/specification/hugr.md index 748c69ebf..304eb053c 100644 --- a/specification/hugr.md +++ b/specification/hugr.md @@ -1237,10 +1237,10 @@ The `Replace` method takes as input: $G$ and `SrcNode` in $\Gamma \setminus R$; - a list $\mu\_\textrm{out}$ of `NewEdgeSpec` which all have their `SrcNode`in $G$ and `TgtNode` in $\Gamma \setminus R$, where `TgtNode` and `TgtPos` describe - an existing incoming edge of that kind from a node in $R$. + an existing incoming edge of that kind from a node in $S\*$. - a list $\mu\_\textrm{new}$ of `NewEdgeSpec` which all have both `SrcNode` and `TgtNode` in $\Gamma \setminus R$, where `TgtNode` and `TgtPos` describe an existing incoming - edge of that kind from a node in $R$. + edge of that kind from a node in $S\*$. Note that considering all three $\mu$ lists together, - the `TgtNode` + `TgtPos`s of all `NewEdgeSpec`s with `EdgeKind` == `Value` will be unique diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 651a47b4a..cc7c04397 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -155,15 +155,27 @@ impl Rewrite for Replacement { e.tgt ))); } + fn strict_desc(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { + while let Some(p) = h.get_parent(descendant) { + if ancestor == p { + return true; + }; + descendant = p; + } + return false; + } match e.kind { NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } => match h .linked_ports(e.tgt, Port::new(Direction::Incoming, tgt_pos)) .exactly_one() { - // TODO What about an edge from a 'moved' subtree of the original Hugr. - // ATM we require this to be unchanged, yet moving could invalidate - // a nonlocal edge.... - Ok((src_n, _)) if removed.contains(&src_n) => (), + // The descendant check is to allow the case where the old edge is nonlocal + // from a part of the Hugr being moved (which may require changing source, + // depending on where the transplanted portion ends up). While this subsumes + // the first "removed.contains" check, we'll keep that as a common-case fast-path. + Ok((src_n, _)) if removed.contains(&src_n) || strict_desc(h, parent, src_n) => { + () + } _ => { return Err(ReplaceError::Msg(format!( "Target of Edge {:?} did not have incoming edge being removed", @@ -172,9 +184,8 @@ impl Rewrite for Replacement { } }, _ => (), - } + }; } - Ok(()) } From 50abda1a08668766b217f9e0498c5ef225c437a8 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 17 Oct 2023 20:32:42 +0100 Subject: [PATCH 06/70] xxx_edges => mu_xxx (xxx = out, new, in -> inp) --- src/hugr/rewrite/replace.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index cc7c04397..bf13891ae 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -49,14 +49,14 @@ pub struct Replacement { transfers: HashMap, /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. - in_edges: Vec, + mu_inp: Vec, /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. - out_edges: Vec, + mu_out: Vec, /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). /// For example, in cases where the source had an edge to a removed node, and the target had an /// edge from a removed node, this would allow source to be directly connected to target. - new_edges: Vec, + mu_new: Vec, } impl Rewrite for Replacement { @@ -123,7 +123,7 @@ impl Rewrite for Replacement { )); } // Edge sources... - for e in self.in_edges.iter().chain(self.new_edges.iter()) { + for e in self.mu_inp.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.src) || removed.contains(&e.src) { return Err(ReplaceError::Msg(format!( "Edge source not in retained nodes: {:?}", @@ -131,7 +131,7 @@ impl Rewrite for Replacement { ))); } } - self.out_edges.iter().try_for_each(|e| { + self.mu_out.iter().try_for_each(|e| { self.replacement.valid_non_root(e.src).map_err(|_| { ReplaceError::Msg(format!( "Out-edge source not in replacement Hugr: {:?}", @@ -140,7 +140,7 @@ impl Rewrite for Replacement { }) })?; // Edge targets... - self.in_edges.iter().try_for_each(|e| { + self.mu_inp.iter().try_for_each(|e| { self.replacement.valid_non_root(e.tgt).map_err(|_| { ReplaceError::Msg(format!( "In-edge target not in replacement Hugr: {:?}", @@ -148,7 +148,7 @@ impl Rewrite for Replacement { )) }) })?; - for e in self.out_edges.iter().chain(self.new_edges.iter()) { + for e in self.mu_out.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.tgt) || removed.contains(&e.tgt) { return Err(ReplaceError::Msg(format!( "Edge target not in retained nodes: {:?}", @@ -197,15 +197,15 @@ impl Rewrite for Replacement { // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| *node_map.get(&n).unwrap(); let id = |n| n; - transfer_edges(h, self.in_edges.iter(), id, translate_idx, false)?; + transfer_edges(h, self.mu_inp.iter(), id, translate_idx, false)?; // 3. Add new edges from copied to existing nodes according to mu_out, // replacing existing value/static edges incoming to targets - transfer_edges(h, self.out_edges.iter(), translate_idx, id, true)?; + transfer_edges(h, self.mu_out.iter(), translate_idx, id, true)?; //4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets - transfer_edges(h, self.new_edges.iter(), id, id, true)?; + transfer_edges(h, self.mu_new.iter(), id, id, true)?; // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) From 366209c518141e39e0d09404ef57f4c2939e7ea6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 17 Oct 2023 21:07:18 +0100 Subject: [PATCH 07/70] Make NewEdgeSpec+Replacement fields pub; run SimpleReplacement tests via applicator --- src/hugr/rewrite/replace.rs | 18 +++---- src/hugr/rewrite/simple_replace.rs | 79 ++++++++++++++++++++++++++++-- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index bf13891ae..cfa7ed195 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -18,9 +18,9 @@ use super::Rewrite; #[derive(Clone, Debug, PartialEq, Eq)] pub struct NewEdgeSpec { - src: Node, - tgt: Node, - kind: NewEdgeKind, + pub src: Node, + pub tgt: Node, + pub kind: NewEdgeKind, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -38,25 +38,25 @@ pub struct Replacement { /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. /// Must be non-empty - otherwise there is no parent under which to place [replacement], /// and no possible [in_edges], [out_edges] or [transfers]. - removal: HashSet, + pub removal: HashSet, /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. - replacement: Hugr, + pub replacement: Hugr, /// Map from container nodes in [replacement] that have no children, to container nodes /// that are descended from [nodes]. The keys are the new parents for the children of the /// values, i.e. said children are transferred to new locations rather than removed from the graph. /// Note no value may be ancestor/descendant of another. "R" is the set of descendants of [nodes] /// that are not descendants of values here. - transfers: HashMap, + pub transfers: HashMap, /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. - mu_inp: Vec, + pub mu_inp: Vec, /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. - mu_out: Vec, + pub mu_out: Vec, /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). /// For example, in cases where the source had an edge to a removed node, and the target had an /// edge from a removed node, this would allow source to be directly connected to target. - mu_new: Vec, + pub mu_new: Vec, } impl Rewrite for Replacement { diff --git a/src/hugr/rewrite/simple_replace.rs b/src/hugr/rewrite/simple_replace.rs index 53e3f68a7..3d3a8baf3 100644 --- a/src/hugr/rewrite/simple_replace.rs +++ b/src/hugr/rewrite/simple_replace.rs @@ -352,7 +352,11 @@ pub(in crate::hugr::rewrite) mod test { /// ├───┤┌─┴─┐ /// ┤ H ├┤ X ├ /// └───┘└───┘ - fn test_simple_replacement(simple_hugr: Hugr, dfg_hugr: Hugr) { + fn test_simple_replacement( + simple_hugr: Hugr, + dfg_hugr: Hugr, + #[values(apply_simple, apply_replace)] applicator: impl Fn(&mut Hugr, SimpleReplacement), + ) { let mut h: Hugr = simple_hugr; // 1. Locate the CX and its successor H's in h let h_node_cx: Node = h @@ -409,7 +413,7 @@ pub(in crate::hugr::rewrite) mod test { HashSet::<_>::from_iter([h_node_cx, h_node_h0, h_node_h1, h_outp_node]), ); - h.apply_rewrite(r).unwrap(); + applicator(&mut h, r); // Expect [DFG] to be replaced with: // ┌───┐┌───┐ // ┤ H ├┤ H ├──■── @@ -437,7 +441,10 @@ pub(in crate::hugr::rewrite) mod test { /// ┌───┐ /// ┤ H ├ /// └───┘ - fn test_simple_replacement_with_empty_wires(simple_hugr: Hugr, dfg_hugr2: Hugr) { + fn test_simple_replacement_with_empty_wires( + simple_hugr: Hugr, + dfg_hugr2: Hugr + ) { let mut h: Hugr = simple_hugr; // 1. Locate the CX in h @@ -585,4 +592,70 @@ pub(in crate::hugr::rewrite) mod test { // Nothing changed assert_eq!(h.node_count(), orig.node_count()); } + + use crate::hugr::rewrite::replace::Replacement; + fn to_replace(h: &impl HugrView, s: SimpleReplacement) -> Replacement { + use crate::hugr::rewrite::replace::{NewEdgeKind, NewEdgeSpec}; + use crate::hugr::PortIndex; + let mut replacement = s.replacement; + let (in_, out) = replacement + .children(replacement.root()) + .take(2) + .collect_tuple() + .unwrap(); + let mu_inp = s + .nu_inp + .iter() + .map(|((tgt, tgt_port), (r_n, r_p))| { + if *tgt == out {unimplemented!()}; + let (src, src_port) = h.linked_ports(*r_n, *r_p).exactly_one().ok().unwrap(); + NewEdgeSpec { + src, + tgt: *tgt, + kind: NewEdgeKind::Value { + src_pos: src_port.index(), + tgt_pos: tgt_port.index(), + }, + } + }) + .collect(); + let mu_out = s + .nu_out + .iter() + .map(|((tgt, tgt_port), out_port)| { + let (src, src_port) = replacement + .linked_ports(out, *out_port) + .exactly_one() + .ok() + .unwrap(); + if src == in_ {unimplemented!()}; + NewEdgeSpec { + src, + tgt: *tgt, + kind: NewEdgeKind::Value { + src_pos: src_port.index(), + tgt_pos: tgt_port.index(), + }, + } + }) + .collect(); + replacement.remove_node(in_).unwrap(); + replacement.remove_node(out).unwrap(); + Replacement { + removal: s.subgraph.nodes().iter().copied().collect(), + replacement, + transfers: HashMap::new(), + mu_inp, + mu_out, + mu_new: vec![], + } + } + + fn apply_simple(h: &mut Hugr, rw: SimpleReplacement) { + h.apply_rewrite(rw).unwrap() + } + + fn apply_replace(h: &mut Hugr, rw: SimpleReplacement) { + h.apply_rewrite(to_replace(h, rw)).unwrap() + } } From 2b8c86f1f02581ae362e4b78c54f2dcb5f7d2b36 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 18 Oct 2023 14:57:45 +0100 Subject: [PATCH 08/70] fmt --- src/hugr/rewrite/simple_replace.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/hugr/rewrite/simple_replace.rs b/src/hugr/rewrite/simple_replace.rs index 3d3a8baf3..0608f35ee 100644 --- a/src/hugr/rewrite/simple_replace.rs +++ b/src/hugr/rewrite/simple_replace.rs @@ -441,10 +441,7 @@ pub(in crate::hugr::rewrite) mod test { /// ┌───┐ /// ┤ H ├ /// └───┘ - fn test_simple_replacement_with_empty_wires( - simple_hugr: Hugr, - dfg_hugr2: Hugr - ) { + fn test_simple_replacement_with_empty_wires(simple_hugr: Hugr, dfg_hugr2: Hugr) { let mut h: Hugr = simple_hugr; // 1. Locate the CX in h @@ -607,7 +604,9 @@ pub(in crate::hugr::rewrite) mod test { .nu_inp .iter() .map(|((tgt, tgt_port), (r_n, r_p))| { - if *tgt == out {unimplemented!()}; + if *tgt == out { + unimplemented!() + }; let (src, src_port) = h.linked_ports(*r_n, *r_p).exactly_one().ok().unwrap(); NewEdgeSpec { src, @@ -628,7 +627,9 @@ pub(in crate::hugr::rewrite) mod test { .exactly_one() .ok() .unwrap(); - if src == in_ {unimplemented!()}; + if src == in_ { + unimplemented!() + }; NewEdgeSpec { src, tgt: *tgt, From de14f04b249cac4b78ef6e6372dc2edb0a64bda6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 18 Oct 2023 14:57:52 +0100 Subject: [PATCH 09/70] fix - new_node already in `h`, no need to translate --- src/hugr/rewrite/replace.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index cfa7ed195..413d469dc 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -211,7 +211,6 @@ impl Rewrite for Replacement { // (these will be correct places after removing nodes) let mut remove = self.removal.iter(); for new_node in h.children(new_root).collect::>().into_iter() { - let new_node = *node_map.get(&new_node).unwrap(); if let Some(to_remove) = remove.next() { h.move_before_sibling(new_node, *to_remove).unwrap(); } else { From a996f470f8f70a8becdaf0a9b7406bc763eca0c5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 10:33:28 +0100 Subject: [PATCH 10/70] Convert ReplaceError::Msg into cases --- src/hugr/rewrite/replace.rs | 127 +++++++++++++++++++++--------------- 1 file changed, 75 insertions(+), 52 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 413d469dc..a203b689e 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -10,7 +10,7 @@ use thiserror::Error; use crate::hugr::hugrmut::InsertionResult; use crate::hugr::{HugrError, HugrMut}; -use crate::ops::OpTrait; +use crate::ops::{OpTag, OpTrait}; use crate::types::EdgeKind; use crate::{Direction, Hugr, HugrView, Node, Port}; @@ -84,27 +84,23 @@ impl Rewrite for Replacement { .exactly_one() { Ok(Some(p)) => Ok(p), - Ok(None) => Err(ReplaceError::Msg( - "Cannot replace the top node of the Hugr".to_string(), - )), // maybe we SHOULD be able to?? - Err(e) => Err(ReplaceError::Msg(format!( - "Nodes to remove do not share a parent: {}", - e - ))), + Ok(None) => Err(ReplaceError::CantReplaceRoot), + Err(e) => Err(ReplaceError::MultipleParents(e.flatten().collect())), }?; // Should we require exactly equality? - let expected_tag = h.get_optype(parent).tag(); - let found_tag = self.replacement.root_type().tag(); - if expected_tag != found_tag { - return Err(ReplaceError::Msg(format!( - "Expected replacement to have root node w/tag {} but found {}", - expected_tag, found_tag - ))); + let expected = h.get_optype(parent).tag(); + let actual = self.replacement.root_type().tag(); + if expected != actual { + return Err(ReplaceError::WrongRootNodeTag { expected, actual }); }; let mut transferred: HashSet = self.transfers.values().copied().collect(); if transferred.len() != self.transfers.values().len() { - return Err(ReplaceError::Msg( - "Repeated nodes in RHS of transfer map".to_string(), + return Err(ReplaceError::ConflictingTransfers( + self.transfers + .values() + .filter(|v| !transferred.remove(v)) + .copied() + .collect(), )); } let mut removed = HashSet::new(); @@ -117,43 +113,38 @@ impl Rewrite for Replacement { } } if !transferred.is_empty() { - // This also occurs if some RHS was reachable from another - return Err(ReplaceError::Msg( - "Some transferred nodes were not to be removed".to_string(), + return Err(ReplaceError::TransfersNotSeparateDescendants( + transferred.into_iter().collect(), )); } // Edge sources... for e in self.mu_inp.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.src) || removed.contains(&e.src) { - return Err(ReplaceError::Msg(format!( - "Edge source not in retained nodes: {:?}", - e.src - ))); + return Err(ReplaceError::BadEdgeSpec( + "Edge source", + WhichHugr::Retained, + e.src, + )); } } self.mu_out.iter().try_for_each(|e| { self.replacement.valid_non_root(e.src).map_err(|_| { - ReplaceError::Msg(format!( - "Out-edge source not in replacement Hugr: {:?}", - e.src - )) + ReplaceError::BadEdgeSpec("Out-edge source", WhichHugr::Replacement, e.src) }) })?; // Edge targets... self.mu_inp.iter().try_for_each(|e| { self.replacement.valid_non_root(e.tgt).map_err(|_| { - ReplaceError::Msg(format!( - "In-edge target not in replacement Hugr: {:?}", - e.tgt - )) + ReplaceError::BadEdgeSpec("In-edge target", WhichHugr::Replacement, e.tgt) }) })?; for e in self.mu_out.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.tgt) || removed.contains(&e.tgt) { - return Err(ReplaceError::Msg(format!( - "Edge target not in retained nodes: {:?}", - e.tgt - ))); + return Err(ReplaceError::BadEdgeSpec( + "Edge target", + WhichHugr::Retained, + e.tgt, + )); } fn strict_desc(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { while let Some(p) = h.get_parent(descendant) { @@ -176,16 +167,12 @@ impl Rewrite for Replacement { Ok((src_n, _)) if removed.contains(&src_n) || strict_desc(h, parent, src_n) => { () } - _ => { - return Err(ReplaceError::Msg(format!( - "Target of Edge {:?} did not have incoming edge being removed", - e - ))) - } + _ => return Err(ReplaceError::NoRemovedEdge(e.clone())), }, _ => (), }; } + // TODO check ports and/or node types appropriate for kind of edge added Ok(()) } @@ -263,14 +250,7 @@ fn transfer_edges<'a>( match e.kind { NewEdgeKind::Order => { if h.get_optype(src).other_output() != Some(EdgeKind::StateOrder) { - return Err(ReplaceError::Msg( - "Can't insert Order edge except from DFG node".to_string(), - )); - } - if h.get_parent(tgt) != h.get_parent(src) { - return Err(ReplaceError::Msg( - "Order edge target not at top level".to_string(), - )); + return Err(ReplaceError::BadEdgeKind(e.clone())); } h.add_other_edge(src, tgt).unwrap(); } @@ -290,8 +270,51 @@ fn transfer_edges<'a>( /// Error in a [`Replacement`] #[derive(Clone, Debug, PartialEq, Eq, Error)] pub enum ReplaceError { - #[error("{0}")] - Msg(String), + /// The node(s) to replace had no parent i.e. were root(s). + // (Perhaps if there is only one node to replace we should be able to?) + #[error("Cannot replace the root node of the Hugr")] + CantReplaceRoot, + /// The nodes to replace did not have a unique common parent + #[error("Removed nodes had different parents {0:?}")] + MultipleParents(Vec), + /// Replacement root node had different tag from parent of removed nodes + #[error("Expected replacement root with tag {expected} but found {actual}")] + #[allow(missing_docs)] + WrongRootNodeTag { expected: OpTag, actual: OpTag }, + /// Values in transfer map were not unique - contains the repeated elements + #[error("Nodes cannot be transferred to multiple locations: {0:?}")] + ConflictingTransfers(Vec), + /// Some values in the transfer map were either descendants of other values, + /// or not descendants of the removed nodes + #[error("Nodes not free to be moved into new locations: {0:?}")] + TransfersNotSeparateDescendants(Vec), + /// A node at one end of a [NewEdgeSpec] was not found + #[error("{0} not in {1}: {2:?}")] + BadEdgeSpec(&'static str, WhichHugr, Node), + /// The target of the edge was found, but there was no existing edge to replace + #[error("Target of edge {0:?} did not have a corresponding incoming edge being removed")] + NoRemovedEdge(NewEdgeSpec), + /// The {NewEdgeKind} was not applicable for the source/target node(s) + #[error("The edge kind was not applicable to the node(s)")] + BadEdgeKind(NewEdgeSpec), #[error(transparent)] Hugr(#[from] HugrError), } + +/// A Hugr or portion thereof that is part of the [Replacement] +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum WhichHugr { + /// The newly-inserted nodes, i.e. the [Replacement::replacement] + Replacement, + /// Nodes in the existing Hugr that are not [Replacement::removal] + Retained, +} + +impl std::fmt::Display for WhichHugr { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + Self::Replacement => "replacement Hugr", + Self::Retained => "retained portion of Hugr", + }) + } +} From d78cbf2c89952f0ad083b085f57b922e8cbb8b4d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 10:35:54 +0100 Subject: [PATCH 11/70] clippy --- src/hugr/rewrite/replace.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index a203b689e..72425a135 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -153,7 +153,7 @@ impl Rewrite for Replacement { }; descendant = p; } - return false; + false } match e.kind { NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } => match h @@ -165,7 +165,6 @@ impl Rewrite for Replacement { // depending on where the transplanted portion ends up). While this subsumes // the first "removed.contains" check, we'll keep that as a common-case fast-path. Ok((src_n, _)) if removed.contains(&src_n) || strict_desc(h, parent, src_n) => { - () } _ => return Err(ReplaceError::NoRemovedEdge(e.clone())), }, From e6c3d8c806cf750b297b46203d8a95ad2f4aa709 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 10:45:44 +0100 Subject: [PATCH 12/70] Rewrite incoming-edge check --- src/hugr/rewrite/replace.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 72425a135..e72042421 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -155,21 +155,22 @@ impl Rewrite for Replacement { } false } - match e.kind { - NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } => match h + if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = e.kind + { + let found_incoming = h .linked_ports(e.tgt, Port::new(Direction::Incoming, tgt_pos)) .exactly_one() - { - // The descendant check is to allow the case where the old edge is nonlocal - // from a part of the Hugr being moved (which may require changing source, - // depending on where the transplanted portion ends up). While this subsumes - // the first "removed.contains" check, we'll keep that as a common-case fast-path. - Ok((src_n, _)) if removed.contains(&src_n) || strict_desc(h, parent, src_n) => { - } - _ => return Err(ReplaceError::NoRemovedEdge(e.clone())), - }, - _ => (), - }; + .is_ok_and(|(src_n, _)| { + // The descendant check is to allow the case where the old edge is nonlocal + // from a part of the Hugr being moved (which may require changing source, + // depending on where the transplanted portion ends up). While this subsumes + // the first "removed.contains" check, we'll keep that as a common-case fast-path. + removed.contains(&src_n) || strict_desc(h, parent, src_n) + }); + if !found_incoming { + return Err(ReplaceError::NoRemovedEdge(e.clone())); + }; + } } // TODO check ports and/or node types appropriate for kind of edge added Ok(()) From d70444e713b5c4a34c278c5facae59c5984699bd Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 10:51:52 +0100 Subject: [PATCH 13/70] Shorter 'descends' --- src/hugr/rewrite/replace.rs | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index e72042421..ac6ebe66b 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -146,17 +146,15 @@ impl Rewrite for Replacement { e.tgt, )); } - fn strict_desc(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { - while let Some(p) = h.get_parent(descendant) { - if ancestor == p { - return true; - }; - descendant = p; - } - false - } if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = e.kind { + fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { + while descendant != ancestor { + let Some(p) = h.get_parent(descendant) else {return false}; + descendant = p; + } + true + } let found_incoming = h .linked_ports(e.tgt, Port::new(Direction::Incoming, tgt_pos)) .exactly_one() @@ -165,7 +163,7 @@ impl Rewrite for Replacement { // from a part of the Hugr being moved (which may require changing source, // depending on where the transplanted portion ends up). While this subsumes // the first "removed.contains" check, we'll keep that as a common-case fast-path. - removed.contains(&src_n) || strict_desc(h, parent, src_n) + removed.contains(&src_n) || descends(h, parent, src_n) }); if !found_incoming { return Err(ReplaceError::NoRemovedEdge(e.clone())); From 6e944679ca9c392f2134f9f162bf2897b83d0538 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 11:30:40 +0100 Subject: [PATCH 14/70] Check parent in apply --- src/hugr/rewrite/replace.rs | 40 ++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index ac6ebe66b..be6ebabfe 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -59,6 +59,26 @@ pub struct Replacement { pub mu_new: Vec, } +impl Replacement { + fn check_parent(&self, h: &impl HugrView) -> Result { + let parent = self + .removal + .iter() + .map(|n| h.get_parent(*n)) + .unique() + .exactly_one() + .map_err(|ex_one| ReplaceError::MultipleParents(ex_one.flatten().collect()))? + .ok_or(ReplaceError::CantReplaceRoot)?; // If no parent + + // Check replacement parent is of same tag. Should we require exactly OpType equality? + let expected = h.get_optype(parent).tag(); + let actual = self.replacement.root_type().tag(); + if expected != actual { + return Err(ReplaceError::WrongRootNodeTag { expected, actual }); + }; + Ok(parent) + } +} impl Rewrite for Replacement { type Error = ReplaceError; @@ -76,23 +96,7 @@ impl Rewrite for Replacement { const UNCHANGED_ON_FAILURE: bool = false; fn verify(&self, h: &impl crate::HugrView) -> Result<(), Self::Error> { - let parent = match self - .removal - .iter() - .map(|n| h.get_parent(*n)) - .unique() - .exactly_one() - { - Ok(Some(p)) => Ok(p), - Ok(None) => Err(ReplaceError::CantReplaceRoot), - Err(e) => Err(ReplaceError::MultipleParents(e.flatten().collect())), - }?; - // Should we require exactly equality? - let expected = h.get_optype(parent).tag(); - let actual = self.replacement.root_type().tag(); - if expected != actual { - return Err(ReplaceError::WrongRootNodeTag { expected, actual }); - }; + let parent = self.check_parent(h)?; let mut transferred: HashSet = self.transfers.values().copied().collect(); if transferred.len() != self.transfers.values().len() { return Err(ReplaceError::ConflictingTransfers( @@ -175,7 +179,7 @@ impl Rewrite for Replacement { } fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { - let parent = h.get_parent(*self.removal.iter().next().unwrap()).unwrap(); + let parent = self.check_parent(h)?; // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement)?; From dcccde927f1cd631a7f50cacb6071fa8d3a6a126 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 11:33:30 +0100 Subject: [PATCH 15/70] insertion shouldn't fail?? Drop HugrError --- src/hugr/rewrite/replace.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index be6ebabfe..9c85f4212 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -9,7 +9,7 @@ use itertools::Itertools; use thiserror::Error; use crate::hugr::hugrmut::InsertionResult; -use crate::hugr::{HugrError, HugrMut}; +use crate::hugr::HugrMut; use crate::ops::{OpTag, OpTrait}; use crate::types::EdgeKind; use crate::{Direction, Hugr, HugrView, Node, Port}; @@ -180,8 +180,10 @@ impl Rewrite for Replacement { fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { let parent = self.check_parent(h)?; - // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want - let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement)?; + // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want. + // TODO what would an error here mean? e.g. malformed self.replacement?? + let InsertionResult { new_root, node_map } = + h.insert_hugr(parent, self.replacement).unwrap(); // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| *node_map.get(&n).unwrap(); @@ -299,8 +301,6 @@ pub enum ReplaceError { /// The {NewEdgeKind} was not applicable for the source/target node(s) #[error("The edge kind was not applicable to the node(s)")] BadEdgeKind(NewEdgeSpec), - #[error(transparent)] - Hugr(#[from] HugrError), } /// A Hugr or portion thereof that is part of the [Replacement] From 7366aa72060cc98ad8983c1cd3c6c9c1e569d304 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 12:03:33 +0100 Subject: [PATCH 16/70] v1 verify checks port kind --- src/hugr/rewrite/replace.rs | 51 +++++++++++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 9c85f4212..8dbbf22e3 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -31,6 +31,49 @@ pub enum NewEdgeKind { ControlFlow { src_pos: usize }, } +impl NewEdgeSpec { + fn check_src(&self, h: &impl HugrView) -> Result<(), ReplaceError> { + let optype = h.get_optype(self.src); + let ok = match self.kind { + NewEdgeKind::Order => optype.other_output() == Some(EdgeKind::StateOrder), + NewEdgeKind::Value { src_pos, .. } => matches!( + optype.port_kind(Port::new_outgoing(src_pos)), + Some(EdgeKind::Value(_)) + ), + NewEdgeKind::Static { src_pos, .. } => matches!( + optype.port_kind(Port::new_outgoing(src_pos)), + Some(EdgeKind::Static(_)) + ), + NewEdgeKind::ControlFlow { src_pos } => matches!( + optype.port_kind(Port::new_outgoing(src_pos)), + Some(EdgeKind::ControlFlow) + ), + }; + ok.then_some(()) + .ok_or(ReplaceError::BadEdgeKind(self.clone())) + } + fn check_tgt(&self, h: &impl HugrView) -> Result<(), ReplaceError> { + let optype = h.get_optype(self.tgt); + let ok = match self.kind { + NewEdgeKind::Order => optype.other_input() == Some(EdgeKind::StateOrder), + NewEdgeKind::Value { tgt_pos, .. } => matches!( + optype.port_kind(Port::new_incoming(tgt_pos)), + Some(EdgeKind::Value(_)) + ), + NewEdgeKind::Static { tgt_pos, .. } => matches!( + optype.port_kind(Port::new_incoming(tgt_pos)), + Some(EdgeKind::Static(_)) + ), + NewEdgeKind::ControlFlow { .. } => matches!( + optype.port_kind(Port::new_incoming(0)), + Some(EdgeKind::ControlFlow) + ), + }; + ok.then_some(()) + .ok_or(ReplaceError::BadEdgeKind(self.clone())) + } +} + /// Specification of a `Replace` operation #[derive(Debug, Clone)] // PartialEq? Eq probably doesn't make sense because of ordering. pub struct Replacement { @@ -130,17 +173,20 @@ impl Rewrite for Replacement { e.src, )); } + e.check_src(h)?; } self.mu_out.iter().try_for_each(|e| { self.replacement.valid_non_root(e.src).map_err(|_| { ReplaceError::BadEdgeSpec("Out-edge source", WhichHugr::Replacement, e.src) - }) + })?; + e.check_src(&self.replacement) })?; // Edge targets... self.mu_inp.iter().try_for_each(|e| { self.replacement.valid_non_root(e.tgt).map_err(|_| { ReplaceError::BadEdgeSpec("In-edge target", WhichHugr::Replacement, e.tgt) - }) + })?; + e.check_tgt(&self.replacement) })?; for e in self.mu_out.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.tgt) || removed.contains(&e.tgt) { @@ -150,6 +196,7 @@ impl Rewrite for Replacement { e.tgt, )); } + e.check_tgt(h)?; if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = e.kind { fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { From 46d2f48a1f27c64c00127a748fc44476dae388e8 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 12:38:26 +0100 Subject: [PATCH 17/70] BadEdgeKind stores Direction (temp remove check on adding Order edges) --- src/hugr/rewrite/replace.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 8dbbf22e3..66b51b365 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -50,7 +50,7 @@ impl NewEdgeSpec { ), }; ok.then_some(()) - .ok_or(ReplaceError::BadEdgeKind(self.clone())) + .ok_or(ReplaceError::BadEdgeKind(Direction::Outgoing, self.clone())) } fn check_tgt(&self, h: &impl HugrView) -> Result<(), ReplaceError> { let optype = h.get_optype(self.tgt); @@ -70,7 +70,7 @@ impl NewEdgeSpec { ), }; ok.then_some(()) - .ok_or(ReplaceError::BadEdgeKind(self.clone())) + .ok_or(ReplaceError::BadEdgeKind(Direction::Incoming, self.clone())) } } @@ -300,9 +300,6 @@ fn transfer_edges<'a>( let tgt = trans_tgt(e.tgt); match e.kind { NewEdgeKind::Order => { - if h.get_optype(src).other_output() != Some(EdgeKind::StateOrder) { - return Err(ReplaceError::BadEdgeKind(e.clone())); - } h.add_other_edge(src, tgt).unwrap(); } NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { @@ -345,9 +342,9 @@ pub enum ReplaceError { /// The target of the edge was found, but there was no existing edge to replace #[error("Target of edge {0:?} did not have a corresponding incoming edge being removed")] NoRemovedEdge(NewEdgeSpec), - /// The {NewEdgeKind} was not applicable for the source/target node(s) - #[error("The edge kind was not applicable to the node(s)")] - BadEdgeKind(NewEdgeSpec), + /// The [NewEdgeKind] was not applicable for the source/target node(s) + #[error("The edge kind was not applicable to the {0:?} node: {1:?}")] + BadEdgeKind(Direction, NewEdgeSpec), } /// A Hugr or portion thereof that is part of the [Replacement] From 1d15e7265ba29e808baca7948432eb6d874d8c38 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 12:40:30 +0100 Subject: [PATCH 18/70] Break out check_existing_edge; transfer_edge does checks too --- src/hugr/rewrite/replace.rs | 84 +++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 66b51b365..302b2c371 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -72,6 +72,24 @@ impl NewEdgeSpec { ok.then_some(()) .ok_or(ReplaceError::BadEdgeKind(Direction::Incoming, self.clone())) } + + fn check_existing_edge( + &self, + h: &impl HugrView, + src_ok: impl Fn(Node) -> bool, + ) -> Result<(), ReplaceError> { + if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind + { + let found_incoming = h + .linked_ports(self.tgt, Port::new_incoming(tgt_pos)) + .exactly_one() + .is_ok_and(|(src_n, _)| src_ok(src_n)); + if !found_incoming { + return Err(ReplaceError::NoRemovedEdge(self.clone())); + }; + }; + Ok(()) + } } /// Specification of a `Replace` operation @@ -197,31 +215,12 @@ impl Rewrite for Replacement { )); } e.check_tgt(h)?; - if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = e.kind - { - fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { - while descendant != ancestor { - let Some(p) = h.get_parent(descendant) else {return false}; - descendant = p; - } - true - } - let found_incoming = h - .linked_ports(e.tgt, Port::new(Direction::Incoming, tgt_pos)) - .exactly_one() - .is_ok_and(|(src_n, _)| { - // The descendant check is to allow the case where the old edge is nonlocal - // from a part of the Hugr being moved (which may require changing source, - // depending on where the transplanted portion ends up). While this subsumes - // the first "removed.contains" check, we'll keep that as a common-case fast-path. - removed.contains(&src_n) || descends(h, parent, src_n) - }); - if !found_incoming { - return Err(ReplaceError::NoRemovedEdge(e.clone())); - }; - } + // The descendant check is to allow the case where the old edge is nonlocal + // from a part of the Hugr being moved (which may require changing source, + // depending on where the transplanted portion ends up). While this subsumes + // the first "removed.contains" check, we'll keep that as a common-case fast-path. + e.check_existing_edge(h, |n| removed.contains(&n) || descends(h, parent, n))?; } - // TODO check ports and/or node types appropriate for kind of edge added Ok(()) } @@ -235,15 +234,15 @@ impl Rewrite for Replacement { // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| *node_map.get(&n).unwrap(); let id = |n| n; - transfer_edges(h, self.mu_inp.iter(), id, translate_idx, false)?; + transfer_edges(h, self.mu_inp.iter(), id, translate_idx, None)?; // 3. Add new edges from copied to existing nodes according to mu_out, // replacing existing value/static edges incoming to targets - transfer_edges(h, self.mu_out.iter(), translate_idx, id, true)?; + transfer_edges(h, self.mu_out.iter(), translate_idx, id, Some(parent))?; //4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets - transfer_edges(h, self.mu_new.iter(), id, id, true)?; + transfer_edges(h, self.mu_new.iter(), id, id, Some(parent))?; // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) @@ -288,28 +287,41 @@ impl Rewrite for Replacement { } } +fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { + while descendant != ancestor { + let Some(p) = h.get_parent(descendant) else {return false}; + descendant = p; + } + true +} + fn transfer_edges<'a>( h: &mut impl HugrMut, edges: impl Iterator, trans_src: impl Fn(Node) -> Node, trans_tgt: impl Fn(Node) -> Node, - remove_existing: bool, + existing_src_ancestor: Option, ) -> Result<(), ReplaceError> { for e in edges { - let src = trans_src(e.src); - let tgt = trans_tgt(e.tgt); + let e = NewEdgeSpec { + src: trans_src(e.src), + tgt: trans_tgt(e.tgt), + ..e.clone() + }; + e.check_src(h)?; + e.check_tgt(h)?; match e.kind { NewEdgeKind::Order => { - h.add_other_edge(src, tgt).unwrap(); + h.add_other_edge(e.src, e.tgt).unwrap(); } NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { - if remove_existing { - h.disconnect(tgt, Port::new(Direction::Incoming, tgt_pos)) - .unwrap(); + if let Some(anc) = existing_src_ancestor { + e.check_existing_edge(h, |n| descends(h, anc, n))?; + h.disconnect(e.tgt, Port::new_incoming(tgt_pos)).unwrap(); } - h.connect(src, src_pos, tgt, tgt_pos).unwrap(); + h.connect(e.src, src_pos, e.tgt, tgt_pos).unwrap(); } - NewEdgeKind::ControlFlow { src_pos } => h.connect(src, src_pos, tgt, 0).unwrap(), + NewEdgeKind::ControlFlow { src_pos } => h.connect(e.src, src_pos, e.tgt, 0).unwrap(), } } Ok(()) From e371fc36eb575b0e36e462a30143b5c71121ca3f Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 13:29:10 +0100 Subject: [PATCH 19/70] BadEdgeSpec: str -> Direction, Node -> Edge --- src/hugr/rewrite/replace.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 302b2c371..20e51e8ea 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -186,32 +186,32 @@ impl Rewrite for Replacement { for e in self.mu_inp.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.src) || removed.contains(&e.src) { return Err(ReplaceError::BadEdgeSpec( - "Edge source", + Direction::Outgoing, WhichHugr::Retained, - e.src, + e.clone(), )); } e.check_src(h)?; } self.mu_out.iter().try_for_each(|e| { self.replacement.valid_non_root(e.src).map_err(|_| { - ReplaceError::BadEdgeSpec("Out-edge source", WhichHugr::Replacement, e.src) + ReplaceError::BadEdgeSpec(Direction::Outgoing, WhichHugr::Replacement, e.clone()) })?; e.check_src(&self.replacement) })?; // Edge targets... self.mu_inp.iter().try_for_each(|e| { self.replacement.valid_non_root(e.tgt).map_err(|_| { - ReplaceError::BadEdgeSpec("In-edge target", WhichHugr::Replacement, e.tgt) + ReplaceError::BadEdgeSpec(Direction::Incoming, WhichHugr::Replacement, e.clone()) })?; e.check_tgt(&self.replacement) })?; for e in self.mu_out.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.tgt) || removed.contains(&e.tgt) { return Err(ReplaceError::BadEdgeSpec( - "Edge target", + Direction::Incoming, WhichHugr::Retained, - e.tgt, + e.clone(), )); } e.check_tgt(h)?; @@ -349,8 +349,8 @@ pub enum ReplaceError { #[error("Nodes not free to be moved into new locations: {0:?}")] TransfersNotSeparateDescendants(Vec), /// A node at one end of a [NewEdgeSpec] was not found - #[error("{0} not in {1}: {2:?}")] - BadEdgeSpec(&'static str, WhichHugr, Node), + #[error("{0:?} end of edge {2:?} not found in {1}")] + BadEdgeSpec(Direction, WhichHugr, NewEdgeSpec), /// The target of the edge was found, but there was no existing edge to replace #[error("Target of edge {0:?} did not have a corresponding incoming edge being removed")] NoRemovedEdge(NewEdgeSpec), From 16b52b4158284cbb0eda440297998f301140d4e2 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 13:35:25 +0100 Subject: [PATCH 20/70] apply fails w/BadEdgeSpec for replacement nodes --- src/hugr/rewrite/replace.rs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 20e51e8ea..19fca02e6 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -232,8 +232,8 @@ impl Rewrite for Replacement { h.insert_hugr(parent, self.replacement).unwrap(); // 2. Add new edges from existing to copied nodes according to mu_in - let translate_idx = |n| *node_map.get(&n).unwrap(); - let id = |n| n; + let translate_idx = |n| node_map.get(&n).copied(); + let id = |n| Some(n); transfer_edges(h, self.mu_inp.iter(), id, translate_idx, None)?; // 3. Add new edges from copied to existing nodes according to mu_out, @@ -298,14 +298,23 @@ fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { fn transfer_edges<'a>( h: &mut impl HugrMut, edges: impl Iterator, - trans_src: impl Fn(Node) -> Node, - trans_tgt: impl Fn(Node) -> Node, + trans_src: impl Fn(Node) -> Option, + trans_tgt: impl Fn(Node) -> Option, existing_src_ancestor: Option, ) -> Result<(), ReplaceError> { for e in edges { let e = NewEdgeSpec { - src: trans_src(e.src), - tgt: trans_tgt(e.tgt), + // Translation can only fail for Nodes that are supposed to be in the replacement + src: trans_src(e.src).ok_or(ReplaceError::BadEdgeSpec( + Direction::Outgoing, + WhichHugr::Replacement, + e.clone(), + ))?, + tgt: trans_tgt(e.tgt).ok_or(ReplaceError::BadEdgeSpec( + Direction::Incoming, + WhichHugr::Replacement, + e.clone(), + ))?, ..e.clone() }; e.check_src(h)?; From 3701d501f5bfad5e94a1ef6ed029b3431a25cda5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 13:50:27 +0100 Subject: [PATCH 21/70] Note what checks required --- src/hugr/rewrite/replace.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 19fca02e6..81581e8c1 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -234,14 +234,19 @@ impl Rewrite for Replacement { // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| node_map.get(&n).copied(); let id = |n| Some(n); + // TODO would be good to check the edge sources here are not being removed. Could do after removal? transfer_edges(h, self.mu_inp.iter(), id, translate_idx, None)?; // 3. Add new edges from copied to existing nodes according to mu_out, // replacing existing value/static edges incoming to targets + // TODO good to check targets are not being removed, but we can't do after removal, + // as then we'd be unable to check that the targets had edges FROM (re/)moved nodes. transfer_edges(h, self.mu_out.iter(), translate_idx, id, Some(parent))?; - //4. Add new edges between existing nodes according to mu_new, - // replacing existing value/static edges incoming to targets + // 4. Add new edges between existing nodes according to mu_new, + // replacing existing value/static edges incoming to targets. + // TODO check neither edge sources nor targets are being removed, + // but can't do after removal as we need to check there are existing edges FROM (re/)moved nodes transfer_edges(h, self.mu_new.iter(), id, id, Some(parent))?; // 5. Put newly-added copies into correct places in hierarchy From 677692a223c4fb544584a65fee9d5684e500d278 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 13:57:42 +0100 Subject: [PATCH 22/70] Factor out get_removed_nodes --- src/hugr/rewrite/replace.rs | 42 +++++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 81581e8c1..265ca9650 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -139,25 +139,8 @@ impl Replacement { }; Ok(parent) } -} -impl Rewrite for Replacement { - type Error = ReplaceError; - - type ApplyResult = (); - - type InvalidationSet<'a> = // as IntoIterator>::IntoIter - Chain< - Chain< - Copied>, - Copied>>, - Copied>>> - where - Self: 'a; - - const UNCHANGED_ON_FAILURE: bool = false; - fn verify(&self, h: &impl crate::HugrView) -> Result<(), Self::Error> { - let parent = self.check_parent(h)?; + fn get_removed_nodes(&self, h: &impl HugrView) -> Result, ReplaceError> { let mut transferred: HashSet = self.transfers.values().copied().collect(); if transferred.len() != self.transfers.values().len() { return Err(ReplaceError::ConflictingTransfers( @@ -168,6 +151,7 @@ impl Rewrite for Replacement { .collect(), )); } + let mut removed = HashSet::new(); let mut queue = VecDeque::from_iter(self.removal.iter().copied()); while let Some(n) = queue.pop_front() { @@ -182,6 +166,28 @@ impl Rewrite for Replacement { transferred.into_iter().collect(), )); } + Ok(removed) + } +} +impl Rewrite for Replacement { + type Error = ReplaceError; + + type ApplyResult = (); + + type InvalidationSet<'a> = // as IntoIterator>::IntoIter + Chain< + Chain< + Copied>, + Copied>>, + Copied>>> + where + Self: 'a; + + const UNCHANGED_ON_FAILURE: bool = false; + + fn verify(&self, h: &impl crate::HugrView) -> Result<(), Self::Error> { + let parent = self.check_parent(h)?; + let removed = self.get_removed_nodes(h)?; // Edge sources... for e in self.mu_inp.iter().chain(self.mu_new.iter()) { if !h.contains_node(e.src) || removed.contains(&e.src) { From b2a7f000462ce99381c726abbffaf9297e8efab1 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 14:04:52 +0100 Subject: [PATCH 23/70] transfer_edges: translation func returns Result --- src/hugr/rewrite/replace.rs | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 265ca9650..c8b47411c 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -238,8 +238,8 @@ impl Rewrite for Replacement { h.insert_hugr(parent, self.replacement).unwrap(); // 2. Add new edges from existing to copied nodes according to mu_in - let translate_idx = |n| node_map.get(&n).copied(); - let id = |n| Some(n); + let translate_idx = |n| node_map.get(&n).copied().ok_or(WhichHugr::Replacement); + let id = |n| Ok(n); // TODO would be good to check the edge sources here are not being removed. Could do after removal? transfer_edges(h, self.mu_inp.iter(), id, translate_idx, None)?; @@ -309,23 +309,17 @@ fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { fn transfer_edges<'a>( h: &mut impl HugrMut, edges: impl Iterator, - trans_src: impl Fn(Node) -> Option, - trans_tgt: impl Fn(Node) -> Option, + trans_src: impl Fn(Node) -> Result, + trans_tgt: impl Fn(Node) -> Result, existing_src_ancestor: Option, ) -> Result<(), ReplaceError> { for e in edges { let e = NewEdgeSpec { // Translation can only fail for Nodes that are supposed to be in the replacement - src: trans_src(e.src).ok_or(ReplaceError::BadEdgeSpec( - Direction::Outgoing, - WhichHugr::Replacement, - e.clone(), - ))?, - tgt: trans_tgt(e.tgt).ok_or(ReplaceError::BadEdgeSpec( - Direction::Incoming, - WhichHugr::Replacement, - e.clone(), - ))?, + src: trans_src(e.src) + .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Outgoing, h, e.clone()))?, + tgt: trans_tgt(e.tgt) + .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Incoming, h, e.clone()))?, ..e.clone() }; e.check_src(h)?; From 86d6f39734b07615b9752e3190cbca7fadedc548 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 14:21:34 +0100 Subject: [PATCH 24/70] Use get_removed in apply --- src/hugr/rewrite/replace.rs | 51 ++++++++++++++++++++----------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index c8b47411c..85b156ae8 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -232,6 +232,10 @@ impl Rewrite for Replacement { fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { let parent = self.check_parent(h)?; + // Calculate removed nodes here. (Does not include transfers, so enumerates only + // nodes we are going to remove, individually, anyway; so no *asymptotic* speed penalty) + let to_remove = self.get_removed_nodes(h)?; + // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want. // TODO what would an error here mean? e.g. malformed self.replacement?? let InsertionResult { new_root, node_map } = @@ -239,28 +243,26 @@ impl Rewrite for Replacement { // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| node_map.get(&n).copied().ok_or(WhichHugr::Replacement); - let id = |n| Ok(n); - // TODO would be good to check the edge sources here are not being removed. Could do after removal? - transfer_edges(h, self.mu_inp.iter(), id, translate_idx, None)?; + let kept = |n| { + let keep = !to_remove.contains(&n); + keep.then_some(n).ok_or(WhichHugr::Retained) + }; + transfer_edges(h, self.mu_inp.iter(), kept, translate_idx, None)?; // 3. Add new edges from copied to existing nodes according to mu_out, // replacing existing value/static edges incoming to targets - // TODO good to check targets are not being removed, but we can't do after removal, - // as then we'd be unable to check that the targets had edges FROM (re/)moved nodes. - transfer_edges(h, self.mu_out.iter(), translate_idx, id, Some(parent))?; + transfer_edges(h, self.mu_out.iter(), translate_idx, kept, Some(parent))?; // 4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets. - // TODO check neither edge sources nor targets are being removed, - // but can't do after removal as we need to check there are existing edges FROM (re/)moved nodes - transfer_edges(h, self.mu_new.iter(), id, id, Some(parent))?; + transfer_edges(h, self.mu_new.iter(), kept, kept, Some(parent))?; // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) - let mut remove = self.removal.iter(); + let mut remove_top_sibs = self.removal.iter(); for new_node in h.children(new_root).collect::>().into_iter() { - if let Some(to_remove) = remove.next() { - h.move_before_sibling(new_node, *to_remove).unwrap(); + if let Some(top_sib) = remove_top_sibs.next() { + h.move_before_sibling(new_node, *top_sib).unwrap(); } else { h.set_parent(new_node, parent).unwrap(); } @@ -280,12 +282,7 @@ impl Rewrite for Replacement { } // 7. Remove remaining nodes - let mut to_remove = VecDeque::from_iter(self.removal); - while let Some(n) = to_remove.pop_front() { - // Nodes may have no children if they were moved in step 6 - to_remove.extend(h.children(n)); - h.remove_node(n).unwrap(); - } + to_remove.into_iter().for_each(|n| h.remove_node(n).unwrap()); Ok(()) } @@ -313,15 +310,21 @@ fn transfer_edges<'a>( trans_tgt: impl Fn(Node) -> Result, existing_src_ancestor: Option, ) -> Result<(), ReplaceError> { - for e in edges { + for oe in edges { let e = NewEdgeSpec { // Translation can only fail for Nodes that are supposed to be in the replacement - src: trans_src(e.src) - .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Outgoing, h, e.clone()))?, - tgt: trans_tgt(e.tgt) - .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Incoming, h, e.clone()))?, - ..e.clone() + src: trans_src(oe.src) + .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Outgoing, h, oe.clone()))?, + tgt: trans_tgt(oe.tgt) + .map_err(|h| ReplaceError::BadEdgeSpec(Direction::Incoming, h, oe.clone()))?, + ..oe.clone() }; + h.valid_node(e.src).map_err(|_| { + ReplaceError::BadEdgeSpec(Direction::Outgoing, WhichHugr::Retained, oe.clone()) + })?; + h.valid_node(e.tgt).map_err(|_| { + ReplaceError::BadEdgeSpec(Direction::Incoming, WhichHugr::Retained, oe.clone()) + })?; e.check_src(h)?; e.check_tgt(h)?; match e.kind { From 473871071e8d5db0ebc8ca7904c06776d48a39dc Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 14:43:08 +0100 Subject: [PATCH 25/70] Add missing docs; move 'impl NewEdgeSpec' after struct def for Replacement --- src/hugr/rewrite/replace.rs | 87 ++++++++++++++++++++++++------------- 1 file changed, 56 insertions(+), 31 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 85b156ae8..cbe453138 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -16,19 +16,71 @@ use crate::{Direction, Hugr, HugrView, Node, Port}; use super::Rewrite; +/// Specifies how to create a new edge. #[derive(Clone, Debug, PartialEq, Eq)] pub struct NewEdgeSpec { + /// The source of the new edge. For [Replacement::mu_inp] and [Replacement::mu_new], this is in the + /// existing Hugr; for edges in [Replacement::mu_out] this is in the [Replacement::replacement] pub src: Node, + /// The target of the new edge. For [Replacement::mu_inp], this is in the [Replacement::replacement]; + /// for edges in [Replacement::mu_out] and [Replacement::mu_new], this is in the existing Hugr. pub tgt: Node, + /// The kind of edge to create, and any port specifiers required pub kind: NewEdgeKind, } +/// Describes an edge that should be created between two nodes already given #[derive(Clone, Debug, PartialEq, Eq)] pub enum NewEdgeKind { + /// An [EdgeKind::StateOrder] edge (between DFG nodes only) Order, - Value { src_pos: usize, tgt_pos: usize }, - Static { src_pos: usize, tgt_pos: usize }, - ControlFlow { src_pos: usize }, + /// An [EdgeKind::Value] edge (between DFG nodes only) + Value { + /// The source port + src_pos: usize, + /// The target port + tgt_pos: usize, + }, + /// An [EdgeKind::Static] edge + Static { + /// The source port + src_pos: usize, + /// The target port + tgt_pos: usize, + }, + /// A [EdgeKind::ControlFlow] edge (between CFG nodes only) + ControlFlow { + /// Identifies a control-flow output (successor) of the source node. + src_pos: usize, + }, +} + +/// Specification of a `Replace` operation +#[derive(Debug, Clone)] // PartialEq? Eq probably doesn't make sense because of ordering. +pub struct Replacement { + /// The nodes to remove from the existing Hugr (known as Gamma). + /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. + /// Must be non-empty - otherwise there is no parent under which to place [replacement], + /// and no possible [in_edges], [out_edges] or [transfers]. + pub removal: HashSet, + /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. + pub replacement: Hugr, + /// Map from container nodes in [replacement] that have no children, to container nodes + /// that are descended from [nodes]. The keys are the new parents for the children of the + /// values, i.e. said children are transferred to new locations rather than removed from the graph. + /// Note no value may be ancestor/descendant of another. "R" is the set of descendants of [nodes] + /// that are not descendants of values here. + pub transfers: HashMap, + /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) + /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. + pub mu_inp: Vec, + /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed + /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. + pub mu_out: Vec, + /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). + /// For example, in cases where the source had an edge to a removed node, and the target had an + /// edge from a removed node, this would allow source to be directly connected to target. + pub mu_new: Vec, } impl NewEdgeSpec { @@ -92,34 +144,6 @@ impl NewEdgeSpec { } } -/// Specification of a `Replace` operation -#[derive(Debug, Clone)] // PartialEq? Eq probably doesn't make sense because of ordering. -pub struct Replacement { - /// The nodes to remove from the existing Hugr (known as Gamma). - /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. - /// Must be non-empty - otherwise there is no parent under which to place [replacement], - /// and no possible [in_edges], [out_edges] or [transfers]. - pub removal: HashSet, - /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. - pub replacement: Hugr, - /// Map from container nodes in [replacement] that have no children, to container nodes - /// that are descended from [nodes]. The keys are the new parents for the children of the - /// values, i.e. said children are transferred to new locations rather than removed from the graph. - /// Note no value may be ancestor/descendant of another. "R" is the set of descendants of [nodes] - /// that are not descendants of values here. - pub transfers: HashMap, - /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) - /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. - pub mu_inp: Vec, - /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed - /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. - pub mu_out: Vec, - /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). - /// For example, in cases where the source had an edge to a removed node, and the target had an - /// edge from a removed node, this would allow source to be directly connected to target. - pub mu_new: Vec, -} - impl Replacement { fn check_parent(&self, h: &impl HugrView) -> Result { let parent = self @@ -382,6 +406,7 @@ pub enum WhichHugr { /// The newly-inserted nodes, i.e. the [Replacement::replacement] Replacement, /// Nodes in the existing Hugr that are not [Replacement::removal] + /// (or are on the RHS of an entry in [Replacement::transfers]) Retained, } From 94f0586c5efc3062d9edb6aef88e844c2c7fd276 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 16:19:20 +0100 Subject: [PATCH 26/70] fixup! Use get_removed --- src/hugr/rewrite/replace.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index cbe453138..90f51c082 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -306,7 +306,9 @@ impl Rewrite for Replacement { } // 7. Remove remaining nodes - to_remove.into_iter().for_each(|n| h.remove_node(n).unwrap()); + to_remove + .into_iter() + .for_each(|n| h.remove_node(n).unwrap()); Ok(()) } From c4d5996fd45e5f8d33c1c1f82123e343432fe76f Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Fri, 20 Oct 2023 16:19:39 +0100 Subject: [PATCH 27/70] WIP test - failing in extension inference?! --- src/hugr/rewrite/replace.rs | 117 ++++++++++++++++++++++++++++++++++++ 1 file changed, 117 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 90f51c082..1578ddd2b 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -420,3 +420,120 @@ impl std::fmt::Display for WhichHugr { }) } } + +#[cfg(test)] +mod test { + use std::collections::{HashMap, HashSet}; + + use crate::builder::{Dataflow, HugrBuilder}; + use crate::extension::prelude::USIZE_T; + use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; + use crate::hugr::{HugrMut, NodeType}; + use crate::ops::handle::NodeHandle; + use crate::ops::{self, BasicBlock, Const, LeafOp, DFG}; + use crate::std_extensions::collections; + use crate::types::{Type, TypeArg, TypeRow}; + use crate::{builder::CFGBuilder, types::FunctionType}; + use crate::{type_row, Hugr, HugrView}; + + use super::{NewEdgeKind, NewEdgeSpec, Replacement}; + + #[test] + fn cfg() -> Result<(), Box> { + let reg: ExtensionRegistry = [PRELUDE.to_owned(), collections::EXTENSION.to_owned()].into(); + let listy = Type::new_extension( + collections::EXTENSION + .get_type(collections::LIST_TYPENAME.as_str()) + .unwrap() + .instantiate_concrete([TypeArg::Type { ty: USIZE_T }]) + .unwrap(), + ); + let pop: LeafOp = collections::EXTENSION + .instantiate_extension_op("pop", [TypeArg::Type { ty: USIZE_T }], ®) + .unwrap() + .into(); + let push: LeafOp = collections::EXTENSION + .instantiate_extension_op("push", [TypeArg::Type { ty: USIZE_T }], ®) + .unwrap() + .into(); + let intermed = TypeRow::from(vec![listy.clone(), USIZE_T]); + + let mut cfg = CFGBuilder::new(FunctionType::new(vec![listy.clone()], vec![listy.clone()]))?; + let mut entry = cfg.simple_entry_builder( + intermed.clone(), + 1, + ExtensionSet::singleton(&collections::EXTENSION_NAME), + )?; + let [ent_in] = entry.input_wires_arr(); + let popped = entry.add_dataflow_op(pop, [ent_in])?; + let pred = entry.add_load_const(Const::simple_unary_predicate(), ExtensionSet::new())?; + let entry = entry.finish_with_outputs(pred, popped.outputs())?; + + let mut bb2 = + cfg.simple_block_builder(FunctionType::new(intermed.clone(), vec![listy.clone()]), 1)?; + let pushed = bb2.add_dataflow_op(push, bb2.input_wires())?; + let pred = bb2.add_load_const(Const::simple_unary_predicate(), ExtensionSet::new())?; + let bb2 = bb2.finish_with_outputs(pred, pushed.outputs())?; + cfg.branch(&entry, 0, &bb2)?; + + let exit = cfg.exit_block(); + cfg.branch(&bb2, 0, &exit)?; + let mut h = cfg.finish_hugr(®)?; + + // Replacement: one BB with two DFGs inside. + // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). + let mut replacement = Hugr::new(NodeType::open_extensions(BasicBlock::DFB { + inputs: vec![listy.clone()].into(), + predicate_variants: vec![type_row![]], + other_outputs: vec![listy.clone()].into(), + extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), + })); + let rroot = replacement.root(); + let inp = replacement.add_op_with_parent( + rroot, + ops::Input { + types: vec![listy.clone()].into(), + }, + )?; + let df1 = replacement.add_op_with_parent( + rroot, + DFG { + signature: FunctionType::new(vec![listy.clone()], intermed.clone()), + }, + )?; + replacement.connect(inp, 0, df1, 0)?; + + let df2 = replacement.add_op_with_parent( + rroot, + DFG { + signature: FunctionType::new(intermed, vec![listy.clone()]), + }, + )?; + [0, 1] + .iter() + .try_for_each(|p| replacement.connect(df1, *p, df2, *p))?; + + let ex = replacement.add_op_with_parent( + rroot, + ops::Output { + types: vec![listy.clone()].into(), + }, + )?; + replacement.connect(df2, 0, ex, 0)?; + + h.apply_rewrite(Replacement { + removal: HashSet::from([entry.node(), bb2.node()]), + replacement, + transfers: HashMap::from([(df1.node(), entry.node()), (df2.node(), bb2.node())]), + mu_inp: vec![], + mu_out: vec![NewEdgeSpec { + src: rroot, + tgt: exit.node(), + kind: NewEdgeKind::ControlFlow { src_pos: 0 }, + }], + mu_new: vec![], + })?; + h.validate(®)?; + Ok(()) + } +} From c23b79840e41ecca5c9791ef7fb3a94103dd4eef Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 23 Oct 2023 15:05:26 +0100 Subject: [PATCH 28/70] Directly build Hugr rather than using Builder --- src/hugr/rewrite/replace.rs | 101 ++++++++++++++++++++++++++---------- 1 file changed, 75 insertions(+), 26 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 1578ddd2b..54a87f47a 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -425,16 +425,15 @@ impl std::fmt::Display for WhichHugr { mod test { use std::collections::{HashMap, HashSet}; - use crate::builder::{Dataflow, HugrBuilder}; use crate::extension::prelude::USIZE_T; use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; - use crate::hugr::{HugrMut, NodeType}; + use crate::hugr::hugrmut::sealed::HugrMutInternals; + use crate::hugr::{HugrError, HugrMut, NodeType}; use crate::ops::handle::NodeHandle; - use crate::ops::{self, BasicBlock, Const, LeafOp, DFG}; + use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; - use crate::types::{Type, TypeArg, TypeRow}; - use crate::{builder::CFGBuilder, types::FunctionType}; - use crate::{type_row, Hugr, HugrView}; + use crate::types::{FunctionType, Type, TypeArg, TypeRow}; + use crate::{type_row, Hugr, HugrView, Node}; use super::{NewEdgeKind, NewEdgeSpec, Replacement}; @@ -456,30 +455,31 @@ mod test { .instantiate_extension_op("push", [TypeArg::Type { ty: USIZE_T }], ®) .unwrap() .into(); + let just_list = TypeRow::from(vec![listy.clone()]); + let exset = ExtensionSet::singleton(&collections::EXTENSION_NAME); let intermed = TypeRow::from(vec![listy.clone(), USIZE_T]); - let mut cfg = CFGBuilder::new(FunctionType::new(vec![listy.clone()], vec![listy.clone()]))?; - let mut entry = cfg.simple_entry_builder( - intermed.clone(), - 1, - ExtensionSet::singleton(&collections::EXTENSION_NAME), + let mut h = Hugr::new(NodeType::open_extensions(ops::CFG { + signature: FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), + })); + let pred_const = h.add_op_with_parent(h.root(), ops::Const::simple_unary_predicate())?; + + let entry = single_node_block(&mut h, pop, pred_const)?; + let bb2 = single_node_block(&mut h, push, pred_const)?; + + let exit = h.add_node_with_parent( + h.root(), + NodeType::open_extensions(BasicBlock::Exit { + cfg_outputs: just_list.clone(), + }), )?; - let [ent_in] = entry.input_wires_arr(); - let popped = entry.add_dataflow_op(pop, [ent_in])?; - let pred = entry.add_load_const(Const::simple_unary_predicate(), ExtensionSet::new())?; - let entry = entry.finish_with_outputs(pred, popped.outputs())?; - - let mut bb2 = - cfg.simple_block_builder(FunctionType::new(intermed.clone(), vec![listy.clone()]), 1)?; - let pushed = bb2.add_dataflow_op(push, bb2.input_wires())?; - let pred = bb2.add_load_const(Const::simple_unary_predicate(), ExtensionSet::new())?; - let bb2 = bb2.finish_with_outputs(pred, pushed.outputs())?; - cfg.branch(&entry, 0, &bb2)?; - - let exit = cfg.exit_block(); - cfg.branch(&bb2, 0, &exit)?; - let mut h = cfg.finish_hugr(®)?; + h.move_before_sibling(entry, pred_const)?; + h.move_before_sibling(exit, pred_const)?; + + h.connect(entry, 0, bb2, 0)?; + h.connect(bb2, 0, exit, 0)?; + h.update_validate(®)?; // Replacement: one BB with two DFGs inside. // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). let mut replacement = Hugr::new(NodeType::open_extensions(BasicBlock::DFB { @@ -536,4 +536,53 @@ mod test { h.validate(®)?; Ok(()) } + + fn single_node_block( + hugr: &mut Hugr, + op: impl Into, + pred_const: Node, + ) -> Result { + let op: OpType = op.into(); + let op_sig = op.signature(); + + let bb = hugr.add_node_with_parent( + hugr.root(), + NodeType::open_extensions(BasicBlock::DFB { + inputs: op_sig.input.clone(), + other_outputs: op_sig.output.clone(), + predicate_variants: vec![type_row![]], + extension_delta: op_sig.extension_reqs.clone(), + }), + )?; + let input = hugr.add_node_with_parent( + bb, + NodeType::open_extensions(ops::Input { + types: op_sig.input.clone(), + }), + )?; + let output = hugr.add_node_with_parent( + bb, + NodeType::open_extensions(ops::Output { + types: op_sig.output.clone(), + }), + )?; + let op = hugr.add_node_with_parent(bb, NodeType::open_extensions(op))?; + + for (p, _) in op_sig.input().iter().enumerate() { + hugr.connect(input, p, op, p)?; + } + let pred = hugr.add_node_with_parent( + bb, + NodeType::open_extensions(ops::LoadConstant { + datatype: Type::new_simple_predicate(1), + }), + )?; + + hugr.connect(pred_const, 0, pred, 0)?; + hugr.connect(pred, 0, output, 0)?; + for (p, _) in op_sig.output().iter().enumerate() { + hugr.connect(op, p, output, p + 1)?; + } + Ok(bb) + } } From 476f0a3864ae58c9fd940a945e28dad62f497009 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 23 Oct 2023 20:20:54 +0100 Subject: [PATCH 29/70] Fix initial Hugr building (using Lift!) --- src/hugr/rewrite/replace.rs | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 54a87f47a..44b9081ad 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -563,23 +563,35 @@ mod test { let output = hugr.add_node_with_parent( bb, NodeType::open_extensions(ops::Output { - types: op_sig.output.clone(), + types: TypeRow::from( + Some(Type::new_simple_predicate(1)) + .into_iter() + .chain(op_sig.output.iter().cloned()).collect::>()) + } ))?; + + const PRED_T: Type = Type::new_simple_predicate(1); + let load_pred = hugr.add_node_with_parent( + bb, + NodeType::pure(ops::LoadConstant { + datatype: PRED_T }), )?; + let mut load_pred_lifted = load_pred; + for e in op.signature().extension_reqs.iter() { + let new_lift = hugr.add_node_with_parent(bb, + NodeType::pure(LeafOp::Lift { type_row: type_row![PRED_T], new_extension: e.clone() }))?; + hugr.connect(load_pred_lifted, 0, new_lift, 0)?; + load_pred_lifted = new_lift; + } + hugr.connect(pred_const, 0, load_pred, 0)?; + hugr.connect(load_pred_lifted, 0, output, 0)?; + let op = hugr.add_node_with_parent(bb, NodeType::open_extensions(op))?; for (p, _) in op_sig.input().iter().enumerate() { hugr.connect(input, p, op, p)?; } - let pred = hugr.add_node_with_parent( - bb, - NodeType::open_extensions(ops::LoadConstant { - datatype: Type::new_simple_predicate(1), - }), - )?; - hugr.connect(pred_const, 0, pred, 0)?; - hugr.connect(pred, 0, output, 0)?; for (p, _) in op_sig.output().iter().enumerate() { hugr.connect(op, p, output, p + 1)?; } From 52c7c54236773de9af16fd091f3d39fdd69e7b7e Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 08:31:38 +0100 Subject: [PATCH 30/70] Fix root type of replacement, fmt --- src/hugr/rewrite/replace.rs | 51 ++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 21 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 44b9081ad..f6bb42d86 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -482,21 +482,26 @@ mod test { h.update_validate(®)?; // Replacement: one BB with two DFGs inside. // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). - let mut replacement = Hugr::new(NodeType::open_extensions(BasicBlock::DFB { - inputs: vec![listy.clone()].into(), - predicate_variants: vec![type_row![]], - other_outputs: vec![listy.clone()].into(), - extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), + let mut replacement = Hugr::new(NodeType::open_extensions(ops::CFG { + signature: FunctionType::new_linear(just_list.clone()), })); - let rroot = replacement.root(); + let bb = replacement.add_node_with_parent( + replacement.root(), + NodeType::open_extensions(BasicBlock::DFB { + inputs: vec![listy.clone()].into(), + predicate_variants: vec![type_row![]], + other_outputs: vec![listy.clone()].into(), + extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), + }), + )?; let inp = replacement.add_op_with_parent( - rroot, + bb, ops::Input { types: vec![listy.clone()].into(), }, )?; let df1 = replacement.add_op_with_parent( - rroot, + bb, DFG { signature: FunctionType::new(vec![listy.clone()], intermed.clone()), }, @@ -504,7 +509,7 @@ mod test { replacement.connect(inp, 0, df1, 0)?; let df2 = replacement.add_op_with_parent( - rroot, + bb, DFG { signature: FunctionType::new(intermed, vec![listy.clone()]), }, @@ -514,7 +519,7 @@ mod test { .try_for_each(|p| replacement.connect(df1, *p, df2, *p))?; let ex = replacement.add_op_with_parent( - rroot, + bb, ops::Output { types: vec![listy.clone()].into(), }, @@ -527,7 +532,7 @@ mod test { transfers: HashMap::from([(df1.node(), entry.node()), (df2.node(), bb2.node())]), mu_inp: vec![], mu_out: vec![NewEdgeSpec { - src: rroot, + src: bb, tgt: exit.node(), kind: NewEdgeKind::ControlFlow { src_pos: 0 }, }], @@ -566,20 +571,24 @@ mod test { types: TypeRow::from( Some(Type::new_simple_predicate(1)) .into_iter() - .chain(op_sig.output.iter().cloned()).collect::>()) - } ))?; - - const PRED_T: Type = Type::new_simple_predicate(1); - let load_pred = hugr.add_node_with_parent( - bb, - NodeType::pure(ops::LoadConstant { - datatype: PRED_T + .chain(op_sig.output.iter().cloned()) + .collect::>(), + ), }), )?; + + const PRED_T: Type = Type::new_simple_predicate(1); + let load_pred = + hugr.add_node_with_parent(bb, NodeType::pure(ops::LoadConstant { datatype: PRED_T }))?; let mut load_pred_lifted = load_pred; for e in op.signature().extension_reqs.iter() { - let new_lift = hugr.add_node_with_parent(bb, - NodeType::pure(LeafOp::Lift { type_row: type_row![PRED_T], new_extension: e.clone() }))?; + let new_lift = hugr.add_node_with_parent( + bb, + NodeType::pure(LeafOp::Lift { + type_row: type_row![PRED_T], + new_extension: e.clone(), + }), + )?; hugr.connect(load_pred_lifted, 0, new_lift, 0)?; load_pred_lifted = new_lift; } From 50b7b5c63199b524fb04010d2ca20e91f9862321 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 15:51:37 +0100 Subject: [PATCH 31/70] Fix: translate transfer-keys using node_map --- src/hugr/rewrite/replace.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index f6bb42d86..af454c88d 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -295,12 +295,14 @@ impl Rewrite for Replacement { h.remove_node(new_root).unwrap(); // 6. Transfer to keys of `transfers` children of the corresponding values. - fn first_child(h: &impl HugrView, parent: Node) -> Option { - h.children(parent).next() - } - for (new_parent, old_parent) in self.transfers.iter() { - debug_assert!(h.children(*old_parent).next().is_some()); - while let Some(ch) = first_child(h, *old_parent) { + for (new_parent, &old_parent) in self.transfers.iter() { + let new_parent = node_map.get(new_parent).unwrap(); + debug_assert!(h.children(old_parent).next().is_some()); + loop { + let ch = match h.children(old_parent).next() { + None => break, + Some(c) => c, + }; h.set_parent(ch, *new_parent).unwrap(); } } From 1bef6331979581b033ebcc776f29618941d28b03 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 16:02:37 +0100 Subject: [PATCH 32/70] Check transfer keys too --- src/hugr/rewrite/replace.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index af454c88d..3b673705f 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -165,6 +165,14 @@ impl Replacement { } fn get_removed_nodes(&self, h: &impl HugrView) -> Result, ReplaceError> { + // Check the keys of the transfer map too, the values we'll use imminently + self.transfers.keys().try_for_each(|&n| { + (self.replacement.contains_node(n) + && self.replacement.get_optype(n).is_container() + && self.replacement.children(n).next().is_none()) + .then_some(()) + .ok_or(ReplaceError::InvalidTransferTarget(n)) + })?; let mut transferred: HashSet = self.transfers.values().copied().collect(); if transferred.len() != self.transfers.values().len() { return Err(ReplaceError::ConflictingTransfers( @@ -389,6 +397,9 @@ pub enum ReplaceError { /// Values in transfer map were not unique - contains the repeated elements #[error("Nodes cannot be transferred to multiple locations: {0:?}")] ConflictingTransfers(Vec), + /// Keys in transfer map were not valid container nodes in replacement + #[error("Node {0:?} was not an empty container node in the replacement")] + InvalidTransferTarget(Node), /// Some values in the transfer map were either descendants of other values, /// or not descendants of the removed nodes #[error("Nodes not free to be moved into new locations: {0:?}")] From 72f9bf23ee69ad2bceb663aea09f435e15173d8d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 08:32:21 +0100 Subject: [PATCH 33/70] WIP debug --- src/hugr/rewrite/replace.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 3b673705f..edc43fad5 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -264,15 +264,16 @@ impl Rewrite for Replacement { fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { let parent = self.check_parent(h)?; + println!("ALAN got parent"); // Calculate removed nodes here. (Does not include transfers, so enumerates only // nodes we are going to remove, individually, anyway; so no *asymptotic* speed penalty) let to_remove = self.get_removed_nodes(h)?; - + println!("ALAN got removed nodes"); // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want. // TODO what would an error here mean? e.g. malformed self.replacement?? let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement).unwrap(); - + println!("ALAN inserted"); // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| node_map.get(&n).copied().ok_or(WhichHugr::Replacement); let kept = |n| { @@ -288,7 +289,7 @@ impl Rewrite for Replacement { // 4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets. transfer_edges(h, self.mu_new.iter(), kept, kept, Some(parent))?; - + println!("ALAN edges transferred"); // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) let mut remove_top_sibs = self.removal.iter(); @@ -301,7 +302,7 @@ impl Rewrite for Replacement { } debug_assert!(h.children(new_root).next().is_none()); h.remove_node(new_root).unwrap(); - + println!("ALAN transferring subtrees..."); // 6. Transfer to keys of `transfers` children of the corresponding values. for (new_parent, &old_parent) in self.transfers.iter() { let new_parent = node_map.get(new_parent).unwrap(); @@ -314,6 +315,7 @@ impl Rewrite for Replacement { h.set_parent(ch, *new_parent).unwrap(); } } + println!("ALAN Transfers done"); // 7. Remove remaining nodes to_remove @@ -538,7 +540,7 @@ mod test { }, )?; replacement.connect(df2, 0, ex, 0)?; - + println!("ALAN calling apply_rewrite"); h.apply_rewrite(Replacement { removal: HashSet::from([entry.node(), bb2.node()]), replacement, @@ -551,6 +553,7 @@ mod test { }], mu_new: vec![], })?; + panic!("ALAN apply_rewrite done; validating"); h.validate(®)?; Ok(()) } From be28f215081aa5bea1b0be130dc8e31e7a9c6b7a Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 16:05:15 +0100 Subject: [PATCH 34/70] Revert "WIP debug" This reverts commit 72f9bf23ee69ad2bceb663aea09f435e15173d8d. --- src/hugr/rewrite/replace.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index edc43fad5..3b673705f 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -264,16 +264,15 @@ impl Rewrite for Replacement { fn apply(self, h: &mut impl HugrMut) -> Result<(), Self::Error> { let parent = self.check_parent(h)?; - println!("ALAN got parent"); // Calculate removed nodes here. (Does not include transfers, so enumerates only // nodes we are going to remove, individually, anyway; so no *asymptotic* speed penalty) let to_remove = self.get_removed_nodes(h)?; - println!("ALAN got removed nodes"); + // 1. Add all the new nodes. Note this includes replacement.root(), which we don't want. // TODO what would an error here mean? e.g. malformed self.replacement?? let InsertionResult { new_root, node_map } = h.insert_hugr(parent, self.replacement).unwrap(); - println!("ALAN inserted"); + // 2. Add new edges from existing to copied nodes according to mu_in let translate_idx = |n| node_map.get(&n).copied().ok_or(WhichHugr::Replacement); let kept = |n| { @@ -289,7 +288,7 @@ impl Rewrite for Replacement { // 4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets. transfer_edges(h, self.mu_new.iter(), kept, kept, Some(parent))?; - println!("ALAN edges transferred"); + // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) let mut remove_top_sibs = self.removal.iter(); @@ -302,7 +301,7 @@ impl Rewrite for Replacement { } debug_assert!(h.children(new_root).next().is_none()); h.remove_node(new_root).unwrap(); - println!("ALAN transferring subtrees..."); + // 6. Transfer to keys of `transfers` children of the corresponding values. for (new_parent, &old_parent) in self.transfers.iter() { let new_parent = node_map.get(new_parent).unwrap(); @@ -315,7 +314,6 @@ impl Rewrite for Replacement { h.set_parent(ch, *new_parent).unwrap(); } } - println!("ALAN Transfers done"); // 7. Remove remaining nodes to_remove @@ -540,7 +538,7 @@ mod test { }, )?; replacement.connect(df2, 0, ex, 0)?; - println!("ALAN calling apply_rewrite"); + h.apply_rewrite(Replacement { removal: HashSet::from([entry.node(), bb2.node()]), replacement, @@ -553,7 +551,6 @@ mod test { }], mu_new: vec![], })?; - panic!("ALAN apply_rewrite done; validating"); h.validate(®)?; Ok(()) } From 3ae6d10ce55a46cd86374a140f0746cfe6e9e41d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 17:02:31 +0100 Subject: [PATCH 35/70] Removal a list not a set, order matters --- src/hugr/rewrite/replace.rs | 8 ++++---- src/hugr/rewrite/simple_replace.rs | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 3b673705f..43feb73d0 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -1,9 +1,9 @@ //! Implementation of the `Replace` operation. use std::collections::hash_map::Values; -use std::collections::hash_set::Iter; use std::collections::{HashMap, HashSet, VecDeque}; use std::iter::{Chain, Copied, Take}; +use std::slice::Iter; use itertools::Itertools; use thiserror::Error; @@ -62,7 +62,7 @@ pub struct Replacement { /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. /// Must be non-empty - otherwise there is no parent under which to place [replacement], /// and no possible [in_edges], [out_edges] or [transfers]. - pub removal: HashSet, + pub removal: Vec, /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. pub replacement: Hugr, /// Map from container nodes in [replacement] that have no children, to container nodes @@ -436,7 +436,7 @@ impl std::fmt::Display for WhichHugr { #[cfg(test)] mod test { - use std::collections::{HashMap, HashSet}; + use std::collections::HashMap; use crate::extension::prelude::USIZE_T; use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; @@ -540,7 +540,7 @@ mod test { replacement.connect(df2, 0, ex, 0)?; h.apply_rewrite(Replacement { - removal: HashSet::from([entry.node(), bb2.node()]), + removal: vec![entry.node(), bb2.node()], replacement, transfers: HashMap::from([(df1.node(), entry.node()), (df2.node(), bb2.node())]), mu_inp: vec![], diff --git a/src/hugr/rewrite/simple_replace.rs b/src/hugr/rewrite/simple_replace.rs index 0608f35ee..c95350a5c 100644 --- a/src/hugr/rewrite/simple_replace.rs +++ b/src/hugr/rewrite/simple_replace.rs @@ -643,7 +643,7 @@ pub(in crate::hugr::rewrite) mod test { replacement.remove_node(in_).unwrap(); replacement.remove_node(out).unwrap(); Replacement { - removal: s.subgraph.nodes().iter().copied().collect(), + removal: s.subgraph.nodes().to_vec(), replacement, transfers: HashMap::new(), mu_inp, From d41300a510961ae175d6620bdc7d3242393e8e05 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 17:08:24 +0100 Subject: [PATCH 36/70] Rename variables in test --- src/hugr/rewrite/replace.rs | 47 +++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 43feb73d0..f15fe302a 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -498,7 +498,7 @@ mod test { let mut replacement = Hugr::new(NodeType::open_extensions(ops::CFG { signature: FunctionType::new_linear(just_list.clone()), })); - let bb = replacement.add_node_with_parent( + let r_bb = replacement.add_node_with_parent( replacement.root(), NodeType::open_extensions(BasicBlock::DFB { inputs: vec![listy.clone()].into(), @@ -507,45 +507,46 @@ mod test { extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), }), )?; - let inp = replacement.add_op_with_parent( - bb, - ops::Input { - types: vec![listy.clone()].into(), - }, - )?; - let df1 = replacement.add_op_with_parent( - bb, + let r_df1 = replacement.add_op_with_parent( + r_bb, DFG { signature: FunctionType::new(vec![listy.clone()], intermed.clone()), }, )?; - replacement.connect(inp, 0, df1, 0)?; - - let df2 = replacement.add_op_with_parent( - bb, + let r_df2 = replacement.add_op_with_parent( + r_bb, DFG { signature: FunctionType::new(intermed, vec![listy.clone()]), }, )?; [0, 1] .iter() - .try_for_each(|p| replacement.connect(df1, *p, df2, *p))?; + .try_for_each(|p| replacement.connect(r_df1, *p, r_df2, *p))?; - let ex = replacement.add_op_with_parent( - bb, - ops::Output { - types: vec![listy.clone()].into(), - }, - )?; - replacement.connect(df2, 0, ex, 0)?; + { + let inp = replacement.add_op_before( + r_df1, + ops::Input { + types: vec![listy.clone()].into(), + }, + )?; + let out = replacement.add_op_with_parent( + r_bb, + ops::Output { + types: vec![listy.clone()].into(), + }, + )?; + replacement.connect(inp, 0, r_df1, 0)?; + replacement.connect(r_df2, 0, out, 0)?; + } h.apply_rewrite(Replacement { removal: vec![entry.node(), bb2.node()], replacement, - transfers: HashMap::from([(df1.node(), entry.node()), (df2.node(), bb2.node())]), + transfers: HashMap::from([(r_df1.node(), entry.node()), (r_df2.node(), bb2.node())]), mu_inp: vec![], mu_out: vec![NewEdgeSpec { - src: bb, + src: r_bb, tgt: exit.node(), kind: NewEdgeKind::ControlFlow { src_pos: 0 }, }], From 1c96bbd800463b936c3c2b334af5058868aa7988 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 24 Oct 2023 17:27:08 +0100 Subject: [PATCH 37/70] Update_validate, open_extensions, fix ports+wiring for predicate --- src/hugr/rewrite/replace.rs | 57 ++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 26 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index f15fe302a..79dcbc190 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -507,37 +507,41 @@ mod test { extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), }), )?; - let r_df1 = replacement.add_op_with_parent( + let r_df1 = replacement.add_node_with_parent( r_bb, - DFG { - signature: FunctionType::new(vec![listy.clone()], intermed.clone()), - }, + NodeType::open_extensions(DFG { + signature: FunctionType::new( + vec![listy.clone()], + simple_unary_plus(intermed.clone()), + ), + }), )?; - let r_df2 = replacement.add_op_with_parent( + let r_df2 = replacement.add_node_with_parent( r_bb, - DFG { - signature: FunctionType::new(intermed, vec![listy.clone()]), - }, + NodeType::open_extensions(DFG { + signature: FunctionType::new(intermed, simple_unary_plus(just_list.clone())), + }), )?; [0, 1] .iter() - .try_for_each(|p| replacement.connect(r_df1, *p, r_df2, *p))?; + .try_for_each(|p| replacement.connect(r_df1, *p + 1, r_df2, *p))?; { - let inp = replacement.add_op_before( + let inp = replacement.add_node_before( r_df1, - ops::Input { - types: vec![listy.clone()].into(), - }, + NodeType::open_extensions(ops::Input { + types: just_list.clone(), + }), )?; - let out = replacement.add_op_with_parent( - r_bb, - ops::Output { - types: vec![listy.clone()].into(), - }, + let out = replacement.add_node_before( + r_df1, + NodeType::open_extensions(ops::Output { + types: simple_unary_plus(just_list), + }), )?; replacement.connect(inp, 0, r_df1, 0)?; replacement.connect(r_df2, 0, out, 0)?; + replacement.connect(r_df2, 1, out, 1)?; } h.apply_rewrite(Replacement { @@ -552,7 +556,7 @@ mod test { }], mu_new: vec![], })?; - h.validate(®)?; + h.update_validate(®)?; Ok(()) } @@ -582,12 +586,7 @@ mod test { let output = hugr.add_node_with_parent( bb, NodeType::open_extensions(ops::Output { - types: TypeRow::from( - Some(Type::new_simple_predicate(1)) - .into_iter() - .chain(op_sig.output.iter().cloned()) - .collect::>(), - ), + types: simple_unary_plus(op_sig.output.clone()), }), )?; @@ -598,7 +597,7 @@ mod test { for e in op.signature().extension_reqs.iter() { let new_lift = hugr.add_node_with_parent( bb, - NodeType::pure(LeafOp::Lift { + NodeType::open_extensions(LeafOp::Lift { type_row: type_row![PRED_T], new_extension: e.clone(), }), @@ -620,4 +619,10 @@ mod test { } Ok(bb) } + + fn simple_unary_plus(t: TypeRow) -> TypeRow { + let mut v = t.into_owned(); + v.insert(0, Type::new_simple_predicate(1)); + v.into() + } } From 5eaf13a73a29da0dabbc04fbf88fa23577f2e3b5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 25 Oct 2023 14:08:30 +0100 Subject: [PATCH 38/70] Update renamed predicate->tuple/unit_sum --- src/hugr/rewrite/replace.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 79dcbc190..547878cff 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -475,7 +475,7 @@ mod test { let mut h = Hugr::new(NodeType::open_extensions(ops::CFG { signature: FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), })); - let pred_const = h.add_op_with_parent(h.root(), ops::Const::simple_unary_predicate())?; + let pred_const = h.add_op_with_parent(h.root(), ops::Const::unary_unit_sum())?; let entry = single_node_block(&mut h, pop, pred_const)?; let bb2 = single_node_block(&mut h, push, pred_const)?; @@ -502,7 +502,7 @@ mod test { replacement.root(), NodeType::open_extensions(BasicBlock::DFB { inputs: vec![listy.clone()].into(), - predicate_variants: vec![type_row![]], + tuple_sum_rows: vec![type_row![]], other_outputs: vec![listy.clone()].into(), extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), }), @@ -573,7 +573,7 @@ mod test { NodeType::open_extensions(BasicBlock::DFB { inputs: op_sig.input.clone(), other_outputs: op_sig.output.clone(), - predicate_variants: vec![type_row![]], + tuple_sum_rows: vec![type_row![]], extension_delta: op_sig.extension_reqs.clone(), }), )?; @@ -590,7 +590,7 @@ mod test { }), )?; - const PRED_T: Type = Type::new_simple_predicate(1); + const PRED_T: Type = Type::new_unit_sum(1); let load_pred = hugr.add_node_with_parent(bb, NodeType::pure(ops::LoadConstant { datatype: PRED_T }))?; let mut load_pred_lifted = load_pred; @@ -622,7 +622,7 @@ mod test { fn simple_unary_plus(t: TypeRow) -> TypeRow { let mut v = t.into_owned(); - v.insert(0, Type::new_simple_predicate(1)); + v.insert(0, Type::new_unit_sum(1)); v.into() } } From 3fec96abcd7f56e9d245a33625d795ebe8ded45d Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 30 Oct 2023 08:54:58 +0000 Subject: [PATCH 39/70] Remove lift nodes, use open_extensions for Const + load-constant --- src/hugr/rewrite/replace.rs | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 547878cff..a8f08bbd3 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -475,7 +475,10 @@ mod test { let mut h = Hugr::new(NodeType::open_extensions(ops::CFG { signature: FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), })); - let pred_const = h.add_op_with_parent(h.root(), ops::Const::unary_unit_sum())?; + let pred_const = h.add_node_with_parent( + h.root(), + NodeType::open_extensions(ops::Const::unary_unit_sum()), + )?; let entry = single_node_block(&mut h, pop, pred_const)?; let bb2 = single_node_block(&mut h, push, pred_const)?; @@ -591,22 +594,12 @@ mod test { )?; const PRED_T: Type = Type::new_unit_sum(1); - let load_pred = - hugr.add_node_with_parent(bb, NodeType::pure(ops::LoadConstant { datatype: PRED_T }))?; - let mut load_pred_lifted = load_pred; - for e in op.signature().extension_reqs.iter() { - let new_lift = hugr.add_node_with_parent( - bb, - NodeType::open_extensions(LeafOp::Lift { - type_row: type_row![PRED_T], - new_extension: e.clone(), - }), - )?; - hugr.connect(load_pred_lifted, 0, new_lift, 0)?; - load_pred_lifted = new_lift; - } + let load_pred = hugr.add_node_with_parent( + bb, + NodeType::open_extensions(ops::LoadConstant { datatype: PRED_T }), + )?; hugr.connect(pred_const, 0, load_pred, 0)?; - hugr.connect(load_pred_lifted, 0, output, 0)?; + hugr.connect(load_pred, 0, output, 0)?; let op = hugr.add_node_with_parent(bb, NodeType::open_extensions(op))?; From 51164a0a508204ad5c548425506e740ca28b2040 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 30 Oct 2023 09:24:20 +0000 Subject: [PATCH 40/70] CI fmt --- src/hugr/rewrite/replace.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index a8f08bbd3..a4c5b2ca9 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -333,7 +333,9 @@ impl Rewrite for Replacement { fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { while descendant != ancestor { - let Some(p) = h.get_parent(descendant) else {return false}; + let Some(p) = h.get_parent(descendant) else { + return false + }; descendant = p; } true From 0ce6900e96f113d7b24f98a11e2ae83e3202884e Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 30 Oct 2023 13:38:40 +0000 Subject: [PATCH 41/70] Update re. port directions --- src/hugr/rewrite/replace.rs | 59 ++++++++++++++---------------- src/hugr/rewrite/simple_replace.rs | 14 +++---- 2 files changed, 35 insertions(+), 38 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index a4c5b2ca9..18b825b4b 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -12,7 +12,7 @@ use crate::hugr::hugrmut::InsertionResult; use crate::hugr::HugrMut; use crate::ops::{OpTag, OpTrait}; use crate::types::EdgeKind; -use crate::{Direction, Hugr, HugrView, Node, Port}; +use crate::{Direction, Hugr, HugrView, IncomingPort, Node, OutgoingPort}; use super::Rewrite; @@ -37,21 +37,21 @@ pub enum NewEdgeKind { /// An [EdgeKind::Value] edge (between DFG nodes only) Value { /// The source port - src_pos: usize, + src_pos: OutgoingPort, /// The target port - tgt_pos: usize, + tgt_pos: IncomingPort, }, /// An [EdgeKind::Static] edge Static { /// The source port - src_pos: usize, + src_pos: OutgoingPort, /// The target port - tgt_pos: usize, + tgt_pos: IncomingPort, }, /// A [EdgeKind::ControlFlow] edge (between CFG nodes only) ControlFlow { /// Identifies a control-flow output (successor) of the source node. - src_pos: usize, + src_pos: OutgoingPort, }, } @@ -88,18 +88,15 @@ impl NewEdgeSpec { let optype = h.get_optype(self.src); let ok = match self.kind { NewEdgeKind::Order => optype.other_output() == Some(EdgeKind::StateOrder), - NewEdgeKind::Value { src_pos, .. } => matches!( - optype.port_kind(Port::new_outgoing(src_pos)), - Some(EdgeKind::Value(_)) - ), - NewEdgeKind::Static { src_pos, .. } => matches!( - optype.port_kind(Port::new_outgoing(src_pos)), - Some(EdgeKind::Static(_)) - ), - NewEdgeKind::ControlFlow { src_pos } => matches!( - optype.port_kind(Port::new_outgoing(src_pos)), - Some(EdgeKind::ControlFlow) - ), + NewEdgeKind::Value { src_pos, .. } => { + matches!(optype.port_kind(src_pos), Some(EdgeKind::Value(_))) + } + NewEdgeKind::Static { src_pos, .. } => { + matches!(optype.port_kind(src_pos), Some(EdgeKind::Static(_))) + } + NewEdgeKind::ControlFlow { src_pos } => { + matches!(optype.port_kind(src_pos), Some(EdgeKind::ControlFlow)) + } }; ok.then_some(()) .ok_or(ReplaceError::BadEdgeKind(Direction::Outgoing, self.clone())) @@ -108,16 +105,14 @@ impl NewEdgeSpec { let optype = h.get_optype(self.tgt); let ok = match self.kind { NewEdgeKind::Order => optype.other_input() == Some(EdgeKind::StateOrder), - NewEdgeKind::Value { tgt_pos, .. } => matches!( - optype.port_kind(Port::new_incoming(tgt_pos)), - Some(EdgeKind::Value(_)) - ), - NewEdgeKind::Static { tgt_pos, .. } => matches!( - optype.port_kind(Port::new_incoming(tgt_pos)), - Some(EdgeKind::Static(_)) - ), + NewEdgeKind::Value { tgt_pos, .. } => { + matches!(optype.port_kind(tgt_pos), Some(EdgeKind::Value(_))) + } + NewEdgeKind::Static { tgt_pos, .. } => { + matches!(optype.port_kind(tgt_pos), Some(EdgeKind::Static(_))) + } NewEdgeKind::ControlFlow { .. } => matches!( - optype.port_kind(Port::new_incoming(0)), + optype.port_kind(IncomingPort::from(0)), Some(EdgeKind::ControlFlow) ), }; @@ -133,7 +128,7 @@ impl NewEdgeSpec { if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind { let found_incoming = h - .linked_ports(self.tgt, Port::new_incoming(tgt_pos)) + .linked_ports(self.tgt, tgt_pos) .exactly_one() .is_ok_and(|(src_n, _)| src_ok(src_n)); if !found_incoming { @@ -372,7 +367,7 @@ fn transfer_edges<'a>( NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { if let Some(anc) = existing_src_ancestor { e.check_existing_edge(h, |n| descends(h, anc, n))?; - h.disconnect(e.tgt, Port::new_incoming(tgt_pos)).unwrap(); + h.disconnect(e.tgt, IncomingPort::from(tgt_pos)).unwrap(); } h.connect(e.src, src_pos, e.tgt, tgt_pos).unwrap(); } @@ -448,7 +443,7 @@ mod test { use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; - use crate::{type_row, Hugr, HugrView, Node}; + use crate::{type_row, Hugr, HugrView, Node, OutgoingPort}; use super::{NewEdgeKind, NewEdgeSpec, Replacement}; @@ -557,7 +552,9 @@ mod test { mu_out: vec![NewEdgeSpec { src: r_bb, tgt: exit.node(), - kind: NewEdgeKind::ControlFlow { src_pos: 0 }, + kind: NewEdgeKind::ControlFlow { + src_pos: OutgoingPort::from(0), + }, }], mu_new: vec![], })?; diff --git a/src/hugr/rewrite/simple_replace.rs b/src/hugr/rewrite/simple_replace.rs index 7bd84d426..d64255332 100644 --- a/src/hugr/rewrite/simple_replace.rs +++ b/src/hugr/rewrite/simple_replace.rs @@ -584,7 +584,7 @@ pub(in crate::hugr::rewrite) mod test { use crate::hugr::rewrite::replace::Replacement; fn to_replace(h: &impl HugrView, s: SimpleReplacement) -> Replacement { use crate::hugr::rewrite::replace::{NewEdgeKind, NewEdgeSpec}; - use crate::hugr::PortIndex; + let mut replacement = s.replacement; let (in_, out) = replacement .children(replacement.root()) @@ -598,13 +598,13 @@ pub(in crate::hugr::rewrite) mod test { if *tgt == out { unimplemented!() }; - let (src, src_port) = h.linked_ports(*r_n, *r_p).exactly_one().ok().unwrap(); + let (src, src_port) = h.linked_outputs(*r_n, *r_p).exactly_one().ok().unwrap(); NewEdgeSpec { src, tgt: *tgt, kind: NewEdgeKind::Value { - src_pos: src_port.index(), - tgt_pos: tgt_port.index(), + src_pos: src_port, + tgt_pos: *tgt_port, }, } }) @@ -614,7 +614,7 @@ pub(in crate::hugr::rewrite) mod test { .iter() .map(|((tgt, tgt_port), out_port)| { let (src, src_port) = replacement - .linked_ports(out, *out_port) + .linked_outputs(out, *out_port) .exactly_one() .ok() .unwrap(); @@ -625,8 +625,8 @@ pub(in crate::hugr::rewrite) mod test { src, tgt: *tgt, kind: NewEdgeKind::Value { - src_pos: src_port.index(), - tgt_pos: tgt_port.index(), + src_pos: src_port, + tgt_pos: *tgt_port, }, } }) From 002e89ae461f7bbea410b5bd5c3fabbc4f626cdf Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 30 Oct 2023 09:44:50 +0000 Subject: [PATCH 42/70] CI fmt more --- src/hugr/rewrite/replace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 18b825b4b..1b3fc74c9 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -329,7 +329,7 @@ impl Rewrite for Replacement { fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { while descendant != ancestor { let Some(p) = h.get_parent(descendant) else { - return false + return false; }; descendant = p; } From b35eb86da224214c46f65c48b458f28d4b249e38 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 09:18:18 +0000 Subject: [PATCH 43/70] Rewrite test using builder again; no lift nodes, Const has singleton-collections --- src/hugr/rewrite/replace.rs | 127 ++++++++++++++---------------------- 1 file changed, 49 insertions(+), 78 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 1b3fc74c9..7b26ab6a8 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -435,11 +435,11 @@ impl std::fmt::Display for WhichHugr { mod test { use std::collections::HashMap; + use crate::builder::{BuildError, CFGBuilder, Container, Dataflow, HugrBuilder}; use crate::extension::prelude::USIZE_T; use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; - use crate::hugr::hugrmut::sealed::HugrMutInternals; - use crate::hugr::{HugrError, HugrMut, NodeType}; - use crate::ops::handle::NodeHandle; + use crate::hugr::{HugrMut, NodeType}; + use crate::ops::handle::{BasicBlockID, ConstID, NodeHandle}; use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; @@ -469,69 +469,64 @@ mod test { let exset = ExtensionSet::singleton(&collections::EXTENSION_NAME); let intermed = TypeRow::from(vec![listy.clone(), USIZE_T]); - let mut h = Hugr::new(NodeType::open_extensions(ops::CFG { - signature: FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), - })); - let pred_const = h.add_node_with_parent( - h.root(), - NodeType::open_extensions(ops::Const::unary_unit_sum()), + let mut cfg = CFGBuilder::new( + FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), )?; - let entry = single_node_block(&mut h, pop, pred_const)?; - let bb2 = single_node_block(&mut h, push, pred_const)?; - - let exit = h.add_node_with_parent( - h.root(), - NodeType::open_extensions(BasicBlock::Exit { - cfg_outputs: just_list.clone(), - }), + let pred_const = cfg.add_constant( + ops::Const::unary_unit_sum(), + // Slightly awkward we have to specify this, and the choice is non-obvious: + ExtensionSet::singleton(&collections::EXTENSION_NAME), )?; - h.move_before_sibling(entry, pred_const)?; - h.move_before_sibling(exit, pred_const)?; - h.connect(entry, 0, bb2, 0)?; - h.connect(bb2, 0, exit, 0)?; + let entry = single_node_block(&mut cfg, pop, &pred_const, true)?; + let bb2 = single_node_block(&mut cfg, push, &pred_const, false)?; + + let exit = cfg.exit_block(); + cfg.branch(&entry, 0, &bb2)?; + cfg.branch(&bb2, 0, &exit)?; + + let mut h = cfg.finish_hugr(®).unwrap(); - h.update_validate(®)?; // Replacement: one BB with two DFGs inside. // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). let mut replacement = Hugr::new(NodeType::open_extensions(ops::CFG { signature: FunctionType::new_linear(just_list.clone()), })); - let r_bb = replacement.add_node_with_parent( + let r_bb = replacement.add_op_with_parent( replacement.root(), - NodeType::open_extensions(BasicBlock::DFB { + BasicBlock::DFB { inputs: vec![listy.clone()].into(), tuple_sum_rows: vec![type_row![]], other_outputs: vec![listy.clone()].into(), extension_delta: ExtensionSet::singleton(&collections::EXTENSION_NAME), - }), + }, )?; - let r_df1 = replacement.add_node_with_parent( + let r_df1 = replacement.add_op_with_parent( r_bb, - NodeType::open_extensions(DFG { + DFG { signature: FunctionType::new( vec![listy.clone()], simple_unary_plus(intermed.clone()), ), - }), + }, )?; - let r_df2 = replacement.add_node_with_parent( + let r_df2 = replacement.add_op_with_parent( r_bb, - NodeType::open_extensions(DFG { + DFG { signature: FunctionType::new(intermed, simple_unary_plus(just_list.clone())), - }), + }, )?; [0, 1] .iter() .try_for_each(|p| replacement.connect(r_df1, *p + 1, r_df2, *p))?; { - let inp = replacement.add_node_before( + let inp = replacement.add_op_before( r_df1, - NodeType::open_extensions(ops::Input { + ops::Input { types: just_list.clone(), - }), + }, )?; let out = replacement.add_node_before( r_df1, @@ -562,54 +557,30 @@ mod test { Ok(()) } - fn single_node_block( - hugr: &mut Hugr, + fn single_node_block + AsMut>( + h: &mut CFGBuilder, op: impl Into, - pred_const: Node, - ) -> Result { + pred_const: &ConstID, + entry: bool, + ) -> Result { let op: OpType = op.into(); let op_sig = op.signature(); + let mut bb = if entry { + assert_eq!( + match h.hugr().get_optype(h.container_node()) { + OpType::CFG(c) => &c.signature.input, + _ => panic!(), + }, + op_sig.input() + ); + h.simple_entry_builder(op_sig.output, 1, op_sig.extension_reqs.clone())? + } else { + h.simple_block_builder(op_sig, 1)? + }; - let bb = hugr.add_node_with_parent( - hugr.root(), - NodeType::open_extensions(BasicBlock::DFB { - inputs: op_sig.input.clone(), - other_outputs: op_sig.output.clone(), - tuple_sum_rows: vec![type_row![]], - extension_delta: op_sig.extension_reqs.clone(), - }), - )?; - let input = hugr.add_node_with_parent( - bb, - NodeType::open_extensions(ops::Input { - types: op_sig.input.clone(), - }), - )?; - let output = hugr.add_node_with_parent( - bb, - NodeType::open_extensions(ops::Output { - types: simple_unary_plus(op_sig.output.clone()), - }), - )?; - - const PRED_T: Type = Type::new_unit_sum(1); - let load_pred = hugr.add_node_with_parent( - bb, - NodeType::open_extensions(ops::LoadConstant { datatype: PRED_T }), - )?; - hugr.connect(pred_const, 0, load_pred, 0)?; - hugr.connect(load_pred, 0, output, 0)?; - - let op = hugr.add_node_with_parent(bb, NodeType::open_extensions(op))?; - - for (p, _) in op_sig.input().iter().enumerate() { - hugr.connect(input, p, op, p)?; - } - - for (p, _) in op_sig.output().iter().enumerate() { - hugr.connect(op, p, output, p + 1)?; - } - Ok(bb) + let op = bb.add_dataflow_op(op, bb.input_wires())?; + let load_pred = bb.load_const(pred_const)?; + bb.finish_with_outputs(load_pred, op.outputs()) } fn simple_unary_plus(t: TypeRow) -> TypeRow { From 7a3029d6436d7ad6364487d5049e8ab13eda1e67 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Thu, 2 Nov 2023 09:27:58 +0000 Subject: [PATCH 44/70] clippy --- src/hugr/rewrite/replace.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 7b26ab6a8..187956715 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -367,7 +367,7 @@ fn transfer_edges<'a>( NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { if let Some(anc) = existing_src_ancestor { e.check_existing_edge(h, |n| descends(h, anc, n))?; - h.disconnect(e.tgt, IncomingPort::from(tgt_pos)).unwrap(); + h.disconnect(e.tgt, tgt_pos).unwrap(); } h.connect(e.src, src_pos, e.tgt, tgt_pos).unwrap(); } @@ -443,7 +443,7 @@ mod test { use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; - use crate::{type_row, Hugr, HugrView, Node, OutgoingPort}; + use crate::{type_row, Hugr, HugrView, OutgoingPort}; use super::{NewEdgeKind, NewEdgeSpec, Replacement}; From b463fd314c1d88ab097505bc79393c66242b13e4 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:08:36 +0000 Subject: [PATCH 45/70] Spec tweaks courtesy of Alec Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com> --- specification/hugr.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/specification/hugr.md b/specification/hugr.md index 86d694234..a72560ddb 100644 --- a/specification/hugr.md +++ b/specification/hugr.md @@ -1236,10 +1236,10 @@ The `Replace` method takes as input: $G$ and `SrcNode` in $\Gamma \setminus R$; - a list $\mu\_\textrm{out}$ of `NewEdgeSpec` which all have their `SrcNode`in $G$ and `TgtNode` in $\Gamma \setminus R$, where `TgtNode` and `TgtPos` describe - an existing incoming edge of that kind from a node in $S\*$. + an existing incoming edge of that kind from a node in $S^\*$. - a list $\mu\_\textrm{new}$ of `NewEdgeSpec` which all have both `SrcNode` and `TgtNode` in $\Gamma \setminus R$, where `TgtNode` and `TgtPos` describe an existing incoming - edge of that kind from a node in $S\*$. + edge of that kind from a node in $S^\*$. Note that considering all three $\mu$ lists together, - the `TgtNode` + `TgtPos`s of all `NewEdgeSpec`s with `EdgeKind` == `Value` will be unique From 31a743df585b35f9c1b39a3bd69f5f3ab0db9098 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:17:41 +0000 Subject: [PATCH 46/70] Yes PartialEq, no Eq --- src/hugr/rewrite/replace.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 187956715..4afdf5706 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -56,7 +56,7 @@ pub enum NewEdgeKind { } /// Specification of a `Replace` operation -#[derive(Debug, Clone)] // PartialEq? Eq probably doesn't make sense because of ordering. +#[derive(Debug, Clone, PartialEq)] pub struct Replacement { /// The nodes to remove from the existing Hugr (known as Gamma). /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. From 8ace22b0ee04e3047a646e28a922f5a9856db596 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:26:46 +0000 Subject: [PATCH 47/70] transfers->adoptions, fix doclinks, spec names --- src/hugr/rewrite/replace.rs | 42 ++++++++++++++++-------------- src/hugr/rewrite/simple_replace.rs | 2 +- 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 4afdf5706..d892a169a 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -60,22 +60,24 @@ pub enum NewEdgeKind { pub struct Replacement { /// The nodes to remove from the existing Hugr (known as Gamma). /// These must all have a common parent (i.e. be siblings). Called "S" in the spec. - /// Must be non-empty - otherwise there is no parent under which to place [replacement], - /// and no possible [in_edges], [out_edges] or [transfers]. + /// Must be non-empty - otherwise there is no parent under which to place [Self::replacement], + /// and there would be no possible [Self::mu_inp], [Self::mu_out] or [Self::adoptions]. pub removal: Vec, - /// A hugr whose root is the same type as the parent of all the [nodes]. "G" in the spec. + /// A hugr (not necessarily valid, as it may be missing edges and/or nodes), whose root + /// is the same type as the root of [Self::replacement]. "G" in the spec. pub replacement: Hugr, - /// Map from container nodes in [replacement] that have no children, to container nodes - /// that are descended from [nodes]. The keys are the new parents for the children of the - /// values, i.e. said children are transferred to new locations rather than removed from the graph. - /// Note no value may be ancestor/descendant of another. "R" is the set of descendants of [nodes] - /// that are not descendants of values here. - pub transfers: HashMap, + /// Describes how parts of the Hugr that would otherwise be removed should instead be preserved but + /// with new parents amongst the newly-inserted nodes. This is a Map from container nodes in + /// [Self::replacement] that have no children, to container nodes that are descended from [Self::removal]. + /// The keys are the new parents for the children of the values. Note no value may be ancestor or + /// descendant of another. This is "B" in the spec; "R" is the set of descendants of [Self::removal] + /// that are not descendants of values here. + pub adoptions: HashMap, /// Edges from nodes in the existing Hugr that are not removed ([NewEdgeSpec::src] in Gamma\R) - /// to inserted nodes ([NewEdgeSpec::tgt] in [replacement]). `$\mu_\inp$` in the spec. + /// to inserted nodes ([NewEdgeSpec::tgt] in [Self::replacement]). pub mu_inp: Vec, - /// Edges from inserted nodes ([NewEdgeSpec::src] in [replacement]) to existing nodes not removed - /// ([NewEdgeSpec::tgt] in Gamma \ R). `$\mu_\out$` in the spec. + /// Edges from inserted nodes ([NewEdgeSpec::src] in [Self::replacement]) to existing nodes not removed + /// ([NewEdgeSpec::tgt] in Gamma \ R). pub mu_out: Vec, /// Edges to add between existing nodes (both [NewEdgeSpec::src] and [NewEdgeSpec::tgt] in Gamma \ R). /// For example, in cases where the source had an edge to a removed node, and the target had an @@ -161,17 +163,17 @@ impl Replacement { fn get_removed_nodes(&self, h: &impl HugrView) -> Result, ReplaceError> { // Check the keys of the transfer map too, the values we'll use imminently - self.transfers.keys().try_for_each(|&n| { + self.adoptions.keys().try_for_each(|&n| { (self.replacement.contains_node(n) && self.replacement.get_optype(n).is_container() && self.replacement.children(n).next().is_none()) .then_some(()) .ok_or(ReplaceError::InvalidTransferTarget(n)) })?; - let mut transferred: HashSet = self.transfers.values().copied().collect(); - if transferred.len() != self.transfers.values().len() { + let mut transferred: HashSet = self.adoptions.values().copied().collect(); + if transferred.len() != self.adoptions.values().len() { return Err(ReplaceError::ConflictingTransfers( - self.transfers + self.adoptions .values() .filter(|v| !transferred.remove(v)) .copied() @@ -298,7 +300,7 @@ impl Rewrite for Replacement { h.remove_node(new_root).unwrap(); // 6. Transfer to keys of `transfers` children of the corresponding values. - for (new_parent, &old_parent) in self.transfers.iter() { + for (new_parent, &old_parent) in self.adoptions.iter() { let new_parent = node_map.get(new_parent).unwrap(); debug_assert!(h.children(old_parent).next().is_some()); loop { @@ -321,7 +323,7 @@ impl Rewrite for Replacement { self.removal .iter() .copied() - .chain(self.transfers.values().copied()) + .chain(self.adoptions.values().copied()) .chain(self.removal.iter().take(1).copied()) } } @@ -418,7 +420,7 @@ pub enum WhichHugr { /// The newly-inserted nodes, i.e. the [Replacement::replacement] Replacement, /// Nodes in the existing Hugr that are not [Replacement::removal] - /// (or are on the RHS of an entry in [Replacement::transfers]) + /// (or are on the RHS of an entry in [Replacement::adoptions]) Retained, } @@ -542,7 +544,7 @@ mod test { h.apply_rewrite(Replacement { removal: vec![entry.node(), bb2.node()], replacement, - transfers: HashMap::from([(r_df1.node(), entry.node()), (r_df2.node(), bb2.node())]), + adoptions: HashMap::from([(r_df1.node(), entry.node()), (r_df2.node(), bb2.node())]), mu_inp: vec![], mu_out: vec![NewEdgeSpec { src: r_bb, diff --git a/src/hugr/rewrite/simple_replace.rs b/src/hugr/rewrite/simple_replace.rs index d64255332..2de921f82 100644 --- a/src/hugr/rewrite/simple_replace.rs +++ b/src/hugr/rewrite/simple_replace.rs @@ -636,7 +636,7 @@ pub(in crate::hugr::rewrite) mod test { Replacement { removal: s.subgraph.nodes().to_vec(), replacement, - transfers: HashMap::new(), + adoptions: HashMap::new(), mu_inp, mu_out, mu_new: vec![], From 0014577b2ee56dce6a86db2e670fc57ca32b8abb Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:31:00 +0000 Subject: [PATCH 48/70] Note non-signature equality --- src/hugr/rewrite/replace.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index d892a169a..786c5ea82 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -152,7 +152,8 @@ impl Replacement { .map_err(|ex_one| ReplaceError::MultipleParents(ex_one.flatten().collect()))? .ok_or(ReplaceError::CantReplaceRoot)?; // If no parent - // Check replacement parent is of same tag. Should we require exactly OpType equality? + // Check replacement parent is of same tag. Note we do not require exact equality + // of OpType/Signature, e.g. to ease changing of Input/Output node signatures too. let expected = h.get_optype(parent).tag(); let actual = self.replacement.root_type().tag(); if expected != actual { From 543625603d9f07b8b8aba2eb1d69a2b7a0445bd2 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:49:36 +0000 Subject: [PATCH 49/70] WrongRootNodeTag: expected->removed, actual->replacement --- src/hugr/rewrite/replace.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 786c5ea82..ff9d747ee 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -153,11 +153,14 @@ impl Replacement { .ok_or(ReplaceError::CantReplaceRoot)?; // If no parent // Check replacement parent is of same tag. Note we do not require exact equality - // of OpType/Signature, e.g. to ease changing of Input/Output node signatures too. - let expected = h.get_optype(parent).tag(); - let actual = self.replacement.root_type().tag(); - if expected != actual { - return Err(ReplaceError::WrongRootNodeTag { expected, actual }); + // of OpType/Signature, e.g. to ease changing of Input/Output node signatures too. + let removed = h.get_optype(parent).tag(); + let replacement = self.replacement.root_type().tag(); + if removed != replacement { + return Err(ReplaceError::WrongRootNodeTag { + removed, + replacement, + }); }; Ok(parent) } @@ -391,9 +394,13 @@ pub enum ReplaceError { #[error("Removed nodes had different parents {0:?}")] MultipleParents(Vec), /// Replacement root node had different tag from parent of removed nodes - #[error("Expected replacement root with tag {expected} but found {actual}")] - #[allow(missing_docs)] - WrongRootNodeTag { expected: OpTag, actual: OpTag }, + #[error("Expected replacement root with tag {removed} but found {replacement}")] + WrongRootNodeTag { + /// The tag of the parent of the removed nodes + removed: OpTag, + /// The tag of the root in the replacement Hugr + replacement: OpTag, + }, /// Values in transfer map were not unique - contains the repeated elements #[error("Nodes cannot be transferred to multiple locations: {0:?}")] ConflictingTransfers(Vec), From da568ca1e5c26e62399f3219b2e36c2474577e79 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 16:57:04 +0000 Subject: [PATCH 50/70] no open_extensions --- src/hugr/rewrite/replace.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index ff9d747ee..485114578 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -500,7 +500,7 @@ mod test { // Replacement: one BB with two DFGs inside. // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). - let mut replacement = Hugr::new(NodeType::open_extensions(ops::CFG { + let mut replacement = Hugr::new(NodeType::new_open(ops::CFG { signature: FunctionType::new_linear(just_list.clone()), })); let r_bb = replacement.add_op_with_parent( @@ -538,11 +538,11 @@ mod test { types: just_list.clone(), }, )?; - let out = replacement.add_node_before( + let out = replacement.add_op_before( r_df1, - NodeType::open_extensions(ops::Output { + ops::Output { types: simple_unary_plus(just_list), - }), + }, )?; replacement.connect(inp, 0, r_df1, 0)?; replacement.connect(r_df2, 0, out, 0)?; From f1c2114e8a797f57704ee9cc75538b77303ec6f8 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 19:23:34 +0000 Subject: [PATCH 51/70] TransfersNotSeparateDescendants is a general case of ConflictingTransfers --- src/hugr/rewrite/replace.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 485114578..00e170210 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -176,7 +176,7 @@ impl Replacement { })?; let mut transferred: HashSet = self.adoptions.values().copied().collect(); if transferred.len() != self.adoptions.values().len() { - return Err(ReplaceError::ConflictingTransfers( + return Err(ReplaceError::TransfersNotSeparateDescendants( self.adoptions .values() .filter(|v| !transferred.remove(v)) @@ -401,9 +401,6 @@ pub enum ReplaceError { /// The tag of the root in the replacement Hugr replacement: OpTag, }, - /// Values in transfer map were not unique - contains the repeated elements - #[error("Nodes cannot be transferred to multiple locations: {0:?}")] - ConflictingTransfers(Vec), /// Keys in transfer map were not valid container nodes in replacement #[error("Node {0:?} was not an empty container node in the replacement")] InvalidTransferTarget(Node), From a5cca09bb2042c4f4f6dad4d64804114242a8257 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 17:28:16 +0000 Subject: [PATCH 52/70] Assert about the Hugr --- src/hugr/rewrite/replace.rs | 51 +++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 00e170210..37e1f6090 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -442,6 +442,9 @@ impl std::fmt::Display for WhichHugr { mod test { use std::collections::HashMap; + use itertools::Itertools; + + use crate::algorithm::nest_cfgs::test::depth; use crate::builder::{BuildError, CFGBuilder, Container, Dataflow, HugrBuilder}; use crate::extension::prelude::USIZE_T; use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; @@ -494,6 +497,26 @@ mod test { cfg.branch(&bb2, 0, &exit)?; let mut h = cfg.finish_hugr(®).unwrap(); + { + let pop = find_node(&h, "pop"); + let push = find_node(&h, "push"); + assert_eq!(depth(&h, pop), 2); // BB, CFG + assert_eq!(depth(&h, push), 2); + + let popp = h.get_parent(pop).unwrap(); + let pushp = h.get_parent(push).unwrap(); + assert_ne!(popp, pushp); // Two different BBs + assert!(matches!( + h.get_optype(popp), + OpType::BasicBlock(BasicBlock::DFB { .. }) + )); + assert!(matches!( + h.get_optype(pushp), + OpType::BasicBlock(BasicBlock::DFB { .. }) + )); + + assert_eq!(h.get_parent(popp).unwrap(), h.get_parent(pushp).unwrap()); + } // Replacement: one BB with two DFGs inside. // Use Hugr rather than Builder because DFGs must be empty (not even Input/Output). @@ -561,9 +584,37 @@ mod test { mu_new: vec![], })?; h.update_validate(®)?; + { + let pop = find_node(&h, "pop"); + let push = find_node(&h, "push"); + assert_eq!(depth(&h, pop), 3); // DFG, BB, CFG + assert_eq!(depth(&h, push), 3); + + let popp = h.get_parent(pop).unwrap(); + let pushp = h.get_parent(push).unwrap(); + assert_ne!(popp, pushp); // Two different DFGs + assert!(matches!(h.get_optype(popp), OpType::DFG(_))); + assert!(matches!(h.get_optype(pushp), OpType::DFG(_))); + + let grandp = h.get_parent(popp).unwrap(); + assert_eq!(grandp, h.get_parent(pushp).unwrap()); + assert!(matches!( + h.get_optype(grandp), + OpType::BasicBlock(BasicBlock::DFB { .. }) + )); + } + Ok(()) } + fn find_node(h: &Hugr, s: &str) -> crate::Node { + h.nodes() + .filter(|n| format!("{:?}", h.get_optype(*n)).contains(s)) + .exactly_one() + .ok() + .unwrap() + } + fn single_node_block + AsMut>( h: &mut CFGBuilder, op: impl Into, From 961026f5b0a1285b43b6295c6d79616af2b4bdbe Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Mon, 6 Nov 2023 17:53:19 +0000 Subject: [PATCH 53/70] Start on test - build conditional Hugr --- src/hugr/rewrite/replace.rs | 49 ++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 37e1f6090..b39cf95ac 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -445,10 +445,16 @@ mod test { use itertools::Itertools; use crate::algorithm::nest_cfgs::test::depth; - use crate::builder::{BuildError, CFGBuilder, Container, Dataflow, HugrBuilder}; - use crate::extension::prelude::USIZE_T; - use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; + use crate::builder::{ + BuildError, CFGBuilder, Container, DFGBuilder, Dataflow, DataflowHugr, + DataflowSubContainer, HugrBuilder, SubContainer, + }; + use crate::extension::prelude::{BOOL_T, USIZE_T}; + use crate::extension::{ + ExtensionId, ExtensionRegistry, ExtensionSet, PRELUDE, PRELUDE_REGISTRY, + }; use crate::hugr::{HugrMut, NodeType}; + use crate::ops::custom::{ExternalOp, OpaqueOp}; use crate::ops::handle::{BasicBlockID, ConstID, NodeHandle}; use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; @@ -646,4 +652,41 @@ mod test { v.insert(0, Type::new_unit_sum(1)); v.into() } + + #[test] + fn test_invalid() -> Result<(), Box> { + let utou = FunctionType::new_linear(vec![USIZE_T]); + let mk_op = |s| { + LeafOp::from(ExternalOp::Opaque(OpaqueOp::new( + ExtensionId::new("unknown_ext").unwrap(), + s, + String::new(), + vec![], + Some(utou.clone()), + ))) + }; + let mut h = DFGBuilder::new(FunctionType::new( + type_row![USIZE_T, BOOL_T], + type_row![USIZE_T], + ))?; + let [i, b] = h.input_wires_arr(); + let mut cond = h.conditional_builder( + (vec![type_row![]; 2], b), + [(USIZE_T, i)], + type_row![USIZE_T], + ExtensionSet::new(), + )?; + let mut case_t = cond.case_builder(0)?; + let foo = case_t.add_dataflow_op(mk_op("foo"), case_t.input_wires())?; + case_t.finish_with_outputs(foo.outputs())?; + let mut case_f = cond.case_builder(1)?; + let bar = case_f.add_dataflow_op(mk_op("bar"), case_f.input_wires())?; + let mut baz_dfg = case_f.dfg_builder(utou.clone(), None, bar.outputs())?; + let baz = baz_dfg.add_dataflow_op(mk_op("baz"), baz_dfg.input_wires())?; + let baz_dfg = baz_dfg.finish_with_outputs(baz.outputs())?; + case_f.finish_with_outputs(baz_dfg.outputs())?; + let cond = cond.finish_sub_container()?; + let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; + Ok(()) + } } From b8180d76c9af7a3c43325309d6f78b8646f47cf0 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 09:22:11 +0000 Subject: [PATCH 54/70] Test duplicate 'adoptions' rhs --- src/hugr/rewrite/replace.rs | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index b39cf95ac..180b5ecf4 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -453,15 +453,15 @@ mod test { use crate::extension::{ ExtensionId, ExtensionRegistry, ExtensionSet, PRELUDE, PRELUDE_REGISTRY, }; - use crate::hugr::{HugrMut, NodeType}; + use crate::hugr::{HugrMut, NodeType, Rewrite}; use crate::ops::custom::{ExternalOp, OpaqueOp}; use crate::ops::handle::{BasicBlockID, ConstID, NodeHandle}; - use crate::ops::{self, BasicBlock, LeafOp, OpTrait, OpType, DFG}; + use crate::ops::{self, BasicBlock, Case, LeafOp, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; use crate::{type_row, Hugr, HugrView, OutgoingPort}; - use super::{NewEdgeKind, NewEdgeSpec, Replacement}; + use super::{NewEdgeKind, NewEdgeSpec, ReplaceError, Replacement}; #[test] fn cfg() -> Result<(), Box> { @@ -678,15 +678,41 @@ mod test { )?; let mut case_t = cond.case_builder(0)?; let foo = case_t.add_dataflow_op(mk_op("foo"), case_t.input_wires())?; - case_t.finish_with_outputs(foo.outputs())?; + let case_t = case_t.finish_with_outputs(foo.outputs())?.node(); let mut case_f = cond.case_builder(1)?; let bar = case_f.add_dataflow_op(mk_op("bar"), case_f.input_wires())?; let mut baz_dfg = case_f.dfg_builder(utou.clone(), None, bar.outputs())?; let baz = baz_dfg.add_dataflow_op(mk_op("baz"), baz_dfg.input_wires())?; let baz_dfg = baz_dfg.finish_with_outputs(baz.outputs())?; - case_f.finish_with_outputs(baz_dfg.outputs())?; + let case_f = case_f.finish_with_outputs(baz_dfg.outputs())?.node(); let cond = cond.finish_sub_container()?; let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; + + let mut rep1 = Hugr::new(NodeType::new_open(h.get_optype(cond.node()).clone())); + let r_t = rep1.add_op_with_parent( + rep1.root(), + Case { + signature: utou.clone(), + }, + )?; + let r_f = rep1.add_op_with_parent( + rep1.root(), + Case { + signature: utou.clone(), + }, + )?; + let r = Replacement { + removal: vec![case_t, case_f], + replacement: rep1, + adoptions: HashMap::from_iter([(r_t, case_t), (r_f, case_t)]), + mu_inp: vec![], + mu_out: vec![], + mu_new: vec![], + }; + assert_eq!( + r.verify(&h), + Err(ReplaceError::TransfersNotSeparateDescendants(vec![case_t])) + ); Ok(()) } } From e5c426d0bebb3247374f63b2de45fde1c047da26 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 10:13:48 +0000 Subject: [PATCH 55/70] Rename {case,r}_t/f to {case,r}1/2 --- src/hugr/rewrite/replace.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 180b5ecf4..0d10566d8 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -676,42 +676,42 @@ mod test { type_row![USIZE_T], ExtensionSet::new(), )?; - let mut case_t = cond.case_builder(0)?; - let foo = case_t.add_dataflow_op(mk_op("foo"), case_t.input_wires())?; - let case_t = case_t.finish_with_outputs(foo.outputs())?.node(); - let mut case_f = cond.case_builder(1)?; - let bar = case_f.add_dataflow_op(mk_op("bar"), case_f.input_wires())?; - let mut baz_dfg = case_f.dfg_builder(utou.clone(), None, bar.outputs())?; + let mut case1 = cond.case_builder(0)?; + let foo = case1.add_dataflow_op(mk_op("foo"), case1.input_wires())?; + let case1 = case1.finish_with_outputs(foo.outputs())?.node(); + let mut case2 = cond.case_builder(1)?; + let bar = case2.add_dataflow_op(mk_op("bar"), case2.input_wires())?; + let mut baz_dfg = case2.dfg_builder(utou.clone(), None, bar.outputs())?; let baz = baz_dfg.add_dataflow_op(mk_op("baz"), baz_dfg.input_wires())?; let baz_dfg = baz_dfg.finish_with_outputs(baz.outputs())?; - let case_f = case_f.finish_with_outputs(baz_dfg.outputs())?.node(); + let case2 = case2.finish_with_outputs(baz_dfg.outputs())?.node(); let cond = cond.finish_sub_container()?; let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; let mut rep1 = Hugr::new(NodeType::new_open(h.get_optype(cond.node()).clone())); - let r_t = rep1.add_op_with_parent( + let r1 = rep1.add_op_with_parent( rep1.root(), Case { signature: utou.clone(), }, )?; - let r_f = rep1.add_op_with_parent( + let r2 = rep1.add_op_with_parent( rep1.root(), Case { signature: utou.clone(), }, )?; let r = Replacement { - removal: vec![case_t, case_f], + removal: vec![case1, case2], replacement: rep1, - adoptions: HashMap::from_iter([(r_t, case_t), (r_f, case_t)]), + adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), mu_inp: vec![], mu_out: vec![], mu_new: vec![], }; assert_eq!( r.verify(&h), - Err(ReplaceError::TransfersNotSeparateDescendants(vec![case_t])) + Err(ReplaceError::TransfersNotSeparateDescendants(vec![case1])) ); Ok(()) } From 326365fb5ae198db342e8d7c4f0005f09112d209 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 10:20:47 +0000 Subject: [PATCH 56/70] Test wrong root node type --- src/hugr/rewrite/replace.rs | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 0d10566d8..46bf7edcd 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -453,10 +453,11 @@ mod test { use crate::extension::{ ExtensionId, ExtensionRegistry, ExtensionSet, PRELUDE, PRELUDE_REGISTRY, }; + use crate::hugr::hugrmut::sealed::HugrMutInternals; use crate::hugr::{HugrMut, NodeType, Rewrite}; use crate::ops::custom::{ExternalOp, OpaqueOp}; use crate::ops::handle::{BasicBlockID, ConstID, NodeHandle}; - use crate::ops::{self, BasicBlock, Case, LeafOp, OpTrait, OpType, DFG}; + use crate::ops::{self, BasicBlock, Case, LeafOp, OpTag, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; use crate::{type_row, Hugr, HugrView, OutgoingPort}; @@ -688,7 +689,7 @@ mod test { let cond = cond.finish_sub_container()?; let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; - let mut rep1 = Hugr::new(NodeType::new_open(h.get_optype(cond.node()).clone())); + let mut rep1 = Hugr::new(h.root_type().clone()); let r1 = rep1.add_op_with_parent( rep1.root(), Case { @@ -701,18 +702,39 @@ mod test { signature: utou.clone(), }, )?; - let r = Replacement { + let mut r = Replacement { removal: vec![case1, case2], replacement: rep1, - adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), + adoptions: HashMap::from_iter([(r1, case1), (r2, baz_dfg.node())]), mu_inp: vec![], mu_out: vec![], mu_new: vec![], }; + assert_eq!( + r.verify(&h), + Err(ReplaceError::WrongRootNodeTag { + removed: OpTag::Conditional, + replacement: OpTag::Dfg + }) + ); + r.replacement.replace_op( + r.replacement.root(), + NodeType::new_open(h.get_optype(cond.node()).clone()), + )?; + assert_eq!(r.verify(&h), Ok(())); + r.adoptions = HashMap::from_iter([(r1, case1), (r2, case1)]); assert_eq!( r.verify(&h), Err(ReplaceError::TransfersNotSeparateDescendants(vec![case1])) ); + r.adoptions = HashMap::from_iter([(r1, case2), (r2, baz_dfg.node())]); + assert_eq!( + r.verify(&h), + Err(ReplaceError::TransfersNotSeparateDescendants(vec![ + baz_dfg.node() + ])) + ); + Ok(()) } } From 2d6a7b6b1ceb909a960a22123d0f5f5f6654cd48 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:28:06 +0000 Subject: [PATCH 57/70] Functional style using `..r.clone()`, test CantReplaceRoot --- src/hugr/rewrite/replace.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 46bf7edcd..5049c6163 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -722,14 +722,30 @@ mod test { NodeType::new_open(h.get_optype(cond.node()).clone()), )?; assert_eq!(r.verify(&h), Ok(())); - r.adoptions = HashMap::from_iter([(r1, case1), (r2, case1)]); + + // And test some bad variants (using the same `replacement` Hugr) assert_eq!( - r.verify(&h), + Replacement { + removal: vec![h.root()], + ..r.clone() + } + .verify(&h), + Err(ReplaceError::CantReplaceRoot) + ); + assert_eq!( + Replacement { + adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), + ..r.clone() + } + .verify(&h), Err(ReplaceError::TransfersNotSeparateDescendants(vec![case1])) ); - r.adoptions = HashMap::from_iter([(r1, case2), (r2, baz_dfg.node())]); assert_eq!( - r.verify(&h), + Replacement { + adoptions: HashMap::from_iter([(r1, case2), (r2, baz_dfg.node())]), + ..r.clone() + } + .verify(&h), Err(ReplaceError::TransfersNotSeparateDescendants(vec![ baz_dfg.node() ])) From 8b5ed23c5dc6456ee638efd55b43bbb3a9dd60c4 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:31:59 +0000 Subject: [PATCH 58/70] MultipleParents --- src/hugr/rewrite/replace.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 5049c6163..fc109473d 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -740,6 +740,14 @@ mod test { .verify(&h), Err(ReplaceError::TransfersNotSeparateDescendants(vec![case1])) ); + assert_eq!( + Replacement { + removal: vec![case1, baz_dfg.node()], + ..r.clone() + } + .verify(&h), + Err(ReplaceError::MultipleParents(vec![cond.node(), case2])) + ); assert_eq!( Replacement { adoptions: HashMap::from_iter([(r1, case2), (r2, baz_dfg.node())]), From 71786a9a2556ecdfdc237a645b611a682c2d44cb Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:37:19 +0000 Subject: [PATCH 59/70] InvalidTransferTarget --- src/hugr/rewrite/replace.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index fc109473d..67444d9fc 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -732,6 +732,14 @@ mod test { .verify(&h), Err(ReplaceError::CantReplaceRoot) ); + assert_eq!( + Replacement { + adoptions: HashMap::from([(r1, case1), (r.replacement.root(), case2)]), + ..r.clone() + } + .verify(&h), + Err(ReplaceError::InvalidTransferTarget(r.replacement.root())) + ); assert_eq!( Replacement { adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), From 65abd613e7e00ed7f63c3d57a13f6377e7d50b08 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:40:38 +0000 Subject: [PATCH 60/70] Invalid(TransferTarget=>AdoptingParent), (Transfers=>Adoptees)NotSeparateDescendants, doclinks --- src/hugr/rewrite/replace.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 67444d9fc..503db533c 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -172,11 +172,11 @@ impl Replacement { && self.replacement.get_optype(n).is_container() && self.replacement.children(n).next().is_none()) .then_some(()) - .ok_or(ReplaceError::InvalidTransferTarget(n)) + .ok_or(ReplaceError::InvalidAdoptingParent(n)) })?; let mut transferred: HashSet = self.adoptions.values().copied().collect(); if transferred.len() != self.adoptions.values().len() { - return Err(ReplaceError::TransfersNotSeparateDescendants( + return Err(ReplaceError::AdopteesNotSeparateDescendants( self.adoptions .values() .filter(|v| !transferred.remove(v)) @@ -195,7 +195,7 @@ impl Replacement { } } if !transferred.is_empty() { - return Err(ReplaceError::TransfersNotSeparateDescendants( + return Err(ReplaceError::AdopteesNotSeparateDescendants( transferred.into_iter().collect(), )); } @@ -401,13 +401,13 @@ pub enum ReplaceError { /// The tag of the root in the replacement Hugr replacement: OpTag, }, - /// Keys in transfer map were not valid container nodes in replacement + /// Keys in [Replacement::adoptions] were not valid container nodes in [Replacement::replacement] #[error("Node {0:?} was not an empty container node in the replacement")] - InvalidTransferTarget(Node), - /// Some values in the transfer map were either descendants of other values, - /// or not descendants of the removed nodes + InvalidAdoptingParent(Node), + /// Some values in [Replacement::adoptions] were either descendants of other values, + /// or not descendants of the [Replacement::removal] #[error("Nodes not free to be moved into new locations: {0:?}")] - TransfersNotSeparateDescendants(Vec), + AdopteesNotSeparateDescendants(Vec), /// A node at one end of a [NewEdgeSpec] was not found #[error("{0:?} end of edge {2:?} not found in {1}")] BadEdgeSpec(Direction, WhichHugr, NewEdgeSpec), @@ -738,7 +738,7 @@ mod test { ..r.clone() } .verify(&h), - Err(ReplaceError::InvalidTransferTarget(r.replacement.root())) + Err(ReplaceError::InvalidAdoptingParent(r.replacement.root())) ); assert_eq!( Replacement { @@ -746,7 +746,7 @@ mod test { ..r.clone() } .verify(&h), - Err(ReplaceError::TransfersNotSeparateDescendants(vec![case1])) + Err(ReplaceError::AdopteesNotSeparateDescendants(vec![case1])) ); assert_eq!( Replacement { @@ -762,7 +762,7 @@ mod test { ..r.clone() } .verify(&h), - Err(ReplaceError::TransfersNotSeparateDescendants(vec![ + Err(ReplaceError::AdopteesNotSeparateDescendants(vec![ baz_dfg.node() ])) ); From 804a005937bb19c8a9b37b70c52503dfcc5cde80 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:49:34 +0000 Subject: [PATCH 61/70] Comments, one BadEdgeSpec --- src/hugr/rewrite/replace.rs | 38 +++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 503db533c..e2aeb82c2 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -454,13 +454,14 @@ mod test { ExtensionId, ExtensionRegistry, ExtensionSet, PRELUDE, PRELUDE_REGISTRY, }; use crate::hugr::hugrmut::sealed::HugrMutInternals; + use crate::hugr::rewrite::replace::WhichHugr; use crate::hugr::{HugrMut, NodeType, Rewrite}; use crate::ops::custom::{ExternalOp, OpaqueOp}; use crate::ops::handle::{BasicBlockID, ConstID, NodeHandle}; use crate::ops::{self, BasicBlock, Case, LeafOp, OpTag, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; - use crate::{type_row, Hugr, HugrView, OutgoingPort}; + use crate::{type_row, Direction, Hugr, HugrView, Node, OutgoingPort}; use super::{NewEdgeKind, NewEdgeSpec, ReplaceError, Replacement}; @@ -689,6 +690,7 @@ mod test { let cond = cond.finish_sub_container()?; let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; + // Note wrong root type here - we'll replace children of the *Conditional* let mut rep1 = Hugr::new(h.root_type().clone()); let r1 = rep1.add_op_with_parent( rep1.root(), @@ -723,7 +725,8 @@ mod test { )?; assert_eq!(r.verify(&h), Ok(())); - // And test some bad variants (using the same `replacement` Hugr) + // And test some bad Replacements (using the same `replacement` Hugr). + // First, removed nodes... assert_eq!( Replacement { removal: vec![h.root()], @@ -734,27 +737,28 @@ mod test { ); assert_eq!( Replacement { - adoptions: HashMap::from([(r1, case1), (r.replacement.root(), case2)]), + removal: vec![case1, baz_dfg.node()], ..r.clone() } .verify(&h), - Err(ReplaceError::InvalidAdoptingParent(r.replacement.root())) + Err(ReplaceError::MultipleParents(vec![cond.node(), case2])) ); + // Adoptions... assert_eq!( Replacement { - adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), + adoptions: HashMap::from([(r1, case1), (r.replacement.root(), case2)]), ..r.clone() } .verify(&h), - Err(ReplaceError::AdopteesNotSeparateDescendants(vec![case1])) + Err(ReplaceError::InvalidAdoptingParent(r.replacement.root())) ); assert_eq!( Replacement { - removal: vec![case1, baz_dfg.node()], + adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), ..r.clone() } .verify(&h), - Err(ReplaceError::MultipleParents(vec![cond.node(), case2])) + Err(ReplaceError::AdopteesNotSeparateDescendants(vec![case1])) ); assert_eq!( Replacement { @@ -766,6 +770,24 @@ mod test { baz_dfg.node() ])) ); + // Edges.... + let bad_edge = NewEdgeSpec { + src: cond.node(), + tgt: h.nodes().max().unwrap(), + kind: NewEdgeKind::Order, + }; + assert_eq!( + Replacement { + mu_inp: vec![bad_edge.clone()], + ..r.clone() + } + .verify(&h), + Err(ReplaceError::BadEdgeSpec( + Direction::Incoming, + WhichHugr::Replacement, + bad_edge + )) + ); Ok(()) } From ed440b6b0fe69620b285eb223a8e75e9ebf2c139 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 13:53:17 +0000 Subject: [PATCH 62/70] Another BadEdgeSpec test --- src/hugr/rewrite/replace.rs | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index e2aeb82c2..cf5dbedd3 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -461,7 +461,7 @@ mod test { use crate::ops::{self, BasicBlock, Case, LeafOp, OpTag, OpTrait, OpType, DFG}; use crate::std_extensions::collections; use crate::types::{FunctionType, Type, TypeArg, TypeRow}; - use crate::{type_row, Direction, Hugr, HugrView, Node, OutgoingPort}; + use crate::{type_row, Direction, Hugr, HugrView, OutgoingPort}; use super::{NewEdgeKind, NewEdgeSpec, ReplaceError, Replacement}; @@ -771,21 +771,38 @@ mod test { ])) ); // Edges.... - let bad_edge = NewEdgeSpec { - src: cond.node(), - tgt: h.nodes().max().unwrap(), + let edge_from_removed = NewEdgeSpec { + src: case1, + tgt: r2, kind: NewEdgeKind::Order, }; assert_eq!( Replacement { - mu_inp: vec![bad_edge.clone()], + mu_inp: vec![edge_from_removed.clone()], ..r.clone() } .verify(&h), Err(ReplaceError::BadEdgeSpec( - Direction::Incoming, + Direction::Outgoing, + WhichHugr::Retained, + edge_from_removed + )) + ); + let bad_out_edge = NewEdgeSpec { + src: h.nodes().max().unwrap(), // not valid in replacement + tgt: cond.node(), + kind: NewEdgeKind::Order, + }; + assert_eq!( + Replacement { + mu_out: vec![bad_out_edge.clone()], + ..r.clone() + } + .verify(&h), + Err(ReplaceError::BadEdgeSpec( + Direction::Outgoing, WhichHugr::Replacement, - bad_edge + bad_out_edge )) ); From c502018259dfc797cee4b4673bb8aa42ee91bcc9 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 14:59:14 +0000 Subject: [PATCH 63/70] NotSeparateDescendants - comment nodes are best-effort --- src/hugr/rewrite/replace.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index cf5dbedd3..22cbef49f 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -404,8 +404,8 @@ pub enum ReplaceError { /// Keys in [Replacement::adoptions] were not valid container nodes in [Replacement::replacement] #[error("Node {0:?} was not an empty container node in the replacement")] InvalidAdoptingParent(Node), - /// Some values in [Replacement::adoptions] were either descendants of other values, - /// or not descendants of the [Replacement::removal] + /// Some values in [Replacement::adoptions] were either descendants of other values, or not + /// descendants of the [Replacement::removal]. The nodes are indicated on a best-effort basis. #[error("Nodes not free to be moved into new locations: {0:?}")] AdopteesNotSeparateDescendants(Vec), /// A node at one end of a [NewEdgeSpec] was not found From b1dbfc4e207ebc17ee5aace78ba4a7f8b6a2c734 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 14:59:58 +0000 Subject: [PATCH 64/70] BadEdgeKind --- src/hugr/rewrite/replace.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 22cbef49f..626bb4d69 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -442,6 +442,7 @@ impl std::fmt::Display for WhichHugr { mod test { use std::collections::HashMap; + use cool_asserts::assert_matches; use itertools::Itertools; use crate::algorithm::nest_cfgs::test::depth; @@ -805,7 +806,18 @@ mod test { bad_out_edge )) ); - + let bad_order_edge = NewEdgeSpec { + src: cond.node(), + tgt: r1.node(), + kind: NewEdgeKind::Order, + }; + assert_matches!( + Replacement { + mu_inp: vec![bad_order_edge.clone()], + ..r.clone() + }.verify(&h), + Err(ReplaceError::BadEdgeKind(_, e)) => e == bad_order_edge + ); Ok(()) } } From fcf88275dcd00a7d1cc305241cd25572b42d99ac Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 15:46:55 +0000 Subject: [PATCH 65/70] Run tests on both verify + apply, include one on mu_new --- src/hugr/rewrite/replace.rs | 55 ++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 626bb4d69..7dab1591c 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -690,6 +690,12 @@ mod test { let case2 = case2.finish_with_outputs(baz_dfg.outputs())?.node(); let cond = cond.finish_sub_container()?; let h = h.finish_hugr_with_outputs(cond.outputs(), &PRELUDE_REGISTRY)?; + let verify_apply = |r: Replacement| { + let verify_res = r.verify(&h); + let apply_res = r.apply(&mut h.clone()); + assert_eq!(verify_res, apply_res); + verify_res + }; // Note wrong root type here - we'll replace children of the *Conditional* let mut rep1 = Hugr::new(h.root_type().clone()); @@ -714,7 +720,7 @@ mod test { mu_new: vec![], }; assert_eq!( - r.verify(&h), + verify_apply(r.clone()), Err(ReplaceError::WrongRootNodeTag { removed: OpTag::Conditional, replacement: OpTag::Dfg @@ -724,49 +730,44 @@ mod test { r.replacement.root(), NodeType::new_open(h.get_optype(cond.node()).clone()), )?; - assert_eq!(r.verify(&h), Ok(())); + assert_eq!(verify_apply(r.clone()), Ok(())); // And test some bad Replacements (using the same `replacement` Hugr). // First, removed nodes... assert_eq!( - Replacement { + verify_apply(Replacement { removal: vec![h.root()], ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::CantReplaceRoot) ); assert_eq!( - Replacement { + verify_apply(Replacement { removal: vec![case1, baz_dfg.node()], ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::MultipleParents(vec![cond.node(), case2])) ); // Adoptions... assert_eq!( - Replacement { + verify_apply(Replacement { adoptions: HashMap::from([(r1, case1), (r.replacement.root(), case2)]), ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::InvalidAdoptingParent(r.replacement.root())) ); assert_eq!( - Replacement { + verify_apply(Replacement { adoptions: HashMap::from_iter([(r1, case1), (r2, case1)]), ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::AdopteesNotSeparateDescendants(vec![case1])) ); assert_eq!( - Replacement { + verify_apply(Replacement { adoptions: HashMap::from_iter([(r1, case2), (r2, baz_dfg.node())]), ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::AdopteesNotSeparateDescendants(vec![ baz_dfg.node() ])) @@ -778,11 +779,10 @@ mod test { kind: NewEdgeKind::Order, }; assert_eq!( - Replacement { + verify_apply(Replacement { mu_inp: vec![edge_from_removed.clone()], ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::BadEdgeSpec( Direction::Outgoing, WhichHugr::Retained, @@ -795,11 +795,10 @@ mod test { kind: NewEdgeKind::Order, }; assert_eq!( - Replacement { + verify_apply(Replacement { mu_out: vec![bad_out_edge.clone()], ..r.clone() - } - .verify(&h), + }), Err(ReplaceError::BadEdgeSpec( Direction::Outgoing, WhichHugr::Replacement, @@ -808,14 +807,14 @@ mod test { ); let bad_order_edge = NewEdgeSpec { src: cond.node(), - tgt: r1.node(), - kind: NewEdgeKind::Order, + tgt: h.get_io(h.root()).unwrap()[1], + kind: NewEdgeKind::ControlFlow { src_pos: 0.into() }, }; assert_matches!( - Replacement { - mu_inp: vec![bad_order_edge.clone()], + verify_apply(Replacement { + mu_new: vec![bad_order_edge.clone()], ..r.clone() - }.verify(&h), + }), Err(ReplaceError::BadEdgeKind(_, e)) => e == bad_order_edge ); Ok(()) From eb3c5190a45360ea32823698b52777ad997d17a6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 16:12:14 +0000 Subject: [PATCH 66/70] WIP test NoRemovedEdge...failing --- src/hugr/rewrite/replace.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 7dab1591c..f6dc1a64b 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -817,6 +817,23 @@ mod test { }), Err(ReplaceError::BadEdgeKind(_, e)) => e == bad_order_edge ); + let op = OutgoingPort::from(0); + let (tgt, ip) = h.linked_inputs(cond.node(), op).next().unwrap(); + let new_out_edge = NewEdgeSpec { + src: r1.node(), + tgt, + kind: NewEdgeKind::Value { + src_pos: op, + tgt_pos: ip, + }, + }; + assert_eq!( + verify_apply(Replacement { + mu_out: vec![new_out_edge.clone()], + ..r.clone() + }), + Err(ReplaceError::NoRemovedEdge(new_out_edge)) + ); Ok(()) } } From 17b1618a8038ad176e81ca1e1bb37bdc253d72f5 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 16:08:19 +0000 Subject: [PATCH 67/70] Fix for both --- src/hugr/rewrite/replace.rs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index f6dc1a64b..4b2591627 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -122,10 +122,13 @@ impl NewEdgeSpec { .ok_or(ReplaceError::BadEdgeKind(Direction::Incoming, self.clone())) } + // Note that we return () for error here because the caller needs to construct the error + // in order to report useful edge indices (in `self` they may have been translated). fn check_existing_edge( &self, h: &impl HugrView, src_ok: impl Fn(Node) -> bool, + err_edge: impl Fn() -> NewEdgeSpec, ) -> Result<(), ReplaceError> { if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind { @@ -134,7 +137,7 @@ impl NewEdgeSpec { .exactly_one() .is_ok_and(|(src_n, _)| src_ok(src_n)); if !found_incoming { - return Err(ReplaceError::NoRemovedEdge(self.clone())); + return Err(ReplaceError::NoRemovedEdge(err_edge())); }; }; Ok(()) @@ -219,7 +222,7 @@ impl Rewrite for Replacement { const UNCHANGED_ON_FAILURE: bool = false; fn verify(&self, h: &impl crate::HugrView) -> Result<(), Self::Error> { - let parent = self.check_parent(h)?; + self.check_parent(h)?; let removed = self.get_removed_nodes(h)?; // Edge sources... for e in self.mu_inp.iter().chain(self.mu_new.iter()) { @@ -258,7 +261,11 @@ impl Rewrite for Replacement { // from a part of the Hugr being moved (which may require changing source, // depending on where the transplanted portion ends up). While this subsumes // the first "removed.contains" check, we'll keep that as a common-case fast-path. - e.check_existing_edge(h, |n| removed.contains(&n) || descends(h, parent, n))?; + e.check_existing_edge( + h, + |n| removed.contains(&n) || descends_any(h, &removed, n), + || e.clone(), + )?; } Ok(()) } @@ -284,11 +291,11 @@ impl Rewrite for Replacement { // 3. Add new edges from copied to existing nodes according to mu_out, // replacing existing value/static edges incoming to targets - transfer_edges(h, self.mu_out.iter(), translate_idx, kept, Some(parent))?; + transfer_edges(h, self.mu_out.iter(), translate_idx, kept, Some(&to_remove))?; // 4. Add new edges between existing nodes according to mu_new, // replacing existing value/static edges incoming to targets. - transfer_edges(h, self.mu_new.iter(), kept, kept, Some(parent))?; + transfer_edges(h, self.mu_new.iter(), kept, kept, Some(&to_remove))?; // 5. Put newly-added copies into correct places in hierarchy // (these will be correct places after removing nodes) @@ -332,8 +339,8 @@ impl Rewrite for Replacement { } } -fn descends(h: &impl HugrView, ancestor: Node, mut descendant: Node) -> bool { - while descendant != ancestor { +fn descends_any(h: &impl HugrView, ancestors: &HashSet, mut descendant: Node) -> bool { + while !ancestors.contains(&descendant) { let Some(p) = h.get_parent(descendant) else { return false; }; @@ -347,7 +354,7 @@ fn transfer_edges<'a>( edges: impl Iterator, trans_src: impl Fn(Node) -> Result, trans_tgt: impl Fn(Node) -> Result, - existing_src_ancestor: Option, + legal_src_ancestors: Option<&HashSet>, ) -> Result<(), ReplaceError> { for oe in edges { let e = NewEdgeSpec { @@ -371,8 +378,12 @@ fn transfer_edges<'a>( h.add_other_edge(e.src, e.tgt).unwrap(); } NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { - if let Some(anc) = existing_src_ancestor { - e.check_existing_edge(h, |n| descends(h, anc, n))?; + if let Some(legal_src_ancestors) = legal_src_ancestors { + e.check_existing_edge( + h, + |n| descends_any(h, legal_src_ancestors, n), + || oe.clone(), + )?; h.disconnect(e.tgt, tgt_pos).unwrap(); } h.connect(e.src, src_pos, e.tgt, tgt_pos).unwrap(); From 8788dc76e399c7a528866fc7d4ee2b7caccaed36 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 15:51:52 +0000 Subject: [PATCH 68/70] refactor: move descends_any inside check_existing_edge --- src/hugr/rewrite/replace.rs | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 4b2591627..3f54b39de 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -122,20 +122,27 @@ impl NewEdgeSpec { .ok_or(ReplaceError::BadEdgeKind(Direction::Incoming, self.clone())) } - // Note that we return () for error here because the caller needs to construct the error - // in order to report useful edge indices (in `self` they may have been translated). fn check_existing_edge( &self, h: &impl HugrView, - src_ok: impl Fn(Node) -> bool, + legal_src_ancestors: &HashSet, err_edge: impl Fn() -> NewEdgeSpec, ) -> Result<(), ReplaceError> { if let NewEdgeKind::Static { tgt_pos, .. } | NewEdgeKind::Value { tgt_pos, .. } = self.kind { + let descends_from_legal = |mut descendant: Node| -> bool { + while !legal_src_ancestors.contains(&descendant) { + let Some(p) = h.get_parent(descendant) else { + return false; + }; + descendant = p; + } + true + }; let found_incoming = h .linked_ports(self.tgt, tgt_pos) .exactly_one() - .is_ok_and(|(src_n, _)| src_ok(src_n)); + .is_ok_and(|(src_n, _)| descends_from_legal(src_n)); if !found_incoming { return Err(ReplaceError::NoRemovedEdge(err_edge())); }; @@ -261,11 +268,7 @@ impl Rewrite for Replacement { // from a part of the Hugr being moved (which may require changing source, // depending on where the transplanted portion ends up). While this subsumes // the first "removed.contains" check, we'll keep that as a common-case fast-path. - e.check_existing_edge( - h, - |n| removed.contains(&n) || descends_any(h, &removed, n), - || e.clone(), - )?; + e.check_existing_edge(h, &removed, || e.clone())?; } Ok(()) } @@ -339,16 +342,6 @@ impl Rewrite for Replacement { } } -fn descends_any(h: &impl HugrView, ancestors: &HashSet, mut descendant: Node) -> bool { - while !ancestors.contains(&descendant) { - let Some(p) = h.get_parent(descendant) else { - return false; - }; - descendant = p; - } - true -} - fn transfer_edges<'a>( h: &mut impl HugrMut, edges: impl Iterator, @@ -379,11 +372,7 @@ fn transfer_edges<'a>( } NewEdgeKind::Value { src_pos, tgt_pos } | NewEdgeKind::Static { src_pos, tgt_pos } => { if let Some(legal_src_ancestors) = legal_src_ancestors { - e.check_existing_edge( - h, - |n| descends_any(h, legal_src_ancestors, n), - || oe.clone(), - )?; + e.check_existing_edge(h, legal_src_ancestors, || oe.clone())?; h.disconnect(e.tgt, tgt_pos).unwrap(); } h.connect(e.src, src_pos, e.tgt, tgt_pos).unwrap(); From 1b39df738f913d1a961a2999788f5d49b59932c6 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Tue, 7 Nov 2023 16:15:12 +0000 Subject: [PATCH 69/70] Generalize Container::add_constant, pass None --- src/builder/build_traits.rs | 4 ++-- src/hugr/rewrite/replace.rs | 6 +----- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/builder/build_traits.rs b/src/builder/build_traits.rs index 950a85903..513e7f989 100644 --- a/src/builder/build_traits.rs +++ b/src/builder/build_traits.rs @@ -73,9 +73,9 @@ pub trait Container { fn add_constant( &mut self, constant: ops::Const, - extensions: ExtensionSet, + extensions: impl Into>, ) -> Result { - let const_n = self.add_child_node(NodeType::new(constant, extensions))?; + let const_n = self.add_child_node(NodeType::new(constant, extensions.into()))?; Ok(const_n.into()) } diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index 3f54b39de..1db50e4d4 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -492,11 +492,7 @@ mod test { FunctionType::new_linear(just_list.clone()).with_extension_delta(&exset), )?; - let pred_const = cfg.add_constant( - ops::Const::unary_unit_sum(), - // Slightly awkward we have to specify this, and the choice is non-obvious: - ExtensionSet::singleton(&collections::EXTENSION_NAME), - )?; + let pred_const = cfg.add_constant(ops::Const::unary_unit_sum(), None)?; let entry = single_node_block(&mut cfg, pop, &pred_const, true)?; let bb2 = single_node_block(&mut cfg, push, &pred_const, false)?; From bbb172d4d4ea2d41a637c295edf264bb60277401 Mon Sep 17 00:00:00 2001 From: Alan Lawrence Date: Wed, 8 Nov 2023 14:20:22 +0000 Subject: [PATCH 70/70] Simplify invalidation_set --- src/hugr/rewrite/replace.rs | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/src/hugr/rewrite/replace.rs b/src/hugr/rewrite/replace.rs index f94bd6f3e..ac250ad60 100644 --- a/src/hugr/rewrite/replace.rs +++ b/src/hugr/rewrite/replace.rs @@ -1,8 +1,7 @@ //! Implementation of the `Replace` operation. -use std::collections::hash_map::Values; use std::collections::{HashMap, HashSet, VecDeque}; -use std::iter::{Chain, Copied, Take}; +use std::iter::Copied; use std::slice::Iter; use itertools::Itertools; @@ -217,12 +216,7 @@ impl Rewrite for Replacement { type ApplyResult = (); - type InvalidationSet<'a> = // as IntoIterator>::IntoIter - Chain< - Chain< - Copied>, - Copied>>, - Copied>>> + type InvalidationSet<'a> = Copied> where Self: 'a; @@ -334,11 +328,7 @@ impl Rewrite for Replacement { } fn invalidation_set(&self) -> Self::InvalidationSet<'_> { - self.removal - .iter() - .copied() - .chain(self.adoptions.values().copied()) - .chain(self.removal.iter().take(1).copied()) + self.removal.iter().copied() } }