From 8fb5065293a131b1aea251c698a9615996cfbf33 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 15 Jul 2021 10:45:39 +0100 Subject: [PATCH 1/2] Make variant index explicit --- derive/src/lib.rs | 10 ++++++---- src/build.rs | 48 +++++++++++++++++++++++++++++++++-------------- src/impls.rs | 16 +++++++++++----- src/tests.rs | 20 +++++++++++++------- src/ty/variant.rs | 18 +++++++++--------- 5 files changed, 73 insertions(+), 39 deletions(-) diff --git a/derive/src/lib.rs b/derive/src/lib.rs index 2da4cce1..f1893781 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -259,12 +259,14 @@ fn generate_variant_type(data_enum: &DataEnum, scale_info: &Ident) -> TokenStrea let variants = variants .into_iter() - .filter(|v| !utils::should_skip(&v.attrs)) - .map(|v| { + .enumerate() + .filter(|(_, v)| !utils::should_skip(&v.attrs)) + .map(|(i, v)| { let ident = &v.ident; let v_name = quote! {::core::stringify!(#ident) }; let docs = generate_docs(&v.attrs); - let index = utils::maybe_index(v).map(|i| quote!(.index(#i))); + // let index = utils::maybe_index(v).map(|i| quote!(.index(#i))); + let index = utils::variant_index(v, i); let fields = match v.fields { Fields::Named(ref fs) => { @@ -291,9 +293,9 @@ fn generate_variant_type(data_enum: &DataEnum, scale_info: &Ident) -> TokenStrea quote! { .variant(#v_name, |v| v + .index(#i as u8) .fields(#fields) #docs - #index ) } }); diff --git a/src/build.rs b/src/build.rs index 9cff2a93..4d6d2870 100644 --- a/src/build.rs +++ b/src/build.rs @@ -135,7 +135,7 @@ use crate::{ Variant, }; -/// State types for type builders which require a Path +/// State types for type builders which require a Path. pub mod state { /// State where the builder has not assigned a Path to the type pub enum PathNotAssigned {} @@ -429,7 +429,7 @@ impl Variants { /// Add a variant pub fn variant(mut self, name: &'static str, builder: F) -> Self where - F: Fn(VariantBuilder) -> VariantBuilder, + F: Fn(VariantBuilder) -> VariantBuilder, { let builder = builder(VariantBuilder::new(name)); self.variants.push(builder.finalize()); @@ -437,8 +437,8 @@ impl Variants { } /// Add a unit variant (without fields). - pub fn variant_unit(mut self, name: &'static str) -> Self { - let builder = VariantBuilder::new(name); + pub fn variant_unit(mut self, name: &'static str, index: u8) -> Self { + let builder = VariantBuilder::new(name).index(index); self.variants.push(builder.finalize()); self } @@ -449,16 +449,25 @@ impl Variants { } } +/// State types for the `VariantBuilder` which requires an index. +pub mod variant_state { + /// State where the builder has not assigned an index to a variant. + pub enum IndexNotAssigned {} + /// State where the builder has assigned an index to a variant. + pub enum IndexAssigned {} +} + /// Build a [`Variant`]. -pub struct VariantBuilder { +pub struct VariantBuilder { name: &'static str, - fields: Vec>, index: Option, + fields: Vec>, discriminant: Option, docs: Vec<&'static str>, + marker: PhantomData, } -impl VariantBuilder { +impl VariantBuilder { /// Create a new [`VariantBuilder`]. pub fn new(name: &'static str) -> Self { Self { @@ -467,21 +476,30 @@ impl VariantBuilder { discriminant: None, index: None, docs: Vec::new(), + marker: Default::default(), + } + } + + /// Set the variant's codec index. + pub fn index(mut self, index: u8) -> VariantBuilder { + VariantBuilder { + name: self.name, + index: Some(index), + fields: self.fields, + discriminant: self.discriminant, + docs: self.docs, + marker: Default::default(), } } +} +impl VariantBuilder { /// Set the variant's discriminant. pub fn discriminant(mut self, discriminant: u64) -> Self { self.discriminant = Some(discriminant); self } - /// Set the variant's codec index. - pub fn index(mut self, index: u8) -> Self { - self.index = Some(index); - self - } - /// Initialize the variant's fields. pub fn fields(mut self, fields_builder: FieldsBuilder) -> Self { self.fields = fields_builder.finalize(); @@ -493,13 +511,15 @@ impl VariantBuilder { self.docs = docs.to_vec(); self } +} +impl VariantBuilder { /// Complete building and create final [`Variant`] instance. pub fn finalize(self) -> Variant { Variant::new( self.name, self.fields, - self.index, + self.index.expect("Index should be assigned by the builder"), self.discriminant, self.docs, ) diff --git a/src/impls.rs b/src/impls.rs index 94753340..7540e08a 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -188,9 +188,13 @@ where Type::builder() .path(Path::prelude("Option")) .type_params(type_params![T]) - .variant(Variants::new().variant("None", |v| v).variant("Some", |v| { - v.fields(Fields::unnamed().field(|f| f.ty::())) - })) + .variant( + Variants::new() + .variant("None", |v| v.index(0)) + .variant("Some", |v| { + v.index(1).fields(Fields::unnamed().field(|f| f.ty::())) + }), + ) } } @@ -207,9 +211,11 @@ where .type_params(type_params!(T, E)) .variant( Variants::new() - .variant("Ok", |v| v.fields(Fields::unnamed().field(|f| f.ty::()))) + .variant("Ok", |v| { + v.index(0).fields(Fields::unnamed().field(|f| f.ty::())) + }) .variant("Err", |v| { - v.fields(Fields::unnamed().field(|f| f.ty::())) + v.index(1).fields(Fields::unnamed().field(|f| f.ty::())) }), ) } diff --git a/src/tests.rs b/src/tests.rs index 4b54ee48..666e53ef 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -62,9 +62,13 @@ fn prelude_items() { Type::builder() .path(Path::prelude("Option")) .type_params(named_type_params![(T, u128)]) - .variant(Variants::new().variant("None", |v| v).variant("Some", |v| { - v.fields(Fields::unnamed().field(|f| f.ty::())) - })) + .variant(Variants::new().variant("None", |v| v.index(0)).variant( + "Some", + |v| { + v.index(1) + .fields(Fields::unnamed().field(|f| f.ty::())) + } + )) ); assert_type!( Result, @@ -75,10 +79,12 @@ fn prelude_items() { Variants::new() .variant( "Ok", |v| v + .index(0) .fields(Fields::unnamed().field(|f| f.ty::())) ) .variant( "Err", |v| v + .index(1) .fields(Fields::unnamed().field(|f| f.ty::())) ) ) @@ -301,13 +307,13 @@ fn basic_enum_with_index() { ) }) .variant("C", |v| { - v.fields( + v.index(2).fields( Fields::unnamed() .field(|f| f.ty::().type_name("u16")) .field(|f| f.ty::().type_name("u32")), ) }) - .variant_unit("D"), + .variant_unit("D", 3), ) } } @@ -327,13 +333,13 @@ fn basic_enum_with_index() { ) }) .variant("C", |v| { - v.fields( + v.index(2).fields( Fields::unnamed() .field(|f| f.ty::().type_name("u16")) .field(|f| f.ty::().type_name("u32")), ) }) - .variant_unit("D"), + .variant_unit("D", 3), ); assert_type!(IndexedRustEnum, ty); diff --git a/src/ty/variant.rs b/src/ty/variant.rs index c3479c32..852ac8b3 100644 --- a/src/ty/variant.rs +++ b/src/ty/variant.rs @@ -157,12 +157,12 @@ pub struct Variant { serde(skip_serializing_if = "Vec::is_empty", default) )] fields: Vec>, - /// Index of the variant, used in `parity-scale-codec` - #[cfg_attr( - feature = "serde", - serde(skip_serializing_if = "Option::is_none", default) - )] - index: Option, + /// Index of the variant, used in `parity-scale-codec`. + /// + /// The value of this will be, in order of precedence: + /// 1. The explicit index defined by a `#[codec(index = N)]` attribute. + /// 2. The implicit index from the position of the variant in the `enum` definition. + index: u8, /// The discriminant of the variant. /// /// # Note @@ -202,7 +202,7 @@ impl Variant { pub(crate) fn new( name: &'static str, fields: Vec>, - index: Option, + index: u8, discriminant: Option, docs: Vec<&'static str>, ) -> Self { @@ -230,8 +230,8 @@ where &self.fields } - /// Returns the index of the variant, if specified. - pub fn index(&self) -> Option { + /// Returns the index of the variant. + pub fn index(&self) -> u8 { self.index } From 90c8aa46935350be426d69e9ca7ad61f21a6a70b Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Thu, 15 Jul 2021 11:05:19 +0100 Subject: [PATCH 2/2] Remove discriminant, replace with explicit index --- derive/src/lib.rs | 7 ++--- src/build.rs | 21 ++++++++----- src/ty/variant.rs | 20 ------------- test_suite/tests/derive.rs | 61 +++++++++++++++++++------------------- test_suite/tests/json.rs | 30 ++++++++++++++----- 5 files changed, 69 insertions(+), 70 deletions(-) diff --git a/derive/src/lib.rs b/derive/src/lib.rs index f1893781..ffc95adf 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -225,12 +225,12 @@ fn generate_c_like_enum_def(variants: &VariantList, scale_info: &Ident) -> Token .filter(|(_, v)| !utils::should_skip(&v.attrs)) .map(|(i, v)| { let name = &v.ident; - let discriminant = utils::variant_index(v, i); + let index = utils::variant_index(v, i); let docs = generate_docs(&v.attrs); quote! { .variant(::core::stringify!(#name), |v| v - .discriminant(#discriminant as ::core::primitive::u64) + .index(#index as ::core::primitive::u8) #docs ) } @@ -265,7 +265,6 @@ fn generate_variant_type(data_enum: &DataEnum, scale_info: &Ident) -> TokenStrea let ident = &v.ident; let v_name = quote! {::core::stringify!(#ident) }; let docs = generate_docs(&v.attrs); - // let index = utils::maybe_index(v).map(|i| quote!(.index(#i))); let index = utils::variant_index(v, i); let fields = match v.fields { @@ -293,7 +292,7 @@ fn generate_variant_type(data_enum: &DataEnum, scale_info: &Ident) -> TokenStrea quote! { .variant(#v_name, |v| v - .index(#i as u8) + .index(#index as ::core::primitive::u8) .fields(#fields) #docs ) diff --git a/src/build.rs b/src/build.rs index 4d6d2870..6f724a83 100644 --- a/src/build.rs +++ b/src/build.rs @@ -84,9 +84,15 @@ //! .type_params(type_params!(T)) //! .variant( //! Variants::new() -//! .variant("A", |v| v.fields(Fields::unnamed().field(|f| f.ty::().type_name("T")))) -//! .variant("B", |v| v.fields(Fields::named().field(|f| f.ty::().name("f").type_name("u32")))) -//! .variant_unit("A") +//! .variant("A", |v| v +//! .index(0) +//! .fields(Fields::unnamed().field(|f| f.ty::().type_name("T"))) +//! ) +//! .variant("B", |v| v +//! .index(1) +//! .fields(Fields::named().field(|f| f.ty::().name("f").type_name("u32"))) +//! ) +//! .variant_unit("A", 2) //! ) //! } //! } @@ -108,9 +114,9 @@ //! .path(Path::new("Foo", module_path!())) //! .variant( //! Variants::new() -//! .variant("A", |v| v.discriminant(1)) -//! .variant("B", |v| v.discriminant(2)) -//! .variant("C", |v| v.discriminant(33)) +//! .variant("A", |v| v.index(1)) +//! .variant("B", |v| v.index(2)) +//! .variant("C", |v| v.index(33)) //! ) //! } //! } @@ -481,7 +487,7 @@ impl VariantBuilder { } /// Set the variant's codec index. - pub fn index(mut self, index: u8) -> VariantBuilder { + pub fn index(self, index: u8) -> VariantBuilder { VariantBuilder { name: self.name, index: Some(index), @@ -520,7 +526,6 @@ impl VariantBuilder { self.name, self.fields, self.index.expect("Index should be assigned by the builder"), - self.discriminant, self.docs, ) } diff --git a/src/ty/variant.rs b/src/ty/variant.rs index 852ac8b3..8eac8ec5 100644 --- a/src/ty/variant.rs +++ b/src/ty/variant.rs @@ -163,18 +163,6 @@ pub struct Variant { /// 1. The explicit index defined by a `#[codec(index = N)]` attribute. /// 2. The implicit index from the position of the variant in the `enum` definition. index: u8, - /// The discriminant of the variant. - /// - /// # Note - /// - /// Even though setting the discriminant is optional - /// every C-like enum variant has a discriminant specified - /// upon compile-time. - #[cfg_attr( - feature = "serde", - serde(skip_serializing_if = "Option::is_none", default) - )] - discriminant: Option, /// Documentation #[cfg_attr( feature = "serde", @@ -191,7 +179,6 @@ impl IntoPortable for Variant { name: self.name.into_portable(registry), fields: registry.map_into_portable(self.fields), index: self.index, - discriminant: self.discriminant, docs: registry.map_into_portable(self.docs), } } @@ -203,14 +190,12 @@ impl Variant { name: &'static str, fields: Vec>, index: u8, - discriminant: Option, docs: Vec<&'static str>, ) -> Self { Self { name, fields, index, - discriminant, docs, } } @@ -235,11 +220,6 @@ where self.index } - /// Returns the discriminant of the variant. - pub fn discriminant(&self) -> Option { - self.discriminant - } - /// Returns the documentation of the variant. pub fn docs(&self) -> &[T::String] { &self.docs diff --git a/test_suite/tests/derive.rs b/test_suite/tests/derive.rs index 7693bc75..9de08fc6 100644 --- a/test_suite/tests/derive.rs +++ b/test_suite/tests/derive.rs @@ -173,10 +173,8 @@ fn c_like_enum_derive() { .docs(&["Enum docs."]) .variant( Variants::new() - .variant("A", |v| v.discriminant(0).docs(&["Unit variant."])) - .variant("B", |v| { - v.discriminant(10).docs(&["Variant with discriminant."]) - }), + .variant("A", |v| v.index(0).docs(&["Unit variant."])) + .variant("B", |v| v.index(10).docs(&["Variant with discriminant."])), ); assert_type!(E, ty); @@ -198,11 +196,11 @@ fn c_like_enum_derive_with_scale_index_set() { let ty = Type::builder().path(Path::new("E", "derive")).variant( Variants::new() - .variant("A", |v| v.discriminant(0)) - .variant("B", |v| v.discriminant(10)) - .variant("C", |v| v.discriminant(13)) - .variant("D", |v| v.discriminant(3)) - .variant("E", |v| v.discriminant(14)), + .variant("A", |v| v.index(0)) + .variant("B", |v| v.index(10)) + .variant("C", |v| v.index(13)) + .variant("D", |v| v.index(3)) + .variant("E", |v| v.index(14)), ); assert_type!(E, ty); @@ -235,21 +233,23 @@ fn enum_derive() { .variant( Variants::new() .variant("A", |v| { - v.fields(Fields::unnamed().field(|f| { - f.ty::().type_name("T").docs(&["Unnamed field."]) - })) - .docs(&["Unnamed fields variant."]) + v.index(0) + .fields(Fields::unnamed().field(|f| { + f.ty::().type_name("T").docs(&["Unnamed field."]) + })) + .docs(&["Unnamed fields variant."]) }) .variant("B", |v| { - v.fields(Fields::named().field(|f| { - f.ty::() - .name("b") - .type_name("T") - .docs(&["Named field."]) - })) - .docs(&["Named fields variant."]) + v.index(1) + .fields(Fields::named().field(|f| { + f.ty::() + .name("b") + .type_name("T") + .docs(&["Named field."]) + })) + .docs(&["Named fields variant."]) }) - .variant("C", |v| v.docs(&["Unit variant."])), + .variant("C", |v| v.index(2).docs(&["Unit variant."])), ); assert_type!(E, ty); @@ -302,13 +302,13 @@ fn recursive_type_derive() { let ty = Type::builder().path(Path::new("Tree", "derive")).variant( Variants::new() .variant("Leaf", |v| { - v.fields( + v.index(0).fields( Fields::named() .field(|f| f.ty::().name("value").type_name("i32")), ) }) .variant("Node", |v| { - v.fields( + v.index(1).fields( Fields::named() .field(|f| { f.ty::>().name("right").type_name("Box") @@ -440,18 +440,18 @@ fn scale_compact_types_work_in_enums() { .variant( Variants::new() .variant("Id", |v| { - v.fields( + v.index(0).fields( Fields::unnamed().field(|f| f.ty::().type_name("AccountId")), ) }) .variant("Index", |v| { - v.fields( + v.index(1).fields( Fields::unnamed() .field(|f| f.compact::().type_name("AccountIndex")), ) }) .variant("Address32", |v| { - v.fields( + v.index(2).fields( Fields::unnamed() .field(|f| f.ty::<[u8; 32]>().type_name("[u8; 32]")), ) @@ -495,8 +495,8 @@ fn enum_variants_marked_scale_skip_are_skipped() { let ty = Type::builder().path(Path::new("Skippy", "derive")).variant( Variants::new() - .variant("A", |v| v.discriminant(0)) - .variant("C", |v| v.discriminant(2)), + .variant("A", |v| v.index(0)) + .variant("C", |v| v.index(2)), ); assert_type!(Skippy, ty); } @@ -519,12 +519,13 @@ fn enum_variants_with_fields_marked_scale_skip_are_skipped() { let ty = Type::builder().path(Path::new("Skippy", "derive")).variant( Variants::new() .variant("Bajs", |v| { - v.fields( + v.index(1).fields( Fields::named().field(|f| f.ty::().name("b").type_name("bool")), ) }) .variant("Coo", |v| { - v.fields(Fields::unnamed().field(|f| f.ty::().type_name("bool"))) + v.index(2) + .fields(Fields::unnamed().field(|f| f.ty::().type_name("bool"))) }), ); assert_type!(Skippy, ty); diff --git a/test_suite/tests/json.rs b/test_suite/tests/json.rs index 7b70993c..373ec9c6 100644 --- a/test_suite/tests/json.rs +++ b/test_suite/tests/json.rs @@ -126,9 +126,11 @@ fn test_builtins() { "variants": [ { "name": "None", + "index": 0, }, { "name": "Some", + "index": 1, "fields": [ { "type": 0 } ] }, ] @@ -146,10 +148,12 @@ fn test_builtins() { "variants": [ { "name": "Ok", + "index": 0, "fields": [ { "type": 0 } ] }, { "name": "Err", + "index": 1, "fields": [ { "type": 1 } ] } ] @@ -310,9 +314,9 @@ fn test_clike_enum() { "def": { "variant": { "variants": [ - { "name": "A", "discriminant": 0, }, - { "name": "B", "discriminant": 42, }, - { "name": "C", "discriminant": 2, }, + { "name": "A", "index": 0, }, + { "name": "B", "index": 42, }, + { "name": "C", "index": 2, }, ], }, } @@ -333,9 +337,13 @@ fn test_enum() { "def": { "variant": { "variants": [ - { "name": "ClikeVariant" }, + { + "name": "ClikeVariant", + "index": 0, + }, { "name": "TupleStructVariant", + "index": 1, "fields": [ { "type": 0, "typeName": "u32" }, { "type": 1, "typeName": "bool" }, @@ -343,6 +351,7 @@ fn test_enum() { }, { "name": "StructVariant", + "index": 2, "fields": [ { "name": "a", "type": 0, "typeName": "u32" }, { "name": "b", "type": 2, "typeName": "[u8; 32]" }, @@ -427,12 +436,14 @@ fn test_recursive_type_with_box() { "variants": [ { "name": "Leaf", + "index": 0, "fields": [ { "name": "value", "type": 1, "typeName": "i32" }, ], }, { "name": "Node", + "index": 1, "fields": [ { "name": "right", "type": 0, "typeName": "Box" }, { "name": "left", "type": 0, "typeName": "Box" }, @@ -645,15 +656,15 @@ fn test_registry() { "variants": [ { "name": "A", - "discriminant": 0, + "index": 0, }, { "name": "B", - "discriminant": 1, + "index": 1, }, { "name": "C", - "discriminant": 2, + "index": 2, }, ] } @@ -668,10 +679,12 @@ fn test_registry() { "variant": { "variants": [ { - "name": "A" + "name": "A", + "index": 0, }, { "name": "B", + "index": 1, "fields": [ { "type": 2, "typeName": "u8" }, // u8 { "type": 3, "typeName": "u32" }, // u32 @@ -679,6 +692,7 @@ fn test_registry() { }, { "name": "C", + "index": 2, "fields": [ { "name": "a",