Add StyleEditionDefault trait, StyleEdition enum, and #[style_edition] proc macro
#5854
Conversation
The StyleEditionDefault trait implement the foundation for the `style_edition` mechanism. Eventually all configs will be required to implement this trait so rustfmt can dynamically determine what default value to assign for a given style edition. A proc macro is also added so types can conveniently implement the new trait.
| #[proc_macro_attribute] | ||
| pub fn style_edition(args: TokenStream, input: TokenStream) -> TokenStream { |
There was a problem hiding this comment.
I think it would make more sense as a derive macro, with the configuration of defaults as a helper attribute.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I prefer derives because
- custom derives make it so that you can't change the original type definition.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@fee1-dead do you have any suggestions on improving / simplifying the proposed implementation?
|
|
||
| use crate::attrs; | ||
|
|
||
| /// Returns `true` if the given attribute configures the deafult StyleEdition value |
There was a problem hiding this comment.
| /// Returns `true` if the given attribute configures the deafult StyleEdition value | |
| /// Returns `true` if the given attribute configures the default StyleEdition value |
| /// 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 |
| 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; | ||
| } |
There was a problem hiding this comment.
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?
| 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(()); | ||
| } |
There was a problem hiding this comment.
seems repetitive. Would this be easier to read?
| 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(()); | |
| } | |
| } |
| 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; | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
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;
}
}
})
});| impl Default for StyleEditionDefault { | ||
| fn default() -> Self { | ||
| Self { | ||
| default: None, | ||
| se2015: None, | ||
| se2018: None, | ||
| se2021: None, | ||
| se2024: None, | ||
| } | ||
| } | ||
| } |
|
@fee1-dead thanks for taking the time to review this. I think there are a lot of good suggestions here that I'll try to incorporate! I also wanted to let you know that I opened up #5933, which is a simpler version of this PR without all the proc macro code. I think the proc macro / derive would be nice to have, but it isn't essential for |
|
closing this one as the work for style edition is being done in other PRs. The most recent being #5937 |
The
StyleEditionDefaulttrait implement the foundation for thestyle_editionmechanism. Eventually all configs will be required to implement this trait so rustfmt can dynamically determine what default value to assign for a given style edition.A proc macro is also added so types can conveniently implement the new trait.
r? @calebcartwright
This is the first of the PRs intended to break up #5787 into smaller chunks. Appreciate any and all feedback!