Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat!: Make the rewrite errors more useful #1174

Merged
merged 3 commits into from
Jun 7, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 69 additions & 16 deletions hugr-core/src/hugr/views/sibling_subgraph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a strangely named error. It's not that the optype isn't a DataflowParent - that's guaranteed from the previous check that it's a Dfg. (Odd that we are checking OpTag::Dfg.is_superset(dfg_optype) rather than OpTag::Dfg == dfg_optype but there.)

So, it's that the input/output are missing. Indeed that's what the #[error] message for InvalidDataflowParent says! I'd suggest renaming to, say, MissingInputOutput.

(Also we kind of know what the op is, it's an OpType::DFG - so this just puts the input/output typerows in, well I guess that's fair.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can replace this error with an expect. It can only trigger with invalid hugrs.

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.
Expand Down Expand Up @@ -503,7 +512,20 @@ fn validate_subgraph<H: HugrView>(
}
// 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
Expand Down Expand Up @@ -624,18 +646,36 @@ fn has_other_edge<H: HugrView>(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 {}.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Replacement graph boundary size mismatch. Expected {expected}, got {}.",
"Replacement graph boundary type mismatch. Expected {expected}, got {}.",

Or maybe just "graph type" without the "boundary"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(And the comment on the previous line too)

actual.clone().map_or("none".to_string(), |t| t.to_string()))
]
InvalidSignature {
/// The expected signature.
expected: FunctionType,
/// The actual signature.
actual: Option<FunctionType>,
},
/// SiblingSubgraph is not convex.
#[error("SiblingSubgraph is not convex.")]
NonConvexSubgraph,
Expand All @@ -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 {}.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quite like "not a" sibling subgraph (the definition of such is that they are all siblings!), but ok

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<Node>,
/// The other node.
other_node: Node,
/// The parent of the other node.
other_parent: Option<Node>,
},
/// Empty subgraphs are not supported.
#[error("Empty subgraphs are not supported.")]
EmptySubgraph,
Expand Down Expand Up @@ -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(())
}
Expand Down
Loading