From 6bfad73268d2fa558e34331d64cb3b8e1aec511e Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio Date: Wed, 3 Jul 2024 19:12:31 +0200 Subject: [PATCH] Fix issue --- CHANGELOG.md | 3 ++ impl/doc/debug.md | 23 ++++++++++-- impl/doc/display.md | 27 +++++++++++--- impl/src/fmt/debug.rs | 79 +++++++++++++++++++++++++++++++++++------ impl/src/fmt/display.rs | 67 ++++++++++++++++++++++++++++++---- impl/src/fmt/mod.rs | 6 ++-- tests/debug.rs | 26 +++++++++++--- tests/display.rs | 57 ++++++++++++++++++++--------- 8 files changed, 243 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0ba4f43..1fffe737 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -104,6 +104,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Hygiene of macro expansions in presence of custom `core` crate. ([#327](https://github.com/JelteF/derive_more/pull/327)) - Fix documentation of generated methods in `IsVariant` derive. +- Make `{field:p}` do the expected thing in format strings for `Display` and + `Debug`. Also document weirdness around `Pointer` formatting when using + expressions, due to field variables being references. ## 0.99.10 - 2020-09-11 diff --git a/impl/doc/debug.md b/impl/doc/debug.md index 3d95b90a..be0ad8bd 100644 --- a/impl/doc/debug.md +++ b/impl/doc/debug.md @@ -14,8 +14,27 @@ This derive macro is a clever superset of `Debug` from standard library. Additio You supply a format by placing an attribute on a struct or enum variant, or its particular field: `#[debug("...", args...)]`. The format is exactly like in [`format!()`] or any other [`format_args!()`]-based macros. -The variables available in the arguments is `self` and each member of the struct or enum variant, with members of tuple -structs being named with a leading underscore and their index, i.e. `_0`, `_1`, `_2`, etc. +The variables available in the arguments is `self` and each member of the +struct or enum variant, with members of tuple structs being named with a +leading underscore and their index, i.e. `_0`, `_1`, `_2`, etc. Due to +ownership/lifetime limitations the member variables are all references to the +fields, except when used directly in the format string. For most purposes this +detail doesn't matter, but it is quite important when using `Pointer` +formatting. If you don't use the `{field:p}` syntax, you have to dereference +once to get the address of the field itself, instead of the reference to the +field: + + +```rust +#[derive(derive_more::Debug)] +#[debug("{field:p} {:p}", *field)] +struct RefInt<'a> { + field: &'a i32, +} + +let a = &123; +assert_eq!(format!("{:?}", RefInt{field: &a}), format!("{a:p} {:p}", a)); +``` ### Generic data types diff --git a/impl/doc/display.md b/impl/doc/display.md index 6a7139e6..3b88a797 100644 --- a/impl/doc/display.md +++ b/impl/doc/display.md @@ -22,9 +22,28 @@ inner variable. If there is no such variable, or there is more than 1, an error You supply a format by attaching an attribute of the syntax: `#[display("...", args...)]`. The format supplied is passed verbatim to `write!`. -The variables available in the arguments is `self` and each member of the variant, -with members of tuple structs being named with a leading underscore and their index, -i.e. `_0`, `_1`, `_2`, etc. +The variables available in the arguments is `self` and each member of the +variant, with members of tuple structs being named with a leading underscore +and their index, i.e. `_0`, `_1`, `_2`, etc. Due to ownership/lifetime +limitations the member variables are all references to the fields, except when +used directly in the format string. For most purposes this detail doesn't +matter, but it is quite important when using `Pointer` formatting. If you don't +use the `{field:p}` syntax, you have to dereference once to get the address of +the field itself, instead of the reference to the field: + + +```rust +# use derive_more::Display; +# +#[derive(Display)] +#[display("{field:p} {:p}", *field)] +struct RefInt<'a> { + field: &'a i32, +} + +let a = &123; +assert_eq!(format!("{}", RefInt{field: &a}), format!("{a:p} {:p}", a)); +``` For enums you can also specify a shared format on the enum itself instead of the variant. This format is used for each of the variants, and can be @@ -55,7 +74,7 @@ E.g., for a structure `Foo` defined like this: # trait Trait { type Type; } # #[derive(Display)] -#[display("{} {} {:?} {:p}", a, b, c, d)] +#[display("{a} {b} {c:?} {d:p}")] struct Foo<'a, T1, T2: Trait, T3> { a: T1, b: ::Type, diff --git a/impl/src/fmt/debug.rs b/impl/src/fmt/debug.rs index 524f0218..3e75f7ac 100644 --- a/impl/src/fmt/debug.rs +++ b/impl/src/fmt/debug.rs @@ -11,7 +11,7 @@ use crate::utils::{ Either, Spanning, }; -use super::{trait_name_to_attribute_name, ContainerAttributes, FmtAttribute}; +use super::{parsing, trait_name_to_attribute_name, ContainerAttributes, FmtAttribute}; /// Expands a [`fmt::Debug`] derive macro. /// @@ -241,15 +241,66 @@ impl<'a> Expansion<'a> { Ok(()) } + fn used_arguments<'s>( + &self, + format_string: &'s str, + ) -> impl Iterator + where + 'a: 's, + { + parsing::format_string_formats(format_string) + .into_iter() + .flatten() + .flat_map(|f| match f.arg { + Some(parsing::Argument::Identifier(name)) => Some(name), + _ => None, + }) + } + + fn field_idents(&self) -> impl Iterator + '_ { + self.fields + .iter() + .enumerate() + .map(|(i, f)| f.ident.clone().unwrap_or_else(|| format_ident!("_{i}"))) + } + + fn field_params<'selff, 's>( + &'selff self, + format_string: &'s str, + ) -> impl Iterator + 's + where + 'a: 's, + 'selff: 's, + { + let used_arguments: Vec<&'s str> = self.used_arguments(format_string).collect(); + self.field_idents().filter_map(move |var| { + if used_arguments.contains(&var.to_string().as_str()) { + Some(quote! { #var = *#var }) + } else { + None + } + }) + } + /// Generates [`Debug::fmt()`] implementation for a struct or an enum variant. /// /// [`Debug::fmt()`]: std::fmt::Debug::fmt() fn generate_body(&self) -> syn::Result { if let Some(fmt) = &self.attr.fmt { + let format_string = fmt.lit.value(); return Ok(if let Some((expr, trait_ident)) = fmt.transparent_call() { - quote! { derive_more::core::fmt::#trait_ident::fmt(&(#expr), __derive_more_f) } + let expr = if self.field_idents().any(|var| { + self.used_arguments(&format_string) + .any(|arg| arg == var.to_string().as_str()) + }) { + quote! { #expr } + } else { + quote! { &(#expr) } + }; + quote! { derive_more::core::fmt::#trait_ident::fmt(#expr, __derive_more_f) } } else { - quote! { derive_more::core::write!(__derive_more_f, #fmt) } + let field_params = self.field_params(&format_string); + quote! { derive_more::core::write!(__derive_more_f, #fmt, #(#field_params),*) } }); }; @@ -285,12 +336,15 @@ impl<'a> Expansion<'a> { exhaustive = false; Ok::<_, syn::Error>(out) } - Some(FieldAttribute::Right(fmt_attr)) => Ok(quote! { - derive_more::__private::DebugTuple::field( - #out, - &derive_more::core::format_args!(#fmt_attr), + Some(FieldAttribute::Right(fmt_attr)) => { + let format_string = fmt_attr.lit.value(); + let field_params = self.field_params(&format_string); + Ok(quote! { + derive_more::__private::DebugTuple::field( + #out, + &derive_more::core::format_args!(#fmt_attr, #(#field_params),*), ) - }), + })}, None => { let ident = format_ident!("_{i}"); Ok(quote! { @@ -327,13 +381,16 @@ impl<'a> Expansion<'a> { exhaustive = false; Ok::<_, syn::Error>(out) } - Some(FieldAttribute::Right(fmt_attr)) => Ok(quote! { + Some(FieldAttribute::Right(fmt_attr)) => { + let format_string = fmt_attr.lit.value(); + let field_params = self.field_params(&format_string); + Ok(quote! { derive_more::core::fmt::DebugStruct::field( #out, #field_str, - &derive_more::core::format_args!(#fmt_attr), + &derive_more::core::format_args!(#fmt_attr, #(#field_params),*), ) - }), + })}, None => Ok(quote! { derive_more::core::fmt::DebugStruct::field(#out, #field_str, &#field_ident) }), diff --git a/impl/src/fmt/display.rs b/impl/src/fmt/display.rs index 089f44dc..2ce50a93 100644 --- a/impl/src/fmt/display.rs +++ b/impl/src/fmt/display.rs @@ -269,7 +269,10 @@ impl<'a> Expansion<'a> { )); } if !current_format.is_empty() { - tokens.extend(quote! { derive_more::core::write!(__derive_more_f, #current_format)?; }); + let field_params = self.field_params(¤t_format); + tokens.extend(quote! { + derive_more::core::write!(__derive_more_f, #current_format, #(#field_params),*)?; + }); current_format.clear(); } if maybe_body.is_none() { @@ -293,9 +296,10 @@ impl<'a> Expansion<'a> { }; } if !current_format.is_empty() { - tokens.extend( - quote! { derive_more::core::write!(__derive_more_f, #current_format) }, - ) + let field_params = self.field_params(¤t_format); + tokens.extend(quote! { + derive_more::core::write!(__derive_more_f, #current_format, #(#field_params),*) + }); } else { tokens.extend(quote! { Ok(()) }); } @@ -309,10 +313,20 @@ impl<'a> Expansion<'a> { fn generate_body_impl(&self) -> syn::Result { match &self.attrs.fmt { Some(fmt) => { + let format_string = fmt.lit.value(); Ok(if let Some((expr, trait_ident)) = fmt.transparent_call() { - quote! { derive_more::core::fmt::#trait_ident::fmt(&(#expr), __derive_more_f) } + let expr = if self.field_idents().any(|var| { + self.used_arguments(&format_string) + .any(|arg| arg == var.to_string().as_str()) + }) { + quote! { #expr } + } else { + quote! { &(#expr) } + }; + quote! { derive_more::core::fmt::#trait_ident::fmt(#expr, __derive_more_f) } } else { - quote! { derive_more::core::write!(__derive_more_f, #fmt) } + let field_params = self.field_params(&format_string); + quote! { derive_more::core::write!(__derive_more_f, #fmt, #(#field_params),*) } }) } None if self.fields.is_empty() => { @@ -346,6 +360,47 @@ impl<'a> Expansion<'a> { } } + fn used_arguments<'s>( + &self, + format_string: &'s str, + ) -> impl Iterator + where + 'a: 's, + { + parsing::format_string_formats(format_string) + .into_iter() + .flatten() + .flat_map(|f| match f.arg { + Some(parsing::Argument::Identifier(name)) => Some(name), + _ => None, + }) + } + + fn field_idents(&self) -> impl Iterator + '_ { + self.fields + .iter() + .enumerate() + .map(|(i, f)| f.ident.clone().unwrap_or_else(|| format_ident!("_{i}"))) + } + + fn field_params<'selff, 's>( + &'selff self, + format_string: &'s str, + ) -> impl Iterator + 's + where + 'a: 's, + 'selff: 's, + { + let used_arguments: Vec<&'s str> = self.used_arguments(format_string).collect(); + self.field_idents().filter_map(move |var| { + if used_arguments.contains(&var.to_string().as_str()) { + Some(quote! { #var = *#var }) + } else { + None + } + }) + } + /// Generates trait bounds for a struct or an enum variant. fn generate_bounds(&self) -> Vec { let mut bounds: Vec = diff --git a/impl/src/fmt/mod.rs b/impl/src/fmt/mod.rs index b28a773f..96bb18c2 100644 --- a/impl/src/fmt/mod.rs +++ b/impl/src/fmt/mod.rs @@ -113,14 +113,16 @@ impl Parse for FmtAttribute { fn parse(input: ParseStream<'_>) -> syn::Result { Self::check_legacy_fmt(input)?; - Ok(Self { + let mut parsed = Self { lit: input.parse()?, comma: input .peek(token::Comma) .then(|| input.parse()) .transpose()?, args: input.parse_terminated(FmtArgument::parse, token::Comma)?, - }) + }; + parsed.args.pop_punct(); + Ok(parsed) } } diff --git a/tests/debug.rs b/tests/debug.rs index 8944bd42..1c483ef2 100644 --- a/tests/debug.rs +++ b/tests/debug.rs @@ -262,6 +262,16 @@ mod structs { field: &'a i32, } + #[derive(Debug)] + #[debug("{_0:p}")] + struct TupleTransparent<'a>(&'a i32); + + #[derive(Debug)] + #[debug("{field:p}")] + struct StructTransparent<'a> { + field: &'a i32, + } + #[test] fn assert() { let a = 42; @@ -273,6 +283,14 @@ mod structs { format!("{:?}", Struct { field: &a }), format!("Struct {{ field: {0:p}.{0:p} }}", &a), ); + assert_eq!( + format!("{:?}", TupleTransparent(&a)), + format!("{0:p}", &a), + ); + assert_eq!( + format!("{:?}", StructTransparent { field: &a }), + format!("{0:p}", &a), + ); } } } @@ -564,11 +582,11 @@ mod structs { use derive_more::Debug; #[derive(Debug)] - #[debug("{_0:p} * {_1:p}", _0 = self.0)] + #[debug("{_0:p} * {_1:p}")] struct Tuple<'a, 'b>(&'a u8, &'b bool); #[derive(Debug)] - #[debug("{a:p} * {b:p}", a = self.a)] + #[debug("{a:p} * {b:p}")] struct Struct<'a, 'b> { a: &'a u8, b: &'b bool, @@ -1305,7 +1323,7 @@ mod generic { #[derive(Debug)] struct AliasedFieldNamedGenericStruct { - #[debug("{field1}", field1 = field2)] + #[debug("{field3}", field3 = field2)] field1: T, field2: i32, } @@ -1423,7 +1441,7 @@ mod generic { } #[derive(Debug)] - struct AliasedFieldUnnamedGenericStruct(#[debug("{_0}", _0 = _1)] T, i32); + struct AliasedFieldUnnamedGenericStruct(#[debug("{_2}", _2 = _1)] T, i32); #[test] fn aliased_field_unnamed_generic_struct() { assert_eq!( diff --git a/tests/display.rs b/tests/display.rs index a2f56af3..c5e40b0f 100644 --- a/tests/display.rs +++ b/tests/display.rs @@ -443,7 +443,7 @@ mod structs { } #[derive(Display)] - #[display("{}", format_args!("{field:p}"))] + #[display("{}", format_args!("{field:p}", field = *field))] struct StructPointer<'a> { field: &'a i32, } @@ -562,15 +562,15 @@ mod structs { #[derive(Display)] #[display( - "{_0} {ident} {_1} {} {}", - _1, _0 + _1, ident = 123, _1 = _0, + "{_0} {ident} {_2} {} {}", + _1, _0 + _1, ident = 123, _2 = _0, )] struct Tuple(i32, i32); #[derive(Display)] #[display( - "{field1} {ident} {field2} {} {}", - field2, field1 + field2, ident = 123, field2 = field1, + "{field1} {ident} {field3} {} {}", + field2, field1 + field2, ident = 123, field3 = field1, )] struct Struct { field1: i32, @@ -1004,7 +1004,7 @@ mod enums { #[derive(Display)] enum Pointer<'a> { - #[display("{:p}", _0)] + #[display("{:p}", *_0)] A(&'a i32), #[display("{field:p}")] B { field: &'a u8 }, @@ -1115,9 +1115,9 @@ mod enums { #[derive(Display)] enum Pointer<'a> { - #[display("{}", format_args!("{:p}", _0))] + #[display("{}", format_args!("{:p}", *_0))] A(&'a i32), - #[display("{}", format_args!("{field:p}"))] + #[display("{}", format_args!("{field:p}", field = *field))] B { field: &'a u8 }, } @@ -1160,13 +1160,13 @@ mod enums { #[display("named")] StrNamed { field1: i32, field2: i32 }, #[display( - "{_0} {ident} {_1} {} {}", - _1, _0 + _1, ident = 123, _1 = _0, + "{_0} {ident} {_2} {} {}", + _1, _0 + _1, ident = 123, _2 = _0, )] InterpolatedUnnamed(i32, i32), #[display( - "{field1} {ident} {field2} {} {}", - field2, field1 + field2, ident = 123, field2 = field1, + "{field1} {ident} {field3} {} {}", + field2, field1 + field2, ident = 123, field3 = field1, )] InterpolatedNamed { field1: i32, field2: i32 }, } @@ -1265,7 +1265,7 @@ mod enums { #[derive(Display)] enum Pointer<'a> { - #[display("{:p}", _1)] + #[display("{:p}", *_1)] A(&'a f64, &'a f32), #[display("{a:p}")] B { a: &'a f64, b: &'a f32 }, @@ -1431,6 +1431,31 @@ mod enums { assert_eq!(Enum::::C(9).to_string(), "Variant C 9",); } } + + mod pointer { + use super::*; + + #[derive(Display)] + #[display("Pointer {_0:p} {_variant} {_0:p}")] + enum Pointer<'a> { + #[display("A")] + A(&'a f64), + #[display("B")] + B(&'a f32), + } + #[test] + fn assert() { + let (a, b) = (8.3, 42.1); + assert_eq!( + format!("{}", Pointer::A(&a)), + format!("Pointer {0:p} A {0:p}", &a), + ); + assert_eq!( + format!("{}", Pointer::B(&b)), + format!("Pointer {0:p} B {0:p}", &b), + ); + } + } } } } @@ -2071,7 +2096,7 @@ mod generic { struct TupleUpperExp(T); #[derive(Display)] - #[display("{:p}", _0)] + #[display("{_0:p}")] struct TuplePointer(T); #[derive(Display)] @@ -2119,7 +2144,7 @@ mod generic { } #[derive(Display)] - #[display("{:p}", field)] + #[display("{field:p}")] struct StructPointer { field: T, } @@ -2182,7 +2207,7 @@ mod generic { #[derive(Display)] enum EnumPointer { - #[display("{:p}", _0)] + #[display("{_0:p}")] A(A), #[display("{field:p}")] B { field: B },