From 1a56ed5fb0b84d6f6293ce7a4cd7a7d18007251a Mon Sep 17 00:00:00 2001 From: Agustin Borgna Date: Thu, 6 Jun 2024 17:18:58 +0100 Subject: [PATCH 1/2] feat: Make the rewrite errors more useful --- hugr-core/src/hugr/views/sibling_subgraph.rs | 85 ++++++++++++++++---- 1 file changed, 69 insertions(+), 16 deletions(-) diff --git a/hugr-core/src/hugr/views/sibling_subgraph.rs b/hugr-core/src/hugr/views/sibling_subgraph.rs index ddc307509..98a09eb3c 100644 --- a/hugr-core/src/hugr/views/sibling_subgraph.rs +++ b/hugr-core/src/hugr/views/sibling_subgraph.rs @@ -21,7 +21,7 @@ use crate::builder::{Container, FunctionBuilder}; use crate::hugr::{HugrMut, HugrView, RootTagged}; use crate::ops::dataflow::DataflowOpTrait; use crate::ops::handle::{ContainerHandle, DataflowOpID}; -use crate::ops::{OpTag, OpTrait}; +use crate::ops::{NamedOp, OpTag, OpTrait, OpType}; use crate::types::{FunctionType, Type}; use crate::{Hugr, IncomingPort, Node, OutgoingPort, Port, SimpleReplacement}; @@ -340,13 +340,22 @@ impl SiblingSubgraph { let rep_root = replacement.root(); let dfg_optype = replacement.get_optype(rep_root); if !OpTag::Dfg.is_superset(dfg_optype.tag()) { - return Err(InvalidReplacement::InvalidDataflowGraph); + return Err(InvalidReplacement::InvalidDataflowGraph { + node: rep_root, + op: dfg_optype.clone(), + }); } let Some([rep_input, rep_output]) = replacement.get_io(rep_root) else { - return Err(InvalidReplacement::InvalidDataflowParent); + return Err(InvalidReplacement::InvalidDataflowParent { + node: rep_root, + op: dfg_optype.clone(), + }); }; if dfg_optype.dataflow_signature() != Some(self.signature(hugr)) { - return Err(InvalidReplacement::InvalidSignature); + return Err(InvalidReplacement::InvalidSignature { + expected: self.signature(hugr), + actual: dfg_optype.dataflow_signature(), + }); } // TODO: handle state order edges. For now panic if any are present. @@ -503,7 +512,20 @@ fn validate_subgraph( } // Check all nodes share parent if !nodes.iter().map(|&n| hugr.get_parent(n)).all_equal() { - return Err(InvalidSubgraph::NoSharedParent); + let first_node = nodes[0]; + let first_parent = hugr.get_parent(first_node); + let other_node = *nodes + .iter() + .skip(1) + .find(|&&n| hugr.get_parent(n) != first_parent) + .unwrap(); + let other_parent = hugr.get_parent(other_node); + return Err(InvalidSubgraph::NoSharedParent { + first_node, + first_parent, + other_node, + other_parent, + }); } // Check there are no linked "other" ports @@ -624,18 +646,36 @@ fn has_other_edge(hugr: &H, node: Node, dir: Direction) -> bool { } /// Errors that can occur while constructing a [`SimpleReplacement`]. -#[derive(Debug, Clone, PartialEq, Eq, Error)] +#[derive(Debug, Clone, PartialEq, Error)] #[non_exhaustive] pub enum InvalidReplacement { /// No DataflowParent root in replacement graph. - #[error("No DataflowParent root in replacement graph.")] - InvalidDataflowGraph, + #[error("The root of the replacement {node} is a {}, but only OpType::DFGs are supported.", op.name())] + InvalidDataflowGraph { + /// The node ID of the root node. + node: Node, + /// The op type of the root node. + op: OpType, + }, /// Malformed DataflowParent in replacement graph. - #[error("Malformed DataflowParent in replacement graph.")] - InvalidDataflowParent, + #[error("The root of the replacement {node} is malformed. It is a {} but does not contain I/O nodes.", op.name())] + InvalidDataflowParent { + /// The node ID of the root node. + node: Node, + /// The op type of the root node. + op: OpType, + }, /// Replacement graph boundary size mismatch. - #[error("Replacement graph boundary size mismatch.")] - InvalidSignature, + #[error( + "Replacement graph boundary size mismatch. Expected {expected}, got {}.", + actual.clone().map_or("none".to_string(), |t| t.to_string())) + ] + InvalidSignature { + /// The expected signature. + expected: FunctionType, + /// The actual signature. + actual: Option, + }, /// SiblingSubgraph is not convex. #[error("SiblingSubgraph is not convex.")] NonConvexSubgraph, @@ -649,8 +689,21 @@ pub enum InvalidSubgraph { #[error("The subgraph is not convex.")] NotConvex, /// Not all nodes have the same parent. - #[error("Not a sibling subgraph.")] - NoSharedParent, + #[error( + "Invalid sibling subgraph. {first_node} has parent {}, but {other_node} has parent {}.", + first_parent.map_or("None".to_string(), |n| n.to_string()), + other_parent.map_or("None".to_string(), |n| n.to_string()) + )] + NoSharedParent { + /// The first node. + first_node: Node, + /// The parent of the first node. + first_parent: Option, + /// The other node. + other_node: Node, + /// The parent of the other node. + other_parent: Option, + }, /// Empty subgraphs are not supported. #[error("Empty subgraphs are not supported.")] EmptySubgraph, @@ -849,9 +902,9 @@ mod tests { builder.finish_prelude_hugr_with_outputs(inputs).unwrap() }; - assert_eq!( + assert_matches!( sub.create_simple_replacement(&func, empty_dfg).unwrap_err(), - InvalidReplacement::InvalidSignature + InvalidReplacement::InvalidSignature { .. } ); Ok(()) } From c8b33d4b5dbdcd11fb7d449f331d47ca297ab8f8 Mon Sep 17 00:00:00 2001 From: Agustin Borgna Date: Fri, 7 Jun 2024 15:06:36 +0100 Subject: [PATCH 2/2] Address review comments --- hugr-core/src/hugr/views/sibling_subgraph.rs | 25 +++++--------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/hugr-core/src/hugr/views/sibling_subgraph.rs b/hugr-core/src/hugr/views/sibling_subgraph.rs index 98a09eb3c..04c46c6f3 100644 --- a/hugr-core/src/hugr/views/sibling_subgraph.rs +++ b/hugr-core/src/hugr/views/sibling_subgraph.rs @@ -323,8 +323,6 @@ impl SiblingSubgraph { /// May return one of the following five errors /// - [`InvalidReplacement::InvalidDataflowGraph`]: the replacement /// graph is not a [`crate::ops::OpTag::DataflowParent`]-rooted graph, - /// - [`InvalidReplacement::InvalidDataflowParent`]: the replacement does - /// not have an input and output node, /// - [`InvalidReplacement::InvalidSignature`]: the signature of the /// replacement DFG does not match the subgraph signature, or /// - [`InvalidReplacement::NonConvexSubgraph`]: the sibling subgraph is not @@ -345,12 +343,9 @@ impl SiblingSubgraph { op: dfg_optype.clone(), }); } - let Some([rep_input, rep_output]) = replacement.get_io(rep_root) else { - return Err(InvalidReplacement::InvalidDataflowParent { - node: rep_root, - op: dfg_optype.clone(), - }); - }; + let [rep_input, rep_output] = replacement + .get_io(rep_root) + .expect("DFG root in the replacement does not have input and output nodes."); if dfg_optype.dataflow_signature() != Some(self.signature(hugr)) { return Err(InvalidReplacement::InvalidSignature { expected: self.signature(hugr), @@ -657,17 +652,9 @@ pub enum InvalidReplacement { /// The op type of the root node. op: OpType, }, - /// Malformed DataflowParent in replacement graph. - #[error("The root of the replacement {node} is malformed. It is a {} but does not contain I/O nodes.", op.name())] - InvalidDataflowParent { - /// The node ID of the root node. - node: Node, - /// The op type of the root node. - op: OpType, - }, - /// Replacement graph boundary size mismatch. + /// Replacement graph type mismatch. #[error( - "Replacement graph boundary size mismatch. Expected {expected}, got {}.", + "Replacement graph type mismatch. Expected {expected}, got {}.", actual.clone().map_or("none".to_string(), |t| t.to_string())) ] InvalidSignature { @@ -690,7 +677,7 @@ pub enum InvalidSubgraph { NotConvex, /// Not all nodes have the same parent. #[error( - "Invalid sibling subgraph. {first_node} has parent {}, but {other_node} has parent {}.", + "Not a sibling subgraph. {first_node} has parent {}, but {other_node} has parent {}.", first_parent.map_or("None".to_string(), |n| n.to_string()), other_parent.map_or("None".to_string(), |n| n.to_string()) )]