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 <douglas.wilson@quantinuum.com>
CQCL · Jun 25, 2024 · c05edd3 · c05edd3
1 parent 4ef4826
commit c05edd3
Show file tree

Hide file tree

Showing 11 changed files with 239 additions and 4 deletions.
diff --git a/hugr-core/resources b/hugr-core/resources
@@ -0,0 +1 @@
+../resources/
diff --git 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
@@ -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
@@ -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
@@ -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>;
         }
     }
 }
@@ -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

diff --git 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<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()],

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
+}