Skip to content

Commit

Permalink
Erase PhantomData fields (#111)
Browse files Browse the repository at this point in the history
* Erase PhantomData fields

* Remove TypeDef::Phantom variant

* Make Composite constructor pub(crate)

* Remove erased PhantomData fields from derive tests

* Don't include PhantomData types in tuples
  • Loading branch information
ascjones authored Jul 15, 2021
1 parent 749b02f commit 53c7cfc
Show file tree
Hide file tree
Showing 8 changed files with 39 additions and 67 deletions.
6 changes: 5 additions & 1 deletion src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,11 @@ impl FieldsBuilder<NamedFields> {
-> FieldBuilder<field_state::NameAssigned, field_state::TypeAssigned>,
{
let builder = builder(FieldBuilder::new());
self.fields.push(builder.finalize());
let field = builder.finalize();
// filter out fields of PhantomData
if !field.ty().is_phantom() {
self.fields.push(field);
}
self
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ use crate::{
Type,
TypeDefArray,
TypeDefCompact,
TypeDefPhantom,
TypeDefPrimitive,
TypeDefSequence,
TypeDefTuple,
Expand Down Expand Up @@ -318,11 +317,13 @@ impl TypeInfo for String {
}
}

pub(crate) type PhantomIdentity = PhantomData<()>;

impl<T> TypeInfo for PhantomData<T> {
type Identity = PhantomData<()>;
type Identity = PhantomIdentity;

fn type_info() -> Type {
TypeDefPhantom.into()
panic!("PhantomData type instances should be filtered out.")
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/meta_type.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::<crate::impls::PhantomIdentity>()
}
}
16 changes: 15 additions & 1 deletion src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ fn prelude_items() {
)
)
);
assert_type!(PhantomData<i32>, TypeDefPhantom);
assert_type!(
Cow<u128>,
Type::builder()
Expand All @@ -100,6 +99,12 @@ fn prelude_items() {
)
}

#[test]
#[should_panic]
fn phantom_data() {
PhantomData::<i32>::type_info();
}

#[test]
fn collections() {
assert_type!(
Expand Down Expand Up @@ -177,6 +182,15 @@ fn tuple_primitives() {
);
}

#[test]
fn tuple_phantom_data_erased() {
// nested tuple
assert_type!(
(u64, PhantomData<u8>),
TypeDefTuple::new(vec![meta_type::<u64>(),])
);
}

#[test]
fn array_primitives() {
// array
Expand Down
2 changes: 1 addition & 1 deletion src/ty/composite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl IntoPortable for TypeDefComposite {

impl TypeDefComposite {
/// Creates a new struct definition with named fields.
pub fn new<I>(fields: I) -> Self
pub(crate) fn new<I>(fields: I) -> Self
where
I: IntoIterator<Item = Field>,
{
Expand Down
24 changes: 4 additions & 20 deletions src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ impl_from_type_def_for_type!(
TypeDefSequence,
TypeDefTuple,
TypeDefCompact,
TypeDefPhantom,
TypeDefBitSequence,
);

Expand Down Expand Up @@ -238,8 +237,6 @@ pub enum TypeDef<T: Form = MetaForm> {
Primitive(TypeDefPrimitive),
/// A type using the [`Compact`] encoding
Compact(TypeDefCompact<T>),
/// A PhantomData type.
Phantom(TypeDefPhantom),
/// A type representing a sequence of bits.
BitSequence(TypeDefBitSequence<T>),
}
Expand All @@ -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(),
}
}
Expand Down Expand Up @@ -380,7 +376,10 @@ impl TypeDefTuple {
T: IntoIterator<Item = MetaType>,
{
Self {
fields: type_params.into_iter().collect(),
fields: type_params
.into_iter()
.filter(|ty| !ty.is_phantom())
.collect(),
}
}

Expand Down Expand Up @@ -486,21 +485,6 @@ where
}
}

/// A type describing a `PhantomData<T>` type.
///
/// In the context of SCALE encoded types, including `PhantomData<T>` 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
Expand Down
40 changes: 4 additions & 36 deletions test_suite/tests/derive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> {
Expand Down Expand Up @@ -667,19 +667,7 @@ fn skip_all_type_params() {
TypeParameter::new("T", None),
TypeParameter::new("U", None),
])
.composite(
Fields::named()
.field(|f| {
f.ty::<PhantomData<NoScaleInfoImpl>>()
.name("a")
.type_name("PhantomData<T>")
})
.field(|f| {
f.ty::<PhantomData<NoScaleInfoImpl>>()
.name("b")
.type_name("PhantomData<U>")
}),
);
.composite(Fields::named());

assert_type!(SkipAllTypeParams<NoScaleInfoImpl, NoScaleInfoImpl>, ty);
}
Expand Down Expand Up @@ -710,15 +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::<PhantomData<NoScaleInfoImpl>>()
.name("a")
.type_name("PhantomData<T>")
})
.field(|f| f.ty::<u32>().name("b").type_name("T::A")),
);
.composite(Fields::named().field(|f| f.ty::<u32>().name("b").type_name("T::A")));

assert_type!(SkipTypeParamsForTraitImpl<NoScaleInfoImpl>, ty);
}
Expand All @@ -741,19 +721,7 @@ fn skip_type_params_with_defaults() {
TypeParameter::new("T", None),
TypeParameter::new("U", None),
])
.composite(
Fields::named()
.field(|f| {
f.ty::<PhantomData<NoScaleInfoImpl>>()
.name("a")
.type_name("PhantomData<T>")
})
.field(|f| {
f.ty::<PhantomData<NoScaleInfoImpl>>()
.name("b")
.type_name("PhantomData<U>")
}),
);
.composite(Fields::named());

assert_type!(SkipAllTypeParamsWithDefaults<NoScaleInfoImpl, NoScaleInfoImpl>, ty);
}
Expand Down
6 changes: 1 addition & 5 deletions test_suite/tests/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,6 @@ fn test_builtins() {
// strings
assert_json_for_type::<String>(json!({ "def": { "primitive": "str" } }));
assert_json_for_type::<str>(json!({ "def": { "primitive": "str" } }));
// PhantomData
assert_json_for_type::<PhantomData<bool>>(json!({ "def": { "phantom": null }, }))
}

#[test]
Expand Down Expand Up @@ -271,7 +269,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<T> {
Expand All @@ -288,8 +286,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<T>" },
],
},
}
Expand Down

0 comments on commit 53c7cfc

Please sign in to comment.