From f523fc9af43d0ed1668d5c9cbcf1d9bfa34a41dc Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Tue, 7 May 2024 17:17:55 +0100 Subject: [PATCH 01/13] opdef roundtrips --- hugr/src/extension.rs | 3 +- hugr/src/extension/op_def.rs | 143 +++++++++++++++++++++++++++++++- hugr/src/hugr/serialize/test.rs | 15 +++- hugr/src/ops/constant/custom.rs | 34 ++++++++ hugr/src/proptest.rs | 4 + 5 files changed, 192 insertions(+), 7 deletions(-) diff --git a/hugr/src/extension.rs b/hugr/src/extension.rs index ebae773ec..85a612852 100644 --- a/hugr/src/extension.rs +++ b/hugr/src/extension.rs @@ -535,7 +535,8 @@ impl FromIterator for ExtensionSet { } #[cfg(test)] -mod test { +pub mod test { + pub use super::op_def::test::SimpleOpDef; mod proptest { diff --git a/hugr/src/extension/op_def.rs b/hugr/src/extension/op_def.rs index 91a49cfa5..951274f02 100644 --- a/hugr/src/extension/op_def.rs +++ b/hugr/src/extension/op_def.rs @@ -471,12 +471,14 @@ impl Extension { } #[cfg(test)] -mod test { +pub mod test { use std::num::NonZeroU64; + use itertools::Itertools; + use super::SignatureFromArgs; use crate::builder::{DFGBuilder, Dataflow, DataflowHugr}; - use crate::extension::op_def::LowerFunc; + use crate::extension::op_def::{CustomValidator, LowerFunc, OpDef, SignatureFunc}; use crate::extension::prelude::USIZE_T; use crate::extension::{ExtensionRegistry, ExtensionSet, PRELUDE}; use crate::extension::{SignatureError, EMPTY_REG, PRELUDE_REGISTRY}; @@ -490,6 +492,65 @@ mod test { const EXT_ID: ExtensionId = "MyExt"; } + #[derive(serde::Serialize, serde::Deserialize, Debug)] + pub struct SimpleOpDef(OpDef); + + impl PartialEq for SimpleOpDef { + fn eq(&self, other: &Self) -> bool { + let OpDef { + extension, + name, + description, + misc, + signature_func, + lower_funcs, + constant_folder, + } = &self.0; + let OpDef { + extension: other_extension, + name: other_name, + description: other_description, + misc: other_misc, + signature_func: other_signature_func, + lower_funcs: other_lower_funcs, + constant_folder: other_constant_folder, + } = &other.0; + + let get_sig = |sf: &_| match sf { + // if SignatureFunc or CustomValidator are changed we should get + // an error here, update do validate the parts of the heirarchy that + // are changed. + SignatureFunc::TypeScheme(CustomValidator { + poly_func, + validate: _, + }) => Some(poly_func.clone()), + SignatureFunc::CustomFunc(_) => None, + }; + + let get_lower_funcs = |lfs: &Vec| { + lfs.iter() + .map(|lf| match lf { + // as with get_sig above, this should break if the heirarchy + // is changed, update similarly. + LowerFunc::FixedHugr { extensions, hugr } => { + Some((extensions.clone(), hugr.clone())) + } + LowerFunc::CustomFunc(_) => None, + }) + .collect_vec() + }; + + extension == other_extension + && name == other_name + && description == other_description + && misc == other_misc + && get_sig(signature_func) == get_sig(other_signature_func) + && get_lower_funcs(lower_funcs) == get_lower_funcs(other_lower_funcs) + && constant_folder.is_none() + && other_constant_folder.is_none() + } + } + #[test] fn op_def_with_type_scheme() -> Result<(), Box> { let list_def = EXTENSION.get_type(&LIST_TYPENAME).unwrap(); @@ -671,4 +732,82 @@ mod test { ); Ok(()) } + + #[cfg(feature = "proptest")] + mod proptest { + use super::SimpleOpDef; + use ::proptest::prelude::*; + + use crate::{ + builder::test::simple_dfg_hugr, + extension::{ + op_def::LowerFunc, CustomValidator, ExtensionId, ExtensionSet, OpDef, SignatureFunc, + }, + types::PolyFuncType, + }; + + impl Arbitrary for SignatureFunc { + type Parameters = (); + type Strategy = BoxedStrategy; + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + // TODO there is also SignatureFunc::CustomFunc, but for now + // this is not serialised. When it is, we should generate + // examples here . + any::() + .prop_map(|x| SignatureFunc::TypeScheme(CustomValidator::from_polyfunc(x))) + .boxed() + } + } + + impl Arbitrary for LowerFunc { + type Parameters = (); + type Strategy = BoxedStrategy; + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + // TODO There is also LowerFunc::CustomFunc, but for now this is + // not serialised. When it is, we should generate examples here. + any::() + .prop_map(|extensions| LowerFunc::FixedHugr { + extensions, + hugr: simple_dfg_hugr(), + }) + .boxed() + } + } + + impl Arbitrary for SimpleOpDef { + type Parameters = (); + type Strategy = BoxedStrategy; + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + use crate::proptest::{any_serde_yaml_value, any_smolstr, any_string}; + use proptest::collection::{hash_map, vec}; + let signature_func: BoxedStrategy = any::(); + let lower_funcs: BoxedStrategy = any::(); + let misc = hash_map(any_string(), any_serde_yaml_value(), 0..3); + ( + any::(), + any_smolstr(), + any_string(), + misc, + signature_func, + vec(lower_funcs, 0..2), + ) + .prop_map( + |(extension, name, description, misc, signature_func, lower_funcs)| { + Self(OpDef { + extension, + name, + description, + misc, + signature_func, + lower_funcs, + // TODO ``constant_folder` is not serialised, we should + // generate examples once it is. + constant_folder: None, + }) + }, + ) + .boxed() + } + } + } } diff --git a/hugr/src/hugr/serialize/test.rs b/hugr/src/hugr/serialize/test.rs index 12bfdb98f..a542bb50d 100644 --- a/hugr/src/hugr/serialize/test.rs +++ b/hugr/src/hugr/serialize/test.rs @@ -5,7 +5,7 @@ use crate::builder::{ }; use crate::extension::prelude::{BOOL_T, PRELUDE_ID, QB_T, USIZE_T}; use crate::extension::simple_op::MakeRegisteredOp; -use crate::extension::{EMPTY_REG, PRELUDE_REGISTRY}; +use crate::extension::{test::SimpleOpDef, EMPTY_REG, PRELUDE_REGISTRY}; use crate::hugr::hugrmut::sealed::HugrMutInternals; use crate::ops::custom::{ExtensionOp, OpaqueOp}; use crate::ops::{self, dataflow::IOTrait, Input, Module, Noop, Output, Value, DFG}; @@ -37,6 +37,7 @@ struct SerTestingV1 { poly_func_type: Option, value: Option, optype: Option, + op_def: Option, } type TestingModel = SerTestingV1; @@ -91,6 +92,7 @@ impl_sertesting_from!(crate::types::SumType, sum_type); impl_sertesting_from!(crate::types::PolyFuncType, poly_func_type); impl_sertesting_from!(crate::ops::Value, value); impl_sertesting_from!(NodeSer, optype); +impl_sertesting_from!(SimpleOpDef, op_def); #[test] fn empty_hugr_serialize() { @@ -451,8 +453,8 @@ fn roundtrip_optype(#[case] optype: impl Into + std::fmt::Debug) { } mod proptest { - use super::super::NodeSer; use super::check_testing_roundtrip; + use super::{NodeSer, SimpleOpDef}; use crate::extension::ExtensionSet; use crate::ops::{OpType, Value}; use crate::types::{PolyFuncType, Type}; @@ -493,8 +495,13 @@ mod proptest { } #[test] - fn prop_roundtrip_optype(ns: NodeSer) { - check_testing_roundtrip(ns) + fn prop_roundtrip_optype(op: NodeSer ) { + check_testing_roundtrip(op) + } + + #[test] + fn prop_roundtrip_opdef(opdef: SimpleOpDef) { + check_testing_roundtrip(opdef) } } } diff --git a/hugr/src/ops/constant/custom.rs b/hugr/src/ops/constant/custom.rs index 54a9af514..9ea37ec1a 100644 --- a/hugr/src/ops/constant/custom.rs +++ b/hugr/src/ops/constant/custom.rs @@ -476,4 +476,38 @@ mod test { serde_yaml::from_value(serde_yaml::to_value(&ev).unwrap()).unwrap() ); } + + #[cfg(feature = "proptest")] + mod proptest { + use ::proptest::prelude::*; + + use crate::{ + extension::ExtensionSet, + ops::constant::CustomSerialized, + proptest::{any_serde_yaml_value, any_string}, + types::Type, + }; + + impl Arbitrary for CustomSerialized { + type Parameters = (); + type Strategy = BoxedStrategy; + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + let typ = any::(); + let extensions = any::(); + let value = (any_serde_yaml_value(), any_string()).prop_map(|(value, c)| { + [("c".into(), c.into()), ("v".into(), value)] + .into_iter() + .collect::() + .into() + }); + (typ, value, extensions) + .prop_map(|(typ, value, extensions)| CustomSerialized { + typ, + value, + extensions, + }) + .boxed() + } + } + } } diff --git a/hugr/src/proptest.rs b/hugr/src/proptest.rs index 37bf46516..344b32a70 100644 --- a/hugr/src/proptest.rs +++ b/hugr/src/proptest.rs @@ -151,6 +151,10 @@ pub fn any_string() -> SBoxedStrategy { ANY_STRING.to_owned() } +pub fn any_smolstr() -> SBoxedStrategy { + ANY_STRING.clone().prop_map_into().sboxed() +} + pub fn any_serde_yaml_value() -> impl Strategy { // use serde_yaml::value::{Tag, TaggedValue, Value}; ANY_SERDE_YAML_VALUE_LEAF From 1011e7a6f493a816b2727f518970a95c18d69439 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Tue, 14 May 2024 16:23:57 +0100 Subject: [PATCH 02/13] Add missing CustomSerialized generator --- hugr/src/ops/constant.rs | 59 +++++++------------ .../std_extensions/arithmetic/int_types.rs | 26 ++++++++ 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/hugr/src/ops/constant.rs b/hugr/src/ops/constant.rs index 551415351..556088c88 100644 --- a/hugr/src/ops/constant.rs +++ b/hugr/src/ops/constant.rs @@ -634,51 +634,34 @@ mod test { mod proptest { use super::super::OpaqueValue; use crate::{ - ops::Value, - std_extensions::arithmetic::int_types::{ConstInt, LOG_WIDTH_MAX}, + ops::{constant::CustomSerialized, Value}, + std_extensions::arithmetic::int_types::ConstInt, std_extensions::collections::ListValue, types::{SumType, Type}, }; - use ::proptest::prelude::*; + use ::proptest::{collection::vec, prelude::*}; impl Arbitrary for OpaqueValue { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - use proptest::collection::vec; - let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { - use i64; - let max_val = (2u64.pow(log_width as u32) / 2) as i64; - let min_val = -max_val - 1; - (min_val..=max_val).prop_map(move |v| { - OpaqueValue::new( - ConstInt::new_s(log_width, v).expect("guaranteed to be in bounds"), - ) - }) - }); - let unsigned_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { - (0..2u64.pow(log_width as u32)).prop_map(move |v| { - OpaqueValue::new( - ConstInt::new_u(log_width, v).expect("guaranteed to be in bounds"), - ) - }) - }); - prop_oneof![unsigned_strat, signed_strat] - .prop_recursive( - 3, // No more than 3 branch levels deep - 32, // Target around 32 total elements - 3, // Each collection is up to 3 elements long - |element| { - (any::(), vec(element.clone(), 0..3)).prop_map( - |(typ, contents)| { - OpaqueValue::new(ListValue::new( - typ, - contents.into_iter().map(|e| Value::Extension { e }), - )) - }, - ) - }, - ) - .boxed() + prop_oneof![ + any::().prop_map_into(), + any::().prop_map_into() + ] + .prop_recursive( + 3, // No more than 3 branch levels deep + 32, // Target around 32 total elements + 3, // Each collection is up to 3 elements long + |element| { + (any::(), vec(element.clone(), 0..3)).prop_map(|(typ, contents)| { + Self::new(ListValue::new( + typ, + contents.into_iter().map(|e| Value::Extension { e }), + )) + }) + }, + ) + .boxed() } } diff --git a/hugr/src/std_extensions/arithmetic/int_types.rs b/hugr/src/std_extensions/arithmetic/int_types.rs index 7b92f82a6..8e2a7e370 100644 --- a/hugr/src/std_extensions/arithmetic/int_types.rs +++ b/hugr/src/std_extensions/arithmetic/int_types.rs @@ -272,4 +272,30 @@ mod test { ConstInt::new_s(50, -2).unwrap_err(); ConstInt::new_u(50, 2).unwrap_err(); } + + #[cfg(feature = "proptest")] + mod proptest { + use super::{ConstInt, LOG_WIDTH_MAX}; + use ::proptest::prelude::*; + impl Arbitrary for ConstInt { + type Parameters = (); + type Strategy = BoxedStrategy; + fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { + use i64; + let max_val = (2u64.pow(log_width as u32) / 2) as i64; + let min_val = -max_val - 1; + (min_val..=max_val).prop_map(move |v| { + Self::new_s(log_width, v).expect("guaranteed to be in bounds") + }) + }); + let unsigned_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { + (0..2u64.pow(log_width as u32)).prop_map(move |v| { + ConstInt::new_u(log_width, v).expect("guaranteed to be in bounds") + }) + }); + prop_oneof![unsigned_strat, signed_strat].boxed() + } + } + } } From 7418e37f31b2c9890dc45828cad5382badebe872 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Wed, 15 May 2024 07:52:11 +0100 Subject: [PATCH 03/13] wip --- hugr/src/extension/op_def.rs | 7 ++++--- hugr/src/ops/constant.rs | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hugr/src/extension/op_def.rs b/hugr/src/extension/op_def.rs index 951274f02..606dc1e32 100644 --- a/hugr/src/extension/op_def.rs +++ b/hugr/src/extension/op_def.rs @@ -518,8 +518,9 @@ pub mod test { let get_sig = |sf: &_| match sf { // if SignatureFunc or CustomValidator are changed we should get - // an error here, update do validate the parts of the heirarchy that - // are changed. + // an error here. To fix: modify the fields matched on here, + // maintaining the lack of `..` and, for each part that is + // serializable, ensure we are checking it for equality below. SignatureFunc::TypeScheme(CustomValidator { poly_func, validate: _, @@ -530,7 +531,7 @@ pub mod test { let get_lower_funcs = |lfs: &Vec| { lfs.iter() .map(|lf| match lf { - // as with get_sig above, this should break if the heirarchy + // as with get_sig above, this should break if the hierarchy // is changed, update similarly. LowerFunc::FixedHugr { extensions, hugr } => { Some((extensions.clone(), hugr.clone())) diff --git a/hugr/src/ops/constant.rs b/hugr/src/ops/constant.rs index 556088c88..b222c75ad 100644 --- a/hugr/src/ops/constant.rs +++ b/hugr/src/ops/constant.rs @@ -646,6 +646,8 @@ mod test { fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { prop_oneof![ any::().prop_map_into(), + // any::()>.prop_map_into() + any::().prop_map_into() ] .prop_recursive( From 6e425d8ec039371d4bad0fd1f3594e4252bc2347 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Thu, 16 May 2024 11:56:41 +0100 Subject: [PATCH 04/13] wip --- hugr/src/extension.rs | 1 + hugr/src/extension/op_def.rs | 8 ++++++- hugr/src/hugr/serialize/test.rs | 1 - hugr/src/ops/constant.rs | 4 ++-- hugr/src/ops/constant/custom.rs | 1 - hugr/src/ops/custom.rs | 23 +++++++------------ .../std_extensions/arithmetic/int_types.rs | 1 - 7 files changed, 18 insertions(+), 21 deletions(-) diff --git a/hugr/src/extension.rs b/hugr/src/extension.rs index 85a612852..704ccfda0 100644 --- a/hugr/src/extension.rs +++ b/hugr/src/extension.rs @@ -547,6 +547,7 @@ pub mod test { impl Arbitrary for ExtensionSet { type Parameters = (); type Strategy = BoxedStrategy; + fn arbitrary_with(_: Self::Parameters) -> Self::Strategy { ( hash_set(0..10usize, 0..3), diff --git a/hugr/src/extension/op_def.rs b/hugr/src/extension/op_def.rs index 606dc1e32..2253bdd72 100644 --- a/hugr/src/extension/op_def.rs +++ b/hugr/src/extension/op_def.rs @@ -495,6 +495,12 @@ pub mod test { #[derive(serde::Serialize, serde::Deserialize, Debug)] pub struct SimpleOpDef(OpDef); + impl From for OpDef { + fn from(value: SimpleOpDef) -> Self { + value.0 + } + } + impl PartialEq for SimpleOpDef { fn eq(&self, other: &Self) -> bool { let OpDef { @@ -547,6 +553,7 @@ pub mod test { && misc == other_misc && get_sig(signature_func) == get_sig(other_signature_func) && get_lower_funcs(lower_funcs) == get_lower_funcs(other_lower_funcs) + // If either constant folder not none, then we are not equal && constant_folder.is_none() && other_constant_folder.is_none() } @@ -734,7 +741,6 @@ pub mod test { Ok(()) } - #[cfg(feature = "proptest")] mod proptest { use super::SimpleOpDef; use ::proptest::prelude::*; diff --git a/hugr/src/hugr/serialize/test.rs b/hugr/src/hugr/serialize/test.rs index a542bb50d..974cc120b 100644 --- a/hugr/src/hugr/serialize/test.rs +++ b/hugr/src/hugr/serialize/test.rs @@ -29,7 +29,6 @@ const NAT: Type = crate::extension::prelude::USIZE_T; const QB: Type = crate::extension::prelude::QB_T; /// Version 1 of the Testing HUGR serialisation format, see `testing_hugr.py`. -#[cfg(test)] #[derive(Serialize, Deserialize, PartialEq, Debug, Default)] struct SerTestingV1 { typ: Option, diff --git a/hugr/src/ops/constant.rs b/hugr/src/ops/constant.rs index b222c75ad..dbad621ef 100644 --- a/hugr/src/ops/constant.rs +++ b/hugr/src/ops/constant.rs @@ -644,10 +644,10 @@ mod test { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { + // We intentionally do not include `ConstF4` because it does not + // roundtrip serialise prop_oneof![ any::().prop_map_into(), - // any::()>.prop_map_into() - any::().prop_map_into() ] .prop_recursive( diff --git a/hugr/src/ops/constant/custom.rs b/hugr/src/ops/constant/custom.rs index 9ea37ec1a..8632f2c52 100644 --- a/hugr/src/ops/constant/custom.rs +++ b/hugr/src/ops/constant/custom.rs @@ -477,7 +477,6 @@ mod test { ); } - #[cfg(feature = "proptest")] mod proptest { use ::proptest::prelude::*; diff --git a/hugr/src/ops/custom.rs b/hugr/src/ops/custom.rs index 31248385f..4c12929ce 100644 --- a/hugr/src/ops/custom.rs +++ b/hugr/src/ops/custom.rs @@ -4,7 +4,9 @@ use std::sync::Arc; use thiserror::Error; #[cfg(test)] use { + crate::extension::test::SimpleOpDef, crate::proptest::{any_nonempty_smolstr, any_nonempty_string}, + ::proptest::prelude::*, ::proptest_derive::Arbitrary, }; @@ -28,6 +30,7 @@ use super::{NamedOp, OpName, OpNameRef, OpTrait, OpType}; /// [`OpaqueOp`]: crate::ops::custom::OpaqueOp /// [`ExtensionOp`]: crate::ops::custom::ExtensionOp #[derive(Clone, Debug, Eq, serde::Serialize, serde::Deserialize)] +#[cfg_attr(test, derive(Arbitrary))] #[serde(into = "OpaqueOp", from = "OpaqueOp")] pub enum CustomOp { /// When we've found (loaded) the [Extension] definition and identified the [OpDef] @@ -170,9 +173,12 @@ impl From for CustomOp { /// /// [Extension]: crate::Extension #[derive(Clone, Debug)] -// TODO when we can geneerate `OpDef`s enable this -// #[cfg_attr(test, derive(proptest_derive::Arbitrary))] +#[cfg_attr(test, derive(Arbitrary))] pub struct ExtensionOp { + #[cfg_attr( + test, + proptest(strategy = "any::().prop_map(|x| Arc::new(x.into()))") + )] def: Arc, args: Vec, signature: FunctionType, // Cache @@ -446,17 +452,4 @@ mod test { assert!(op.is_opaque()); assert!(!op.is_extension_op()); } - - mod proptest { - use ::proptest::prelude::*; - - impl Arbitrary for super::super::CustomOp { - type Parameters = (); - type Strategy = BoxedStrategy; - fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - // TODO when we can geneerate `OpDef`s add an `ExtensionOp` case here - any::().prop_map_into().boxed() - } - } - } } diff --git a/hugr/src/std_extensions/arithmetic/int_types.rs b/hugr/src/std_extensions/arithmetic/int_types.rs index 8e2a7e370..3cfff8cec 100644 --- a/hugr/src/std_extensions/arithmetic/int_types.rs +++ b/hugr/src/std_extensions/arithmetic/int_types.rs @@ -273,7 +273,6 @@ mod test { ConstInt::new_u(50, 2).unwrap_err(); } - #[cfg(feature = "proptest")] mod proptest { use super::{ConstInt, LOG_WIDTH_MAX}; use ::proptest::prelude::*; From 3d9dbb080d4b02a922d8a9573ba5ecfc8e776d49 Mon Sep 17 00:00:00 2001 From: doug-q <141026920+doug-q@users.noreply.github.com> Date: Wed, 29 May 2024 13:11:54 +0100 Subject: [PATCH 05/13] Update hugr-core/src/extension/op_def.rs Co-authored-by: Alan Lawrence --- hugr-core/src/extension/op_def.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hugr-core/src/extension/op_def.rs b/hugr-core/src/extension/op_def.rs index bfa5e7036..f6890edbe 100644 --- a/hugr-core/src/extension/op_def.rs +++ b/hugr-core/src/extension/op_def.rs @@ -528,7 +528,7 @@ pub mod test { let get_sig = |sf: &_| match sf { // if SignatureFunc or CustomValidator are changed we should get - // an error here. To fix: modify the fields matched on here, + // a compile error here. To fix: modify the fields matched on here, // maintaining the lack of `..` and, for each part that is // serializable, ensure we are checking it for equality below. SignatureFunc::TypeScheme(CustomValidator { From d9687e71655a99fe214dd9841e16faff03609e32 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Wed, 29 May 2024 13:35:02 +0100 Subject: [PATCH 06/13] wip --- hugr-core/src/extension.rs | 1 + hugr-core/src/extension/op_def.rs | 6 ++---- hugr-core/src/ops/constant.rs | 8 ++++---- hugr-core/src/std_extensions/arithmetic/int_types.rs | 6 ++++-- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/hugr-core/src/extension.rs b/hugr-core/src/extension.rs index d06dd0403..1ef9c50cd 100644 --- a/hugr-core/src/extension.rs +++ b/hugr-core/src/extension.rs @@ -539,6 +539,7 @@ impl FromIterator for ExtensionSet { #[cfg(test)] pub mod test { + // We re-export this here because mod op_def is private. pub use super::op_def::test::SimpleOpDef; mod proptest { diff --git a/hugr-core/src/extension/op_def.rs b/hugr-core/src/extension/op_def.rs index f6890edbe..28479de4b 100644 --- a/hugr-core/src/extension/op_def.rs +++ b/hugr-core/src/extension/op_def.rs @@ -802,16 +802,14 @@ pub mod test { fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { use crate::proptest::{any_serde_yaml_value, any_smolstr, any_string}; use proptest::collection::{hash_map, vec}; - let signature_func: BoxedStrategy = any::(); - let lower_funcs: BoxedStrategy = any::(); let misc = hash_map(any_string(), any_serde_yaml_value(), 0..3); ( any::(), any_smolstr(), any_string(), misc, - signature_func, - vec(lower_funcs, 0..2), + any::(), + vec(any::(), 0..2), ) .prop_map( |(extension, name, description, misc, signature_func, lower_funcs)| { diff --git a/hugr-core/src/ops/constant.rs b/hugr-core/src/ops/constant.rs index 0a5a1ed3f..d18908c9a 100644 --- a/hugr-core/src/ops/constant.rs +++ b/hugr-core/src/ops/constant.rs @@ -655,12 +655,12 @@ mod test { 3, // No more than 3 branch levels deep 32, // Target around 32 total elements 3, // Each collection is up to 3 elements long - |element| { - (Type::any_non_row_var(), vec(element.clone(), 0..3)).prop_map( - |(typ, contents)| { + |child_strat| { + (Type::any_non_row_var(), vec(child_strat, 0..3)).prop_map( + |(typ, children)| { Self::new(ListValue::new( typ, - contents.into_iter().map(|e| Value::Extension { e }), + children.into_iter().map(|e| Value::Extension { e }), )) }, ) diff --git a/hugr-core/src/std_extensions/arithmetic/int_types.rs b/hugr-core/src/std_extensions/arithmetic/int_types.rs index 489d45e93..0b92ffdd2 100644 --- a/hugr-core/src/std_extensions/arithmetic/int_types.rs +++ b/hugr-core/src/std_extensions/arithmetic/int_types.rs @@ -284,9 +284,11 @@ mod test { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { + // write a test case for this: + Let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { use i64; - let max_val = (2u64.pow(log_width as u32) / 2) as i64; + let width = 2u64.pow(log_width as u32); + let max_val = ((1u64 << (width - 1)) - 1u64) as i64; let min_val = -max_val - 1; (min_val..=max_val).prop_map(move |v| { Self::new_s(log_width, v).expect("guaranteed to be in bounds") From be8e40829e2e0806bae785015c2c340b1c4bd468 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Wed, 29 May 2024 15:37:45 +0100 Subject: [PATCH 07/13] add a test for valid signed integers --- .../std_extensions/arithmetic/int_types.rs | 41 +++++++++++++++++-- 1 file changed, 38 insertions(+), 3 deletions(-) diff --git a/hugr-core/src/std_extensions/arithmetic/int_types.rs b/hugr-core/src/std_extensions/arithmetic/int_types.rs index 0b92ffdd2..26ef7f064 100644 --- a/hugr-core/src/std_extensions/arithmetic/int_types.rs +++ b/hugr-core/src/std_extensions/arithmetic/int_types.rs @@ -278,14 +278,13 @@ mod test { } mod proptest { - use super::{ConstInt, LOG_WIDTH_MAX}; + use super::{is_valid_log_width, ConstInt, LOG_WIDTH_MAX}; use ::proptest::prelude::*; impl Arbitrary for ConstInt { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - // write a test case for this: - Let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { + let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { use i64; let width = 2u64.pow(log_width as u32); let max_val = ((1u64 << (width - 1)) - 1u64) as i64; @@ -299,8 +298,44 @@ mod test { ConstInt::new_u(log_width, v).expect("guaranteed to be in bounds") }) }); + prop_oneof![unsigned_strat, signed_strat].boxed() } } + + fn any_signed_int_with_log_width() -> impl Strategy { + (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { + use i64; + let width = 2u64.pow(log_width as u32); + let max_val = ((1u64 << (width - 1)) - 1u64) as i64; + let min_val = -max_val - 1; + prop_oneof![ + (min_val..=max_val), + Just(min_val), + Just(max_val) + ].prop_map(move |x| (log_width,x as i128)) + }) + } + + proptest! { + #[test] + fn valid_signed_int((log_width, x) in any_signed_int_with_log_width()) { + let (min,max): (i128,i128) = match log_width { + 0 => (-1, 0), + 1 => (-2, 1), + 2 => (-8, 7), + 3 => (i8::MIN as i128, i8::MAX as i128), + 4 => (i16::MIN as i128, i16::MAX as i128), + 5 => (i32::MIN as i128, i32::MAX as i128), + 6 => (i64::MIN as i128, i64::MAX as i128), + _ => unreachable!(), + }; + let width = (2i128.pow(log_width as u32)); + prop_assert_eq!(max - min + 1, 1 << width); + prop_assert!(x >= min); + prop_assert!(x <= max); + prop_assert!(ConstInt::new_s(log_width, x as i64).is_ok()) + } + } } } From 96dc36bed0aaedc7bbccc6a5c9e10d3d7bb51456 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Wed, 29 May 2024 15:46:37 +0100 Subject: [PATCH 08/13] lints + format --- hugr-core/src/extension/op_def.rs | 26 ++++++++++++++----- .../std_extensions/arithmetic/int_types.rs | 13 ++++------ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/hugr-core/src/extension/op_def.rs b/hugr-core/src/extension/op_def.rs index 28479de4b..34b202d71 100644 --- a/hugr-core/src/extension/op_def.rs +++ b/hugr-core/src/extension/op_def.rs @@ -499,6 +499,21 @@ pub mod test { #[derive(serde::Serialize, serde::Deserialize, Debug)] pub struct SimpleOpDef(OpDef); + impl SimpleOpDef { + pub fn new(op_def: OpDef) -> Self { + assert!(op_def.constant_folder.is_none()); + assert!(matches!( + op_def.signature_func, + SignatureFunc::TypeScheme(_) + )); + assert!(op_def + .lower_funcs + .iter() + .all(|lf| matches!(lf, LowerFunc::FixedHugr { .. }))); + Self(op_def) + } + } + impl From for OpDef { fn from(value: SimpleOpDef) -> Self { value.0 @@ -514,7 +529,7 @@ pub mod test { misc, signature_func, lower_funcs, - constant_folder, + constant_folder: _, } = &self.0; let OpDef { extension: other_extension, @@ -523,7 +538,7 @@ pub mod test { misc: other_misc, signature_func: other_signature_func, lower_funcs: other_lower_funcs, - constant_folder: other_constant_folder, + constant_folder: _, } = &other.0; let get_sig = |sf: &_| match sf { @@ -535,6 +550,7 @@ pub mod test { poly_func, validate: _, }) => Some(poly_func.clone()), + // This is ruled out by `new()` but leave it here for later. SignatureFunc::CustomFunc(_) => None, }; @@ -546,6 +562,7 @@ pub mod test { LowerFunc::FixedHugr { extensions, hugr } => { Some((extensions.clone(), hugr.clone())) } + // This is ruled out by `new()` but leave it here for later. LowerFunc::CustomFunc(_) => None, }) .collect_vec() @@ -557,9 +574,6 @@ pub mod test { && misc == other_misc && get_sig(signature_func) == get_sig(other_signature_func) && get_lower_funcs(lower_funcs) == get_lower_funcs(other_lower_funcs) - // If either constant folder not none, then we are not equal - && constant_folder.is_none() - && other_constant_folder.is_none() } } @@ -813,7 +827,7 @@ pub mod test { ) .prop_map( |(extension, name, description, misc, signature_func, lower_funcs)| { - Self(OpDef { + Self::new(OpDef { extension, name, description, diff --git a/hugr-core/src/std_extensions/arithmetic/int_types.rs b/hugr-core/src/std_extensions/arithmetic/int_types.rs index 26ef7f064..5e051b9b8 100644 --- a/hugr-core/src/std_extensions/arithmetic/int_types.rs +++ b/hugr-core/src/std_extensions/arithmetic/int_types.rs @@ -278,7 +278,7 @@ mod test { } mod proptest { - use super::{is_valid_log_width, ConstInt, LOG_WIDTH_MAX}; + use super::{ConstInt, LOG_WIDTH_MAX}; use ::proptest::prelude::*; impl Arbitrary for ConstInt { type Parameters = (); @@ -303,17 +303,14 @@ mod test { } } - fn any_signed_int_with_log_width() -> impl Strategy { + fn any_signed_int_with_log_width() -> impl Strategy { (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { use i64; let width = 2u64.pow(log_width as u32); let max_val = ((1u64 << (width - 1)) - 1u64) as i64; let min_val = -max_val - 1; - prop_oneof![ - (min_val..=max_val), - Just(min_val), - Just(max_val) - ].prop_map(move |x| (log_width,x as i128)) + prop_oneof![(min_val..=max_val), Just(min_val), Just(max_val)] + .prop_map(move |x| (log_width, x as i128)) }) } @@ -330,7 +327,7 @@ mod test { 6 => (i64::MIN as i128, i64::MAX as i128), _ => unreachable!(), }; - let width = (2i128.pow(log_width as u32)); + let width = 2i128.pow(log_width as u32); prop_assert_eq!(max - min + 1, 1 << width); prop_assert!(x >= min); prop_assert!(x <= max); From 5756599e78f18ada0e42cbc0acabb09306098f74 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Wed, 29 May 2024 15:58:37 +0100 Subject: [PATCH 09/13] add comment --- hugr-core/src/ops/constant/custom.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hugr-core/src/ops/constant/custom.rs b/hugr-core/src/ops/constant/custom.rs index 8632f2c52..ff630253e 100644 --- a/hugr-core/src/ops/constant/custom.rs +++ b/hugr-core/src/ops/constant/custom.rs @@ -493,6 +493,10 @@ mod test { fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { let typ = any::(); let extensions = any::(); + // here we manually construct a serialized `dyn CustomConst`. + // The "c" and "v" come from the `typetag::serde` annotation on + // `trait CustomConst`. This is not ideal, if we were to randomly + // generate "ConstInt" things will go wrong. let value = (any_serde_yaml_value(), any_string()).prop_map(|(value, c)| { [("c".into(), c.into()), ("v".into(), value)] .into_iter() From 5e2250142169598ddfab2c25541d04bf58353432 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Thu, 30 May 2024 09:02:29 +0100 Subject: [PATCH 10/13] tweak --- hugr-core/src/ops/constant/custom.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hugr-core/src/ops/constant/custom.rs b/hugr-core/src/ops/constant/custom.rs index ff630253e..86cadb27d 100644 --- a/hugr-core/src/ops/constant/custom.rs +++ b/hugr-core/src/ops/constant/custom.rs @@ -495,10 +495,12 @@ mod test { let extensions = any::(); // here we manually construct a serialized `dyn CustomConst`. // The "c" and "v" come from the `typetag::serde` annotation on - // `trait CustomConst`. This is not ideal, if we were to randomly - // generate "ConstInt" things will go wrong. - let value = (any_serde_yaml_value(), any_string()).prop_map(|(value, c)| { - [("c".into(), c.into()), ("v".into(), value)] + // `trait CustomConst`. + // TODO This is not ideal, if we were to randomly + // generate a valid tag(e.g. "c" = "ConstInt") then things will + // go wrong. + let value = (any_serde_yaml_value(), any_string()).prop_map(|(content, tag)| { + [("c".into(), tag.into()), ("v".into(), content)] .into_iter() .collect::() .into() From 54b100edaaef877716f94c1ba524a10030b324d5 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Thu, 30 May 2024 09:07:50 +0100 Subject: [PATCH 11/13] tweak test --- .../std_extensions/arithmetic/int_types.rs | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/hugr-core/src/std_extensions/arithmetic/int_types.rs b/hugr-core/src/std_extensions/arithmetic/int_types.rs index 5e051b9b8..5ff2abea3 100644 --- a/hugr-core/src/std_extensions/arithmetic/int_types.rs +++ b/hugr-core/src/std_extensions/arithmetic/int_types.rs @@ -280,18 +280,13 @@ mod test { mod proptest { use super::{ConstInt, LOG_WIDTH_MAX}; use ::proptest::prelude::*; + use i64; impl Arbitrary for ConstInt { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - let signed_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { - use i64; - let width = 2u64.pow(log_width as u32); - let max_val = ((1u64 << (width - 1)) - 1u64) as i64; - let min_val = -max_val - 1; - (min_val..=max_val).prop_map(move |v| { - Self::new_s(log_width, v).expect("guaranteed to be in bounds") - }) + let signed_strat = any_signed_int_with_log_width().prop_map(|(log_width, v)| { + ConstInt::new_s(log_width, v).expect("guaranteed to be in bounds") }); let unsigned_strat = (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { (0..2u64.pow(log_width as u32)).prop_map(move |v| { @@ -303,35 +298,39 @@ mod test { } } - fn any_signed_int_with_log_width() -> impl Strategy { + fn any_signed_int_with_log_width() -> impl Strategy { (..=LOG_WIDTH_MAX).prop_flat_map(|log_width| { - use i64; let width = 2u64.pow(log_width as u32); let max_val = ((1u64 << (width - 1)) - 1u64) as i64; let min_val = -max_val - 1; prop_oneof![(min_val..=max_val), Just(min_val), Just(max_val)] - .prop_map(move |x| (log_width, x as i128)) + .prop_map(move |x| (log_width, x)) }) } proptest! { #[test] fn valid_signed_int((log_width, x) in any_signed_int_with_log_width()) { - let (min,max): (i128,i128) = match log_width { + let (min,max) = match log_width { 0 => (-1, 0), 1 => (-2, 1), 2 => (-8, 7), - 3 => (i8::MIN as i128, i8::MAX as i128), - 4 => (i16::MIN as i128, i16::MAX as i128), - 5 => (i32::MIN as i128, i32::MAX as i128), - 6 => (i64::MIN as i128, i64::MAX as i128), + 3 => (i8::MIN as i64, i8::MAX as i64), + 4 => (i16::MIN as i64, i16::MAX as i64), + 5 => (i32::MIN as i64, i32::MAX as i64), + 6 => (i64::MIN, i64::MAX), _ => unreachable!(), }; - let width = 2i128.pow(log_width as u32); - prop_assert_eq!(max - min + 1, 1 << width); + let width = 2i64.pow(log_width as u32); + // the left hand side counts the number of valid values as follows: + // - use i128 to be able to hold the number of valid i64s + // - there are exactly `max` valid positive values; + // - there are exactly `-min` valid negative values; + // - there are exactly 1 zero values. + prop_assert_eq!((max as i128) - (min as i128) + 1, 1 << width); prop_assert!(x >= min); prop_assert!(x <= max); - prop_assert!(ConstInt::new_s(log_width, x as i64).is_ok()) + prop_assert!(ConstInt::new_s(log_width, x).is_ok()) } } } From 0f1af4cfdf68292dd5c6a85e0deeb5a3ad534e97 Mon Sep 17 00:00:00 2001 From: doug-q <141026920+doug-q@users.noreply.github.com> Date: Thu, 30 May 2024 09:27:43 +0100 Subject: [PATCH 12/13] Update hugr-core/src/ops/constant.rs Co-authored-by: Alan Lawrence --- hugr-core/src/ops/constant.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hugr-core/src/ops/constant.rs b/hugr-core/src/ops/constant.rs index d18908c9a..5f059f4f0 100644 --- a/hugr-core/src/ops/constant.rs +++ b/hugr-core/src/ops/constant.rs @@ -645,7 +645,7 @@ mod test { type Parameters = (); type Strategy = BoxedStrategy; fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy { - // We intentionally do not include `ConstF4` because it does not + // We intentionally do not include `ConstF64` because it does not // roundtrip serialise prop_oneof![ any::().prop_map_into(), From 30a6d05cf5037f673196b52c7714c55e3969e6d3 Mon Sep 17 00:00:00 2001 From: Douglas Wilson Date: Thu, 30 May 2024 09:29:44 +0100 Subject: [PATCH 13/13] tweak --- hugr-core/src/extension/op_def.rs | 2 +- hugr-core/src/ops/constant/custom.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/hugr-core/src/extension/op_def.rs b/hugr-core/src/extension/op_def.rs index 34b202d71..42edd56dc 100644 --- a/hugr-core/src/extension/op_def.rs +++ b/hugr-core/src/extension/op_def.rs @@ -474,7 +474,7 @@ impl Extension { } #[cfg(test)] -pub mod test { +pub(super) mod test { use std::num::NonZeroU64; use itertools::Itertools; diff --git a/hugr-core/src/ops/constant/custom.rs b/hugr-core/src/ops/constant/custom.rs index 86cadb27d..eb46a2bb7 100644 --- a/hugr-core/src/ops/constant/custom.rs +++ b/hugr-core/src/ops/constant/custom.rs @@ -496,9 +496,10 @@ mod test { // here we manually construct a serialized `dyn CustomConst`. // The "c" and "v" come from the `typetag::serde` annotation on // `trait CustomConst`. - // TODO This is not ideal, if we were to randomly - // generate a valid tag(e.g. "c" = "ConstInt") then things will - // go wrong. + // TODO This is not ideal, if we were to accidentally + // generate a valid tag(e.g. "ConstInt") then things will + // go wrong: the serde::Deserialize impl for that type will + // interpret "v" and fail. let value = (any_serde_yaml_value(), any_string()).prop_map(|(content, tag)| { [("c".into(), tag.into()), ("v".into(), content)] .into_iter()