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

Add skip_type_params attribute #96

Merged
merged 51 commits into from
Jun 29, 2021
Merged

Add skip_type_params attribute #96

merged 51 commits into from
Jun 29, 2021

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented May 28, 2021

Currently the TypeInfo derive adds all generic type params to the Type::type_params metadata.

Therefore all type params must be constrained to TypeInfo in order for the call to meta_type to compile. So the proc macro automatically adds those bounds.

However, sometimes this requirement forces us to implement TypeInfo or provide additional bounds where it is unnecessary.

This PR introduces the skip_type_params attribute, which removes the requirement for the supplied type params to implement TypeInfo.

Scenario 1: skip_type_params on its own.

Consider a simple example of a type parameter which is used for associated types, but the type itself does not carry any type information. This is a common pattern in substrate:

trait Config {
    type Balance;
}

struct Runtime; // doesn't implement `TypeInfo`

impl Config for Runtime {
    type Balance = u64;
}

#[allow(unused)]
#[derive(TypeInfo)]
#[scale_info(skip_type_params(T))] 
struct A<T: Config> {
    balance: T::Balance,
    marker: core::marker::PhantomData<T>,
}

fn assert_type_info<T: TypeInfo + 'static>() {}

fn main() {
    assert_type_info::<A<Runtime>>(); // without the `skip_type_params` attribute this will fail.
}

Without skip_type_params, the following code is generated:

impl<T> ::scale_info::TypeInfo for A<T>
where
    T: Config,
    T: TypeInfo + 'static,      // <-- TypeInfo required for T
    T::Balance: TypeInfo + 'static,
{
    type Identity = Self;
    fn type_info() -> ::scale_info::Type {
        Type::builder()
        .type_params(vec![
            TypeParameter::new("T", Some(meta_type::<T>())), // <-- MetaType recorded for T
        ])
        /// .. elided
    }
};

With the skip_type_params applied

impl<T> ::scale_info::TypeInfo for A<T>
where
    T: Config,
    T: 'static, // <-- No more `TypeInfo` bound
    T::Balance: TypeInfo + 'static,
{
    type Identity = Self;
    fn type_info() -> ::scale_info::Type {
        Type::builder()
        .type_params(vec![
            TypeParameter::new("T", None), // <-- MetaType not added for T
        ])
        /// .. elided
    }
};

Note how in the above we still get the autogenerated bounds for the associated type T::A

Scenario 2: skip_type_params used in conjunction with bounds.

#88 adds support for specifying custom bounds, which overrides all auto generated bounds. Without skip_type_params, it is therefore required that the user manually adds T: TypeInfo to the custom bounds attribute.

#[derive(TypeInfo)]
#[scale_info(bounds(
  T: TypeInfo + 'static, // <-- The user needs to add this to make it compile
  U: TypeInfo + 'static,
))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

Adding skip_type_params means that we no longer need to add the unnecessary bound to our custom bounds.

#[derive(TypeInfo)]
#[scale_info(bounds(U: TypeInfo + 'static))]
#[scale_info(skip_type_params(T))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

How bounds and skip_type_params interact.

When used independently, both attributes have an effect of the where clause predicate of the generated TypeInfo impl block.

  • bounds replaces all autogenerated bounds with the user supplied ones in the attribute
  • skip_type_params prevents the TypeInfo bound for the given type params being added to the autogenerated bounds.

When they are both used together, skip_type_params has no effect on the final generated where clause at all, because the predicates supplied in bounds are used verbatim instead of any autogenerated bounds.


let ty = Type::builder()
.path(Path::new("Hey", "derive"))
.type_params(tuple_meta_type!(u16))
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems to me more correct to me to have the first type param marked as skipped than ignored, otherwise when looking at the metadata and the doc of the rust type we can't easily tell which one is skipped and which one is kept

Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like .composite(Fields::named().field(|f| f.ty::<Greet<SomeType>>().name("ciao").type_name("Greet<T>").skipped_type_params::<T>())?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually I'm not sure anymore, I don't understand enough the usecase for type_params.

But what I meant is that currently the Hey<SomeType, u16> type information about type params would be u16, whereas I would have expected "skipped", u16.

Copy link
Contributor Author

@ascjones ascjones Jun 24, 2021

Choose a reason for hiding this comment

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

This is a good point, we are losing some information by removing it entirely. I have addressed this by adding a TypeParameter struct with Option<MetaType> which would be None for skipped type params. Addressed in 411e50c

derive/src/utils.rs Outdated Show resolved Hide resolved
test_suite/tests/derive.rs Outdated Show resolved Hide resolved

let ty = Type::builder()
.path(Path::new("Hey", "derive"))
.type_params(tuple_meta_type!(u16))
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean like .composite(Fields::named().field(|f| f.ty::<Greet<SomeType>>().name("ciao").type_name("Greet<T>").skipped_type_params::<T>())?

@ascjones ascjones changed the title Add skip_type_params attribute Remove type parameters not specified in custom bounds Jun 15, 2021
@ascjones ascjones changed the title Remove type parameters not specified in custom bounds Add skip_type_params attribute Jun 15, 2021
@ascjones ascjones changed the title Add skip_type_params attribute Add skip_type_params attribute Jun 15, 2021
@ascjones ascjones marked this pull request as ready for review June 17, 2021 09:33
@Robbepop
Copy link
Contributor

Robbepop commented Jun 24, 2021

Meeting idea/proposal:
Instead of this:

#[derive(TypeInfo)]
#[scale_info(bounds(U: TypeInfo + 'static))]
#[scale_info(skip_type_params(T))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

We could write this:

#[derive(TypeInfo)]
#[scale_info(bounds(
    U: TypeInfo + 'static,
    T = skip,
))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

///
/// `#[scale_info(bounds(..))]`
///
/// Replaces *all* auto-generated trait bounds with the user-defined ones.
Copy link
Contributor

@dvdplm dvdplm Jun 25, 2021

Choose a reason for hiding this comment

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

#[scale_info(skip_type_params(U))]
#[scale_info(bounds("U: Other + Traits + NotTypeInfoStuff))]
struct A<T, U> {}

In the above, I will not get TypeInfo bounds auto-generated for T (because bounds() override all auto generating machinery) but I will get the bounds I manually specified for U (because bounds(…) trumps skip_type_params). Is that correct?

A few examples like above would be good! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the above, I will not get TypeInfo bounds auto-generated for T (because bounds() override all auto generating machinery) but I will get the bounds I manually specified for U (because bounds(…) trumps skip_type_params). Is that correct?

Correct. It trumps it in respect to the generated where clause. However skip_type_params still has its effect on the generated list of type_params for the type.

A few examples like above would be good! :)

I am in the process of adding more rustdocs on the topic, in the meantime I have added some more ui tests and have updated the PR description.

src/lib.rs Show resolved Hide resolved
@ascjones
Copy link
Contributor Author

ascjones commented Jun 25, 2021

Meeting idea/proposal:
Instead of this:

#[derive(TypeInfo)]
#[scale_info(bounds(U: TypeInfo + 'static))]
#[scale_info(skip_type_params(T))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

We could write this:

#[derive(TypeInfo)]
#[scale_info(bounds(
    U: TypeInfo + 'static,
    T = skip,
))]
struct S<T, U> {
    marker: PhantomData<T>,
    field: U,
}

With skip_type_params, we want to keep all the other autogenerated bounds (see "Scenario 1: skip_type_params on its own" in the PR description). In your example this is not a problem, but falls down when associated types are used in the fields of the type. We would have to manually add all of these, since bounds replaces all autogenerated bounds with the user supplied ones.

This makes it unsuitable for the use case in substrate where we add skip_type_params to the proc-macro generated Event, Call, Error types. This solution would require adding back the bounds on any associated types.

T = skip,

Also this would break the parsing of syn::WherePredicates which we use for extracting the bounds from the attribute.

@ascjones ascjones merged commit e7975d2 into master Jun 29, 2021
@ascjones ascjones deleted the aj-skip-type-params branch June 29, 2021 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants