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

refactor: Separate extension validation from the rest #1011

Merged
merged 11 commits into from
May 8, 2024
4 changes: 3 additions & 1 deletion hugr/src/hugr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,10 @@ impl Hugr {
extension_registry: &ExtensionRegistry,
) -> Result<(), ValidationError> {
resolve_extension_ops(self, extension_registry)?;
self.validate_no_extensions(extension_registry)?;
acl-cqc marked this conversation as resolved.
Show resolved Hide resolved
self.infer_extensions()?;
self.validate(extension_registry)?;
#[cfg(feature = "infer_extensions")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put this around the line above too (in a {...} block)? And then drop the #cfg gating inside Hugr.infer_extensions? (So the latter does its stuff regardless of the feature, and we just avoid calling it unless the feature is on - or unless you really want to?) Does that work?

self.validate_extensions(HashMap::new())?;
Ok(())
}

Expand Down
79 changes: 52 additions & 27 deletions hugr/src/hugr/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ struct ValidationContext<'a, 'b> {
hugr: &'a Hugr,
/// Dominator tree for each CFG region, using the container node as index.
dominators: HashMap<Node, Dominators<Node>>,
/// Context for the extension validation.
#[allow(dead_code)]
extension_validator: ExtensionValidator,
/// Registry of available Extensions
extension_registry: &'b ExtensionRegistry,
}
Expand All @@ -47,19 +44,68 @@ impl Hugr {
/// variables.
/// TODO: Add a version of validation which allows for open extension
/// variables (see github issue #457)
#[cfg(feature = "extension_inference")]
pub fn validate(&self, extension_registry: &ExtensionRegistry) -> Result<(), ValidationError> {
self.validate_with_extension_closure(HashMap::new(), extension_registry)
}

/// Check the validity of the HUGR, disregarding extension requirements.
#[cfg(not(feature = "extension_inference"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! :)

Maybe even common-up the signature and put the #[cfg] just inside the method body?

pub fn validate(&self, extension_registry: &ExtensionRegistry) -> Result<(), ValidationError> {
self.validate_no_extensions(extension_registry)
}

/// Check the validity of the HUGR, but don't check consistency of extension
/// requirements between connected nodes or between parents and children.
pub fn validate_no_extensions(
&self,
extension_registry: &ExtensionRegistry,
) -> Result<(), ValidationError> {
let mut validator = ValidationContext::new(self, extension_registry);
validator.validate()
}

/// Validate extensions on the input and output edges of nodes. Check that
/// the target ends of edges require the extensions from the sources, and
/// check extension deltas from parent nodes are reflected in their children
pub fn validate_extensions(&self, closure: ExtensionSolution) -> Result<(), ValidationError> {
let validator = ExtensionValidator::new(self, closure);
for src_node in self.nodes() {
let node_type = self.get_nodetype(src_node);

// FuncDefns have no resources since they're static nodes, but the
// functions they define can have any extension delta.
if node_type.tag() != OpTag::FuncDefn {
// If this is a container with I/O nodes, check that the extension they
// define match the extensions of the container.
if let Some([input, output]) = self.get_io(src_node) {
validator.validate_io_extensions(src_node, input, output)?;
}
}

for src_port in self.node_outputs(src_node) {
for (tgt_node, tgt_port) in self.linked_inputs(src_node, src_port) {
validator.check_extensions_compatible(
&(src_node, src_port.into()),
&(tgt_node, tgt_port.into()),
)?;
}
}
}
Ok(())
}

/// Check the validity of a hugr, taking an argument of a closure for the
/// free extension variables
pub fn validate_with_extension_closure(
&self,
closure: ExtensionSolution,
extension_registry: &ExtensionRegistry,
) -> Result<(), ValidationError> {
let mut validator = ValidationContext::new(self, closure, extension_registry);
validator.validate()
let mut validator = ValidationContext::new(self, extension_registry);
validator.validate()?;
self.validate_extensions(closure)?;
Ok(())
}
}

Expand All @@ -68,15 +114,10 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
// Allow unused "extension_closure" variable for when
// the "extension_inference" feature is disabled.
#[allow(unused_variables)]
pub fn new(
hugr: &'a Hugr,
extension_closure: ExtensionSolution,
extension_registry: &'b ExtensionRegistry,
) -> Self {
pub fn new(hugr: &'a Hugr, extension_registry: &'b ExtensionRegistry) -> Self {
Self {
hugr,
dominators: HashMap::new(),
extension_validator: ExtensionValidator::new(hugr, extension_closure),
extension_registry,
}
}
Expand Down Expand Up @@ -176,18 +217,6 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
// Secondly that the node has correct children
self.validate_children(node, node_type)?;

// FuncDefns have no resources since they're static nodes, but the
// functions they define can have any extension delta.
#[cfg(feature = "extension_inference")]
if node_type.tag() != OpTag::FuncDefn {
// If this is a container with I/O nodes, check that the extension they
// define match the extensions of the container.
if let Some([input, output]) = self.hugr.get_io(node) {
self.extension_validator
.validate_io_extensions(node, input, output)?;
}
}

Ok(())
}

Expand Down Expand Up @@ -247,10 +276,6 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
let other_node: Node = self.hugr.graph.port_node(link).unwrap().into();
let other_offset = self.hugr.graph.port_offset(link).unwrap().into();

#[cfg(feature = "extension_inference")]
self.extension_validator
.check_extensions_compatible(&(node, port), &(other_node, other_offset))?;

let other_op = self.hugr.get_optype(other_node);
let Some(other_kind) = other_op.port_kind(other_offset) else {
panic!("The number of ports in {other_node} does not match the operation definition. This should have been caught by `validate_node`.");
Expand Down