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

[bugfix] Non-local edges: fix validation, Consts inside CFG, no Static Dom edges #258

Merged
merged 29 commits into from
Jul 12, 2023

Conversation

acl-cqc
Copy link
Contributor

@acl-cqc acl-cqc commented Jul 11, 2023

  • Check Static edges meet locality requirements rather than allowing "anywhere"
  • Don't allow Static Dom edges; but, allow Consts in CFGs (they can always be lifted as they have no inputs)
  • Update spec
  • Fix an amusing test inversion which made many tests fails with "Ancestor is not a CFG! ancestor_op: CFG" :-D

Fixes #231

// Inter-graph constant wires do not have restrictions
EdgeKind::Static(typ) => {
if let OpType::Const(ops::Const(val)) = from_optype {
return typecheck_const(&typ, val).map_err(ValidationError::from);
typecheck_const(&typ, val).map_err(ValidationError::from)?;
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 do wonder if this should go in validate_operation. Admittedly there is nothing else there like it (but nor anywhere else that I can see - am I missing somewhere?)

} else {
Err(InterGraphEdgeError::InvalidConstSrc {
if !OpTag::Function.is_superset(from_optype.tag()) {
return Err(InterGraphEdgeError::InvalidConstSrc {
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'm also not really sure about this check. If a node says its other port/edgekind is static, isn't that sufficient - do we really mean to restrict only const/function nodes to doing that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does actually say this in the spec but just seems a bit weird: https://github.com/CQCL-DEV/hugr/blob/main/specification/hugr.md#static-edges

@acl-cqc acl-cqc changed the title [bugfix] Fix validation of (intergraph) edges [bugfix] Non-local edges: fix validation, Consts inside CFG, no Static Dom edges Jul 12, 2023
@acl-cqc acl-cqc marked this pull request as ready for review July 12, 2023 13:15
@acl-cqc acl-cqc requested a review from cqc-alec July 12, 2023 13:15
})?;
// External edge.
if !is_static {
// Must have an order edge§
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Must have an order edge§
// Must have an order edge.

@acl-cqc acl-cqc merged commit e22eee1 into main Jul 12, 2023
6 checks passed
@acl-cqc acl-cqc deleted the bugfix/validate_intergraph branch July 12, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate_intergraph_edge + typecheck_const
2 participants