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.

Sign up for GitHub

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

Jump to bottom

feat: Validate TypeArgs to ExtensionOp #509

Merged
merged 6 commits into from
Sep 12, 2023
Merged

feat: Validate TypeArgs to ExtensionOp #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea11aaf
Validate TypeArgs to ExtensionOp. Is there any point?
acl-cqc Sep 8, 2023
f8f45bb
Add check that args match params...redundant?
acl-cqc Sep 8, 2023
aee2254
Revert "Add check that args match params...redundant?"
acl-cqc Sep 8, 2023
c2db98c
validate_children => validate_op_children
acl-cqc Sep 12, 2023
0019e8b
validate_operation => validate_children
acl-cqc Sep 12, 2023
1573c0b
Update comment, issue link
acl-cqc Sep 12, 2023
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
18 changes: 14 additions & 4 deletions src/hugr/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use crate::extension::{
validate::{ExtensionError, ExtensionValidator},
ExtensionRegistry, ExtensionSolution, InferExtensionError,
};

use crate::ops::validate::{ChildrenEdgeData, ChildrenValidationError, EdgeValidationError};
use crate::ops::{OpTag, OpTrait, OpType, ValidateOp};
use crate::types::{EdgeKind, Type};
Expand Down Expand Up @@ -158,8 +159,17 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
}
}

// Check operation-specific constraints
self.validate_operation(node, node_type)?;
// Check operation-specific constraints. Firstly that type args are correct
// (Good to call `resolve_extension_ops` immediately before this
// - see https://github.com/CQCL-DEV/hugr/issues/508 )
if let OpType::LeafOp(crate::ops::LeafOp::CustomOp(b)) = op_type {
for arg in b.args() {
arg.validate(self.extension_registry)
.map_err(|cause| ValidationError::SignatureError { node, cause })?;
}
}
// Secondly that the node has correct children
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Methinks validate_operation is not a good name, given there's already other validation of the operation here in validate_node. Rather, validate_operation is all about the op's children. However validate_children is already taken (as a subset of those child-validating checks), so the best renaming I can see would be along the lines of

  1. validate_children -> validate_children_op_specific or something
  2. validate_operation -> validate_children

If anyone can think of a good name for the or something then I'm happy to proceed with that ;-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uhm. I guess the OpValidityFlags are focused on a container's children.
I'd say rename that to OpContainerFlags (?) and ValidateOp::validate_children to run_children_validator ?
(on top of your 2. )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, gone with validate_children -> validate_op_children (my #1, your #2 - the only validate_children was that defined in ValidateOp), and validate_operation -> validate_children.

I've left OpValidityFlags because it does have one member that is not related to children - non_df_port. I think we were talking about refactoring non-dataflow-ports more widely, so if non_df_port goes as part of that refactor.... (as perhaps it should: non_df_port doesn't appear to be used in validation, but rather in pub functions of OpType: other_port_count and port_index....)

Happy to move these two renames into a separate PR if you like, I was going to when I thought there'd be 4 but I haven't bothered at the moment since it looks like there are only 2.

self.validate_children(node, node_type)?;

// If this is a container with I/O nodes, check that the extension they
// define match the extensions of the container.
Expand Down Expand Up @@ -260,7 +270,7 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
/// Check operation-specific constraints.
///
/// These are flags defined for each operation type as an [`OpValidityFlags`] object.
fn validate_operation(&self, node: Node, node_type: &NodeType) -> Result<(), ValidationError> {
fn validate_children(&self, node: Node, node_type: &NodeType) -> Result<(), ValidationError> {
let op_type = &node_type.op;
let flags = op_type.validity_flags();

Expand Down Expand Up @@ -301,7 +311,7 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
}
// Additional validations running over the full list of children optypes
let children_optypes = all_children.map(|c| (c.index, self.hugr.get_optype(c)));
if let Err(source) = op_type.validate_children(children_optypes) {
if let Err(source) = op_type.validate_op_children(children_optypes) {
return Err(ValidationError::InvalidChildren {
parent: node,
parent_optype: op_type.clone(),
Expand Down
2 changes: 1 addition & 1 deletion src/ops.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ pub trait ValidateOp {

/// Validate the ordered list of children.
#[inline]
fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
_children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), validate::ChildrenValidationError> {
Expand Down
16 changes: 8 additions & 8 deletions src/ops/validate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
//! Adds a `validity_flags` method to [`OpType`] that returns a series of flags
//! used by the [`crate::hugr::validate`] module.
//!
//! It also defines a `validate_children` method for more complex tests that
//! It also defines a `validate_op_children` method for more complex tests that
//! require traversing the children.

use itertools::Itertools;
Expand Down Expand Up @@ -93,7 +93,7 @@ impl ValidateOp for super::FuncDefn {
}
}

fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand All @@ -118,7 +118,7 @@ impl ValidateOp for super::DFG {
}
}

fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand All @@ -141,7 +141,7 @@ impl ValidateOp for super::Conditional {
}
}

fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand Down Expand Up @@ -188,7 +188,7 @@ impl ValidateOp for super::TailLoop {
}
}

fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand All @@ -214,7 +214,7 @@ impl ValidateOp for super::CFG {
}
}

fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl Iterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand Down Expand Up @@ -334,7 +334,7 @@ impl ValidateOp for BasicBlock {
}

/// Validate the ordered list of children.
fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand Down Expand Up @@ -369,7 +369,7 @@ impl ValidateOp for super::Case {
}

/// Validate the ordered list of children.
fn validate_children<'a>(
fn validate_op_children<'a>(
&self,
children: impl DoubleEndedIterator<Item = (NodeIndex, &'a OpType)>,
) -> Result<(), ChildrenValidationError> {
Expand Down
2 changes: 1 addition & 1 deletion src/types/type_param.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ pub enum TypeArg {
}

impl TypeArg {
pub(super) fn validate(
pub(crate) fn validate(
&self,
extension_registry: &ExtensionRegistry,
) -> Result<(), SignatureError> {
Expand Down