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 StyleEditionDefault trait, StyleEdition enum, and #[style_edition] proc macro #5854

Closed
wants to merge 1 commit into from
Closed
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
2 changes: 1 addition & 1 deletion config_proc_macro/src/attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ fn is_attr_name_value(attr: &syn::Attribute, name: &str) -> bool {
}
}

fn is_attr_path(attr: &syn::Attribute, name: &str) -> bool {
pub fn is_attr_path(attr: &syn::Attribute, name: &str) -> bool {
match &attr.meta {
syn::Meta::Path(path) if path.is_ident(name) => true,
_ => false,
Expand Down
12 changes: 12 additions & 0 deletions config_proc_macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ mod attrs;
mod config_type;
mod item_enum;
mod item_struct;
mod style_edition;
mod utils;

use std::str::FromStr;
Expand Down Expand Up @@ -82,3 +83,14 @@ pub fn rustfmt_only_ci_test(_args: TokenStream, input: TokenStream) -> TokenStre
token_stream
}
}

/// Implement the StyleEditionDefault trait for the given item;
#[proc_macro_attribute]
pub fn style_edition(args: TokenStream, input: TokenStream) -> TokenStream {
Comment on lines +88 to +89
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense as a derive macro, with the configuration of defaults as a helper attribute.

Copy link
Contributor Author

@ytmimi ytmimi Jul 23, 2023

Choose a reason for hiding this comment

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

Interesting idea! I used the #[config_type] proc macro as a reference, which helps us implement the ConfigType trait for all our config options. Are there advantages to defining this as a derive instead of a custom proc macro? would it simplify the implementation?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer derives because

  1. custom derives make it so that you can't change the original type definition.
  2. custom derives make it obvious that you are generating the implementation of a trait, when the naming of the derive macro matches the trait that you are generating impl for.

I'm not sure if it would actually result in any difference in the implementation or simplify anything. It is probably just a stylistic choice here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for laying out your rational! Given that it's more of a stylistic decision I'd prefer to hold off on changing this from a proc macro to a derive macro until after we've got style_edition fully implemented. Then we might consider changing both #[config_type] and #[style_edition] to custom derives.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fee1-dead do you have any suggestions on improving / simplifying the proposed implementation?

let input = parse_macro_input!(input as syn::Item);
let args = parse_macro_input!(args as style_edition::StyleEditionDefault);

let output = style_edition::define_style_edition(args, input).unwrap();
let result = TokenStream::from(output);
result
}
355 changes: 355 additions & 0 deletions config_proc_macro/src/style_edition.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,355 @@
use proc_macro2::{Span, TokenStream};
use quote::{quote, ToTokens};
use syn::parse::{Parse, ParseStream};
use syn::spanned::Spanned;
use syn::Token;

use crate::attrs;

/// Returns `true` if the given attribute configures the deafult StyleEdition value
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns `true` if the given attribute configures the deafult StyleEdition value
/// Returns `true` if the given attribute configures the default StyleEdition value

pub fn se_default(attr: &syn::Attribute) -> bool {
attrs::is_attr_path(attr, "se_default")
}

/// Returns `true` if the given attribute configures the deafult value for StyleEdition2015
pub fn se_2015(attr: &syn::Attribute) -> bool {
attrs::is_attr_path(attr, "se_2015")
}

/// Returns `true` if the given attribute configures the deafult value for StyleEdition2018
pub fn se_2018(attr: &syn::Attribute) -> bool {
attrs::is_attr_path(attr, "se_2018")
}

/// Returns `true` if the given attribute configures the deafult value for StyleEdition2021
pub fn se_2021(attr: &syn::Attribute) -> bool {
attrs::is_attr_path(attr, "se_2021")
}

/// Returns `true` if the given attribute configures the deafult for StyleEdition2024
Comment on lines +14 to +29
Copy link
Member

Choose a reason for hiding this comment

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

same as above

pub fn se_2024(attr: &syn::Attribute) -> bool {
attrs::is_attr_path(attr, "se_2024")
}

/// Defines `style_edition` on enum or struct.
pub fn define_style_edition(
defaults: StyleEditionDefault,
item: syn::Item,
) -> syn::Result<TokenStream> {
match item {
syn::Item::Struct(st) => define_style_edition_struct(defaults, st),
syn::Item::Enum(en) => define_style_edition_enum(defaults, en),
_ => panic!("Expected enum or struct"),
}
}

pub struct StyleEditionDefault {
default: Option<syn::Expr>,
se2015: Option<syn::Expr>,
se2018: Option<syn::Expr>,
se2021: Option<syn::Expr>,
se2024: Option<syn::Expr>,
}

impl StyleEditionDefault {
/// a sinlge default for all style editions
fn single_default(&self) -> bool {
self.default.is_some()
&& self.se2015.is_none()
&& self.se2018.is_none()
&& self.se2021.is_none()
&& self.se2024.is_none()
}
/// Infer the type from the default value
fn ty_from_default(&self) -> syn::Result<syn::Type> {
match &self.default {
Some(syn::Expr::Lit(lit)) => match lit.lit {
syn::Lit::Bool(_) => {
return Ok(syn::TypePath {
qself: None,
path: path_from_str("bool"),
}
.into());
}
syn::Lit::Int(_) => {
return Ok(syn::TypePath {
qself: None,
path: path_from_str("usize"),
}
.into());
}
_ => {}
},
_ => {}
}
Err(syn::parse::Error::new(
Span::call_site(),
"could not determine type from default value",
))
}

fn enum_expr_path(varient: &syn::Variant, en: &syn::ItemEnum) -> syn::ExprPath {
let mut path = path_from_ident(&en.ident);
path.segments.push(varient.ident.clone().into());
syn::ExprPath {
attrs: vec![],
qself: None,
path,
}
}

/// Set the style edition based on the the annotated attribute
/// For example:
/// ```ignore
/// #[style_edition]
/// enum Example {
/// #[se_default] // <-- Default style edition
/// A,
/// #[se_2018] // <-- Explicit override for StypeEdition2018
/// B,
/// }
/// ```
fn set_defaults_by_enum_variant_attr(&mut self, en: &syn::ItemEnum) {
for varient in en.variants.iter() {
for attr in varient.attrs.iter() {
if se_default(attr) {
self.default.replace(Self::enum_expr_path(varient, en).into());
break;
} else if se_2015(attr) {
self.se2015.replace(Self::enum_expr_path(varient, en).into());
break;
} else if se_2018(attr) {
self.se2018.replace(Self::enum_expr_path(varient, en).into());
break;
} else if se_2021(attr) {
self.se2021.replace(Self::enum_expr_path(varient, en).into());
break;
} else if se_2024(attr) {
self.se2024.replace(Self::enum_expr_path(varient, en).into());
break;
}
Comment on lines +115 to +130
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't check whether there are two attributes, which we probably want to error? (i.e. something like #[se_2018] #[se_2021])

Also, I would prefer to replace this by getting the attr path as an ident, getting the ident as a string and then matching on the string, probably via let-else and continue if we can't unwrap it?

}
}
}

/// Set the style edition based on the rhs of the assignment in the attribute
/// e.g. `#[style_edition(true, se_2015=false)]`
fn set_by_assignment(&mut self, assignment: &syn::ExprAssign) -> syn::Result<()> {
match assignment.left.as_ref() {
syn::Expr::Path(expr) => {
let se2015 = syn::Ident::new("se_2015", Span::call_site());
let se2018 = syn::Ident::new("se_2018", Span::call_site());
let se2021 = syn::Ident::new("se_2021", Span::call_site());
let se2024 = syn::Ident::new("se_2024", Span::call_site());
let ident = expr
.path
.segments
.first()
.map(|segment| segment.ident.clone())
.expect("should be at least one ident");

if ident == se2015 {
self.se2015.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2018 {
self.se2018.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2021 {
self.se2021.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2024 {
self.se2024.replace(*assignment.right.clone());
return Ok(());
}
Comment on lines +151 to +163
Copy link
Member

Choose a reason for hiding this comment

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

seems repetitive. Would this be easier to read?

Suggested change
if ident == se2015 {
self.se2015.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2018 {
self.se2018.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2021 {
self.se2021.replace(*assignment.right.clone());
return Ok(());
} else if ident == se2024 {
self.se2024.replace(*assignment.right.clone());
return Ok(());
}
for (edition, field) in [
("se2015", &mut self.se2015),
("se2018", &mut self.se2018),
("se2021", &mut self.sf2021),
("se2024", &mut self.se2024),
]
{
if ident == edition {
field.replace(*assignment.right.clone());
return Ok(());
}
}

}
_ => {}
}
Err(syn::Error::new(
Span::call_site(),
format!(
"Unknown lhs {:?}",
assignment.left.as_ref().to_token_stream().to_string()
),
))
}

fn generate_style_edition_impl_body(&self, name: &syn::Ident) -> TokenStream {
let default = self.default.as_ref();
let se2015 = self.se2015.as_ref().map(|expr| {
quote! {
if #name == crate::config::StyleEdition::Edition2015 {
return #expr;
}
}
});
let se2018 = self.se2018.as_ref().map(|expr| {
quote! {
if #name == crate::config::StyleEdition::Edition2018 {
return #expr;
}
}
});
let se2021 = self.se2021.as_ref().map(|expr| {
quote! {
if #name == crate::config::StyleEdition::Edition2021 {
return #expr;
}
}
});
let se2024 = self.se2024.as_ref().map(|expr| {
quote! {
if #name == crate::config::StyleEdition::Edition2024 {
return #expr;
}
}
});
Comment on lines +178 to +205
Copy link
Member

Choose a reason for hiding this comment

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

We can do something like

let editions = [
    (self.se_2015.as_ref(), "Edition2015"),
    (self.se_2018.as_ref(), "Edition2018"),
    (self.se_2021.as_ref(), "Edition2021"),
    (self.se_2024.as_ref(), "Edition2024"),
].into_iter().flat_map(|(expr, variant)| {
    let variant = syn::Ident::new(variant, Span::call_site());
    expr.map_or_else(TokenStream::new, |expr| {
        quote! {
            if #name == crate::config::StyleEdition::#variant {
                return #expr;
            }
        }
    })
});

quote! {
#se2015
#se2018
#se2021
#se2024
#default
}
}
}

fn path_from_str(s: &str) -> syn::Path {
syn::Path::from(syn::Ident::new(s, Span::call_site()))
}

fn path_from_ident(ident: &syn::Ident) -> syn::Path {
syn::Path::from(ident.clone())
}

impl Default for StyleEditionDefault {
fn default() -> Self {
Self {
default: None,
se2015: None,
se2018: None,
se2021: None,
se2024: None,
}
}
}
Comment on lines +224 to +234
Copy link
Member

Choose a reason for hiding this comment

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

This can be derived


/// Parse StyleEdition values from attribute macro.
/// For example: `#[style_edition(100)]`, which sets the defaul to 100 for all style edtions
/// or `#[style_edition(false, se_2024=true)]`, which sets the default for all style editions except
/// `StyleEdition2024` to false, and explicitly sets `StyleEdition2024=true`
impl Parse for StyleEditionDefault {
fn parse(input: ParseStream) -> syn::Result<Self> {
let mut se_default = StyleEditionDefault::default();
if input.is_empty() {
return Ok(se_default);
}
let defaults = input.parse_terminated(syn::Expr::parse, Token![,])?;
for (idx, pair) in defaults.into_pairs().enumerate() {
let expr = pair.into_value();
match &expr {
syn::Expr::Assign(assign) => {
if idx == 0 {
se_default.default.replace(*assign.right.to_owned());
continue;
}
se_default.set_by_assignment(assign)?;
}
syn::Expr::Lit(_) if idx == 0 => {
se_default.default.replace(expr);
}
syn::Expr::Path(_) if idx == 0 => {
se_default.default.replace(expr);
}
_ => {
return Err(syn::parse::Error::new(
expr.span(),
format!(
"Can't create a style edition default from the expr: {:?}",
expr.to_token_stream().to_string()
),
));
}
}
}
Ok(se_default)
}
}

fn define_style_edition_struct(
defaults: StyleEditionDefault,
st: syn::ItemStruct,
) -> syn::Result<TokenStream> {
let ty = defaults.ty_from_default()?;
let ident = st.ident.clone();
define_style_edition_inner(defaults, ty, ident, st.into())
}

fn define_style_edition_enum(
mut defaults: StyleEditionDefault,
mut en: syn::ItemEnum,
) -> syn::Result<TokenStream> {
let ty = syn::TypePath {
qself: None,
path: syn::Path::from(en.ident.clone()),
};

let ident = en.ident.clone();
defaults.set_defaults_by_enum_variant_attr(&en);
for mut variant in en.variants.iter_mut() {
remove_style_edition_attrs(&mut variant);
}
define_style_edition_inner(defaults, ty.into(), ident, en.into())
}

/// Remove attributes specific to `style_edition` from enum variant fields.
/// These attributes are only used as markers to help us generate `StyleEditionDefault`
/// trait implementations. They should be removed to avoid compilation errors.
fn remove_style_edition_attrs(variant: &mut syn::Variant) {
let metas = variant
.attrs
.iter()
.filter(|attr| {
!se_default(attr)
&& !se_2015(attr)
&& !se_2018(attr)
&& !se_2021(attr)
&& !se_2024(attr)
})
.cloned()
.collect();

variant.attrs = metas;
}

fn define_style_edition_inner(
defaults: StyleEditionDefault,
ty: syn::Type,
ident: syn::Ident,
item: syn::Item,
) -> syn::Result<TokenStream> {
if defaults.default.is_none() {
return Err(syn::Error::new(
Span::call_site(),
format!("Missing default style edition value for {:?}", ident),
));
}

let name = if defaults.single_default() {
syn::Ident::new("_se", Span::call_site())
} else {
syn::Ident::new("style_edition", Span::call_site())
};

let body = defaults.generate_style_edition_impl_body(&name);

Ok(quote! {
#item

impl crate::config::config_type::StyleEditionDefault for #ident {
type ConfigType = #ty;
fn style_edition_default(#name: crate::config::StyleEdition) -> Self::ConfigType {
#body
}
}
})
}
Loading