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.

Sign up for GitHub

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

Jump to bottom

[derive] use known crate name aliases #61

Merged
merged 9 commits into from
Feb 3, 2021
Merged

[derive] use known crate name aliases #61

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4a67533
Add crate imports with known names
ascjones Feb 2, 2021
531dd5d
Use known crate name aliases
ascjones Feb 2, 2021
459e29b
Remove hack for tests, it doesn't apply to all crates
ascjones Feb 2, 2021
63f8f49
Use alias ident
ascjones Feb 2, 2021
159f626
Fix parity_scale_codec crate alias, remove unused imports and args
ascjones Feb 2, 2021
c3cadc6
Fmt
ascjones Feb 2, 2021
38ef7df
Add PR to "Unreleased" section
ascjones Feb 2, 2021
9ab7c51
Update rustfmt config
ascjones Feb 2, 2021
b292cc9
review: use absolute paths with crate names
ascjones Feb 2, 2021
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: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Added

### Changed

### Fixed
- Derive: use known crate name aliases [(#61)](https://github.com/paritytech/scale-info/pull/61)

## [0.5.0] - 2021-01-27
### Added
Expand Down
1 change: 1 addition & 0 deletions derive/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,4 @@ proc-macro = true
quote = "1.0"
syn = { version = "1.0", features = ["derive", "visit", "visit-mut"] }
proc-macro2 = "1.0"
proc-macro-crate = "0.1.5"
34 changes: 19 additions & 15 deletions derive/src/impl_wrapper.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,32 +12,36 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#[cfg(not(feature = "std"))]
use alloc::{
format,
string::ToString,
};

use proc_macro2::{
Span,
TokenStream as TokenStream2,
};
use quote::quote;
use syn::Ident;

pub fn wrap(
ident: &Ident,
trait_name: &'static str,
impl_quote: TokenStream2,
) -> TokenStream2 {
let mut renamed = format!("_IMPL_{}_FOR_", trait_name);
renamed.push_str(ident.to_string().trim_start_matches("r#"));
let dummy_const = Ident::new(&renamed, Span::call_site());
pub fn wrap(impl_quote: TokenStream2) -> TokenStream2 {
let include_scale_info = include_crate("scale-info", "_scale_info");
let include_parity_scale_codec = include_crate("parity-scale-codec", "_scale");
Copy link
Member

Choose a reason for hiding this comment

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

This is in general a bad style, to require some other crate than the macro providing crate in the Cargo.toml.

Why aren't you re-exporting the required stuff from scale-info?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not entirely sold on this.
While I agree that your proposal solves certain problems the scale-info crate works with multiple parity-scale-codec versions. So re-exporting one of the supported versions would case a lock-in and potentially a duplication of dependencies in case the scale-info user does not also use the same version.

The reality is that scale-info and parity-scale-codec are somehow tightly coupled by design (one does encoding, the other does encoding of the encoding) and could actually even live under the same crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed that is a much better idea


quote! {
#[allow(non_upper_case_globals, unused_attributes, unused_qualifications)]
const #dummy_const: () = {
const _: () = {
#include_scale_info
#include_parity_scale_codec

#impl_quote;
};
}
}

/// Include a crate under a known alias, to be robust against renamed dependencies.
fn include_crate(name: &str, alias: &str) -> proc_macro2::TokenStream {
match proc_macro_crate::crate_name(name) {
Ok(crate_name) => {
let crate_name_ident = Ident::new(&crate_name, Span::call_site());
let crate_alias_ident = Ident::new(&alias, Span::call_site());
quote!( extern crate #crate_name_ident as #crate_alias_ident; )
Copy link
Member

Choose a reason for hiding this comment

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

Why not directly accessing the crate with its ident, instead of renaming it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree: #61 (comment)

}
Err(e) => syn::Error::new(Span::call_site(), &e).to_compile_error(),
Copy link
Contributor

@Robbepop Robbepop Feb 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'd personally prefer a solution where we'd still keep having absolute paths everything.
This means that include_crate needed to return a TokenStream in the form of quote! { codec } or quote { scale } or whatever is the actual crate alias for it and users would then use it as:

let parity_scale_codec_alias = include_crate("parity-scale-codec");
quote! { :: #parity_scale_codec_alias ::rest::of::my::path }

This solution would also not need yet another alias, e.g. _scale_info.
The extern crate solution was required in ancient times of Rust edition 2015.

}
}
24 changes: 12 additions & 12 deletions derive/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ fn generate_type(input: TokenStream2) -> Result<TokenStream2> {
let generic_type_ids = ast.generics.type_params().map(|ty| {
let ty_ident = &ty.ident;
quote! {
::scale_info::meta_type::<#ty_ident>()
_scale_info::meta_type::<#ty_ident>()
}
});

Expand All @@ -97,18 +97,18 @@ fn generate_type(input: TokenStream2) -> Result<TokenStream2> {
};
let generic_types = ast.generics.type_params();
let type_info_impl = quote! {
impl <#( #generic_types ),*> ::scale_info::TypeInfo for #ident #ty_generics #where_clause {
impl <#( #generic_types ),*> _scale_info::TypeInfo for #ident #ty_generics #where_clause {
type Identity = Self;
fn type_info() -> ::scale_info::Type {
::scale_info::Type::builder()
.path(::scale_info::Path::new(stringify!(#ident), module_path!()))
.type_params(::scale_info::prelude::vec![ #( #generic_type_ids ),* ])
fn type_info() -> _scale_info::Type {
_scale_info::Type::builder()
.path(_scale_info::Path::new(stringify!(#ident), module_path!()))
.type_params(_scale_info::prelude::vec![ #( #generic_type_ids ),* ])
.#build_type
}
}
};

Ok(impl_wrapper::wrap(ident, "TYPE_INFO", type_info_impl))
Ok(impl_wrapper::wrap(type_info_impl))
}

type FieldsList = Punctuated<Field, Comma>;
Expand Down Expand Up @@ -198,7 +198,7 @@ fn generate_composite_type(data_struct: &DataStruct) -> TokenStream2 {
}
};
quote! {
composite(::scale_info::build::Fields::#fields)
composite(_scale_info::build::Fields::#fields)
}
}

Expand Down Expand Up @@ -228,7 +228,7 @@ fn generate_c_like_enum_def(variants: &VariantList) -> TokenStream2 {
});
quote! {
variant(
::scale_info::build::Variants::fieldless()
_scale_info::build::Variants::fieldless()
#( #variants )*
)
}
Expand Down Expand Up @@ -257,7 +257,7 @@ fn generate_variant_type(data_enum: &DataEnum) -> TokenStream2 {
quote! {
.variant(
#v_name,
::scale_info::build::Fields::named()
_scale_info::build::Fields::named()
#( #fields)*
)
}
Expand All @@ -267,7 +267,7 @@ fn generate_variant_type(data_enum: &DataEnum) -> TokenStream2 {
quote! {
.variant(
#v_name,
::scale_info::build::Fields::unnamed()
_scale_info::build::Fields::unnamed()
#( #fields)*
)
}
Expand All @@ -281,7 +281,7 @@ fn generate_variant_type(data_enum: &DataEnum) -> TokenStream2 {
});
quote! {
variant(
::scale_info::build::Variants::with_fields()
_scale_info::build::Variants::with_fields()
#( #variants)*
)
}
Expand Down
8 changes: 4 additions & 4 deletions derive/src/trait_bounds.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,21 @@ pub fn make_where_clause<'a>(
if is_compact {
where_clause
.predicates
.push(parse_quote!(#ty : ::scale::HasCompact));
.push(parse_quote!(#ty : _scale::HasCompact));
where_clause
.predicates
.push(parse_quote!(<#ty as ::scale::HasCompact>::Type : ::scale_info::TypeInfo + 'static));
.push(parse_quote!(<#ty as _scale::HasCompact>::Type : _scale_info::TypeInfo + 'static));
} else {
where_clause
.predicates
.push(parse_quote!(#ty : ::scale_info::TypeInfo + 'static));
.push(parse_quote!(#ty : _scale_info::TypeInfo + 'static));
}
});

generics.type_params().into_iter().for_each(|type_param| {
let ident = type_param.ident.clone();
let mut bounds = type_param.bounds.clone();
bounds.push(parse_quote!(::scale_info::TypeInfo));
bounds.push(parse_quote!(_scale_info::TypeInfo));
bounds.push(parse_quote!('static));
where_clause
.predicates
Expand Down
5 changes: 1 addition & 4 deletions test_suite/tests/derive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,7 @@
#![cfg_attr(not(feature = "std"), no_std)]

use pretty_assertions::assert_eq;
use scale::{
Compact,
Encode,
};
use scale::Encode;
use scale_info::{
build::*,
prelude::{
Expand Down