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

impl rename_all for #[derive(Display)] (#216) #443

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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 impl/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ constructor = []
debug = ["syn/extra-traits", "dep:unicode-xid"]
deref = []
deref_mut = []
display = ["syn/extra-traits", "dep:unicode-xid"]
display = ["syn/extra-traits", "dep:unicode-xid", "dep:convert_case"]
error = ["syn/extra-traits"]
from = ["syn/extra-traits"]
from_str = []
Expand Down
8 changes: 5 additions & 3 deletions impl/src/fmt/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
//!
//! [`fmt::Debug`]: std::fmt::Debug

use std::convert::Infallible;

use proc_macro2::TokenStream;
use quote::{format_ident, quote};
use syn::{ext::IdentExt as _, parse_quote, spanned::Spanned as _};
Expand Down Expand Up @@ -77,7 +79,7 @@ pub fn expand(input: &syn::DeriveInput, _: &str) -> syn::Result<TokenStream> {
///
/// [`fmt::Debug`]: std::fmt::Debug
fn expand_struct(
attrs: ContainerAttributes,
attrs: ContainerAttributes<Infallible>,
ident: &syn::Ident,
s: &syn::DataStruct,
type_params: &[&syn::Ident],
Expand Down Expand Up @@ -115,7 +117,7 @@ fn expand_struct(
///
/// [`fmt::Debug`]: std::fmt::Debug
fn expand_enum(
mut attrs: ContainerAttributes,
mut attrs: ContainerAttributes<Infallible>,
e: &syn::DataEnum,
type_params: &[&syn::Ident],
attr_name: &syn::Ident,
Expand Down Expand Up @@ -207,7 +209,7 @@ type FieldAttribute = Either<attr::Skip, FmtAttribute>;
/// [`Debug::fmt()`]: std::fmt::Debug::fmt()
#[derive(Debug)]
struct Expansion<'a> {
attr: &'a ContainerAttributes,
attr: &'a ContainerAttributes<Infallible>,

/// Struct or enum [`Ident`](struct@syn::Ident).
ident: &'a syn::Ident,
Expand Down
39 changes: 29 additions & 10 deletions impl/src/fmt/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::utils::{attr::ParseMultiple as _, Spanning};

use super::{
trait_name_to_attribute_name, ContainerAttributes, ContainsGenericsExt as _,
FmtAttribute,
FmtAttribute, RenameAllAttribute,
};

/// Expands a [`fmt::Display`]-like derive macro.
Expand All @@ -29,9 +29,13 @@ pub fn expand(input: &syn::DeriveInput, trait_name: &str) -> syn::Result<TokenSt
let trait_name = normalize_trait_name(trait_name);
let attr_name = format_ident!("{}", trait_name_to_attribute_name(trait_name));

let attrs = ContainerAttributes::parse_attrs(&input.attrs, &attr_name)?
.map(Spanning::into_inner)
.unwrap_or_default();
let attrs = ContainerAttributes::parse_attrs(&input.attrs, &attr_name)?;
if let Some(attrs) = &attrs {
if matches!(input.data, syn::Data::Struct(_)) {
attrs.validate_for_struct(&attr_name)?;
}
}
let attrs = attrs.map(Spanning::into_inner).unwrap_or_default();
let trait_ident = format_ident!("{trait_name}");
let ident = &input.ident;

Expand Down Expand Up @@ -83,7 +87,7 @@ pub fn expand(input: &syn::DeriveInput, trait_name: &str) -> syn::Result<TokenSt
///
/// [`syn::Ident`]: struct@syn::Ident
type ExpansionCtx<'a> = (
&'a ContainerAttributes,
&'a ContainerAttributes<RenameAllAttribute>,
&'a [&'a syn::Ident],
&'a syn::Ident,
&'a syn::Ident,
Expand All @@ -97,6 +101,7 @@ fn expand_struct(
) -> syn::Result<(Vec<syn::WherePredicate>, TokenStream)> {
let s = Expansion {
shared_attr: None,
rename_all: None,
attrs,
fields: &s.fields,
type_params,
Expand Down Expand Up @@ -148,14 +153,18 @@ fn expand_enum(
let (bounds, match_arms) = e.variants.iter().try_fold(
(Vec::new(), TokenStream::new()),
|(mut bounds, mut arms), variant| {
let attrs = ContainerAttributes::parse_attrs(&variant.attrs, attr_name)?
.map(Spanning::into_inner)
.unwrap_or_default();
let attrs = ContainerAttributes::parse_attrs(&variant.attrs, attr_name)?;
if let Some(attrs) = &attrs {
attrs.validate_for_struct(attr_name)?;
};
let attrs = attrs.map(Spanning::into_inner).unwrap_or_default();

let ident = &variant.ident;

if attrs.fmt.is_none()
&& variant.fields.is_empty()
&& attr_name != "display"
&& container_attrs.rename_all.is_none()
{
return Err(syn::Error::new(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ModProg hmmm... I don't see why rename_all argument should allow implicit naming for other traits rather than Display only? Could you elaborate on that?

For me, the situation assert_eq!(format!("{:o}", Enum::VariantOne), "variant_one"); seems quite awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My idea was you could use different display traits for different ways of rename_all if you want to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JelteF what do you think on this? I don't particularly like this idea of abusing different fmt traits for different naming, but maybe it's just me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have seen crates use different format traits for stuff like this in the past, but I don't mind removing the functionality. i.e. make rename_all only available for Display.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree that this is weird: assert_eq!(format!("{:o}", Enum::VariantOne), "variant_one");

But I also think assert_eq!(format!("{:o}", Enum::VariantOne), "VariantOne"); is weird. And as I understand that's what we do currently. I would be happier to see the Octal derive simply fail for unit enum variants without an #[octal(...)] attribute. Instead of defaulting to stringifying them in the same way as we do by default for Display, but that would be a breaking change. And doing another major release to fix that doesn't seem worth it.

So my vote would be, let's do whatever is easiest code wise. And let's create an issue that we add to the 3.0 milestone, to remove this weird behaviour completely then.

e.variants.span(),
Expand All @@ -168,6 +177,7 @@ fn expand_enum(

let v = Expansion {
shared_attr: container_attrs.fmt.as_ref(),
rename_all: container_attrs.rename_all,
attrs: &attrs,
fields: &variant.fields,
type_params,
Expand Down Expand Up @@ -234,8 +244,13 @@ struct Expansion<'a> {
/// [`None`] for a struct.
shared_attr: Option<&'a FmtAttribute>,

/// [`RenameAllAttribute`] placed on enum.
///
/// [`None`] for a struct.
rename_all: Option<RenameAllAttribute>,

/// Derive macro [`ContainerAttributes`].
attrs: &'a ContainerAttributes,
attrs: &'a ContainerAttributes<RenameAllAttribute>,

/// Struct or enum [`syn::Ident`].
///
Expand Down Expand Up @@ -305,7 +320,11 @@ impl Expansion<'_> {
None => {
if shared_attr_is_wrapping || !has_shared_attr {
body = if self.fields.is_empty() {
let ident_str = self.ident.unraw().to_string();
let ident_str = if let Some(rename_all) = &self.rename_all {
rename_all.convert_case(self.ident)
} else {
self.ident.unraw().to_string()
};

if shared_attr_is_wrapping {
quote! { #ident_str }
Expand Down
184 changes: 178 additions & 6 deletions impl/src/fmt/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub(crate) mod debug;
pub(crate) mod display;
mod parsing;

#[cfg(feature = "display")]
use convert_case::{Case, Casing as _};
use proc_macro2::TokenStream;
use quote::{format_ident, quote, ToTokens};
use syn::{
Expand Down Expand Up @@ -88,6 +90,65 @@ impl BoundsAttribute {
}
}

/// Representation of a `rename_all` macro attribute.
///
/// ```rust,ignore
/// #[<attribute>(rename_all = "...")]
/// ```
///
/// Possible Cases:
/// - `lowercase`
/// - `UPPERCASE`
/// - `PascalCase`
/// - `camelCase`
/// - `snake_case`
/// - `SCREAMING_SNAKE_CASE`
/// - `kebab-case`
/// - `SCREAMING-KEBAB-CASE`
#[cfg(feature = "display")]
#[derive(Debug, Clone, Copy)]
struct RenameAllAttribute(#[allow(unused)] Case);

#[cfg(feature = "display")]
impl RenameAllAttribute {
fn convert_case(&self, ident: &syn::Ident) -> String {
ident.unraw().to_string().to_case(self.0)
}
}

#[cfg(feature = "display")]
impl Parse for RenameAllAttribute {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
let _ = input.parse::<syn::Path>().and_then(|p| {
if p.is_ident("rename_all") {
Ok(p)
} else {
Err(syn::Error::new(
p.span(),
"unknown attribute argument, expected `rename_all = \"...\"`",
))
}
})?;

input.parse::<syn::Token![=]>()?;

let value: syn::LitStr = input.parse()?;

// TODO should we really do a case insensitive comparision here?
Ok(Self(match value.value().replace(['-', '_'], "").to_lowercase().as_str() {
"lowercase" => Case::Flat,
"uppercase" => Case::UpperFlat,
"pascalcase" => Case::Pascal,
"camelcase" => Case::Camel,
"snakecase" => Case::Snake,
"screamingsnakecase" => Case::UpperSnake,
"kebabcase" => Case::Kebab,
"screamingkebabcase" => Case::UpperKebab,
_ => return Err(syn::Error::new_spanned(value, "unexpected casing expected one of: \"lowercase\", \"UPPERCASE\", \"PascalCase\", \"camelCase\", \"snake_case\", \"SCREAMING_SNAKE_CASE\", \"kebab-case\", or \"SCREAMING-KEBAB-CASE\""))
}))
}
}

/// Representation of a [`fmt`]-like attribute.
///
/// ```rust,ignore
Expand Down Expand Up @@ -509,16 +570,47 @@ impl Placeholder {
/// are allowed.
///
/// [`fmt::Display`]: std::fmt::Display
#[derive(Debug, Default)]
struct ContainerAttributes {
#[derive(Debug)]
struct ContainerAttributes<T> {
/// Interpolation [`FmtAttribute`].
fmt: Option<FmtAttribute>,

/// Addition trait bounds.
bounds: BoundsAttribute,

/// Rename unit enum variants following a similar behavior as [`serde`](https://serde.rs/container-attrs.html#rename_all).
rename_all: Option<T>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ModProg instead of modifying the ContainerAttributes with an additional slot (which is useless for the Debug), I think it's better to leave it "as is", and:

  1. Move RenameAllAttribute into display module.
  2. Inside display module do:
    struct ContainerAttributes {
        rename_all: RenameAllAttribute,
        common: super::ContainerAttributes,
    }
    and parse it as Either<RenameAllAttribute, super::ContainerAttributes> to omit repeating parsing of the super::ContainerAttributes twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought It'd make sense to allow to have rename_all and bounds in the same attribute. But that will only at most save one line anyway.

}

impl<T> std::default::Default for ContainerAttributes<T> {
fn default() -> Self {
Self {
fmt: None,
bounds: BoundsAttribute::default(),
rename_all: None,
}
}
}

#[cfg(feature = "display")]
impl<T> Spanning<ContainerAttributes<T>> {
fn validate_for_struct(
&self,
attr_name: impl std::fmt::Display,
) -> syn::Result<()> {
if self.rename_all.is_some() {
Err(syn::Error::new(
self.span,
format_args!("`#[{attr_name}(rename_all=\"...\")]` can not be specified on structs or variants"),
))
} else {
Ok(())
}
}
}

impl Parse for ContainerAttributes {
#[cfg(feature = "debug")]
impl Parse for ContainerAttributes<std::convert::Infallible> {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
// We do check `FmtAttribute::check_legacy_fmt` eagerly here, because `Either` will swallow
// any error of the `Either::Left` if the `Either::Right` succeeds.
Expand All @@ -527,13 +619,83 @@ impl Parse for ContainerAttributes {
Either::Left(fmt) => Self {
bounds: BoundsAttribute::default(),
fmt: Some(fmt),
rename_all: None,
},
Either::Right(bounds) => Self { bounds, fmt: None },
Either::Right(bounds) => Self {
bounds,
fmt: None,
rename_all: None,
},
})
}
}

#[cfg(feature = "display")]
impl Parse for ContainerAttributes<RenameAllAttribute> {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
mod kw {
use syn::custom_keyword;

custom_keyword!(rename_all);
custom_keyword!(bounds);
custom_keyword!(bound);
}

// We do check `FmtAttribute::check_legacy_fmt` eagerly here, because `Either` will swallow
// any error of the `Either::Left` if the `Either::Right` succeeds.
FmtAttribute::check_legacy_fmt(input)?;
let lookahead = input.lookahead1();
Ok(if lookahead.peek(syn::LitStr) {
Self {
fmt: Some(input.parse()?),
bounds: BoundsAttribute::default(),
rename_all: None,
}
} else if lookahead.peek(kw::rename_all)
|| lookahead.peek(kw::bounds)
|| lookahead.peek(kw::bound)
|| lookahead.peek(syn::Token![where])
{
let mut bounds = BoundsAttribute::default();
let mut rename_all = None;

while !input.is_empty() {
let lookahead = input.lookahead1();
if lookahead.peek(kw::rename_all) {
if rename_all.is_some() {
return Err(
input.error("`rename_all` can only be specified once")
);
} else {
rename_all = Some(input.parse()?);
}
} else if lookahead.peek(kw::bounds)
|| lookahead.peek(kw::bound)
|| lookahead.peek(syn::Token![where])
{
bounds.0.extend(input.parse::<BoundsAttribute>()?.0)
} else {
return Err(lookahead.error());
}
if !input.is_empty() {
input.parse::<syn::Token![,]>()?;
}
}
Self {
fmt: None,
bounds,
rename_all,
}
} else {
return Err(lookahead.error());
})
}
}

impl attr::ParseMultiple for ContainerAttributes {
impl<T: 'static> attr::ParseMultiple for ContainerAttributes<T>
where
ContainerAttributes<T>: Parse,
{
fn merge_attrs(
prev: Spanning<Self>,
new: Spanning<Self>,
Expand All @@ -554,6 +716,16 @@ impl attr::ParseMultiple for ContainerAttributes {
format!("multiple `#[{name}(\"...\", ...)]` attributes aren't allowed"),
));
}
if new
.rename_all
.and_then(|n| prev.rename_all.replace(n))
.is_some()
{
return Err(syn::Error::new(
new_span,
format!("multiple `#[{name}(rename_all=\"...\")]` attributes aren't allowed"),
));
}
prev.bounds.0.extend(new.bounds.0);

Ok(Spanning::new(
Expand Down Expand Up @@ -582,7 +754,7 @@ where
}
}

/// Extension of a [`syn::Type`] and a [`syn::Path`] allowing to travers its type parameters.
/// Extension of a [`syn::Type`] and a [`syn::Path`] allowing to traverse its type parameters.
trait ContainsGenericsExt {
/// Checks whether this definition contains any of the provided `type_params`.
fn contains_generics(&self, type_params: &[&syn::Ident]) -> bool;
Expand Down
7 changes: 7 additions & 0 deletions tests/compile_fail/display/invalid_casing.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#[derive(derive_more::Display)]
#[display(rename_all = "Whatever")]
enum Enum {
UnitVariant,
}

fn main() {}
5 changes: 5 additions & 0 deletions tests/compile_fail/display/invalid_casing.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
error: unexpected casing expected one of: "lowercase", "UPPERCASE", "PascalCase", "camelCase", "snake_case", "SCREAMING_SNAKE_CASE", "kebab-case", or "SCREAMING-KEBAB-CASE"
--> tests/compile_fail/display/invalid_casing.rs:2:24
|
2 | #[display(rename_all = "Whatever")]
| ^^^^^^^^^^
Loading