-
Notifications
You must be signed in to change notification settings - Fork 30
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
[derive] use known crate name aliases #61
Conversation
derive/src/impl_wrapper.rs
Outdated
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; ) | ||
} | ||
Err(e) => syn::Error::new(Span::call_site(), &e).to_compile_error(), |
There was a problem hiding this comment.
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.
derive/src/impl_wrapper.rs
Outdated
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; ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree: #61 (comment)
derive/src/impl_wrapper.rs
Outdated
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"); |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to thread the crate names all over is a bit unfortunate. Do you think something like this would work?
use once_cell::sync::Lazy;
struct IdentHelper(Lazy<String>);
impl quote::ToTokens for IdentHelper {
fn to_tokens(&self, tokens: &mut TokenStream2) {
Ident::new(&self.0, Span::call_site()).to_tokens(tokens)
}
}
/// Get the name of the scale-info crate, to be robust against renamed dependencies.
static SCALE_INFO: IdentHelper = IdentHelper(Lazy::new(|| {
proc_macro_crate::crate_name("scale-info")
.expect("Missing scale-info dependency. Please add it to your Cargo.toml")
}));
/// Get the name of the parity-scale-codec crate, to be robust against renamed dependencies.
static CODEC: IdentHelper = IdentHelper(Lazy::new(|| {
proc_macro_crate::crate_name("parity-scale-codec")
.expect("Missing parity-scale-codec dependency. Please add it to your Cargo.toml")
}));
…and then use it e.g. like so:
quote! {
:: #SCALE_INFO ::meta_type::<#ty_ident>()
}
It seems to work fine, all tests pass etc, but maybe it will mess up the span?
Brought up in this comment by @Robbepop #53 (comment)
When I read originally, I overlooked the fact that
::scale
is of course referring toparity-scale-codec
, so this means that our derive only works now that crate is imported and renamed toscale
.This PR introduces @bkchr's https://github.com/bkchr/proc-macro-crate to use a common known alias for both
scale_info
andparity-scale-codec
dependencies.