From c79d0bbc6f63ba0a8fe2c5722c27f405fbc1d55c Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 12 Jul 2021 16:51:10 +0100 Subject: [PATCH 1/5] Erase PhantomData fields --- src/build.rs | 6 +++++- src/impls.rs | 4 +++- test_suite/tests/json.rs | 4 +--- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/build.rs b/src/build.rs index 9cff2a93..cf434f02 100644 --- a/src/build.rs +++ b/src/build.rs @@ -270,7 +270,11 @@ impl FieldsBuilder { -> FieldBuilder, { let builder = builder(FieldBuilder::new()); - self.fields.push(builder.finalize()); + let field = builder.finalize(); + // filter out fields of PhantomData + if field.ty() != &MetaType::new::() { + self.fields.push(field); + } self } } diff --git a/src/impls.rs b/src/impls.rs index 94753340..d6802d6a 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -318,8 +318,10 @@ impl TypeInfo for String { } } +pub(crate) type PhantomIdentity = PhantomData<()>; + impl TypeInfo for PhantomData { - type Identity = PhantomData<()>; + type Identity = PhantomIdentity; fn type_info() -> Type { TypeDefPhantom.into() diff --git a/test_suite/tests/json.rs b/test_suite/tests/json.rs index 7b70993c..a1139494 100644 --- a/test_suite/tests/json.rs +++ b/test_suite/tests/json.rs @@ -271,7 +271,7 @@ fn test_struct_with_some_fields_marked_as_compact() { } #[test] -fn test_struct_with_phantom() { +fn test_struct_with_phantom_field_erased() { use scale_info::prelude::marker::PhantomData; #[derive(TypeInfo)] struct Struct { @@ -288,8 +288,6 @@ fn test_struct_with_phantom() { "composite": { "fields": [ { "name": "a", "type": 1, "typeName": "i32" }, - // type 1 is the `u8` in the `PhantomData` - { "name": "b", "type": 2, "typeName": "PhantomData" }, ], }, } From 3b537b17714a9557c58eadbfe78563f5d0f1ca89 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 12 Jul 2021 16:57:28 +0100 Subject: [PATCH 2/5] Remove TypeDef::Phantom variant --- src/impls.rs | 3 +-- src/tests.rs | 7 ++++++- src/ty/mod.rs | 19 ------------------- test_suite/tests/json.rs | 2 -- 4 files changed, 7 insertions(+), 24 deletions(-) diff --git a/src/impls.rs b/src/impls.rs index d6802d6a..f5332a87 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -35,7 +35,6 @@ use crate::{ Type, TypeDefArray, TypeDefCompact, - TypeDefPhantom, TypeDefPrimitive, TypeDefSequence, TypeDefTuple, @@ -324,7 +323,7 @@ impl TypeInfo for PhantomData { type Identity = PhantomIdentity; fn type_info() -> Type { - TypeDefPhantom.into() + panic!("PhantomData types should be filtered out by the FieldsBuilder") } } diff --git a/src/tests.rs b/src/tests.rs index 4b54ee48..fe505c10 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -83,7 +83,6 @@ fn prelude_items() { ) ) ); - assert_type!(PhantomData, TypeDefPhantom); assert_type!( Cow, Type::builder() @@ -100,6 +99,12 @@ fn prelude_items() { ) } +#[test] +#[should_panic] +fn phantom_data() { + PhantomData::::type_info(); +} + #[test] fn collections() { assert_type!( diff --git a/src/ty/mod.rs b/src/ty/mod.rs index 69f50080..b3c9375a 100644 --- a/src/ty/mod.rs +++ b/src/ty/mod.rs @@ -115,7 +115,6 @@ impl_from_type_def_for_type!( TypeDefSequence, TypeDefTuple, TypeDefCompact, - TypeDefPhantom, TypeDefBitSequence, ); @@ -238,8 +237,6 @@ pub enum TypeDef { Primitive(TypeDefPrimitive), /// A type using the [`Compact`] encoding Compact(TypeDefCompact), - /// A PhantomData type. - Phantom(TypeDefPhantom), /// A type representing a sequence of bits. BitSequence(TypeDefBitSequence), } @@ -256,7 +253,6 @@ impl IntoPortable for TypeDef { TypeDef::Tuple(tuple) => tuple.into_portable(registry).into(), TypeDef::Primitive(primitive) => primitive.into(), TypeDef::Compact(compact) => compact.into_portable(registry).into(), - TypeDef::Phantom(phantom) => phantom.into(), TypeDef::BitSequence(bitseq) => bitseq.into_portable(registry).into(), } } @@ -486,21 +482,6 @@ where } } -/// A type describing a `PhantomData` type. -/// -/// In the context of SCALE encoded types, including `PhantomData` types in -/// the type info might seem surprising. The reason to include this information -/// is that there could be situations where it's useful and because removing -/// `PhantomData` items from the derive input quickly becomes a messy -/// syntax-level hack (see PR https://github.com/paritytech/scale-info/pull/31). -/// Instead we take the same approach as `parity-scale-codec` where users are -/// required to explicitly skip fields that cannot be represented in SCALE -/// encoding, using the `#[codec(skip)]` attribute. -#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[cfg_attr(any(feature = "std", feature = "decode"), derive(scale::Decode))] -#[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Encode, Debug)] -pub struct TypeDefPhantom; - /// Type describing a [`bitvec::vec::BitVec`]. /// /// # Note diff --git a/test_suite/tests/json.rs b/test_suite/tests/json.rs index a1139494..57543ed3 100644 --- a/test_suite/tests/json.rs +++ b/test_suite/tests/json.rs @@ -163,8 +163,6 @@ fn test_builtins() { // strings assert_json_for_type::(json!({ "def": { "primitive": "str" } })); assert_json_for_type::(json!({ "def": { "primitive": "str" } })); - // PhantomData - assert_json_for_type::>(json!({ "def": { "phantom": null }, })) } #[test] From 37351fe952f89dd3140d5e2040de0b73cb828763 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 12 Jul 2021 17:29:25 +0100 Subject: [PATCH 3/5] Make Composite constructor pub(crate) --- src/ty/composite.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ty/composite.rs b/src/ty/composite.rs index 769c58b8..829b194b 100644 --- a/src/ty/composite.rs +++ b/src/ty/composite.rs @@ -89,9 +89,9 @@ impl IntoPortable for TypeDefComposite { } } -impl TypeDefComposite { +impl TypeDefComposit0e { /// Creates a new struct definition with named fields. - pub fn new(fields: I) -> Self + pub(crate) fn new(fields: I) -> Self where I: IntoIterator, { From e502a1ced91d24f1da4ce088f2fd98a44b2e75fe Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 12 Jul 2021 17:34:45 +0100 Subject: [PATCH 4/5] Remove erased PhantomData fields from derive tests --- src/ty/composite.rs | 2 +- test_suite/tests/derive.rs | 35 +++-------------------------------- 2 files changed, 4 insertions(+), 33 deletions(-) diff --git a/src/ty/composite.rs b/src/ty/composite.rs index 829b194b..4a43d367 100644 --- a/src/ty/composite.rs +++ b/src/ty/composite.rs @@ -89,7 +89,7 @@ impl IntoPortable for TypeDefComposite { } } -impl TypeDefComposit0e { +impl TypeDefComposite { /// Creates a new struct definition with named fields. pub(crate) fn new(fields: I) -> Self where diff --git a/test_suite/tests/derive.rs b/test_suite/tests/derive.rs index 7693bc75..1bfef0ca 100644 --- a/test_suite/tests/derive.rs +++ b/test_suite/tests/derive.rs @@ -97,7 +97,7 @@ fn struct_derive() { } #[test] -fn phantom_data_is_part_of_the_type_info() { +fn phantom_data_field_is_erased() { #[allow(unused)] #[derive(TypeInfo)] struct P { @@ -667,19 +667,7 @@ fn skip_all_type_params() { TypeParameter::new("T", None), TypeParameter::new("U", None), ]) - .composite( - Fields::named() - .field(|f| { - f.ty::>() - .name("a") - .type_name("PhantomData") - }) - .field(|f| { - f.ty::>() - .name("b") - .type_name("PhantomData") - }), - ); + .composite(Fields::named()); assert_type!(SkipAllTypeParams, ty); } @@ -712,11 +700,6 @@ fn skip_type_params_with_associated_types() { .type_params(vec![TypeParameter::new("T", None)]) .composite( Fields::named() - .field(|f| { - f.ty::>() - .name("a") - .type_name("PhantomData") - }) .field(|f| f.ty::().name("b").type_name("T::A")), ); @@ -741,19 +724,7 @@ fn skip_type_params_with_defaults() { TypeParameter::new("T", None), TypeParameter::new("U", None), ]) - .composite( - Fields::named() - .field(|f| { - f.ty::>() - .name("a") - .type_name("PhantomData") - }) - .field(|f| { - f.ty::>() - .name("b") - .type_name("PhantomData") - }), - ); + .composite(Fields::named()); assert_type!(SkipAllTypeParamsWithDefaults, ty); } From bdd55f57747cac44fec2a41305d3c382a6333063 Mon Sep 17 00:00:00 2001 From: Andrew Jones Date: Mon, 12 Jul 2021 17:46:06 +0100 Subject: [PATCH 5/5] Don't include PhantomData types in tuples --- src/build.rs | 2 +- src/impls.rs | 2 +- src/meta_type.rs | 5 +++++ src/tests.rs | 9 +++++++++ src/ty/mod.rs | 5 ++++- test_suite/tests/derive.rs | 5 +---- 6 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/build.rs b/src/build.rs index cf434f02..9ca0c88b 100644 --- a/src/build.rs +++ b/src/build.rs @@ -272,7 +272,7 @@ impl FieldsBuilder { let builder = builder(FieldBuilder::new()); let field = builder.finalize(); // filter out fields of PhantomData - if field.ty() != &MetaType::new::() { + if !field.ty().is_phantom() { self.fields.push(field); } self diff --git a/src/impls.rs b/src/impls.rs index f5332a87..ea9c55fa 100644 --- a/src/impls.rs +++ b/src/impls.rs @@ -323,7 +323,7 @@ impl TypeInfo for PhantomData { type Identity = PhantomIdentity; fn type_info() -> Type { - panic!("PhantomData types should be filtered out by the FieldsBuilder") + panic!("PhantomData type instances should be filtered out.") } } diff --git a/src/meta_type.rs b/src/meta_type.rs index db5fbc39..ba34fb1e 100644 --- a/src/meta_type.rs +++ b/src/meta_type.rs @@ -105,4 +105,9 @@ impl MetaType { pub fn type_id(&self) -> TypeId { self.type_id } + + /// Returns true if this represents a type of [`core::marker::PhantomData`]. + pub(crate) fn is_phantom(&self) -> bool { + self == &MetaType::new::() + } } diff --git a/src/tests.rs b/src/tests.rs index fe505c10..02323c6b 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -182,6 +182,15 @@ fn tuple_primitives() { ); } +#[test] +fn tuple_phantom_data_erased() { + // nested tuple + assert_type!( + (u64, PhantomData), + TypeDefTuple::new(vec![meta_type::(),]) + ); +} + #[test] fn array_primitives() { // array diff --git a/src/ty/mod.rs b/src/ty/mod.rs index b3c9375a..f5055b29 100644 --- a/src/ty/mod.rs +++ b/src/ty/mod.rs @@ -376,7 +376,10 @@ impl TypeDefTuple { T: IntoIterator, { Self { - fields: type_params.into_iter().collect(), + fields: type_params + .into_iter() + .filter(|ty| !ty.is_phantom()) + .collect(), } } diff --git a/test_suite/tests/derive.rs b/test_suite/tests/derive.rs index 1bfef0ca..a83004ad 100644 --- a/test_suite/tests/derive.rs +++ b/test_suite/tests/derive.rs @@ -698,10 +698,7 @@ fn skip_type_params_with_associated_types() { let ty = Type::builder() .path(Path::new("SkipTypeParamsForTraitImpl", "derive")) .type_params(vec![TypeParameter::new("T", None)]) - .composite( - Fields::named() - .field(|f| f.ty::().name("b").type_name("T::A")), - ); + .composite(Fields::named().field(|f| f.ty::().name("b").type_name("T::A"))); assert_type!(SkipTypeParamsForTraitImpl, ty); }