From 9f203e14343fa5c6a2439cc27ecae1fab6ebe044 Mon Sep 17 00:00:00 2001 From: Alec Edgington Date: Thu, 20 Jun 2024 11:50:30 +0100 Subject: [PATCH 1/6] Add test cases (two failing). --- hugr-core/resources/test/hugr-0.json | 36 ++++++++++++++++++++ hugr-core/resources/test/hugr-1.json | 39 ++++++++++++++++++++++ hugr-core/resources/test/hugr-2.json | 33 ++++++++++++++++++ hugr-core/resources/test/hugr-3.json | 27 +++++++++++++++ hugr-core/src/hugr.rs | 50 ++++++++++++++++++++++++++++ hugr-core/src/macros.rs | 10 ++++++ 6 files changed, 195 insertions(+) create mode 100644 hugr-core/resources/test/hugr-0.json create mode 100644 hugr-core/resources/test/hugr-1.json create mode 100644 hugr-core/resources/test/hugr-2.json create mode 100644 hugr-core/resources/test/hugr-3.json diff --git a/hugr-core/resources/test/hugr-0.json b/hugr-core/resources/test/hugr-0.json new file mode 100644 index 000000000..286e9114e --- /dev/null +++ b/hugr-core/resources/test/hugr-0.json @@ -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 +} diff --git a/hugr-core/resources/test/hugr-1.json b/hugr-core/resources/test/hugr-1.json new file mode 100644 index 000000000..c7622c8d4 --- /dev/null +++ b/hugr-core/resources/test/hugr-1.json @@ -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 +} diff --git a/hugr-core/resources/test/hugr-2.json b/hugr-core/resources/test/hugr-2.json new file mode 100644 index 000000000..d133c8870 --- /dev/null +++ b/hugr-core/resources/test/hugr-2.json @@ -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 +} diff --git a/hugr-core/resources/test/hugr-3.json b/hugr-core/resources/test/hugr-3.json new file mode 100644 index 000000000..66e537713 --- /dev/null +++ b/hugr-core/resources/test/hugr-3.json @@ -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 +} diff --git a/hugr-core/src/hugr.rs b/hugr-core/src/hugr.rs index 25aa28214..fc18a7912 100644 --- a/hugr-core/src/hugr.rs +++ b/hugr-core/src/hugr.rs @@ -221,6 +221,10 @@ pub enum HugrError { #[cfg(test)] mod test { + use std::{fs::File, io::BufReader}; + + use crate::{extension::PRELUDE_REGISTRY, test_file}; + use super::{Hugr, HugrView}; #[test] @@ -240,4 +244,50 @@ mod test { let hugr = simple_dfg_hugr(); assert_matches!(hugr.get_io(hugr.root()), Some(_)); } + + #[test] + 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(), + "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()); + } } diff --git a/hugr-core/src/macros.rs b/hugr-core/src/macros.rs index 10dccee70..dea66c5bc 100644 --- a/hugr-core/src/macros.rs +++ b/hugr-core/src/macros.rs @@ -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) + }; +} From 3c0b2a1cfbdd877cd2336c9c014a42716eb6893c Mon Sep 17 00:00:00 2001 From: Alec Edgington Date: Mon, 24 Jun 2024 11:49:52 +0100 Subject: [PATCH 2/6] Check validity of SumType::Unit nodes. --- hugr-core/src/hugr/validate.rs | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index be2e06003..74536bfc4 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -13,9 +13,9 @@ use crate::extension::{ExtensionRegistry, ExtensionSet, SignatureError}; 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}; +use crate::ops::{FuncDefn, OpParent, OpTag, OpTrait, OpType, ValidateOp, Value}; use crate::types::type_param::TypeParam; -use crate::types::{EdgeKind, FunctionType}; +use crate::types::{EdgeKind, FunctionType, SumType, TypeEnum}; use crate::{Direction, Hugr, Node, Port}; use super::views::{HierarchyView, HugrView, SiblingGraph}; @@ -210,6 +210,23 @@ 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. + if let OpType::Const(c) = op_type { + if let TypeEnum::Sum(SumType::Unit { size: _ }) = c.get_type().as_type_enum() { + if let Value::Sum { + tag: _, + values, + sum_type: _, + } = c.value() + { + if !values.is_empty() { + return Err(ValidationError::UnitSumWithValues {}); + } + } + } + } + Ok(()) } @@ -752,6 +769,9 @@ pub enum ValidationError { /// [Opaque]: crate::ops::CustomOp::Opaque #[error(transparent)] CustomOpError(#[from] CustomOpError), + /// Error in a [SumType::Unit] + #[error("A unit sum contained a non-empty list of values")] + UnitSumWithValues {}, } /// Errors related to the inter-graph edge validations. From e207dd1498c6f8e93b68edd204dc8a7733cac5b2 Mon Sep 17 00:00:00 2001 From: Alec Edgington Date: Mon, 24 Jun 2024 13:42:00 +0100 Subject: [PATCH 3/6] Check validity of "float64" values. --- hugr-core/src/hugr/validate.rs | 35 +++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index 74536bfc4..f9ff801fe 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -1,5 +1,6 @@ //! HUGR invariant checks. +use std::any::TypeId; use std::collections::HashMap; use std::iter; @@ -14,6 +15,7 @@ use crate::extension::{ExtensionRegistry, ExtensionSet, SignatureError}; 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, Value}; +use crate::std_extensions::arithmetic::float_types::ConstF64; use crate::types::type_param::TypeParam; use crate::types::{EdgeKind, FunctionType, SumType, TypeEnum}; use crate::{Direction, Hugr, Node, Port}; @@ -213,17 +215,29 @@ impl<'a, 'b> ValidationContext<'a, 'b> { // OpType-specific checks. // TODO Maybe we should delegate these checks to the OpTypes themselves. if let OpType::Const(c) = op_type { - if let TypeEnum::Sum(SumType::Unit { size: _ }) = c.get_type().as_type_enum() { - if let Value::Sum { - tag: _, - values, - sum_type: _, - } = c.value() - { - if !values.is_empty() { - return Err(ValidationError::UnitSumWithValues {}); + match c.get_type().as_type_enum() { + TypeEnum::Sum(SumType::Unit { size: _ }) => { + if let Value::Sum { + tag: _, + values, + sum_type: _, + } = c.value() + { + if !values.is_empty() { + return Err(ValidationError::UnitSumWithValues {}); + } + } + } + TypeEnum::Extension(custom_type) => { + if custom_type.name() == "float64" { + if let Value::Extension { e } = c.value() { + if e.value().type_id() != TypeId::of::() { + return Err(ValidationError::WrongExtensionValueType {}); + } + } } } + _ => {} } } @@ -772,6 +786,9 @@ pub enum ValidationError { /// Error in a [SumType::Unit] #[error("A unit sum contained a non-empty list of values")] UnitSumWithValues {}, + /// Wrong type for an extension value + #[error("An extension value was of the wrong type")] + WrongExtensionValueType {}, } /// Errors related to the inter-graph edge validations. From 800a6e72d8fc599de7d5fc789ab1815a41f2af36 Mon Sep 17 00:00:00 2001 From: Alec Edgington Date: Mon, 24 Jun 2024 13:47:45 +0100 Subject: [PATCH 4/6] Move `resources` directory up a level and add symlink. --- hugr-core/resources | 1 + {hugr-core/resources => resources}/test/hugr-0.json | 0 {hugr-core/resources => resources}/test/hugr-1.json | 0 {hugr-core/resources => resources}/test/hugr-2.json | 0 {hugr-core/resources => resources}/test/hugr-3.json | 0 5 files changed, 1 insertion(+) create mode 120000 hugr-core/resources rename {hugr-core/resources => resources}/test/hugr-0.json (100%) rename {hugr-core/resources => resources}/test/hugr-1.json (100%) rename {hugr-core/resources => resources}/test/hugr-2.json (100%) rename {hugr-core/resources => resources}/test/hugr-3.json (100%) diff --git a/hugr-core/resources b/hugr-core/resources new file mode 120000 index 000000000..554831148 --- /dev/null +++ b/hugr-core/resources @@ -0,0 +1 @@ +../resources/ \ No newline at end of file diff --git a/hugr-core/resources/test/hugr-0.json b/resources/test/hugr-0.json similarity index 100% rename from hugr-core/resources/test/hugr-0.json rename to resources/test/hugr-0.json diff --git a/hugr-core/resources/test/hugr-1.json b/resources/test/hugr-1.json similarity index 100% rename from hugr-core/resources/test/hugr-1.json rename to resources/test/hugr-1.json diff --git a/hugr-core/resources/test/hugr-2.json b/resources/test/hugr-2.json similarity index 100% rename from hugr-core/resources/test/hugr-2.json rename to resources/test/hugr-2.json diff --git a/hugr-core/resources/test/hugr-3.json b/resources/test/hugr-3.json similarity index 100% rename from hugr-core/resources/test/hugr-3.json rename to resources/test/hugr-3.json From 0fee7e94198ba28bde2f0f80620622b4f2379555 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Tue, 25 Jun 2024 11:36:25 +0100 Subject: [PATCH 5/6] validate const nodes. fix unearthed bug in const_fold::test::test_list_ops --- hugr-core/src/hugr.rs | 1 + hugr-core/src/hugr/validate.rs | 43 ++++++------------------------ hugr-core/src/ops/constant.rs | 28 +++++++++++++++++++ hugr-passes/src/const_fold/test.rs | 2 +- 4 files changed, 38 insertions(+), 36 deletions(-) diff --git a/hugr-core/src/hugr.rs b/hugr-core/src/hugr.rs index fc18a7912..086d867f4 100644 --- a/hugr-core/src/hugr.rs +++ b/hugr-core/src/hugr.rs @@ -246,6 +246,7 @@ mod test { } #[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( diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index f9ff801fe..fcf799aa6 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -1,6 +1,5 @@ //! HUGR invariant checks. -use std::any::TypeId; use std::collections::HashMap; use std::iter; @@ -12,12 +11,12 @@ use thiserror::Error; use crate::extension::{ExtensionRegistry, ExtensionSet, SignatureError}; +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, Value}; -use crate::std_extensions::arithmetic::float_types::ConstF64; +use crate::ops::{FuncDefn, OpParent, OpTag, OpTrait, OpType, ValidateOp}; use crate::types::type_param::TypeParam; -use crate::types::{EdgeKind, FunctionType, SumType, TypeEnum}; +use crate::types::{EdgeKind, FunctionType}; use crate::{Direction, Hugr, Node, Port}; use super::views::{HierarchyView, HugrView, SiblingGraph}; @@ -215,31 +214,8 @@ impl<'a, 'b> ValidationContext<'a, 'b> { // OpType-specific checks. // TODO Maybe we should delegate these checks to the OpTypes themselves. if let OpType::Const(c) = op_type { - match c.get_type().as_type_enum() { - TypeEnum::Sum(SumType::Unit { size: _ }) => { - if let Value::Sum { - tag: _, - values, - sum_type: _, - } = c.value() - { - if !values.is_empty() { - return Err(ValidationError::UnitSumWithValues {}); - } - } - } - TypeEnum::Extension(custom_type) => { - if custom_type.name() == "float64" { - if let Value::Extension { e } = c.value() { - if e.value().type_id() != TypeId::of::() { - return Err(ValidationError::WrongExtensionValueType {}); - } - } - } - } - _ => {} - } - } + c.validate()?; + }; Ok(()) } @@ -783,12 +759,9 @@ pub enum ValidationError { /// [Opaque]: crate::ops::CustomOp::Opaque #[error(transparent)] CustomOpError(#[from] CustomOpError), - /// Error in a [SumType::Unit] - #[error("A unit sum contained a non-empty list of values")] - UnitSumWithValues {}, - /// Wrong type for an extension value - #[error("An extension value was of the wrong type")] - WrongExtensionValueType {}, + /// TODO + #[error(transparent)] + ConstTypeError(#[from] ConstTypeError), } /// Errors related to the inter-graph edge validations. diff --git a/hugr-core/src/ops/constant.rs b/hugr-core/src/ops/constant.rs index 7975c40fe..170d58c5c 100644 --- a/hugr-core/src/ops/constant.rs +++ b/hugr-core/src/ops/constant.rs @@ -48,6 +48,9 @@ impl Const { /// For a Const holding a CustomConst, extract the CustomConst by /// downcasting. pub fn get_custom_value(&self) -> Option<&T>; + + /// Check the value. + pub fn validate(&self) -> Result<(), ConstTypeError>; } } } @@ -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 From for Value diff --git a/hugr-passes/src/const_fold/test.rs b/hugr-passes/src/const_fold/test.rs index b116853ab..60a9ea4cf 100644 --- a/hugr-passes/src/const_fold/test.rs +++ b/hugr-passes/src/const_fold/test.rs @@ -136,7 +136,7 @@ fn test_list_ops() -> Result<(), Box> { 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()], From 08df26ab1ba1234a4e1b402599347f160a8635ff Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Tue, 25 Jun 2024 13:58:58 +0100 Subject: [PATCH 6/6] fix builder test which was constructing an invalid value --- hugr-py/tests/test_hugr_build.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hugr-py/tests/test_hugr_build.py b/hugr-py/tests/test_hugr_build.py index 5d54cb177..89e549108 100644 --- a/hugr-py/tests/test_hugr_build.py +++ b/hugr-py/tests/test_hugr_build.py @@ -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)]), ], )