From 830b6b6d0321c1496e2ed03c84138971b1726647 Mon Sep 17 00:00:00 2001 From: Seyon Sivarajah Date: Wed, 24 Jul 2024 15:29:51 +0100 Subject: [PATCH 1/2] refactor!: use JSON rather than YAML in opaque fields. Closes #1048 BREAKING_CHANGE: use `serde_json::Value` rather than `serde_yaml::Value` in `CustomSerialized`, `CustomTypeArg`, `OpDef`, `OperationDeclaration` --- hugr-core/src/extension/declarative/ops.rs | 2 +- hugr-core/src/extension/op_def.rs | 12 ++-- hugr-core/src/ops/constant.rs | 20 +++---- hugr-core/src/ops/constant/custom.rs | 64 +++++++++++----------- hugr-core/src/proptest.rs | 18 +++--- 5 files changed, 58 insertions(+), 58 deletions(-) diff --git a/hugr-core/src/extension/declarative/ops.rs b/hugr-core/src/extension/declarative/ops.rs index 13a32271a..8bd769e10 100644 --- a/hugr-core/src/extension/declarative/ops.rs +++ b/hugr-core/src/extension/declarative/ops.rs @@ -44,7 +44,7 @@ pub(super) struct OperationDeclaration { /// This data is kept in the Hugr, and may be accessed by the relevant runtime. #[serde(default)] #[serde(skip_serializing_if = "crate::utils::is_default")] - misc: HashMap, + misc: HashMap, /// A pre-compiled lowering routine. /// /// This is not yet supported, and will raise an error if present. diff --git a/hugr-core/src/extension/op_def.rs b/hugr-core/src/extension/op_def.rs index 61b6cd8b8..7babc084a 100644 --- a/hugr-core/src/extension/op_def.rs +++ b/hugr-core/src/extension/op_def.rs @@ -109,7 +109,7 @@ pub trait CustomLowerFunc: Send + Sync { &self, name: &OpNameRef, arg_values: &[TypeArg], - misc: &HashMap, + misc: &HashMap, available_extensions: &ExtensionSet, ) -> Option; } @@ -319,7 +319,7 @@ pub struct OpDef { description: String, /// Miscellaneous data associated with the operation. #[serde(default, skip_serializing_if = "HashMap::is_empty")] - misc: HashMap, + misc: HashMap, #[serde(flatten)] signature_func: SignatureFunc, @@ -434,8 +434,8 @@ impl OpDef { pub fn add_misc( &mut self, k: impl ToString, - v: serde_yaml::Value, - ) -> Option { + v: serde_json::Value, + ) -> Option { self.misc.insert(k.to_string(), v) } @@ -825,9 +825,9 @@ pub(super) mod test { 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 crate::proptest::{any_serde_json_value, any_smolstr, any_string}; use proptest::collection::{hash_map, vec}; - let misc = hash_map(any_string(), any_serde_yaml_value(), 0..3); + let misc = hash_map(any_string(), any_serde_json_value(), 0..3); ( any::(), any_smolstr(), diff --git a/hugr-core/src/ops/constant.rs b/hugr-core/src/ops/constant.rs index b14e64d3a..f2954c476 100644 --- a/hugr-core/src/ops/constant.rs +++ b/hugr-core/src/ops/constant.rs @@ -202,12 +202,12 @@ pub enum Value { /// serialization and deserialization of unknown impls of [CustomConst]. /// /// During serialization we first serialize the internal [`dyn` CustomConst](CustomConst) -/// into a [serde_yaml::Value]. We then create a [CustomSerialized] wrapping +/// into a [serde_json::Value]. We then create a [CustomSerialized] wrapping /// that value. That [CustomSerialized] is then serialized in place of the /// [OpaqueValue]. /// /// During deserialization, first we deserialize a [CustomSerialized]. We -/// attempt to deserialize the internal [serde_yaml::Value] using the [`Box`](CustomConst) impl. This will fail if the appropriate `impl CustomConst` /// is not linked into the running program, in which case we coerce the /// [CustomSerialized] into a [`Box`](CustomConst). The [OpaqueValue] is @@ -235,7 +235,7 @@ pub enum Value { /// assert_eq!(&serde_json::to_value(&ev).unwrap(), &expected_json); /// assert_eq!(ev, serde_json::from_value(expected_json).unwrap()); /// -/// let ev = OpaqueValue::new(CustomSerialized::new(USIZE_T.clone(), serde_yaml::Value::Null, ExtensionSet::default())); +/// let ev = OpaqueValue::new(CustomSerialized::new(USIZE_T.clone(), serde_json::Value::Null, ExtensionSet::default())); /// let expected_json = json!({ /// "extensions": [], /// "typ": USIZE_T, @@ -544,7 +544,6 @@ mod test { }; use cool_asserts::assert_matches; use rstest::{fixture, rstest}; - use serde_yaml::Value as YamlValue; use super::*; @@ -682,7 +681,7 @@ mod test { #[rstest] #[case(Value::unit(), Type::UNIT, "const:seq:{}")] #[case(const_usize(), USIZE_T, "const:custom:ConstUsize(")] - #[case(serialized_float(17.4), FLOAT64_TYPE, "const:custom:yaml:Mapping")] + #[case(serialized_float(17.4), FLOAT64_TYPE, "const:custom:json:Object")] #[case(const_tuple(), Type::new_tuple(type_row![USIZE_T, FLOAT64_TYPE]), "const:seq:{")] fn const_type( #[case] const_value: Value, @@ -709,7 +708,7 @@ mod test { } #[test] - fn test_yaml_const() { + fn test_json_const() { let ex_id: ExtensionId = "my_extension".try_into().unwrap(); let typ_int = CustomType::new( "my_type", @@ -717,16 +716,15 @@ mod test { ex_id.clone(), TypeBound::Eq, ); - let yaml_const: Value = - CustomSerialized::new(typ_int.clone(), YamlValue::Number(6.into()), ex_id.clone()) - .into(); + let json_const: Value = + CustomSerialized::new(typ_int.clone(), 6.into(), ex_id.clone()).into(); let classic_t = Type::new_extension(typ_int.clone()); assert_matches!(classic_t.least_upper_bound(), TypeBound::Eq); - assert_eq!(yaml_const.get_type(), classic_t); + assert_eq!(json_const.get_type(), classic_t); let typ_qb = CustomType::new("my_type", vec![], ex_id, TypeBound::Eq); let t = Type::new_extension(typ_qb.clone()); - assert_ne!(yaml_const.get_type(), t); + assert_ne!(json_const.get_type(), t); } mod proptest { diff --git a/hugr-core/src/ops/constant/custom.rs b/hugr-core/src/ops/constant/custom.rs index 3d6e36480..8ce00bec3 100644 --- a/hugr-core/src/ops/constant/custom.rs +++ b/hugr-core/src/ops/constant/custom.rs @@ -103,18 +103,18 @@ pub fn downcast_equal_consts( } /// Serialize any CustomConst using the `impl Serialize for &dyn CustomConst`. -fn serialize_custom_const(cc: &dyn CustomConst) -> Result { - serde_yaml::to_value(cc) +fn serialize_custom_const(cc: &dyn CustomConst) -> Result { + serde_json::to_value(cc) } /// Deserialize a `Box<&dyn CustomConst>` and attempt to downcast it to `CC`; /// propagating failure. fn deserialize_custom_const( - value: serde_yaml::Value, -) -> Result { + value: serde_json::Value, +) -> Result { match deserialize_dyn_custom_const(value)?.downcast::() { Ok(cc) => Ok(*cc), - Err(dyn_cc) => Err(::custom(format!( + Err(dyn_cc) => Err(::custom(format!( "Failed to deserialize [{}]: {:?}", std::any::type_name::(), dyn_cc @@ -124,9 +124,9 @@ fn deserialize_custom_const( /// Deserialize a `Box<&dyn CustomConst>`. fn deserialize_dyn_custom_const( - value: serde_yaml::Value, -) -> Result, serde_yaml::Error> { - serde_yaml::from_value(value) + value: serde_json::Value, +) -> Result, serde_json::Error> { + serde_json::from_value(value) } impl_downcast!(CustomConst); @@ -136,7 +136,7 @@ impl_box_clone!(CustomConst, CustomConstBoxClone); /// A constant value stored as a serialized blob that can report its own type. pub struct CustomSerialized { typ: Type, - value: serde_yaml::Value, + value: serde_json::Value, extensions: ExtensionSet, } @@ -144,7 +144,7 @@ pub struct CustomSerialized { #[error("Error serializing value into CustomSerialized: err: {err}, value: {payload:?}")] pub struct SerializeError { #[source] - err: serde_yaml::Error, + err: serde_json::Error, payload: Box, } @@ -152,15 +152,15 @@ pub struct SerializeError { #[error("Error deserializing value from CustomSerialized: err: {err}, value: {payload:?}")] pub struct DeserializeError { #[source] - err: serde_yaml::Error, - payload: serde_yaml::Value, + err: serde_json::Error, + payload: serde_json::Value, } impl CustomSerialized { /// Creates a new [`CustomSerialized`]. pub fn new( typ: impl Into, - value: serde_yaml::Value, + value: serde_json::Value, exts: impl Into, ) -> Self { Self { @@ -171,7 +171,7 @@ impl CustomSerialized { } /// Returns the inner value. - pub fn value(&self) -> &serde_yaml::Value { + pub fn value(&self) -> &serde_json::Value { &self.value } @@ -238,7 +238,7 @@ impl CustomSerialized { /// deserialize it. In particular if that inner value were a [Self] whose /// inner value were a `CC`, then we would still fail. pub fn try_into_custom_const(self) -> Result { - // ideally we would not have to clone, but serde_yaml does not allow us + // ideally we would not have to clone, but serde_json does not allow us // to recover the value from the error deserialize_custom_const(self.value.clone()).map_err(|err| DeserializeError { err, @@ -250,7 +250,7 @@ impl CustomSerialized { #[typetag::serde] impl CustomConst for CustomSerialized { fn name(&self) -> ValueName { - format!("yaml:{:?}", self.value).into() + format!("json:{:?}", self.value).into() } fn equal_consts(&self, other: &dyn CustomConst) -> bool { @@ -330,13 +330,13 @@ mod test { struct SerializeCustomConstExample { cc: CC, tag: &'static str, - yaml: serde_yaml::Value, + json: serde_json::Value, } impl SerializeCustomConstExample { fn new(cc: CC, tag: &'static str) -> Self { - let yaml = serde_yaml::to_value(&cc).unwrap(); - Self { cc, tag, yaml } + let json = serde_json::to_value(&cc).unwrap(); + Self { cc, tag, json } } } @@ -363,21 +363,21 @@ mod test { >( #[case] example: SerializeCustomConstExample, ) { - assert_eq!(example.yaml, serde_yaml::to_value(&example.cc).unwrap()); // sanity check - let expected_yaml: serde_yaml::Value = [ + assert_eq!(example.json, serde_json::to_value(&example.cc).unwrap()); // sanity check + let expected_json: serde_json::Value = [ ("c".into(), example.tag.into()), - ("v".into(), example.yaml.clone()), + ("v".into(), example.json.clone()), ] .into_iter() - .collect::() + .collect::>() .into(); // check serialize_custom_const - assert_eq!(expected_yaml, serialize_custom_const(&example.cc).unwrap()); + assert_eq!(expected_json, serialize_custom_const(&example.cc).unwrap()); let expected_custom_serialized = CustomSerialized::new( example.cc.get_type(), - expected_yaml, + expected_json, example.cc.extension_reqs(), ); @@ -412,12 +412,12 @@ mod test { // check OpaqueValue serializes/deserializes as a CustomSerialized let ev: OpaqueValue = example.cc.clone().into(); - let ev_val = serde_yaml::to_value(&ev).unwrap(); + let ev_val = serde_json::to_value(&ev).unwrap(); assert_eq!( &ev_val, - &serde_yaml::to_value(&expected_custom_serialized).unwrap() + &serde_json::to_value(&expected_custom_serialized).unwrap() ); - assert_eq!(ev, serde_yaml::from_value(ev_val).unwrap()); + assert_eq!(ev, serde_json::from_value(ev_val).unwrap()); } fn example_custom_serialized() -> (ConstUsize, CustomSerialized) { @@ -473,7 +473,7 @@ mod test { // A serialization round-trip results in an OpaqueValue with the value of inner assert_eq!( OpaqueValue::new(inner), - serde_yaml::from_value(serde_yaml::to_value(&ev).unwrap()).unwrap() + serde_json::from_value(serde_json::to_value(&ev).unwrap()).unwrap() ); } } @@ -485,7 +485,7 @@ mod proptest { use crate::{ extension::ExtensionSet, ops::constant::CustomSerialized, - proptest::{any_serde_yaml_value, any_string}, + proptest::{any_serde_json_value, any_string}, types::Type, }; @@ -502,10 +502,10 @@ mod proptest { // 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)| { + let value = (any_serde_json_value(), any_string()).prop_map(|(content, tag)| { [("c".into(), tag.into()), ("v".into(), content)] .into_iter() - .collect::() + .collect::>() .into() }); (typ, value, extensions) diff --git a/hugr-core/src/proptest.rs b/hugr-core/src/proptest.rs index 5598cf75e..74b66891c 100644 --- a/hugr-core/src/proptest.rs +++ b/hugr-core/src/proptest.rs @@ -104,13 +104,13 @@ lazy_static! { ].sboxed() }; - /// A strategy for an arbitrary non-recursive [serde_yaml::Value]. + /// A strategy for an arbitrary non-recursive [serde_json::Value]. /// In particular, no `Mapping`, `Sequence`, or `Tagged`. /// /// This is used as the base strategy for the general /// [recursive](Strategy::prop_recursive) strategy. - static ref ANY_SERDE_YAML_VALUE_LEAF: SBoxedStrategy = { - use serde_yaml::value::Value; + static ref ANY_SERDE_JSON_VALUE_LEAF: SBoxedStrategy = { + use serde_json::value::Value; prop_oneof![ Just(Value::Null), any::().prop_map_into(), @@ -155,9 +155,9 @@ 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 +pub fn any_serde_json_value() -> impl Strategy { + // use serde_json::value::{Tag, TaggedValue, Value}; + ANY_SERDE_JSON_VALUE_LEAF .clone() .prop_recursive( 3, // No more than 3 branch levels deep @@ -168,8 +168,10 @@ pub fn any_serde_yaml_value() -> impl Strategy { // TODO TaggedValue doesn't roundtrip through JSON // (any_nonempty_string().prop_map(Tag::new), element.clone()).prop_map(|(tag, value)| Value::Tagged(Box::new(TaggedValue { tag, value }))), proptest::collection::vec(element.clone(), 0..3).prop_map_into(), - vec((any_string().prop_map_into(), element.clone()), 0..3) - .prop_map(|x| x.into_iter().collect::().into()) + vec((any_string().prop_map_into(), element.clone()), 0..3).prop_map(|x| x + .into_iter() + .collect::>() + .into()) ] }, ) From e004681f2c2660f81b600da8d6f98e6a318276de Mon Sep 17 00:00:00 2001 From: Seyon Sivarajah Date: Wed, 24 Jul 2024 15:59:53 +0100 Subject: [PATCH 2/2] Apply suggestions from code review Co-authored-by: Douglas Wilson <141026920+doug-q@users.noreply.github.com> --- hugr-core/src/proptest.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hugr-core/src/proptest.rs b/hugr-core/src/proptest.rs index 74b66891c..6c7fd0135 100644 --- a/hugr-core/src/proptest.rs +++ b/hugr-core/src/proptest.rs @@ -105,7 +105,7 @@ lazy_static! { }; /// A strategy for an arbitrary non-recursive [serde_json::Value]. - /// In particular, no `Mapping`, `Sequence`, or `Tagged`. + /// In particular, no `Array` or `Object` /// /// This is used as the base strategy for the general /// [recursive](Strategy::prop_recursive) strategy. @@ -156,7 +156,6 @@ pub fn any_smolstr() -> SBoxedStrategy { } pub fn any_serde_json_value() -> impl Strategy { - // use serde_json::value::{Tag, TaggedValue, Value}; ANY_SERDE_JSON_VALUE_LEAF .clone() .prop_recursive( @@ -165,8 +164,6 @@ pub fn any_serde_json_value() -> impl Strategy { 3, // Each collection is up to 3 elements long |element| { prop_oneof![ - // TODO TaggedValue doesn't roundtrip through JSON - // (any_nonempty_string().prop_map(Tag::new), element.clone()).prop_map(|(tag, value)| Value::Tagged(Box::new(TaggedValue { tag, value }))), proptest::collection::vec(element.clone(), 0..3).prop_map_into(), vec((any_string().prop_map_into(), element.clone()), 0..3).prop_map(|x| x .into_iter()