From c05edd3cbcb644556bb3b2b23b6d27a211fe7e4f Mon Sep 17 00:00:00 2001 From: Alec Edgington <54802828+cqc-alec@users.noreply.github.com> Date: Tue, 25 Jun 2024 14:29:30 +0100 Subject: [PATCH] fix: Add some validation for const nodes (#1222) Fixes #1185. We add a test for #1091, but the fix unclear now, see #1225. --------- Co-authored-by: Douglas Wilson --- hugr-core/resources | 1 + hugr-core/src/hugr.rs | 55 ++++++++++++++++++++++++++++-- hugr-core/src/hugr/validate.rs | 10 ++++++ hugr-core/src/macros.rs | 10 ++++++ hugr-core/src/ops/constant.rs | 28 +++++++++++++++ hugr-passes/src/const_fold/test.rs | 2 +- hugr-py/tests/test_hugr_build.py | 2 +- resources/test/hugr-0.json | 36 +++++++++++++++++++ resources/test/hugr-1.json | 39 +++++++++++++++++++++ resources/test/hugr-2.json | 33 ++++++++++++++++++ resources/test/hugr-3.json | 27 +++++++++++++++ 11 files changed, 239 insertions(+), 4 deletions(-) create mode 120000 hugr-core/resources create mode 100644 resources/test/hugr-0.json create mode 100644 resources/test/hugr-1.json create mode 100644 resources/test/hugr-2.json create mode 100644 resources/test/hugr-3.json 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/src/hugr.rs b/hugr-core/src/hugr.rs index 978b575c5..c039d58e2 100644 --- a/hugr-core/src/hugr.rs +++ b/hugr-core/src/hugr.rs @@ -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] @@ -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(), + "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"; diff --git a/hugr-core/src/hugr/validate.rs b/hugr-core/src/hugr/validate.rs index c36169134..2318ed7e0 100644 --- a/hugr-core/src/hugr/validate.rs +++ b/hugr-core/src/hugr/validate.rs @@ -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}; @@ -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. + if let OpType::Const(c) = op_type { + c.validate()?; + }; + Ok(()) } @@ -759,6 +766,9 @@ pub enum ValidationError { /// [Opaque]: crate::ops::CustomOp::Opaque #[error(transparent)] CustomOpError(#[from] CustomOpError), + /// TODO + #[error(transparent)] + ConstTypeError(#[from] ConstTypeError), } /// Errors related to the inter-graph edge validations. 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) + }; +} 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()], 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)]), ], ) diff --git a/resources/test/hugr-0.json b/resources/test/hugr-0.json new file mode 100644 index 000000000..286e9114e --- /dev/null +++ b/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/resources/test/hugr-1.json b/resources/test/hugr-1.json new file mode 100644 index 000000000..c7622c8d4 --- /dev/null +++ b/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/resources/test/hugr-2.json b/resources/test/hugr-2.json new file mode 100644 index 000000000..d133c8870 --- /dev/null +++ b/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/resources/test/hugr-3.json b/resources/test/hugr-3.json new file mode 100644 index 000000000..66e537713 --- /dev/null +++ b/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 +}