-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: cfg not validating entry/exit types #1229
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,6 @@ | ||
use std::fs::File; | ||
use std::io::BufReader; | ||
|
||
use cool_asserts::assert_matches; | ||
use rstest::rstest; | ||
|
||
|
@@ -19,7 +22,7 @@ use crate::std_extensions::logic::test::{and_op, or_op}; | |
use crate::std_extensions::logic::{self, NotOp}; | ||
use crate::types::type_param::{TypeArg, TypeArgError}; | ||
use crate::types::{CustomType, FunctionType, PolyFuncType, Type, TypeBound, TypeRow}; | ||
use crate::{const_extension_ids, type_row, Direction, IncomingPort, Node}; | ||
use crate::{const_extension_ids, test_file, type_row, Direction, IncomingPort, Node}; | ||
|
||
const NAT: Type = crate::extension::prelude::USIZE_T; | ||
|
||
|
@@ -926,6 +929,13 @@ fn cfg_children_restrictions() { | |
b.remove_node(exit2); | ||
|
||
// Change the types in the BasicBlock node to work on qubits instead of bits | ||
b.replace_op( | ||
cfg, | ||
ops::CFG { | ||
signature: FunctionType::new(type_row![QB_T], type_row![BOOL_T]), | ||
}, | ||
) | ||
.unwrap(); | ||
b.replace_op( | ||
block, | ||
ops::DataflowBlock { | ||
|
@@ -989,6 +999,23 @@ fn cfg_connections() -> Result<(), Box<dyn std::error::Error>> { | |
Ok(()) | ||
} | ||
|
||
#[test] | ||
fn cfg_entry_io_bug() -> Result<(), Box<dyn std::error::Error>> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we get better error messages on failure if we return |
||
// load test file where input node of entry block has types in reversed | ||
// order compared to parent CFG node. | ||
let mut hugr: Hugr = serde_json::from_reader(BufReader::new( | ||
File::open(test_file!("issue-1189.json")).unwrap(), | ||
)) | ||
.unwrap(); | ||
assert_matches!( | ||
hugr.update_validate(&PRELUDE_REGISTRY), | ||
Err(ValidationError::InvalidChildren { parent, source: ChildrenValidationError::IOSignatureMismatch{..}, .. }) | ||
=> assert_eq!(parent, hugr.root()) | ||
); | ||
|
||
Ok(()) | ||
} | ||
|
||
#[cfg(feature = "extension_inference")] | ||
mod extension_tests { | ||
use self::ops::handle::{BasicBlockID, TailLoopID}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ use thiserror::Error; | |
|
||
use crate::types::TypeRow; | ||
|
||
use super::dataflow::DataflowParent; | ||
use super::dataflow::{DataflowOpTrait, DataflowParent}; | ||
use super::{impl_validate_op, BasicBlock, ExitBlock, OpTag, OpTrait, OpType, ValidateOp}; | ||
|
||
/// A set of property flags required for an operation. | ||
|
@@ -121,9 +121,37 @@ impl ValidateOp for super::CFG { | |
|
||
fn validate_op_children<'a>( | ||
&self, | ||
children: impl Iterator<Item = (NodeIndex, &'a OpType)>, | ||
mut children: impl Iterator<Item = (NodeIndex, &'a OpType)>, | ||
) -> Result<(), ChildrenValidationError> { | ||
for (child, optype) in children.dropping(2) { | ||
let (entry, entry_op) = children.next().unwrap(); | ||
let (exit, exit_op) = children.next().unwrap(); | ||
let entry_op = entry_op | ||
.as_dataflow_block() | ||
.expect("Child check should have already checked valid ops."); | ||
let exit_op = exit_op | ||
.as_exit_block() | ||
.expect("Child check should have already checked valid ops."); | ||
|
||
let sig = self.signature(); | ||
if entry_op.inner_signature().input() != sig.input() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be helpful to add a comment that we only have to check entry and exit nodes here because internal control flow edges are validated by |
||
return Err(ChildrenValidationError::IOSignatureMismatch { | ||
child: entry, | ||
actual: entry_op.inner_signature().input().clone(), | ||
expected: sig.input().clone(), | ||
node_desc: "BasicBlock Input", | ||
container_desc: "CFG", | ||
}); | ||
} | ||
if &exit_op.cfg_outputs != sig.output() { | ||
return Err(ChildrenValidationError::IOSignatureMismatch { | ||
child: exit, | ||
actual: exit_op.cfg_outputs.clone(), | ||
expected: sig.output().clone(), | ||
node_desc: "BasicBlockExit Output", | ||
container_desc: "CFG", | ||
}); | ||
} | ||
for (child, optype) in children { | ||
if optype.tag() == OpTag::BasicBlockExit { | ||
return Err(ChildrenValidationError::InternalExitChildren { child }); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,136 @@ | ||
{ | ||
"version": "v1", | ||
"nodes": [ | ||
{ | ||
"parent": 0, | ||
"op": "CFG", | ||
"signature": { | ||
"t": "G", | ||
"input": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 1 | ||
}, | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
} | ||
], | ||
"output": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
} | ||
], | ||
"extension_reqs": [] | ||
} | ||
}, | ||
{ | ||
"parent": 0, | ||
"op": "DataflowBlock", | ||
"inputs": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
}, | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 1 | ||
} | ||
], | ||
"other_outputs": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
} | ||
], | ||
"sum_rows": [ | ||
[] | ||
], | ||
"extension_delta": [] | ||
}, | ||
{ | ||
"parent": 1, | ||
"op": "Input", | ||
"types": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
}, | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 1 | ||
} | ||
] | ||
}, | ||
{ | ||
"parent": 1, | ||
"op": "Output", | ||
"types": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 1 | ||
}, | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
} | ||
] | ||
}, | ||
{ | ||
"parent": 0, | ||
"op": "ExitBlock", | ||
"cfg_outputs": [ | ||
{ | ||
"t": "Sum", | ||
"s": "Unit", | ||
"size": 2 | ||
} | ||
] | ||
} | ||
], | ||
"edges": [ | ||
[ | ||
[ | ||
2, | ||
1 | ||
], | ||
[ | ||
3, | ||
0 | ||
] | ||
], | ||
[ | ||
[ | ||
2, | ||
0 | ||
], | ||
[ | ||
3, | ||
1 | ||
] | ||
], | ||
[ | ||
[ | ||
1, | ||
0 | ||
], | ||
[ | ||
4, | ||
0 | ||
] | ||
] | ||
], | ||
"metadata": null, | ||
"encoder": "hugr-py v0.2.1" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test failed because it was getting away with not updating the cfg op beforehand