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

fix: Add some validation for const nodes #1222

Merged
merged 7 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions hugr-core/resources
55 changes: 53 additions & 2 deletions hugr-core/src/hugr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,13 +295,17 @@ pub enum HugrError {

#[cfg(test)]
mod test {
use std::{fs::File, io::BufReader};

use super::internal::HugrMutInternals;
#[cfg(feature = "extension_inference")]
use super::ValidationError;
use super::{ExtensionError, Hugr, HugrMut, HugrView, Node};
use crate::extension::{ExtensionId, ExtensionSet, EMPTY_REG, TO_BE_INFERRED};
use crate::extension::{
ExtensionId, ExtensionSet, EMPTY_REG, PRELUDE_REGISTRY, TO_BE_INFERRED,
};
use crate::types::{FunctionType, Type};
use crate::{const_extension_ids, ops, type_row};
use crate::{const_extension_ids, ops, test_file, type_row};
use rstest::rstest;

#[test]
Expand All @@ -322,6 +326,53 @@ mod test {
assert_matches!(hugr.get_io(hugr.root()), Some(_));
}

#[test]
#[ignore = "issue 1225: In serialisation we do not distinguish between unknown CustomConst serialised value invalid but known CustomConst serialised values"]
fn hugr_validation_0() {
// https://github.com/CQCL/hugr/issues/1091 bad case
let mut hugr: Hugr = serde_json::from_reader(BufReader::new(
File::open(test_file!("hugr-0.json")).unwrap(),
))
.unwrap();
assert!(
hugr.update_validate(&PRELUDE_REGISTRY).is_err(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs to include float extensions here and below I think?. Maybe it's fine because CustomConst don't work through extensions.

"HUGR should not validate."
);
}

#[test]
fn hugr_validation_1() {
// https://github.com/CQCL/hugr/issues/1091 good case
let mut hugr: Hugr = serde_json::from_reader(BufReader::new(
File::open(test_file!("hugr-1.json")).unwrap(),
))
.unwrap();
assert!(hugr.update_validate(&PRELUDE_REGISTRY).is_ok());
}

#[test]
fn hugr_validation_2() {
// https://github.com/CQCL/hugr/issues/1185 bad case
let mut hugr: Hugr = serde_json::from_reader(BufReader::new(
File::open(test_file!("hugr-2.json")).unwrap(),
))
.unwrap();
assert!(
hugr.update_validate(&PRELUDE_REGISTRY).is_err(),
"HUGR should not validate."
);
}

#[test]
fn hugr_validation_3() {
// https://github.com/CQCL/hugr/issues/1185 good case
let mut hugr: Hugr = serde_json::from_reader(BufReader::new(
File::open(test_file!("hugr-3.json")).unwrap(),
))
.unwrap();
assert!(hugr.update_validate(&PRELUDE_REGISTRY).is_ok());
}

const_extension_ids! {
const XA: ExtensionId = "EXT_A";
const XB: ExtensionId = "EXT_B";
Expand Down
10 changes: 10 additions & 0 deletions hugr-core/src/hugr/validate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use thiserror::Error;

use crate::extension::{ExtensionRegistry, SignatureError, TO_BE_INFERRED};

use crate::ops::constant::ConstTypeError;
use crate::ops::custom::{resolve_opaque_op, CustomOp, CustomOpError};
use crate::ops::validate::{ChildrenEdgeData, ChildrenValidationError, EdgeValidationError};
use crate::ops::{FuncDefn, OpParent, OpTag, OpTrait, OpType, ValidateOp};
Expand Down Expand Up @@ -214,6 +215,12 @@ impl<'a, 'b> ValidationContext<'a, 'b> {
// Thirdly that the node has correct children
self.validate_children(node, op_type)?;

// OpType-specific checks.
// TODO Maybe we should delegate these checks to the OpTypes themselves.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe, but I can see why you don't. ValidateOp is not set up for op-specific logic. I think it's fine here for now, but I do think you should extract it into a function. i.e.

if let OpType::Const(c) = op_type {
            match c.validate()
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps this TODO can be removed now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it should remain, it does feel like this should call into a trait.

if let OpType::Const(c) = op_type {
c.validate()?;
};

Ok(())
}

Expand Down Expand Up @@ -759,6 +766,9 @@ pub enum ValidationError {
/// [Opaque]: crate::ops::CustomOp::Opaque
#[error(transparent)]
CustomOpError(#[from] CustomOpError),
/// TODO
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add docstring?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awkward, will post a PR to fixup this

#[error(transparent)]
ConstTypeError(#[from] ConstTypeError),
}

/// Errors related to the inter-graph edge validations.
Expand Down
10 changes: 10 additions & 0 deletions hugr-core/src/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,13 @@ macro_rules! const_extension_ids {
}

pub use const_extension_ids;

#[cfg(test)]
/// Get the full path to a test file given its path relative to the
/// `resources/test` directory in this crate.
#[macro_export]
macro_rules! test_file {
($fname:expr) => {
concat!(env!("CARGO_MANIFEST_DIR"), "/resources/test/", $fname)
};
}
28 changes: 28 additions & 0 deletions hugr-core/src/ops/constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ impl Const {
/// For a Const holding a CustomConst, extract the CustomConst by
/// downcasting.
pub fn get_custom_value<T: CustomConst>(&self) -> Option<&T>;

/// Check the value.
pub fn validate(&self) -> Result<(), ConstTypeError>;
}
}
}
Expand Down Expand Up @@ -412,6 +415,31 @@ impl Value {
}
}
}

/// Check the value.
pub fn validate(&self) -> Result<(), ConstTypeError> {
match self {
Self::Extension { e } => Ok(e.value().validate()?),
Self::Function { hugr } => {
mono_fn_type(hugr)?;
Ok(())
}
Self::Tuple { vs } => {
for v in vs {
v.validate()?;
}
Ok(())
}
Self::Sum {
tag,
values,
sum_type,
} => {
sum_type.check_type(*tag, values)?;
Ok(())
}
}
}
}

impl<T> From<T> for Value
Expand Down
2 changes: 1 addition & 1 deletion hugr-passes/src/const_fold/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ fn test_list_ops() -> Result<(), Box<dyn std::error::Error>> {
collections::EXTENSION.to_owned(),
])
.unwrap();
let list: Value = ListValue::new(BOOL_T, [Value::unit_sum(0, 1).unwrap()]).into();
let list: Value = ListValue::new(BOOL_T, [Value::false_val()]).into();
let mut build = DFGBuilder::new(FunctionType::new(
type_row![],
vec![list.get_type().clone()],
Expand Down
2 changes: 1 addition & 1 deletion hugr-py/tests/test_hugr_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,7 @@ def test_ancestral_sibling():
"val",
[
val.Function(simple_id().hugr),
val.Sum(1, tys.Sum([[INT_T], [tys.Bool, INT_T]]), [IntVal(34)]),
val.Sum(1, tys.Sum([[INT_T], [tys.Bool, INT_T]]), [val.TRUE, IntVal(34)]),
val.Tuple([val.TRUE, IntVal(23)]),
],
)
Expand Down
36 changes: 36 additions & 0 deletions resources/test/hugr-0.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"version": "v1",
"nodes":
[
{
"parent": 0,
"input_extensions": null,
"op": "Const",
"v":
{
"v": "Extension",
"extensions":
[
"arithmetic.float.types"
],
"typ":
{
"t": "Opaque",
"extension": "arithmetic.float.types",
"id": "float64",
"args":
[],
"bound": "C"
},
"value":
{
"c": "ConstF64",
"v": 42
}
}
}
],
"edges":
[],
"encoder": null
}
39 changes: 39 additions & 0 deletions resources/test/hugr-1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
{
"version": "v1",
"nodes":
[
{
"parent": 0,
"input_extensions": null,
"op": "Const",
"v":
{
"v": "Extension",
"extensions":
[
"arithmetic.float.types"
],
"typ":
{
"t": "Opaque",
"extension": "arithmetic.float.types",
"id": "float64",
"args":
[],
"bound": "C"
},
"value":
{
"c": "ConstF64",
"v":
{
"value": 42.0
}
}
}
}
],
"edges":
[],
"encoder": null
}
33 changes: 33 additions & 0 deletions resources/test/hugr-2.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"version": "v1",
"nodes":
[
{
"parent": 0,
"input_extensions": null,
"op": "Const",
"v":
{
"v": "Sum",
"tag": 1,
"typ":
{
"t": "Sum",
"s": "Unit",
"size": 2
},
"vs":
[
{
"v": "Tuple",
"vs":
[]
}
]
}
}
],
"edges":
[],
"encoder": null
}
27 changes: 27 additions & 0 deletions resources/test/hugr-3.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"version": "v1",
"nodes":
[
{
"parent": 0,
"input_extensions": null,
"op": "Const",
"v":
{
"v": "Sum",
"tag": 1,
"typ":
{
"t": "Sum",
"s": "Unit",
"size": 2
},
"vs":
[]
}
}
],
"edges":
[],
"encoder": null
}
Loading