Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Erase PhantomData fields #111

Merged
merged 5 commits into from
Jul 15, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So users can't instantiate with any fields containing a PhantomData. They are filtered out in the builder, and this would allow circumventing the builder.

The other option of course would be to do the phantom filtering directly in this method, but I think we should encourage the use of the builder as the way to construct Composite types.

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