Skip to content

Commit

Permalink
feat!: Cleaner error on wiring errors while building (#873)
Browse files Browse the repository at this point in the history 
Adds a new `BuilderWiringError`, that gets wrapper under either
`BuildError::OutputWiring` or `BuildError::OperationWiring` when passing
a wrong set of input wires to a builder method.

The goal of this is to report useful information about the errors to the
user.
Now we can give a bit more context, including the location in the hugr.

Compare the old error:
```
Can't copy linear type: Type(Extension(CustomType { extension: IdentList("prelude"), id: "qubit", args: [], bound: Any }), Any).
```

With:
```
Found an error while setting the outputs of a FuncDefn container, Node(1). Cannot copy linear type qubit from output Port(Outgoing, 0) of node Node(2)
```

Although the have the location of the error, we may still not have
access to the Hugr being built (e.g. if calling `finish_hugr`). I'll
open an issue for addressing that.
  • Loading branch information
@aborgna-q
aborgna-q authored Mar 11, 2024
1 parent 3419fba commit 2c1539c
Show file tree
Hide file tree
Showing 4 changed files with 141 additions and 51 deletions.
57 changes: 52 additions & 5 deletions quantinuum-hugr/src/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,10 @@ use thiserror::Error;
use crate::extension::SignatureError;
use crate::hugr::ValidationError;
use crate::ops::handle::{BasicBlockID, CfgID, ConditionalID, DfgID, FuncID, TailLoopID};
use crate::ops::{OpName, OpType};
use crate::types::ConstTypeError;
use crate::types::Type;
use crate::{Node, Wire};
use crate::{Node, Port, Wire};

pub mod handle;
pub use handle::BuildHandle;
Expand Down Expand Up @@ -122,6 +123,7 @@ mod circuit;
pub use circuit::{CircuitBuildError, CircuitBuilder};

#[derive(Debug, Clone, PartialEq, Error)]
#[non_exhaustive]
/// Error while building the HUGR.
pub enum BuildError {
/// The constructed HUGR is invalid.
Expand Down Expand Up @@ -155,13 +157,58 @@ pub enum BuildError {
#[error("Wire not found in Hugr: {0:?}.")]
WireNotFound(Wire),

/// Can't copy a linear type
#[error("Can't copy linear type: {0:?}.")]
NoCopyLinear(Type),

/// Error in CircuitBuilder
#[error("Error in CircuitBuilder: {0}.")]
CircuitError(#[from] circuit::CircuitBuildError),

/// Invalid wires when setting outputs
#[error("Found an error while setting the outputs of a {} container, {container_node}. {error}", .container_op.name())]
OutputWiring {
container_op: OpType,
container_node: Node,
#[source]
error: BuilderWiringError,
},

/// Invalid input wires to a new operation
///
/// The internal error message already contains the node index.
#[error("Got an input wire while adding a {} to the circuit. {error}", .op.name())]
OperationWiring {
op: OpType,
#[source]
error: BuilderWiringError,
},
}

#[derive(Debug, Clone, PartialEq, Error)]
#[non_exhaustive]
/// Error raised when wiring up a node during the build process.
pub enum BuilderWiringError {
/// Tried to copy a linear type.
#[error("Cannot copy linear type {typ} from output {src_offset} of node {src}")]
NoCopyLinear {
typ: Type,
src: Node,
src_offset: Port,
},
/// The ancestors of an inter-graph edge are not related.
#[error("Cannot connect an inter-graph edge between unrelated nodes. Tried connecting {src} ({src_offset}) with {dst} ({dst_offset}).")]
NoRelationIntergraph {
src: Node,
src_offset: Port,
dst: Node,
dst_offset: Port,
},
/// Inter-Graph edges can only carry copyable data.
#[error("Inter-graph edges cannot carry non-copyable data {typ}. Tried connecting {src} ({src_offset}) with {dst} ({dst_offset}).")]
NonCopyableIntergraph {
src: Node,
src_offset: Port,
dst: Node,
dst_offset: Port,
typ: Type,
},
}

#[cfg(test)]
Expand Down
103 changes: 65 additions & 38 deletions quantinuum-hugr/src/builder/build_traits.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::hugr::hugrmut::InsertionResult;
use crate::hugr::validate::InterGraphEdgeError;
use crate::hugr::views::HugrView;
use crate::hugr::{NodeMetadata, ValidationError};
use crate::ops::{self, LeafOp, OpTag, OpTrait, OpType};
Expand All @@ -8,11 +7,11 @@ use crate::{IncomingPort, Node, OutgoingPort};

use std::iter;

use super::FunctionBuilder;
use super::{
handle::{BuildHandle, Outputs},
CircuitBuilder,
};
use super::{BuilderWiringError, FunctionBuilder};

use crate::{
hugr::NodeType,
Expand Down Expand Up @@ -188,7 +187,7 @@ pub trait Dataflow: Container {
///
/// # Errors
///
/// This function will return an error if there is an error when adding the node.
/// Returns a [`BuildError::OperationWiring`] error if the `input_wires` cannot be connected.
fn add_dataflow_op(
&mut self,
op: impl Into<OpType>,
Expand All @@ -202,13 +201,13 @@ pub trait Dataflow: Container {
///
/// # Errors
///
/// This function will return an error if there is an error when adding the node.
/// Returns a [`BuildError::OperationWiring`] error if the `input_wires` cannot be connected.
fn add_dataflow_node(
&mut self,
nodetype: NodeType,
input_wires: impl IntoIterator<Item = Wire>,
) -> Result<BuildHandle<DataflowOpID>, BuildError> {
let outs = add_node_with_wires(self, nodetype, input_wires.into_iter().collect())?;
let outs = add_node_with_wires(self, nodetype, input_wires)?;

Ok(outs.into())
}
Expand All @@ -225,11 +224,12 @@ pub trait Dataflow: Container {
hugr: Hugr,
input_wires: impl IntoIterator<Item = Wire>,
) -> Result<BuildHandle<DataflowOpID>, BuildError> {
let num_outputs = hugr.get_optype(hugr.root()).value_output_count();
let optype = hugr.get_optype(hugr.root()).clone();
let num_outputs = optype.value_output_count();
let node = self.add_hugr(hugr).new_root;

let inputs = input_wires.into_iter().collect();
wire_up_inputs(inputs, node, self)?;
wire_up_inputs(input_wires, node, self)
.map_err(|error| BuildError::OperationWiring { op: optype, error })?;

Ok((node, num_outputs).into())
}
Expand All @@ -247,10 +247,11 @@ pub trait Dataflow: Container {
input_wires: impl IntoIterator<Item = Wire>,
) -> Result<BuildHandle<DataflowOpID>, BuildError> {
let node = self.add_hugr_view(hugr).new_root;
let num_outputs = hugr.get_optype(hugr.root()).value_output_count();
let optype = hugr.get_optype(hugr.root()).clone();
let num_outputs = optype.value_output_count();

let inputs = input_wires.into_iter().collect();
wire_up_inputs(inputs, node, self)?;
wire_up_inputs(input_wires, node, self)
.map_err(|error| BuildError::OperationWiring { op: optype, error })?;

Ok((node, num_outputs).into())
}
Expand All @@ -265,7 +266,13 @@ pub trait Dataflow: Container {
output_wires: impl IntoIterator<Item = Wire>,
) -> Result<(), BuildError> {
let [_, out] = self.io();
wire_up_inputs(output_wires.into_iter().collect_vec(), out, self)
wire_up_inputs(output_wires.into_iter().collect_vec(), out, self).map_err(|error| {
BuildError::OutputWiring {
container_op: self.hugr().get_optype(self.container_node()).clone(),
container_node: self.container_node(),
error,
}
})
}

/// Return an array of the input wires.
Expand Down Expand Up @@ -297,7 +304,7 @@ pub trait Dataflow: Container {
signature: signature.clone(),
};
let nodetype = NodeType::new(op, input_extensions.clone());
let (dfg_n, _) = add_node_with_wires(self, nodetype, input_wires.into_iter().collect())?;
let (dfg_n, _) = add_node_with_wires(self, nodetype, input_wires)?;

DFGBuilder::create_with_io(self.hugr_mut(), dfg_n, signature, input_extensions)
}
Expand Down Expand Up @@ -601,39 +608,59 @@ pub trait Dataflow: Container {
}
}

/// Add a node to the graph, wiring up the `inputs` to the input ports of the resulting node.
///
/// # Errors
///
/// Returns a [`BuildError::OperationWiring`] if any of the connections produces an
/// invalid edge.
fn add_node_with_wires<T: Dataflow + ?Sized>(
data_builder: &mut T,
nodetype: impl Into<NodeType>,
inputs: Vec<Wire>,
inputs: impl IntoIterator<Item = Wire>,
) -> Result<(Node, usize), BuildError> {
let nodetype: NodeType = nodetype.into();
let num_outputs = nodetype.op().value_output_count();
let op_node = data_builder.add_child_node(nodetype);
let op_node = data_builder.add_child_node(nodetype.clone());

wire_up_inputs(inputs, op_node, data_builder)?;
wire_up_inputs(inputs, op_node, data_builder).map_err(|error| BuildError::OperationWiring {
op: nodetype.into_op(),
error,
})?;

Ok((op_node, num_outputs))
}

/// Connect each of the `inputs` wires sequentially to the input ports of
/// `op_node`.
///
/// # Errors
///
/// Returns a [`BuilderWiringError`] if any of the connections produces an
/// invalid edge.
fn wire_up_inputs<T: Dataflow + ?Sized>(
inputs: Vec<Wire>,
inputs: impl IntoIterator<Item = Wire>,
op_node: Node,
data_builder: &mut T,
) -> Result<(), BuildError> {
) -> Result<(), BuilderWiringError> {
for (dst_port, wire) in inputs.into_iter().enumerate() {
wire_up(data_builder, wire.node(), wire.source(), op_node, dst_port)?;
}
Ok(())
}

/// Add edge from src to dst and report back if they do share a parent
/// Add edge from src to dst.
///
/// # Errors
///
/// Returns a [`BuilderWiringError`] if the edge is invalid.
fn wire_up<T: Dataflow + ?Sized>(
data_builder: &mut T,
src: Node,
src_port: impl Into<OutgoingPort>,
dst: Node,
dst_port: impl Into<IncomingPort>,
) -> Result<bool, BuildError> {
) -> Result<bool, BuilderWiringError> {
let src_port = src_port.into();
let dst_port = dst_port.into();
let base = data_builder.hugr_mut();
Expand All @@ -646,15 +673,13 @@ fn wire_up<T: Dataflow + ?Sized>(
if !local_source {
// Non-local value sources require a state edge to an ancestor of dst
if !typ.copyable() {
let val_err: ValidationError = InterGraphEdgeError::NonCopyableData {
from: src,
from_offset: src_port.into(),
to: dst,
to_offset: dst_port.into(),
ty: EdgeKind::Value(typ),
}
.into();
return Err(val_err.into());
return Err(BuilderWiringError::NonCopyableIntergraph {
src,
src_offset: src_port.into(),
dst,
dst_offset: dst_port.into(),
typ,
});
}

let src_parent = src_parent.expect("Node has no parent");
Expand All @@ -667,14 +692,12 @@ fn wire_up<T: Dataflow + ?Sized>(
.then_some(ancestor)
})
else {
let val_err: ValidationError = InterGraphEdgeError::NoRelation {
from: src,
from_offset: src_port.into(),
to: dst,
to_offset: dst_port.into(),
}
.into();
return Err(val_err.into());
return Err(BuilderWiringError::NoRelationIntergraph {
src,
src_offset: src_port.into(),
dst,
dst_offset: dst_port.into(),
});
};

if !OpTag::BasicBlock.is_superset(base.get_optype(src).tag())
Expand All @@ -685,7 +708,11 @@ fn wire_up<T: Dataflow + ?Sized>(
}
} else if !typ.copyable() & base.linked_ports(src, src_port).next().is_some() {
// Don't copy linear edges.
return Err(BuildError::NoCopyLinear(typ));
return Err(BuilderWiringError::NoCopyLinear {
typ,
src,
src_offset: src_port.into(),
});
}
}

Expand Down
25 changes: 17 additions & 8 deletions quantinuum-hugr/src/builder/dataflow.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ pub(crate) mod test {
use serde_json::json;

use crate::builder::build_traits::DataflowHugr;
use crate::builder::{DataflowSubContainer, ModuleBuilder};
use crate::builder::{BuilderWiringError, DataflowSubContainer, ModuleBuilder};
use crate::extension::prelude::BOOL_T;
use crate::extension::{ExtensionId, EMPTY_REG};
use crate::hugr::validate::InterGraphEdgeError;
Expand Down Expand Up @@ -328,7 +328,14 @@ pub(crate) mod test {
Ok(module_builder.finish_prelude_hugr()?)
};

assert_eq!(builder(), Err(BuildError::NoCopyLinear(QB)));
assert_matches!(
builder(),
Err(BuildError::OutputWiring {
error: BuilderWiringError::NoCopyLinear { typ, .. },
..
})
if typ == QB
);
}

#[test]
Expand Down Expand Up @@ -376,9 +383,10 @@ pub(crate) mod test {
// but the builder catches it earlier
assert_matches!(
id_res.map(|bh| bh.handle().node()), // Transform into something that impl's Debug
Err(BuildError::InvalidHUGR(
ValidationError::InterGraphEdgeError(InterGraphEdgeError::NonCopyableData { .. })
))
Err(BuildError::OperationWiring {
error: BuilderWiringError::NonCopyableIntergraph { .. },
..
})
);

Ok(())
Expand Down Expand Up @@ -539,9 +547,10 @@ pub(crate) mod test {

assert_matches!(
res.map(|h| h.handle().node()), // map to something that implements Debug
Err(BuildError::InvalidHUGR(
ValidationError::InterGraphEdgeError(InterGraphEdgeError::NoRelation { .. })
))
Err(BuildError::OutputWiring {
error: BuilderWiringError::NoRelationIntergraph { .. },
..
})
);
Ok(())
}
Expand Down
7 changes: 7 additions & 0 deletions quantinuum-hugr/src/hugr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,13 @@ impl NodeType {
&self.op
}

/// Returns the underlying [OpType] i.e. without any [input_extensions]
///
/// [input_extensions]: NodeType::input_extensions
pub fn into_op(self) -> OpType {
self.op
}

delegate! {
to self.op {
/// Tag identifying the operation.
Expand Down

0 comments on commit 2c1539c

Please sign in to comment.