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
1 change: 1 addition & 0 deletions hugr/src/hugr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ 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)?;
Ok(())
Expand Down
62 changes: 39 additions & 23 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 @@ -51,15 +48,52 @@ impl Hugr {
self.validate_with_extension_closure(HashMap::new(), 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()
}

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()?;
#[cfg(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.

Is it plausible to move this #[cfg] onto the fn validate_with_extension_closure itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmmm, or, leave this as a way of validating extensions even if extension_inference is disabled? So then the feature-flag doesn't disable any validation, but just changes what the default validate and update_validate do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this as is, updating Hugr::validate to point to different things depending on the flag status instead

self.validate_extensions(closure)?;
Ok(())
}
}

Expand All @@ -70,13 +104,11 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
#[allow(unused_variables)]
pub fn new(
hugr: &'a Hugr,
extension_closure: ExtensionSolution,
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 +208,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 +267,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